Hi Ashutosh, Thanks. v3 attached
On Thu, Jun 18, 2026 at 10:32 PM Ashutosh Bapat <[email protected]> wrote: > > On Tue, Jun 16, 2026 at 3:57 PM Ewan Young <[email protected]> wrote: > > > > Hi Ashutosh, > > > > On Tue, Jun 16, 2026 at 5:51 PM Ashutosh Bapat > > <[email protected]> wrote: > > > > > > On Tue, Jun 16, 2026 at 2:41 PM Ewan Young <[email protected]> wrote: > > > > > > > > Hi, > > > > > > > > While testing SQL/PGQ I found that putting an aggregate, window > > > > function, or set-returning function in the COLUMNS list of a > > > > GRAPH_TABLE query crashes the backend. > > > > > > > > Minimal reproducer: > > > > > > > > CREATE TABLE v (id int PRIMARY KEY); > > > > INSERT INTO v VALUES (1); > > > > CREATE PROPERTY GRAPH g VERTEX TABLES (v); > > > > > > > > SELECT max(c) FROM GRAPH_TABLE (g MATCH (x IS v) COLUMNS (count(*) AS > > > > c)); > > > > On an assert-enabled build this trips > > > > > > > > TRAP: failed Assert("econtext->ecxt_aggvalues != NULL"), > > > > File: "execExprInterp.c", Line: 1969 > > > > and on a non-assert build it fails at execution with > > > > > > > > ERROR: Aggref found in non-Agg plan node > > > > A window function in COLUMNS behaves the same way; a set-returning > > > > function is silently accepted and produces nonsensical results. > > > > > > > > Root cause: transformRangeGraphTable() (parser/parse_clause.c) > > > > transforms the COLUMNS expressions with EXPR_KIND_SELECT_TARGET, which > > > > permits aggregates, window functions and SRFs. GRAPH_TABLE has no > > > > machinery to evaluate them, though: rewriteGraphTable.c copies the > > > > COLUMNS target list verbatim into a freshly built subquery whose > > > > hasAggs/hasWindowFuncs flags are never set, so the planner builds no > > > > Agg/WindowAgg node and the Aggref/WindowFunc reaches the executor. > > > > (As a side effect p_hasAggs also leaks into the enclosing query, > > > > yielding spurious "must appear in the GROUP BY clause" errors for some > > > > other COLUMNS shapes.) > > > > > > > > These constructs are not meaningful in a GRAPH_TABLE COLUMNS list, so > > > > the attached patch rejects them at parse-analysis time, the same way > > > > every other non-aggregating context does. It adds a dedicated > > > > EXPR_KIND_GRAPH_TABLE_COLUMNS and wires it into the aggregate, window > > > > and set-returning-function checks, producing errors such as > > > > > > > > ERROR: aggregate functions are not allowed in GRAPH_TABLE COLUMNS > > > > Plain column references and subqueries are unaffected (subqueries > > > > continue to be rejected as before). A regression test is added to > > > > graph_table.sql, and "make check" passes. > > > > > > Thanks for the report and the patch. > > > > > > Right now we do not support quantified element patterns like > > > (a)->{1-5}, but when we do that it's allowed to have aggregates in > > > COLUMNs e.g. count(a) or sum(a.val). So the patch is not going in the > > > right direction. Instead we should check treat pstate->p_hasAggs and > > > pstate->p_hasWindowFuncs just like pstate->hasSublinks and throw an > > > error in transformRangeGraphTable() for now. When we will support > > > quantified element patterns, we will need to check the arguments of > > > the aggregates. If the arguments are property references with higher > > > degree, we will allow the aggregates, otherwise not. > > > > Makes sense, the dedicated EXPR_KIND was overkill -- and you're right > > that we'll want to allow aggregates over higher-degree property > > references once quantified element patterns land, so a blanket > > rejection keyed off the expression kind would be in the way. > > > > v2 attached. It drops EXPR_KIND_GRAPH_TABLE_COLUMNS entirely and > > instead follows the existing p_hasSubLinks pattern in > > transformRangeGraphTable(): save and clear p_hasAggs / p_hasWindowFuncs > > around the COLUMNS transformation, then throw a "not supported" error > > if either got set. There's a comment noting the aggregate restriction > > is temporary and will need to check the aggregate arguments (degree of > > the property refs) rather than reject everything once quantified > > patterns are supported. > > We might need minor changes to the comment, but this part looks good to me. I expanded the comment to spell out the rationale you raised: the restriction is temporary, and once quantified element patterns land, aggregates over higher-degree property references (e.g. count(a)) should be allowed by inspecting the aggregate's arguments. Let me know if you had something else in mind. > > > > > I also clear and check p_hasTargetSRFs the same way, since > > set-returning functions hit the same class of failure -- the rewriter > > doesn't set hasTargetSRFs on the generated subquery either, so an SRF > > in COLUMNS reaches the executor unevaluated. Happy to drop that hunk if > > you'd rather keep this patch scoped to aggregates and window functions > > > The problem with aggregates and window functions is that there is not > enough context for performing aggregation in COLUMNs. Set returning > functions are different, they can be safely evaluated in queries that > replace the GRAPH_TABLE construct. Looking at the standard graph table > columns clause is a list of graph table column definitions, each of > which is a <value expression> which can be <collection value > expression>. So it looks like the standard doesn't prohibit SRFs in > COLUMNs clause and we are evaluating them correctly. However, I am > wondering whether GRAPH_TABLE is expected to output only one row for > every matching walk/substructure from the graph; there are GRAPH_TABLE > shapes that seem to suggest one row per matching pattern. SRFs violate > that rule. Maybe that's why they should be prohibited in COLUMNs. But > I don't think I have understood it well. Peter, can you please clarify > whether SRFs can be part of COLUMNs clause or not? On SRFs: I looked closer, so I've dropped that handling and scoped the patch to aggregates and window functions (the actual crash). > > -- > Best Wishes, > Ashutosh Bapat Regards, Ewan Young
v3-0001-Disallow-aggregates-and-window-functions-in-GRAPH.patch
Description: Binary data
