Re: SQL Property Graph Queries (SQL/PGQ) - Mailing list pgsql-hackers
From | Ashutosh Bapat |
---|---|
Subject | Re: SQL Property Graph Queries (SQL/PGQ) |
Date | |
Msg-id | CAExHW5tYT5zvfLTV51+eF1VsH+CtzAH=8umbp4BHB==8t_zsrQ@mail.gmail.com Whole thread Raw |
In response to | Re: SQL Property Graph Queries (SQL/PGQ) (Peter Eisentraut <peter@eisentraut.org>) |
List | pgsql-hackers |
Hi Peter, On Mon, Aug 11, 2025 at 7:12 PM Peter Eisentraut <peter@eisentraut.org> wrote: > > I have browsed this patch set a bit to get myself up to date. > > General thoughts: > > This is a large patch set, and it's not going to be perfect at the first > try. Especially, some of the parsing rules, query semantics, that kind > of thing. So I'm thinking that we should aim at integrating this with > an understanding that it would be somewhat experimental, and then > iterate on some of the details in the tree. FWIW, I agree with this plan. > Obviously, that would still > require that the overall architecture is ok, that it doesn't crash, that > it satisfies security requirements. Also, it should as much as possible > follow the "if you don't use it, it doesn't affect you" rule. +1. > > I found two areas where a bit more work could be done to separate the > new code from existing code: > > src/backend/utils/cache/inval.c: This looks suspicious. The existing > code only deals with very fundamental catalogs such as pg_class and > pg_index. It doesn't feel right to hardcode additional arguably very > high-level PGQ-related catalogs there. We should think of a different > approach here. I implemented it that way considering similarity between properties and attributes. But I agree with you. I think it's a matter of calling CacheInvalidateRelcacheByRelid with OID of property graph at appropriate places in AlterPropGraph(), which is the only place where components of the property graph are changed without changing property graph's pg_class tuple. Creating and dropping a property graph will touch the pg_class tuple which will trigger the current invalidation mechanism. > > We also need to make sure that property graphs have security features > equivalent to views. For example, I suspect we need to integrate them > with restrict_nonsystem_relation_kind. There might be other similar > things, to be checked. > Thanks for pointing me to restrict_nonsystem_relation_kind. I can take care of that. Apart from that views have two security related settings - security_invoker and security_barrier. As mentioned in the commit message of 0003, property graphs behave like views with security_invoker set to true. This will avoid any privilege escalations through property graphs. This should be the safest MVP. But it's different from the default security_invoker setting for views. I was thinking that we will integrate the patches with this behaviour and add security_invoker semantics in the next iteration. Please let me know if you think that security_invoker and security_definer semantics should both be supported before the first commit itself. Property graphs do not have security_barrier setting right now. Property graphs do not have any conditions of their own. But GRAPH_TABLE() may have. WHERE conditions in the GRAPH_TABLE are executed like other conditions on the underlying table (if our optimizer can push the conditions below UNION). I guess we need to support security_barrier semantics on property graph so that WHERE conditions in GRAPH_TABLE are executed before other conditions, but it doesn't seem to be critical to me for the first cut as users can leverage security_barrier views for the same, if required. But please let me know if you think that security_barrier semantics should also be supported before the first commit. RLS is already covered. Is there anything that is missing? > > src/test/regress/sql/rowsecurity.sql: I think it would be better to > separate the new test cases into a separate file. This test file is > already large and complicated, and sprinkling a bunch of more stuff all > over the place is going to make review and maintenance more complicated. I sprinkled the queries in that file since the tables, RLS rules, scenarios to test were readily available there. But I agree with you that a file would be better. In fact, it's better to create a separate file graph_table_security.sql for tests related to security aspects of property graph covering RLS, restrict_nonsystem_relation_kind, security_invoker and security_barrier. What do you think? I will work on that. From the first sentence in this email, it seems that you are planning a thorough review of this patch set. Are you waiting for the above issues to be addressed before continuing the review? -- Best Wishes, Ashutosh Bapat
pgsql-hackers by date: