Sami Imseih <[email protected]> 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
From 4ba70c021a02f864f660c09bd89591c4292db691 Mon Sep 17 00:00:00 2001
From: Tom Lane <[email protected]>
Date: Sat, 15 Aug 2026 15:08:13 -0400
Subject: [PATCH v4] Rework GRAPH_TABLE aggregate/window/SRF rejection using
ParseExprKind.
Commit f58567105 disallowed aggregates, window functions, and
set-returning functions in a GRAPH_TABLE COLUMNS list by inspecting
the parse state after transforming the list. This way is not capable
of reporting a parse location for the misplaced construct; worse, the
patch missed checking for aggregates etc. in GRAPH_TABLE WHERE.
Instead give the COLUMNS list and the graph pattern WHERE clause their
own ParseExprKind values and enforce the restriction within the
parser's transformation functions check_agglevels_and_constraints(),
transformWindowFuncCall(), and check_srf_call_placement(). (This
reverts the code changes of f58567105, though we keep the test cases
and add some more.) This is more consistent with how the parser
implements other misplaced-construct checks, and it allows delivery
of better error messages.
The parsing check rejects only aggregates having level zero.
This is intentional: an outer-level aggregate is effectively a
constant within the subquery containing GRAPH_TABLE, so there's no
reason not to allow it. The case did not work before, but that seems
to be only because replace_property_refs_mutator neglected its duty to
mutate agglevelsup in the same way as varlevelsup.
Note that this changes the SQLSTATE for a rejected aggregate from
ERRCODE_FEATURE_NOT_SUPPORTED to ERRCODE_GROUPING_ERROR, consistent
with how misplaced aggregates are rejected in other cases.
Author: Sami Imseih <[email protected]>
Reviewed-by: Tom Lane <[email protected]>
Discussion: https://postgr.es/m/CAA5RZ0tvdYODLQvYwVzAxUPe5=e3vse8zy7tvrqq+syvgkp...@mail.gmail.com
Backpatch-through: 19
---
src/backend/parser/parse_agg.c | 10 +++++
src/backend/parser/parse_clause.c | 32 +--------------
src/backend/parser/parse_expr.c | 8 ++++
src/backend/parser/parse_func.c | 4 ++
src/backend/parser/parse_graphtable.c | 6 +--
src/backend/rewrite/rewriteGraphTable.c | 34 ++++++++++++++++
src/include/parser/parse_node.h | 2 +
src/test/regress/expected/graph_table.out | 49 +++++++++++++++++++++--
src/test/regress/sql/graph_table.sql | 9 ++++-
9 files changed, 115 insertions(+), 39 deletions(-)
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 754a20507d0..a80d11215e2 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -600,6 +600,12 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
break;
+ case EXPR_KIND_GRAPH_TABLE_COLUMNS:
+ case EXPR_KIND_GRAPH_TABLE_WHERE:
+ errkind = true;
+
+ break;
+
/*
* There is intentionally no default: case here, so that the
* compiler will warn if we add a new ParseExprKind without
@@ -1045,6 +1051,10 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
case EXPR_KIND_FOR_PORTION:
err = _("window functions are not allowed in FOR PORTION OF expressions");
break;
+ case EXPR_KIND_GRAPH_TABLE_COLUMNS:
+ case EXPR_KIND_GRAPH_TABLE_WHERE:
+ errkind = true;
+ break;
/*
* There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 68b525ffdcc..b1aa13eeb1d 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -946,9 +946,6 @@ transformRangeGraphTable(ParseState *pstate, RangeGraphTable *rgt)
ListCell *lc;
int resno = 0;
bool saved_hasSublinks;
- bool saved_hasAggs;
- bool saved_hasWindowFuncs;
- bool saved_hasTargetSRFs;
rel = parserOpenPropGraph(pstate, rgt->graph_name, AccessShareLock);
@@ -970,13 +967,6 @@ transformRangeGraphTable(ParseState *pstate, RangeGraphTable *rgt)
saved_hasSublinks = pstate->p_hasSubLinks;
pstate->p_hasSubLinks = false;
- saved_hasAggs = pstate->p_hasAggs;
- pstate->p_hasAggs = false;
- saved_hasWindowFuncs = pstate->p_hasWindowFuncs;
- pstate->p_hasWindowFuncs = false;
- saved_hasTargetSRFs = pstate->p_hasTargetSRFs;
- pstate->p_hasTargetSRFs = false;
-
gp = transformGraphPattern(pstate, rgt->graph_pattern);
/*
@@ -991,7 +981,7 @@ transformRangeGraphTable(ParseState *pstate, RangeGraphTable *rgt)
TargetEntry *te;
char *colname;
- colexpr = transformExpr(pstate, rt->val, EXPR_KIND_SELECT_TARGET);
+ colexpr = transformExpr(pstate, rt->val, EXPR_KIND_GRAPH_TABLE_COLUMNS);
if (rt->name)
colname = rt->name;
@@ -1041,26 +1031,6 @@ transformRangeGraphTable(ParseState *pstate, RangeGraphTable *rgt)
errmsg("subqueries within GRAPH_TABLE reference are not supported")));
pstate->p_hasSubLinks = saved_hasSublinks;
- /*
- * GRAPH_TABLE cannot yet evaluate aggregate, window, or set-returning
- * functions in its COLUMNS list, so prohibit them for now.
- */
- if (pstate->p_hasAggs)
- ereport(ERROR,
- errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("aggregate functions in GRAPH_TABLE COLUMNS are not supported"));
- if (pstate->p_hasWindowFuncs)
- ereport(ERROR,
- errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("window functions in GRAPH_TABLE COLUMNS are not supported"));
- if (pstate->p_hasTargetSRFs)
- ereport(ERROR,
- errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("set-returning functions in GRAPH_TABLE COLUMNS are not supported"));
- pstate->p_hasAggs = saved_hasAggs;
- pstate->p_hasWindowFuncs = saved_hasWindowFuncs;
- pstate->p_hasTargetSRFs = saved_hasTargetSRFs;
-
return addRangeTableEntryForGraphTable(pstate, graphid, castNode(GraphPattern, gp), columns, colnames, rgt->alias, false, true);
}
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 30c889f505f..f1c6aa236cf 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -579,6 +579,8 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
case EXPR_KIND_GENERATED_COLUMN:
case EXPR_KIND_CYCLE_MARK:
case EXPR_KIND_PROPGRAPH_PROPERTY:
+ case EXPR_KIND_GRAPH_TABLE_COLUMNS:
+ case EXPR_KIND_GRAPH_TABLE_WHERE:
/* okay */
break;
@@ -1843,6 +1845,8 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
case EXPR_KIND_VALUES:
case EXPR_KIND_VALUES_SINGLE:
case EXPR_KIND_CYCLE_MARK:
+ case EXPR_KIND_GRAPH_TABLE_COLUMNS:
+ case EXPR_KIND_GRAPH_TABLE_WHERE:
/* okay */
break;
case EXPR_KIND_CHECK_CONSTRAINT:
@@ -3255,6 +3259,10 @@ ParseExprKindName(ParseExprKind exprKind)
return "property definition expression";
case EXPR_KIND_FOR_PORTION:
return "FOR PORTION OF";
+ case EXPR_KIND_GRAPH_TABLE_COLUMNS:
+ return "GRAPH_TABLE COLUMNS";
+ case EXPR_KIND_GRAPH_TABLE_WHERE:
+ return "GRAPH_TABLE WHERE";
/*
* There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index c87804f5d41..0c72b95e4a5 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2848,6 +2848,10 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location)
case EXPR_KIND_FOR_PORTION:
err = _("set-returning functions are not allowed in FOR PORTION OF expressions");
break;
+ case EXPR_KIND_GRAPH_TABLE_COLUMNS:
+ case EXPR_KIND_GRAPH_TABLE_WHERE:
+ errkind = true;
+ break;
/*
* There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_graphtable.c b/src/backend/parser/parse_graphtable.c
index 73fbfb541f7..e323376f0ea 100644
--- a/src/backend/parser/parse_graphtable.c
+++ b/src/backend/parser/parse_graphtable.c
@@ -92,7 +92,7 @@ transformGraphTablePropertyRef(ParseState *pstate, ColumnRef *cref)
if (IsA(field1, A_Star) || IsA(field2, A_Star))
{
- if (pstate->p_expr_kind == EXPR_KIND_SELECT_TARGET)
+ if (pstate->p_expr_kind == EXPR_KIND_GRAPH_TABLE_COLUMNS)
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("\"*\" is not supported here"),
@@ -251,7 +251,7 @@ transformGraphElementPattern(ParseState *pstate, GraphElementPattern *gep)
gep->labelexpr = transformLabelExpr(gpstate, gep->labelexpr);
- gep->whereClause = transformExpr(pstate, gep->whereClause, EXPR_KIND_WHERE);
+ gep->whereClause = transformExpr(pstate, gep->whereClause, EXPR_KIND_GRAPH_TABLE_WHERE);
/*
* Assign collations here for the reason mentioned in the prologue of
@@ -387,7 +387,7 @@ transformGraphPattern(ParseState *pstate, GraphPattern *graph_pattern)
transformPathPatternList(pstate, graph_pattern->path_pattern_list));
graph_pattern->path_pattern_list = path_pattern_list;
- graph_pattern->whereClause = transformExpr(pstate, graph_pattern->whereClause, EXPR_KIND_WHERE);
+ graph_pattern->whereClause = transformExpr(pstate, graph_pattern->whereClause, EXPR_KIND_GRAPH_TABLE_WHERE);
assign_expr_collations(pstate, graph_pattern->whereClause);
return (Node *) graph_pattern;
diff --git a/src/backend/rewrite/rewriteGraphTable.c b/src/backend/rewrite/rewriteGraphTable.c
index 0eaf28b3de5..9b5495ffaa2 100644
--- a/src/backend/rewrite/rewriteGraphTable.c
+++ b/src/backend/rewrite/rewriteGraphTable.c
@@ -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;
+ }
else if (IsA(node, GraphPropertyRef))
{
GraphPropertyRef *gpr = (GraphPropertyRef *) node;
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index f7f4ba6c2a8..2b8ac813554 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -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 */
} ParseExprKind;
diff --git a/src/test/regress/expected/graph_table.out b/src/test/regress/expected/graph_table.out
index cde3114ebf4..04b58a625ad 100644
--- a/src/test/regress/expected/graph_table.out
+++ b/src/test/regress/expected/graph_table.out
@@ -471,13 +471,54 @@ SELECT * FROM GRAPH_TABLE (myshop MATCH (c IS customers WHERE c.* IS NOT NULL)-[
ERROR: "*" not allowed here
LINE 1: ...M GRAPH_TABLE (myshop MATCH (c IS customers WHERE c.* IS NOT...
^
--- aggregate, window, and set-returning functions are not supported in COLUMNS
+-- aggregate, grouping, window, and set-returning functions are not allowed
+-- in the COLUMNS list or the graph pattern WHERE, except for outer-level aggs
SELECT * FROM GRAPH_TABLE (myshop MATCH (c IS customers) COLUMNS (count(*) AS num));
-ERROR: aggregate functions in GRAPH_TABLE COLUMNS are not supported
+ERROR: aggregate functions are not allowed in GRAPH_TABLE COLUMNS
+LINE 1: ...APH_TABLE (myshop MATCH (c IS customers) COLUMNS (count(*) A...
+ ^
+SELECT * FROM GRAPH_TABLE (myshop MATCH (c IS customers) COLUMNS (GROUPING(c.customer_id) AS g));
+ERROR: grouping operations are not allowed in GRAPH_TABLE COLUMNS
+LINE 1: ...APH_TABLE (myshop MATCH (c IS customers) COLUMNS (GROUPING(c...
+ ^
SELECT * FROM GRAPH_TABLE (myshop MATCH (c IS customers) COLUMNS (row_number() OVER () AS rn));
-ERROR: window functions in GRAPH_TABLE COLUMNS are not supported
+ERROR: window functions are not allowed in GRAPH_TABLE COLUMNS
+LINE 1: ...APH_TABLE (myshop MATCH (c IS customers) COLUMNS (row_number...
+ ^
SELECT * FROM GRAPH_TABLE (myshop MATCH (c IS customers) COLUMNS (generate_series(1, 2) AS gs));
-ERROR: set-returning functions in GRAPH_TABLE COLUMNS are not supported
+ERROR: set-returning functions are not allowed in GRAPH_TABLE COLUMNS
+LINE 1: ...APH_TABLE (myshop MATCH (c IS customers) COLUMNS (generate_s...
+ ^
+SELECT * FROM GRAPH_TABLE (myshop MATCH (c IS customers WHERE count(c.customer_id) > 0) COLUMNS (c.name AS nm));
+ERROR: aggregate functions are not allowed in GRAPH_TABLE WHERE
+LINE 1: ...M GRAPH_TABLE (myshop MATCH (c IS customers WHERE count(c.cu...
+ ^
+SELECT EXISTS(SELECT num FROM GRAPH_TABLE (myshop MATCH (c IS customers) COLUMNS (count(o.customer_id) AS num)) t) FROM customers o;
+ exists
+--------
+ t
+(1 row)
+
+SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c IS customers) WHERE count(o.customer_id) > 0 COLUMNS (c.name AS nm)) t) FROM customers o;
+ exists
+--------
+ t
+(1 row)
+
+SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c IS customers WHERE count(o.customer_id) > 0) COLUMNS (c.name AS nm)) t) FROM customers o;
+ exists
+--------
+ t
+(1 row)
+
+SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c IS customers WHERE GROUPING(o.customer_id) = 1) COLUMNS (c.name AS nm)) t) FROM customers o GROUP BY customer_id;
+ exists
+--------
+ f
+ f
+ f
+(3 rows)
+
-- consecutive element patterns with same kind
SELECT * FROM GRAPH_TABLE (g1 MATCH ()() COLUMNS (1 as one));
ERROR: adjacent vertex patterns are not supported
diff --git a/src/test/regress/sql/graph_table.sql b/src/test/regress/sql/graph_table.sql
index 7a4189833d8..8a1173aba92 100644
--- a/src/test/regress/sql/graph_table.sql
+++ b/src/test/regress/sql/graph_table.sql
@@ -306,10 +306,17 @@ SELECT * FROM GRAPH_TABLE (g1 MATCH (src IS el1 | vl1)-[conn]->(dest) COLUMNS (c
SELECT * FROM GRAPH_TABLE (myshop MATCH (c IS customers WHERE c.address = 'US')-[IS customer_orders]->(o IS orders) COLUMNS (c.*));
-- star anywhere else is not allowed as a property reference
SELECT * FROM GRAPH_TABLE (myshop MATCH (c IS customers WHERE c.* IS NOT NULL)-[IS customer_orders]->(o IS orders) COLUMNS (c.name));
--- aggregate, window, and set-returning functions are not supported in COLUMNS
+-- aggregate, grouping, window, and set-returning functions are not allowed
+-- in the COLUMNS list or the graph pattern WHERE, except for outer-level aggs
SELECT * FROM GRAPH_TABLE (myshop MATCH (c IS customers) COLUMNS (count(*) AS num));
+SELECT * FROM GRAPH_TABLE (myshop MATCH (c IS customers) COLUMNS (GROUPING(c.customer_id) AS g));
SELECT * FROM GRAPH_TABLE (myshop MATCH (c IS customers) COLUMNS (row_number() OVER () AS rn));
SELECT * FROM GRAPH_TABLE (myshop MATCH (c IS customers) COLUMNS (generate_series(1, 2) AS gs));
+SELECT * FROM GRAPH_TABLE (myshop MATCH (c IS customers WHERE count(c.customer_id) > 0) COLUMNS (c.name AS nm));
+SELECT EXISTS(SELECT num FROM GRAPH_TABLE (myshop MATCH (c IS customers) COLUMNS (count(o.customer_id) AS num)) t) FROM customers o;
+SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c IS customers) WHERE count(o.customer_id) > 0 COLUMNS (c.name AS nm)) t) FROM customers o;
+SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c IS customers WHERE count(o.customer_id) > 0) COLUMNS (c.name AS nm)) t) FROM customers o;
+SELECT EXISTS(SELECT nm FROM GRAPH_TABLE (myshop MATCH (c IS customers WHERE GROUPING(o.customer_id) = 1) COLUMNS (c.name AS nm)) t) FROM customers o GROUP BY customer_id;
-- consecutive element patterns with same kind
SELECT * FROM GRAPH_TABLE (g1 MATCH ()() COLUMNS (1 as one));
SELECT * FROM GRAPH_TABLE (g1 MATCH -> COLUMNS (1 AS one));
--
2.52.0
