Re: Disallow outer-level and WHERE-clause aggregates in GRAPH_TABLE

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(at)gmail(dot)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