Re: SQL Property Graph Queries (SQL/PGQ)

On Wed, Feb 25, 2026 at 5:04 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> On 25.02.26 09:28, Ashutosh Bapat wrote:
> >> Thanks for committing those patches. Here's a patchset rebased on top
> >> of these commits.
> >>
> >> 0001 is the same as earlier 0001, but with a conflict in
> >> pg_overexplain.sql/.out resolved. It needs 0002 so that a property
> >> graph can be used in the GRAPH_TABLE clause.
> >> 0002 is your patch to add parserOpenPropGraph() with the typo and
> >> comment fixed as mentioned above. It should be squashed into 0001 in
> >> the next patchset.
> > pg_overexplain failed on CI because of new overexplain fields. Here's
> > a patchset fixing the failure by adding those fields to the graph
> > table query in pg_overexplain.
>
> Here are some additional patches for fixups and some bug fixes to apply
> on top.
>

They all LGTM. Squashed into attached 0001 along with 0002 in the
earlier patchset.

> Some additional comments:
>
> - In parse_graphtable.c, maybe some of the functions could have more
> verbose comments, like what is the input structure, what is the output
> structure, what does it check for. Consider for example
> transformLabelExpr().

Done. I don't think we need prologue for transformPathTerm,
transformPathPatternList, which just walk through the lists. But let
me know if you feel otherwise.

The changes are included as 0002 in the attached patchset.

>
> - The structure GraphTableParseState is completely undocumented. Also,
> I don't think it belongs into nodes/parsenodes.h. Maybe better in
> parser/parse_node.h, where ParseState is also?

Done. in 0003.

>
> - Let's remove the debug elog(INFO) in rewriteGraphTable().

Done. Yay!

>
> - In some cases in rewriteGraphTable.c(), it's not clear to me whether
> the elog should be a user-facing ereport, for example in
> generate_queries_for_path_pattern(). Although if there is an invalid
> path pattern structure, this should be handled in the parser already?

Converted user facing elogs to erepots.

Some of the elogs and ereports can be moved to the corresponding
transform* functions in parse_graphtable.c. rewriteGraphTable.c now
has Asserts in those places to document the conditions assumed to be
true by that code.

There are still some elogs left for failed catalog lookups and also
for invalid conditions which better be documented as elogs rather than
Asserts.

>
> - Also, check the error message style: no initial capital letter, no
> period, "can not" -> "cannot".

Fixed.

>
> - The difference between get_gep_kind_name() and
> get_graph_elem_kind_name() is unclear and confusing. Should they be the
> same?
>

They are different - get_gep_kind_name() deals with the graph element
pattern kinds like vertex, and left/right/any edges, nested path
patterns etc. get_graph_elem_kind_name() deals with types of property
graph elements i.e. vertex and edge. I have replaced the second
function with a ternary operator.

> Also, for this kind of internal error:
>
> elog(ERROR, "unsupported element pattern kind: \"%s\"",
> get_gep_kind_name(gep->kind))
>
> we should just print the raw gep->kind. (If it's unsupported, chances
> are that it's not even something that get_gep_kind_name() can translate.)

gram.y sets gep->kind to GraphElementPatternKind enum values.
Reporting these internal values won't make sense to a user. They will
need the name of the kind. Right now it is used to report "nested path
pattern" which accepted in gram.y but is not supported. In future we
may grow the names of kinds supported in grammar but not implemented
or use grow places where certain kinds are accepted but not others.

In case a catalog corruption puts an invalid value for gep->kind in a
cataloged expression/query, I think reporting "unknown" would be
sufficient as done in the patch.

All changes to rewriteGraphTable.c are in 0004.

Will squash 0002-0004 into 0001 once you have reviewed those patches.

--
Best Wishes,
Ashutosh Bapat