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

Hi Junwang,

On Sun, Aug 31, 2025 at 4:35 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
>
> I have some review comments, and hope some of them are helpful.
>
> 1.
>
> doc/src/sgml/ddl.sgml
>
> +<programlisting>
> +CREATE PROPERTY GRAPH myshop
> +    VERTEX TABLES (
> +        products LABEL product,
> +        customers LABEL customer,
> +        orders LABEL order
> +    )
> +    EDGE TABLES (
> +        order_items SOURCE orders DESTINATION products LABEL contains,
> +        customer_orders SOURCE customers DESTINATION orders LABEL has
> +    );
> +</programlisting>
>
> order is a reserved keyword, so the following example will fail, we
> should not give a wrong example in our document.
>

I tried those examples and they all worked. What error are you
getting? What is not working for you?

> 2.
>
> doc/src/sgml/information_schema.sgml
>
> +  <title><literal>pg_element_table_key_columns</literal></title>
> +
> +  <para>
> +   The view <literal>pg_element_key_columns</literal> identifies which columns
> +   are part of the keys of the element tables of property graphs defined in
> +   the current database.  Only those property graphs are shown that the
> +   current user has access to (by way of being the owner or having some
> +   privilege).
> +  </para>
>
>
> +  <title><literal>pg_element_table_properties</literal></title>
> +
> +  <para>
> +   The view <literal>pg_element_table_labels</literal> shows the definitions
> +   of the properties for the element tables of property graphs defined in the
> +   current database.  Only those property graphs are shown that the current
> +   user has access to (by way of being the owner or having some privilege).
> +  </para>
>
> the <title> and the <para> doesn't match.

Don't understand this. What isn't matching here. May be provide a
patch with what you think is correct.

>
> 3.
>
> doc/src/sgml/queries.sgml
>
> +   <para>
> +    A path pattern thus matches a sequence of vertices and edges.  The
> +    simplest possible path pattern is
> +<programlisting>
> +()
> +</programlisting>
> +    which matches a single vertex.  The next simplest pattern would be
> +<programlisting>
> +()-[]->()
> +</programlisting>
> +    which matches a vertex followed by an edge followed by a vertex.  The
> +    characters <literal>()</literal> are a vertex pattern and the characters
> +    <literal>-[]-></literal> are an edge pattern.
> +   </para>
>
> the description seems wrong, when a user writes (), it should match
> all vertices in a property graph, for example:

This description is a continuation of the previous sentence. It should
be read as "() which matches a single vertex (in a sequence of
vertices and edges)."  See how the next sentence is written. This
looks ok to me.

>
> 4.
>
> doc/src/sgml/ref/create_property_graph.sgml
>
> +<programlisting>
> +CREATE PROPERTY GRAPH g1
> +    VERTEX TABLES (
> +        v1 LABEL foo PROPERTIES (x AS a * 2) LABEL bar PROPERTIES (x AS a * 2)
> +    ) ...
> +</programlisting>
> +      but this would not:
> +<programlisting>
> +CREATE PROPERTY GRAPH g1
> +    VERTEX TABLES (
> +        v1 LABEL foo PROPERTIES (x AS a * 2) LABEL bar PROPERTIES (x AS a * 10)
> +    ) ...
> +</programlisting></para>
>
> The expr after  PROPERTIES should be (a * 2 AS x)

Good catch. Fixed.

>
> 5.
>
> src/backend/catalog/sql_features.txt
>
> +G034 Path concatenation YES SQL/PGQ required
>
> Do we support path concatenation?

I don't think so. But let Peter confirm it.

>
> +G070 Label expression: label disjunction NO SQL/PGQ required
>
> I think this should be a YES?

Nice catch. Fixed.

>
> 6.
>
> src/backend/commands/tablecmds.c
>
> The description RenameRelation and RemoveRelations should adapt
> property graph, below are diffs I suggest:
>
>  /*
> - * Execute ALTER TABLE/INDEX/SEQUENCE/VIEW/MATERIALIZED VIEW/FOREIGN TABLE
> + * Execute ALTER TABLE/INDEX/SEQUENCE/VIEW/MATERIALIZED VIEW/FOREIGN
> TABLE/PROPERTY GRAPH
>   * RENAME
>   */
>  ObjectAddress
> @@ -4210,8 +4210,8 @@ RenameRelation(RenameStmt *stmt)
>
>         /*
>          * Grab an exclusive lock on the target table, index, sequence, view,
> -        * materialized view, or foreign table, which we will NOT release until
> -        * end of transaction.
> +        * materialized view, foreign table or property graph, which we will NOT
> +        * release until end of transaction.
>
>
>  /*
>   * RemoveRelations
>   *             Implements DROP TABLE, DROP INDEX, DROP SEQUENCE, DROP VIEW,
> - *             DROP MATERIALIZED VIEW, DROP FOREIGN TABLE
> + *             DROP MATERIALIZED VIEW, DROP FOREIGN TABLE, DROP PROPERTY GRAPH
>

Nice catch. Fixed.

> 7.
>
> src/backend/nodes/nodeFuncs.c
>
> exprLocation should have an arm for T_GraphPropertyRef? I suggest:
>
> @@ -1811,6 +1811,9 @@ exprLocation(const Node *expr)
>                 case T_PartitionRangeDatum:
>                         loc = ((const PartitionRangeDatum *) expr)->location;
>                         break;
> +               case T_GraphPropertyRef:
> +                       loc = ((const GraphPropertyRef *) expr)->location;
> +                       break;
>
> 8.
>
> src/backend/rewrite/rewriteGraphTable.c
>
> patch 0002
>
> + /*
> + * Label expressions from two element patterns need to be
> + * conjucted. Label conjuction is not supported right now
>
> typo, should be conjuncted and conjunction.

Done and improved that comment a bit.

Attaching the patchset with the next email.

--
Best Wishes,
Ashutosh Bapat