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.
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().
- 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?
- Let's remove the debug elog(INFO) in rewriteGraphTable().
- 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?
- Also, check the error message style: no initial capital letter, no
period, "can not" -> "cannot".
- The difference between get_gep_kind_name() and
get_graph_elem_kind_name() is unclear and confusing. Should they be the
same?
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.)
