Re: SQL Property Graph Queries (SQL/PGQ) - Mailing list pgsql-hackers

On Tue, Nov 25, 2025 at 7:22 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> I have done a rough review of the patch set version 20251120.
>
> - There are "No newline at end of file" warnings from git diff for a
> couple of files:
>
>        contrib/pg_overexplain/sql/pg_overexplain.sql
>        src/test/regress/sql/graph_table_rls.sql
>
> Please fix those in the next patch set, and maybe check your editor
> settings.

I was assuming that pg_indent will fix it or at least git diff --check
will show it. But it seems only git diff shows it.

I am trying to find a way to automate the task of detection of such
warnings. Is `git diff | grep '^\\'` a good way to highlight the
existence of such warnings? It still won't show the file which
contains those warnings. I didn't find a non-cumbersome way to list
the files where such warnings exist. Any ideas?

>
> - Attached are two small patches with some small fixes I found in
> passing.  (I think some of them were also reported by Junwang Zhao in
> the meantime.)
>

Included in the attached patch.

> - In the test files, especially src/test/regress/sql/graph_table.sql,
> there are wildly different SQL styles (capitalization, formatting) used,
> depending on who added the test case (I guess).  Let's keep that more
> consistent.

Done. I have also fixed some inconsistent styles in
create_property_graph.sql and examined other modified SQL files. I
have done a couple rounds of fixing inconsistencies, so there
shouldn't be many (if not none) remaining now.

privileges.sql already uses inconsistent styles. The new changes there
just match the surrounding style.

Also removed the DROP SCHEMA statement from graph_table.sql and
create_property_graph.sql but left the command behind.

>
> - In src/backend/parser/analyze.c, the extracted function
> constructSetOpTargetlist() needs a detailed comment.

I have added a prologue to this function, describing what it does.
There are detailed comments in the function itself, which cover the
"how" and "why" part. Let me know if this suffices.

>
> - I'm not so sure about the semantics chosen in the patch "Access
> permissions on property graph".  I think according to the SQL standard,
> once you have access to the property graph, you don't need access to the
> underlying tables as well.  I guess you did this to align with how views
> work?  We might need to think about this a bit more, and document
> whatever the conclusion is.  But for now it's just small amount of code
> affected.

Section 7.1 Access rules seem to just require that the session user
should have SELECT privileges on the property graph. I didn't find an
answer to the question: What privileges should be used while accessing
the element tables. Application of RLS policies depends upon the
answer, for example. They  can be either the owner's privileges
(security_definer semantics) or the privileges of session user
(security_invoker semantics). A view can define what data it exposes
precisely upto a row and column; hence a security_definer view can be
made safer. But a property graph can not constrain rows that it
exposes. So I feel, by default, security_invoker semantics are safer
for property graphs. We may support security_definer semantics later,
but it has to be an explicit choice by the user.

There's another deviation from standard.

Section 11.19, Access rule 2 and General rule 4 talk about the
privileges on the element tables required by the owner of the property
graph. AFAIU, Access rule 2 and General Rule 4 together say that the
owner of the property graph (per Syntax rule 9), should have SELECT
permission on every column that is mentioned in the property graph
definition, SELECT permission on every element table. It does not
require the property graph owner to be the owner of the element
tables. But patch 0001, in the previous patchset, requires that the
owner of the property graph should be the owner of the element tables
or superuser.

Assume two users u1 and u2, both with CREATE permissions on schema public.
postgres@147235=#set session authorization u1;
SET
postgres@147235=#create table t1 (a int, b int);
CREATE TABLE
postgres@147235=#set session authorization u2;
SET
postgres@147235=#create property GRAPH g1 vertex tables (t1 key (a));
ERROR:  must be owner of table t1
postgres@147235=#set session authorization u1;
SET
postgres@147235=#create property GRAPH g1 vertex tables (t1 key (a));
CREATE PROPERTY GRAPH

However, the owner may transfer the ownership to another user who need
not be the owner of the element tables
#alter property graph g1 owner to u2;
ALTER PROPERTY GRAPH

CreatePropGraph() does not mention the reason behind this choice.
Should we remove this constraint and implement what the standard says?

If we allow any user to create a property graph containing elements
owned by other users per the standard, those users in turn may extend
their privileges to other users who do not have access to those
tables, if we don't implement security_invoker semantics. That's
another reason why behaviour as implemented in the patch.

I have added following comment in generate_query_for_graph_path()
/*
* Create RangeTblEntry for this element table.
*
* SQL/PGQ standard (Ref. Section 11.19, Access rule 2 and General rule
* 4) does not specify whose access privileges to use when accessing the
* element tables: property graph owner's or current user's. It is safer
* to use current user's privileges so as not to make property graphs as
* a hole for unpriviledged data access. This is inline with the views
* being security_invoker by default.
*/

The documentation of CREATE PROPERTY GRAPH already has
  <para>
   Access to the base relations underlying the <literal>GRAPH_TABLE</literal>
   clause is determined by the permissions of the user executing the query,
   rather than the property graph owner. Thus, the user of a property graph must
   have the relevant permissions on the property graph and base relations
   underlying the <literal>GRAPH_TABLE</literal> clause.
  </para>

Any other place which needs documentation?

>
> I think you could collapse all the patches into one patch now.  I have
> reviewed all the incremental patches and they all look ok to me.  I have
> made some notes about which things I want to review in more detail, such
> as the access control issue, but that doesn't need to be kept as a
> separate patch.
>

Ok. Attached only a single patch. I have consolidated the earlier
commit messages in the Notes section of the collapsed patch for future
reference. I don't think we need those detailed comments in the final
commit message. However, parts may be useful. We should also remove
WIP from the commit message subject line, but that can be done just
before commit.

> When you create future patches, consider using the git format-patch -v
> option.
>

I am still using timestamp as a version but passing it to -v instead
of treating it as a suffix. I like datestamp as versionstamp for the
reasons mentioned upthread.

> And then you can also just gzip the patch.  That should make it even
> smaller than the .zip file.
>

This time the total patch size seems to be below 1MB, so hopefully it
will not attract moderator's attention.

--
Best Wishes,
Ashutosh Bapat