Disallow outer-level and WHERE-clause aggregates in GRAPH_TABLE

Sami Imseih <samimseih@gmail.com> writes:

Commit f585671055d1 [1] disallowed aggregates, window functions, and SRFs in a
GRAPH_TABLE COLUMNS list. For the aggregate case it tests pstate->p_hasAggs
after transforming the columns. That is not enough. An aggregate that
references an outer query is attributed to a parent query level, so
check_agglevels_and_constraints sets p_hasAggs on that parent ParseState and
not on the GRAPH_TABLE's own, so the aggregate is not caught.

It kind of seems like that should work, since an outer-level Agg is
effectively a constant within any execution of the lower query.
However, I'm content to say that making it work is out of scope for
v19.

The attached patch closes both gaps by walking the transformed COLUMNS list and
the graph pattern for Aggref and GroupingFunc nodes. Unlike p_hasAggs, the walk
detects an aggregate by its presence in those the trees. Window functions
and SRFs only mark the local ParseState, so their checks introduced in
f585671055d1 remain in place.

I do not like this fix approach, and for that matter I don't like
f58567105. This is expensive thanks to the extra tree traversal,
f58567105 is user-unfriendly because it fails to say exactly what
or where is the construct it's rejecting, and neither patch is
following the perfectly good structure that the parser already
has for this kind of check. IMO the correct way to handle this
restriction is to check it in check_agglevels_and_constraints() based
on the ParseExprKind of the surrounding expression. It looks like the
graph transformation steps in the parser think they can get away with
using EXPR_KIND_WHERE and EXPR_KIND_SELECT_TARGET, but that's just
wrong if the constraints on these subexpressions are any different
from what they are for regular WHERE and SELECT targets. The right
way to go about this is to back-fill new ParseExprKind(s) as needed
and then make the necessary checks in a way similar to existing code.
Doing that might help you find other comparable oversights, too.

regards, tom lane

Thanks for the comments!

I do not like this fix approach, and for that matter I don't like
f58567105. This is expensive thanks to the extra tree traversal,

I was not feeling totally convinced about this either, but was not
sure if the tree traversal is that big of a problem. We should
definitely avoid doing this, if we can.

f58567105 is user-unfriendly because it fails to say exactly what
or where is the construct it's rejecting, and neither patch is
following the perfectly good structure that the parser already
has for this kind of check. IMO the correct way to handle this
restriction is to check it in check_agglevels_and_constraints() based
on the ParseExprKind of the surrounding expression.

You are right. This makes sense to me now. We can introduce
EXPR_KIND_GRAPH_TABLE_COLUMNS and
EXPR_KIND_GRAPH_TABLE_WHERE ParseExprKind's.

Inside check_agglevels_and_constraints(), we can check the restriction
before we walk up to the query level the aggregate belongs to.

Also, transformWindowFuncCall() and check_srf_call_placement() should
do the same thing, meaning the work done in f58567105is effectively
reverted.

The attached patch does this. Is this what you have in mind?

--
Sami

v3 attached. No .c changes from v2.

Added tests for GROUPING and for an aggregate in the graph-pattern WHERE.
Also dropped the inner-reference element-pattern WHERE test, since it would
have passed under f585671055d1 and proved nothing. The outer-reference
element-pattern test was already present.

Also, I created a CF entry https://commitfest.postgresql.org/patch/7144/

--
Sami

Sami Imseih <samimseih@gmail.com> writes:

v3 attached. No .c changes from v2.

I looked this over. The general pattern of adding more EXPR_KIND
values looks fine, but I really didn't like the nonstandard way in
which you tested for GRAPH_TABLE in check_agglevels_and_constraints.
Testing before running up to the aggregate's semantic level is just
wrong: it would fail to reject an aggregate within a subquery within
one of these GRAPH_TABLE clauses. And if we're testing at some other
level than the semantic level, should we also reject GRAPH_TABLE
context at intermediate parse levels?

This was ugly enough that it motivated me to go look at exactly why
an outer aggregate doesn't work here, in hopes of removing the
inconsistent restriction. I found it: replace_property_refs_mutator,
which increments Vars' varlevelsup to account for the fact that
they're being pushed into a subquery, failed to do the equivalent
thing for Aggrefs and GroupingFuncs. (Compare, for instance,
IncrementVarSublevelsUp.) The case seems to Just Work after fixing
that, so I changed check_agglevels_and_constraints to enforce the
restriction in just the same way as it does for other clauses where
we disallow aggs.

As a minor improvement, you can just use the "errkind = true" option
in these error-reporting functions; less code, fewer strings for the
translators to deal with, same or better message wording.

v4 attached. I think this is committable if you don't see anything
else to change.

regards, tom lane

Thanks for v4, Tom!

This was ugly enough that it motivated me to go look at exactly why
an outer aggregate doesn't work here, in hopes of removing the
inconsistent restriction. I found it: replace_property_refs_mutator,
which increments Vars' varlevelsup to account for the fact that
they're being pushed into a subquery,

You are right. This is not something I considered. No reason
why an outer aggregate can't be used inside the GRAPH_TABLE.
The restriction only applies to the same-level aggregate,
which there's no machinery for.

I tested v4 with a mixed same-level and outer-level aggregate, and
this combination is not rejected as it should be.

Using the same EXISTS test from v4, none of these produce the clean
parse-time rejection we expect. The COLUMNS case is worse, it returns
a row instead of erroring. Under EXISTS the aggregate's value is never
required, so the incorrect same-level aggregate is not evaluated and
the bad query is accepted.

```
postgres=# SELECT EXISTS(SELECT num FROM GRAPH_TABLE (myshop MATCH (c
IS customers) COLUMNS (count(c.customer_id + o.customer_id) AS num))
t) FROM customers o;
exists
--------
t
(1 row)

postgres=# SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c
IS customers WHERE count(c.customer_id + o.customer_id) > 0) COLUMNS
(c.name AS nm)) t) FROM customers o;
ERROR: Upper-level Var found where not expected
postgres=# SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c
IS customers WHERE GROUPING(c.customer_id, o.customer_id) = 1) COLUMNS
(c.name AS nm)) t) FROM customers o GROUP BY customer_id;
ERROR: arguments to GROUPING must be grouping expressions of the
associated query level
```

So, based on your explanation earlier, this led me to find that the
leveling is still wrong. Specifically, min_varlevel > 0, since the
level-zero GraphPropertyRef is not accounted for in
check_agg_arguments_walker. In this case min_varlevel should be 0.

The fix is to teach check_agg_arguments_walker that a GraphPropertyRef
carries a level, the GRAPH_TABLE's own level. This is safe because a
GraphPropertyRef can only ever refer to its own GRAPH_TABLE's level
and can never appear inside a sub-select, so treating it as a level-zero
reference is always correct. Such an aggregate then resolves to the
GRAPH_TABLE's own level and the existing same-level rejection fires.
With that in place all of the cases above are rejected cleanly at
parse time.

```
postgres=# SELECT EXISTS(SELECT num FROM GRAPH_TABLE (myshop MATCH (c
IS customers) COLUMNS (count(c.customer_id + o.customer_id) AS num))
t) FROM customers o;
ERROR: aggregate functions are not allowed in GRAPH_TABLE COLUMNS
postgres=# SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c
IS customers WHERE count(c.customer_id + o.customer_id) > 0) COLUMNS
(c.name AS nm)) t) FROM customers o;
ERROR: aggregate functions are not allowed in GRAPH_TABLE WHERE
postgres=# SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c
IS customers WHERE GROUPING(c.customer_id, o.customer_id) = 1) COLUMNS
(c.name AS nm)) t) FROM customers o GROUP BY customer_id;
ERROR: grouping operations are not allowed in GRAPH_TABLE WHERE
```

v5 attached fixes the leveling problem, folded into your v4, with a
mixed-level test cases added.

What do you think?

--
Sami Imseih
Amazon Web Services (AWS)

Sami Imseih <samimseih@gmail.com> writes:

postgres=# SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c
IS customers WHERE count(c.customer_id + o.customer_id) > 0) COLUMNS
(c.name AS nm)) t) FROM customers o;
ERROR: Upper-level Var found where not expected

Ouch.

So, based on your explanation earlier, this led me to find that the
leveling is still wrong. Specifically, min_varlevel > 0, since the
level-zero GraphPropertyRef is not accounted for in
check_agg_arguments_walker. In this case min_varlevel should be 0.

Right, I came to the same conclusion.

The fix is to teach check_agg_arguments_walker that a GraphPropertyRef
carries a level, the GRAPH_TABLE's own level. This is safe because a
GraphPropertyRef can only ever refer to its own GRAPH_TABLE's level
and can never appear inside a sub-select, so treating it as a level-zero
reference is always correct.

I find that argument pretty shaky. If I can write
MATCH WHERE c.customer_id > 0
why can't I write
MATCH WHERE (SELECT c.customer_id > 0)
? I see that that in fact doesn't work, but that seems like a bug
in itself.

More generally, what this suggests to me is that we have a ton of
other bugs-of-omission in places that process Vars and don't know that
a GraphPropertyRef acts like a Var. So what I'm thinking right now is
that this is a fundamental design error, and that we should nuke
GraphPropertyRef altogether in favor of using a Var that references
the appropriate column of the RTE_GRAPH_TABLE relation. We have
learned the hard way in the past that you don't want to invent things
that act like Vars but aren't Vars; if you must, you are going to be
doing a heck of a lot of work to patch up everyplace that will need to
know about them. The hazard here is limited somewhat by the fact that
the planner and executor will never see GraphPropertyRefs, but I'm
afraid that still leaves plenty of scope for bugs (in the rewriter, in
particular).

regards, tom lane

I find that argument pretty shaky.

You are right. I was wrong to assume a GraphPropertyRef can only ever refer
to its own GRAPH_TABLE's level. Written inside a sub-select it does not.

If I can write
MATCH WHERE c.customer_id > 0
why can't I write
MATCH WHERE (SELECT c.customer_id > 0)
? I see that that in fact doesn't work, but that seems like a bug
in itself.

Disallowing subqueries in GRAPH_TABLE is the current intent of the
feature. The code
in upstream today only checks p_hasSubLinks at the end of
transformRangeGraphTable, after the WHERE and COLUMNS list is transformed, so
it only catches subqueries that transform cleanly, i.e. "(SELECT 1)" gets the
intended error, but "(SELECT c.customer_id > 0)" fails with "missing FROM-clause
entry"

The attached v6 does two things. First, it rejects a SubLink under the
GRAPH_TABLE
expr kinds in transformSubLink, the moment it is seen and before the
sub-select body
is analyzed. So MATCH WHERE (SELECT c.customer_id > 0) now reports

```
ERROR: subqueries within GRAPH_TABLE reference are not supported
```

instead of

```
ERROR: missing FROM-clause entry for table "c"
```

Also, because GRAPH_TABLE now rejects subqueries, a property reference
can
never sit inside one, so it is always at its own GRAPH_TABLE's level. That lets
check_agg_arguments_walker treat a GraphPropertyRef as a Var with
varlevelsup 0,
which it otherwise cannot see since a GraphPropertyRef is not a Var.
The walker still
adjusts for the level it is found at, the same as for a Var.

This corrects the aggregate case I showed earlier. The patch includes
regression tests
for it.

More generally, what this suggests to me is that we have a ton of
other bugs-of-omission in places that process Vars and don't know that
a GraphPropertyRef acts like a Var.

Agreed, and v6 is an example of this. It teaches check_agg_arguments_walker
that a GraphPropertyRef behaves like a Var.

Besides other potential bugs, It also limits the feature in the future.
Keeping subqueries out of GRAPH_TABLE is part of what makes some
of the bugs discovered fixable with the current GraphPropertyRef, but
I suspect it will be a real limitation if we try to relax these restrictions.

So what I'm thinking right now is
that this is a fundamental design error, and that we should nuke
GraphPropertyRef altogether in favor of using a Var that references
the appropriate column of the RTE_GRAPH_TABLE relation.

I spent time today on this and I think the direction is right. In what
I have running
locally, a property reference is emitted as a plain Var over the
RTE_GRAPH_TABLE relation,
so the Var handles leveling and the rest for free. GraphPropertyRef
does not go away
entirely, though. It moves onto a side list on the RTE that the
rewriter uses to resolve
each property Var back to its graph property.

The design has some open questions, but I will share if this is the direction
to go.

--
Sami Imseih
Amazon Web Services (AWS)

Hi Sami,

Thanks for working on this. I agree that the changes in
transformRangeGraphTable() to reject aggregates, window functions, and
set-returning functions in GRAPH_TABLE were not sufficient. This patch
fixes it properly.

The patch has two parts
1. Rework the parser to reject aggregates, window functions, and
set-returning functions in GRAPH_TABLE COLUMNS list and graph pattern
WHERE clause using ParseExprKind. It correctly reverts the changes in
transformRangeGraphTable() which tried to do the same but did not
cover all the cases. It adds new ParseExprKind values for GRAPH_TABLE
COLUMNS and GRAPH_TABLE WHERE. I see that they are added to all the
necessary places. We need two separate ParseExprKind values to report
appropriate "all properties reference" error when it appears in
GRAPH_TABLE COLUMNS list versus in the WHERE clause. Otherwise the two
values are used in the same way to reject aggregates, window
functions, and set-returning functions.

2. Treat GraphPropertyRef as a Var and handle all Var-like nodes in
replace_property_refs(). I will cover this below.

On Tue, Aug 18, 2026 at 4:59 AM Sami Imseih <samimseih@gmail.com> wrote:

Besides other potential bugs, It also limits the feature in the future.
Keeping subqueries out of GRAPH_TABLE is part of what makes some
of the bugs discovered fixable with the current GraphPropertyRef, but
I suspect it will be a real limitation if we try to relax these restrictions.

This is a temporary restriction. Sooner or later we will support
subqueries nested under GraphPropertyRef.

So what I'm thinking right now is
that this is a fundamental design error, and that we should nuke
GraphPropertyRef altogether in favor of using a Var that references
the appropriate column of the RTE_GRAPH_TABLE relation.

I spent time today on this and I think the direction is right. In what
I have running
locally, a property reference is emitted as a plain Var over the
RTE_GRAPH_TABLE relation,
so the Var handles leveling and the rest for free. GraphPropertyRef
does not go away
entirely, though. It moves onto a side list on the RTE that the
rewriter uses to resolve
each property Var back to its graph property.

I share the concern raised by Tom about bugs due to missing
GraphPropertyRef handling. However, I don't think we can use Var
instead of GraphPropertyRef. It represents a reference to a property
of all the graph elements that are bound to an element pattern in a
GRAPH_TABLE. The element pattern is not represented by a range table
and Var node does not have a field to represent an element pattern
variable. Further the same GraphPropertyRef may be rewritten as
different expressions depending on the element table it gets
associated with when generating a subquery for a given path. Achieving
this with Var will be quite complex, if not impossible. The question
is whether we can find all the places where we need to treat
GraphPropertyRef as a Var. Given that the GraphPropertyRefs are
generated during transformation and vanish after rewrite, the places
where we need to handle them are limited, as noted by Tom already.
Many of those are already covered by the patch. Going through all the
places, rather painfully, where we use levelsup for Var-like nodes,
looking at var.c, I see that most of that code is applicable to
planning, optimization and execution. The additional places which I
think we need to handle are:
a. locate_var_of_level_walker() to handle GraphPropertyRef so that the
correct error location can be reported. We have separate
locate_aggref_of_level_walker(). So first I thought that it would be
appropriate to add a separate
locate_graphpropertyref_of_level_walker() function. But we want to
report an error from a place where the distinction between Var and
GraphPropertyRef is lost. So a separate function doesn't make sense.
b. contain_vars_of_level() should not see GraphPropertyRef since the
latter lives within GRAPH_TABLE only. But the function is used during
query transformation, so it will be good to add a GraphPropertyRef
case and Assert()/throw error if it is seen. We should add a comment
there to expect GraphPropertyRef there once we start supporting nested
subqueries in GRAPH_TABLE.
c. IncrementVarSublevelsUp() will need to handle GraphPropertyRef once
we support nested subqueries in GRAPH_TABLE. For now we may want to
add a GraphPropertyRef case and Assert()/throw error if it is seen. We
should add a comment there to expect GraphPropertyRef there once we
start supporting nested subqueries in GRAPH_TABLE. Please note, I
don't expect ChangeVarNodes to handle GraphPropertyRef since there is
no rtindex in GraphPropertyRef.

I didn't find any other place where we need to handle
GraphPropertyRef. But there are just many places where we use Vars and
varlevelsup. So there is a non-zero possibility that I may have missed
a few. I wish we could have differentiated between Var in the planner,
optimizer code and ColumnRef in the parser, transformer and rewriter
code. That would have made it easier to find the places where we need
to handle GraphPropertyRef. But the ship has sailed long back.

Here are some comments on v6 patches.

@@ -769,9 +775,17 @@ check_agg_arguments_walker(Node *node,
 {
  if (node == NULL)
  return false;
- if (IsA(node, Var))
+ if (IsA(node, Var) || IsA(node, GraphPropertyRef))
  {

I would rather add a separate GraphPropertyRef case instead of
combining it with Var. When we start supporting nested subqueries in
GRAPH_TABLE, we will need to add levelsup to GraphPropertyRef and
handle it appropriately. I am leaning towards doing that now and
setting it to 0. Then use here instead of hardcoded 0. Also pass it on
to the Vars in the expression that replaces the GraphPropertyRef by
calling ChangeVarNodes() with sublevelsup = GraphPropertyRef::levelsup
instead of 0. Once subquery support is added, we will set
GraphPropertyRef::levelsup to the appropriate value and the
ChangeVarNodes() will propagate it to the Vars in the replacement
expression. If we go about it this way, we should add an Assert in
replace_property_refs() to make sure that GraphPropertyRef::levelsup
== 0. But I am also fine with implicitly treating GraphPropertyRef as
level 0 with a comment that we should pass on the levelsup to the Vars
in the replacement expression once we support nested subqueries in
GRAPH_TABLE.

@@ -1845,6 +1847,10 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
  case EXPR_KIND_CYCLE_MARK:
  /* okay */
  break;
+ case EXPR_KIND_GRAPH_TABLE_COLUMNS:
+ case EXPR_KIND_GRAPH_TABLE_WHERE:
+ err = _("subqueries within GRAPH_TABLE reference are not supported");

I would use "cannot use subquery in GRAPH_TABLE reference" to be
consistent with the other error messages in this function.

@@ -1039,6 +1039,40 @@ replace_property_refs_mutator(Node *node,
struct replace_property_refs_context *

  return (Node *) newvar;
  }
+ else if (IsA(node, Aggref))
+ {
+ Aggref   *aggref;
+
+ /* Copy the Aggref node and mutate its sub-structure */
+ aggref = (Aggref *) expression_tree_mutator(node,
+ replace_property_refs_mutator,
+ context);
+
+ /*
+ * An aggregate is allowed in a graph table expression, but only if
+ * it's an outer aggregate.  Since it will be in a subquery after the
+ * rewrite, we have to increase the level by one.
+ */
+ Assert(aggref->agglevelsup > 0);
+ aggref->agglevelsup++;
+
+ return (Node *) aggref;
+ }
+ else if (IsA(node, GroupingFunc))
+ {
+ GroupingFunc *grp;
+
+ /* Copy the GroupingFunc node and mutate its sub-structure */
+ grp = (GroupingFunc *) expression_tree_mutator(node,
+   replace_property_refs_mutator,
+   context);
+
+ /* Like Aggref, this should be an outer-level reference */
+ Assert(grp->agglevelsup > 0);
+ grp->agglevelsup++;
+
+ return (Node *) grp;
+ }

I think we could make this function lean by letting
IncrementVarSublevelsUp() do the levelsup adjustment before the
GraphPropertyRef is replaced. It will require traversing the
expression tree twice, but it will eliminate the possibility of
replace_property_refs_mutator() missing a node type that has levelsup.

@@ -84,6 +84,8 @@ typedef enum ParseExprKind
  EXPR_KIND_GENERATED_COLUMN, /* generation expression for a column */
  EXPR_KIND_CYCLE_MARK, /* cycle mark value */
  EXPR_KIND_PROPGRAPH_PROPERTY, /* derived property expression */
+ EXPR_KIND_GRAPH_TABLE_COLUMNS, /* GRAPH_TABLE COLUMNS list item */
+ EXPR_KIND_GRAPH_TABLE_WHERE, /* WHERE in a GRAPH_TABLE pattern */

Nit. I would just write WHERE in a GRAPH_TABLE since GRAPH_TABLE is
not a pattern, but a table function.

--
Best Wishes,
Ashutosh Bapat

Hi Ashutosh,

Thanks for the review.

So what I'm thinking right now is
that this is a fundamental design error, and that we should nuke
GraphPropertyRef altogether in favor of using a Var that references
the appropriate column of the RTE_GRAPH_TABLE relation.

I spent time today on this and I think the direction is right. In what
I have running
locally, a property reference is emitted as a plain Var over the
RTE_GRAPH_TABLE relation,
so the Var handles leveling and the rest for free. GraphPropertyRef
does not go away
entirely, though. It moves onto a side list on the RTE that the
rewriter uses to resolve
each property Var back to its graph property.

I share the concern raised by Tom about bugs due to missing
GraphPropertyRef handling. However, I don't think we can use Var
instead of GraphPropertyRef. It represents a reference to a property
of all the graph elements that are bound to an element pattern in a
GRAPH_TABLE. The element pattern is not represented by a range table
and Var node does not have a field to represent an element pattern
variable. Further the same GraphPropertyRef may be rewritten as
different expressions depending on the element table it gets
associated with when generating a subquery for a given path.

You are right. A Var does not seem correct here after looking at it a
bit more. GraphPropertyRef is not a column of an existing RTE. Also,
the same reference can resolve differently in different generated path
subqueries depending on which element table that path uses. So keeping a
separate placeholder seems more natural.

this with Var will be quite complex, if not impossible. The question
is whether we can find all the places where we need to treat
GraphPropertyRef as a Var. Given that the GraphPropertyRefs are
generated during transformation and vanish after rewrite, the places
where we need to handle them are limited, as noted by Tom already.
Many of those are already covered by the patch. Going through all the
places, rather painfully, where we use levelsup for Var-like nodes,
looking at var.c, I see that most of that code is applicable to
planning, optimization and execution. The additional places which I
think we need to handle are:
a. locate_var_of_level_walker() to handle GraphPropertyRef so that the
correct error location can be reported. We have separate
locate_aggref_of_level_walker(). So first I thought that it would be
appropriate to add a separate
locate_graphpropertyref_of_level_walker() function. But we want to
report an error from a place where the distinction between Var and
GraphPropertyRef is lost. So a separate function doesn't make sense.
b. contain_vars_of_level() should not see GraphPropertyRef since the
latter lives within GRAPH_TABLE only. But the function is used during
query transformation, so it will be good to add a GraphPropertyRef
case and Assert()/throw error if it is seen. We should add a comment
there to expect GraphPropertyRef there once we start supporting nested
subqueries in GRAPH_TABLE.
c. IncrementVarSublevelsUp() will need to handle GraphPropertyRef once
we support nested subqueries in GRAPH_TABLE. For now we may want to
add a GraphPropertyRef case and Assert()/throw error if it is seen. We
should add a comment there to expect GraphPropertyRef there once we
start supporting nested subqueries in GRAPH_TABLE. Please note, I
don't expect ChangeVarNodes to handle GraphPropertyRef since there is
no rtindex in GraphPropertyRef.

Right. The places where GraphPropertyRef needs Var-like treatment seem
fairly limited. In v7, I handled the walkers that need query level
information, such as locate_var_of_level_walker() and
check_agg_arguments_walker(). I also added a GraphPropertyRef case to
contain_vars_of_level(), since that can be reached during query
transformation before rewrite removes GraphPropertyRef.

IncrementVarSublevelsUp() is different. There I added a
GraphPropertyRef case that leaves it alone and asserts
gprlevelsup == 0, since we do not yet support subqueries inside
GRAPH_TABLE. Once we do, we will need to adjust the levels there as
well.

Here are some comments on v6 patches.

@@ -769,9 +775,17 @@ check_agg_arguments_walker(Node *node,
{
if (node == NULL)
return false;
- if (IsA(node, Var))
+ if (IsA(node, Var) || IsA(node, GraphPropertyRef))
{

I would rather add a separate GraphPropertyRef case instead of
combining it with Var. When we start supporting nested subqueries in
GRAPH_TABLE, we will need to add levelsup to GraphPropertyRef and
handle it appropriately. I am leaning towards doing that now and
setting it to 0. Then use here instead of hardcoded 0.

Done in v7. I added gprlevelsup to carry the level.

Also pass it on
to the Vars in the expression that replaces the GraphPropertyRef by
calling ChangeVarNodes() with sublevelsup = GraphPropertyRef::levelsup
instead of 0. Once subquery support is added, we will set
GraphPropertyRef::levelsup to the appropriate value and the
ChangeVarNodes() will propagate it to the Vars in the replacement
expression. If we go about it this way, we should add an Assert in
replace_property_refs() to make sure that GraphPropertyRef::levelsup
== 0. But I am also fine with implicitly treating GraphPropertyRef as
level 0 with a comment that we should pass on the levelsup to the Vars
in the replacement expression once we support nested subqueries in
GRAPH_TABLE.

... replace_property_refs() now passes gprlevelsup through to
ChangeVarNodes() instead of hardcoding 0. I also added
gprlevelsup == 0 asserts in both IncrementVarSublevelsUp_walker() and
replace_property_refs_mutator(), since subqueries inside GRAPH_TABLE
are not supported yet.

@@ -1845,6 +1847,10 @@ transformSubLink(ParseState *pstate, SubLink

*sublink)

case EXPR_KIND_CYCLE_MARK:
/* okay */
break;
+ case EXPR_KIND_GRAPH_TABLE_COLUMNS:
+ case EXPR_KIND_GRAPH_TABLE_WHERE:
+ err = _("subqueries within GRAPH_TABLE reference are not supported");

I would use "cannot use subquery in GRAPH_TABLE reference" to be
consistent with the other error messages in this function.

Done in v7.

I think we could make this function lean by letting
IncrementVarSublevelsUp() do the levelsup adjustment before the
GraphPropertyRef is replaced. It will require traversing the
expression tree twice, but it will eliminate the possibility of
replace_property_refs_mutator() missing a node type that has levelsup.

Done in v7. I removed the special Var-levelsup handling from
replace_property_refs_mutator() and let IncrementVarSublevelsUp()
adjust the copied expression in generate_query_for_graph_path() before
GraphPropertyRef is replaced.

@@ -84,6 +84,8 @@ typedef enum ParseExprKind
EXPR_KIND_GENERATED_COLUMN, /* generation expression for a column */
EXPR_KIND_CYCLE_MARK, /* cycle mark value */
EXPR_KIND_PROPGRAPH_PROPERTY, /* derived property expression */
+ EXPR_KIND_GRAPH_TABLE_COLUMNS, /* GRAPH_TABLE COLUMNS list item */
+ EXPR_KIND_GRAPH_TABLE_WHERE, /* WHERE in a GRAPH_TABLE pattern */

Nit. I would just write WHERE in a GRAPH_TABLE since GRAPH_TABLE is
not a pattern, but a table function.

Done in v7.

Also, with these changes, I did see what it would take to support
subqueries and
I have something that I can show in a different thread. I think the
aub-query support
will be a v20 item, while v7- should be targeted for v19.

--
Sami Imseih
Amazon Web Services (AWS)

Sorry for the spam.

I made one small change here. I don't think gprlevelsup is a good
name for the new leveling field. It should be called varlevelsup,
because in this patch it is really just carrying query levelsup in
the same sense as Var.varlevelsup.

Using gprlevelsup here suggests GRAPH_TABLE leveling, which
is a different thing. If we later support nested subqueries inside
GRAPH_TABLE, we will need a separate level to correlate an inner
GRAPH_TABLE with an outer one.

--
Sami Imseih
Amazon Web Services (AWS)

On Tue, Aug 25, 2026 at 11:57 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

So what I'm thinking right now is
that this is a fundamental design error, and that we should nuke
GraphPropertyRef altogether in favor of using a Var that references
the appropriate column of the RTE_GRAPH_TABLE relation.

I spent time today on this and I think the direction is right. In what
I have running
locally, a property reference is emitted as a plain Var over the
RTE_GRAPH_TABLE relation,
so the Var handles leveling and the rest for free. GraphPropertyRef
does not go away
entirely, though. It moves onto a side list on the RTE that the
rewriter uses to resolve
each property Var back to its graph property.

I share the concern raised by Tom about bugs due to missing
GraphPropertyRef handling. However, I don't think we can use Var
instead of GraphPropertyRef. It represents a reference to a property
of all the graph elements that are bound to an element pattern in a
GRAPH_TABLE. The element pattern is not represented by a range table
and Var node does not have a field to represent an element pattern
variable. Further the same GraphPropertyRef may be rewritten as
different expressions depending on the element table it gets
associated with when generating a subquery for a given path. Achieving
this with Var will be quite complex, if not impossible. The question
is whether we can find all the places where we need to treat
GraphPropertyRef as a Var. Given that the GraphPropertyRefs are
generated during transformation and vanish after rewrite, the places
where we need to handle them are limited, as noted by Tom already.
Many of those are already covered by the patch. Going through all the
places, rather painfully, where we use levelsup for Var-like nodes,
looking at var.c, I see that most of that code is applicable to
planning, optimization and execution. The additional places which I
think we need to handle are:
a. locate_var_of_level_walker() to handle GraphPropertyRef so that the
correct error location can be reported. We have separate
locate_aggref_of_level_walker(). So first I thought that it would be
appropriate to add a separate
locate_graphpropertyref_of_level_walker() function. But we want to
report an error from a place where the distinction between Var and
GraphPropertyRef is lost. So a separate function doesn't make sense.
b. contain_vars_of_level() should not see GraphPropertyRef since the
latter lives within GRAPH_TABLE only. But the function is used during
query transformation, so it will be good to add a GraphPropertyRef
case and Assert()/throw error if it is seen. We should add a comment
there to expect GraphPropertyRef there once we start supporting nested
subqueries in GRAPH_TABLE.
c. IncrementVarSublevelsUp() will need to handle GraphPropertyRef once
we support nested subqueries in GRAPH_TABLE. For now we may want to
add a GraphPropertyRef case and Assert()/throw error if it is seen. We
should add a comment there to expect GraphPropertyRef there once we
start supporting nested subqueries in GRAPH_TABLE. Please note, I
don't expect ChangeVarNodes to handle GraphPropertyRef since there is
no rtindex in GraphPropertyRef.

I didn't find any other place where we need to handle
GraphPropertyRef. But there are just many places where we use Vars and
varlevelsup. So there is a non-zero possibility that I may have missed
a few. I wish we could have differentiated between Var in the planner,
optimizer code and ColumnRef in the parser, transformer and rewriter
code. That would have made it easier to find the places where we need
to handle GraphPropertyRef. But the ship has sailed long back.

[RMT hat]

RMT is wondering if the ongoing work on this is an enhancement for 20
or proposed work for 19. Does Ashutosh's reply satisfy Tom and Sami's
concerns with the design. Is there work here that is still being
proposed for v19 and is there consensus on it?

- Melanie

Melanie Plageman <melanieplageman@gmail.com> writes:

RMT is wondering if the ongoing work on this is an enhancement for 20
or proposed work for 19. Does Ashutosh's reply satisfy Tom and Sami's
concerns with the design. Is there work here that is still being
proposed for v19 and is there consensus on it?

TBH, I stopped paying attention to this thread after I saw Andres'
concerns about PGQ [1]/messages/by-id/dqa5mstx5mna3i7s23pdwl4m6bek7gqsgfccef44wjpswizufi@3aa6vzri3cat. I figured there was no chance of PGQ not
getting reverted for 19, so it wasn't urgent to fix this aspect
of it. Hence, I don't have an immediate answer on whether this is
a good-enough fix.

I will say that I do not like GraphPropertyRef one bit and that the
best way forward if we're not building an emergency band-aid for 19
is to get rid of it. "We have to deal with this everyplace we deal
with Vars" is not an appetizing prospect. I did realize after the
initial discussion that we already have a binding for Vars pointing at
the RTE_GRAPH_TABLE RTE, so we can't commandeer that representation
for what GraphPropertyRef represents. But there's more than one way
to skin that cat. The next idea I'd propose is to create a second RTE
of a different RTEKind, and then replace GraphPropertyRefs with Vars
pointing at that one. This would be kind of analogous to RTE_GROUP
Vars, or at least that's a handy precedent.

regards, tom lane

[1]: /messages/by-id/dqa5mstx5mna3i7s23pdwl4m6bek7gqsgfccef44wjpswizufi@3aa6vzri3cat

Hi,

Due to b1f106c80cbe, I withdrew this CF entry for this:
https://commitfest.postgresql.org/patch/7144/

--
Sami Imseih
Amazon Web Services (AWS)