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 <[email protected]> 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
