Re: SQL Property Graph Queries (SQL/PGQ)

Hi Junwang,

On Sun, Aug 31, 2025 at 4:35 PM Junwang Zhao <zhjwpku(at)gmail(dot)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