| From: | Ewan Young <kdbase(dot)hack(at)gmail(dot)com> |
|---|---|
| To: | Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Peter Eisentraut <peter(at)eisentraut(dot)org> |
| Subject: | Re: GRAPH_TABLE: aggregates/window/set-returning functions in COLUMNS crash the backend |
| Date: | 2026-06-22 08:57:00 |
| Message-ID: | CAON2xHMKEfxXx2+hT9xSv=Z1eZz5gR=t7FykRYL7Kqo-CvLwag@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Ashutosh,
Thanks. v3 attached
On Thu, Jun 18, 2026 at 10:32 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> On Tue, Jun 16, 2026 at 3:57 PM Ewan Young <kdbase(dot)hack(at)gmail(dot)com> wrote:
> >
> > Hi Ashutosh,
> >
> > On Tue, Jun 16, 2026 at 5:51 PM Ashutosh Bapat
> > <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> > >
> > > On Tue, Jun 16, 2026 at 2:41 PM Ewan Young <kdbase(dot)hack(at)gmail(dot)com> 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
| Attachment | Content-Type | Size |
|---|---|---|
| v3-0001-Disallow-aggregates-and-window-functions-in-GRAPH.patch | application/octet-stream | 5.5 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Amit Kapila | 2026-06-22 09:07:53 | Re: Proposal: Conflict log history table for Logical Replication |
| Previous Message | Dean Rasheed | 2026-06-22 08:56:54 | Re: Global temporary tables |