From f8d005d9af6355f9788994360c177ae68c774e7b Mon Sep 17 00:00:00 2001 From: Andrey Rachitskiy Date: Mon, 26 Oct 2026 14:00:07 +0500 Subject: [PATCH v3] Don't replace Var-free subexpressions with grouped Vars Since f5050f795ae, substitute_grouped_columns_mutator() checks every subexpression against the GROUP BY items with equal() before accepting Consts and Params. A constant grouping item is then marked nullable by grouping sets. The same match also replaces any other Const that happens to be equal. Such a Const may sit where Vars are not allowed, such as the elemexpr of an ArrayCoerceExpr. The isExplicit flag of a length coercion is a boolean Const. For example: SELECT ARRAY[]::varchar(500)[] GROUP BY CAST('true' AS boolean); crashed the planner, because constant-folding evaluated a Var without a tuple slot. A grouping expression that contains no Vars of the current query level is not a reference to any input column. An equal expression elsewhere is not a reference to that grouping column. Replace only the targetlist entry that GROUP BY identified, and leave these items out of the equal() search. This covers constants, Params and Var-free expressions such as 1+2. A Const or Param can no longer match a grouping item, so accept them at the top of the mutator again, as before f5050f795ae. Author: Andrey Rachitskiy Co-authored-by: Matheus Alcantara Reviewed-by: Matheus Alcantara Discussion: https://www.postgresql.org/message-id/19752-928339cd23b58a33@postgresql.org --- src/backend/parser/parse_agg.c | 74 +++++++++++++++++++--- src/test/regress/expected/aggregates.out | 13 ++++ src/test/regress/expected/groupingsets.out | 44 +++++++++++++ src/test/regress/sql/aggregates.sql | 4 ++ src/test/regress/sql/groupingsets.sql | 18 ++++++ 5 files changed, 143 insertions(+), 10 deletions(-) diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c index a5424c9d24c..af54d6d576a 100644 --- a/src/backend/parser/parse_agg.c +++ b/src/backend/parser/parse_agg.c @@ -53,6 +53,8 @@ typedef struct List *groupClauseSubLevels; /* list of lists of TargetEntry */ List *gset_common; /* integer list of sortgrouprefs */ bool have_non_var_grouping; + Bitmapset *varfree_grouping; /* groupClauses items with no level-zero + * Vars, by position */ List **func_grouped_rels; int sublevels_up; bool in_agg_direct_args; @@ -69,6 +71,7 @@ static Node *substitute_grouped_columns(Node *node, ParseState *pstate, Query *q List *groupClauses, List *groupClauseCommonVars, List *gset_common, bool have_non_var_grouping, + Bitmapset *varfree_grouping, List **func_grouped_rels); static Node *substitute_grouped_columns_mutator(Node *node, substitute_grouped_columns_context *context); @@ -1142,8 +1145,10 @@ parseCheckAggregates(ParseState *pstate, Query *qry) List *groupClauses = NIL; List *groupClauseCommonVars = NIL; bool have_non_var_grouping; + Bitmapset *varfree_grouping = NULL; List *func_grouped_rels = NIL; ListCell *l; + int attnum; bool hasJoinRTEs; bool hasSelfRefRTEs; Node *clause; @@ -1267,12 +1272,22 @@ parseCheckAggregates(ParseState *pstate, Query *qry) * Track Vars that are included in all grouping sets separately in * groupClauseCommonVars, since these are the only ones we can use to * check for functional dependencies. + * + * Also remember which grouping expressions contain no Vars of this query + * level (constants, Params, outer references and the like). Those can + * only be referenced through the targetlist entry that GROUP BY + * identified; see substitute_grouped_columns_mutator. */ have_non_var_grouping = false; + attnum = 0; foreach(l, groupClauses) { TargetEntry *tle = lfirst(l); + attnum++; + if (!contain_vars_of_level((Node *) tle->expr, 0)) + varfree_grouping = bms_add_member(varfree_grouping, attnum); + if (!IsA(tle->expr, Var)) { have_non_var_grouping = true; @@ -1308,6 +1323,7 @@ parseCheckAggregates(ParseState *pstate, Query *qry) groupClauses, groupClauseCommonVars, gset_common, have_non_var_grouping, + varfree_grouping, &func_grouped_rels); clause = (Node *) qry->havingQual; @@ -1321,6 +1337,7 @@ parseCheckAggregates(ParseState *pstate, Query *qry) groupClauses, groupClauseCommonVars, gset_common, have_non_var_grouping, + varfree_grouping, &func_grouped_rels); /* @@ -1351,6 +1368,7 @@ substitute_grouped_columns(Node *node, ParseState *pstate, Query *qry, List *groupClauses, List *groupClauseCommonVars, List *gset_common, bool have_non_var_grouping, + Bitmapset *varfree_grouping, List **func_grouped_rels) { substitute_grouped_columns_context context; @@ -1363,6 +1381,7 @@ substitute_grouped_columns(Node *node, ParseState *pstate, Query *qry, context.groupClauseSubLevels = NIL; context.gset_common = gset_common; context.have_non_var_grouping = have_non_var_grouping; + context.varfree_grouping = varfree_grouping; context.func_grouped_rels = func_grouped_rels; context.sublevels_up = 0; context.in_agg_direct_args = false; @@ -1377,6 +1396,9 @@ substitute_grouped_columns_mutator(Node *node, if (node == NULL) return NULL; + if (IsA(node, Const) || + IsA(node, Param)) + return node; /* constants are always acceptable */ if (IsA(node, Aggref)) { @@ -1424,6 +1446,42 @@ substitute_grouped_columns_mutator(Node *node, return node; } + /* + * A grouping expression that contains no Vars of this query level, such + * as a constant, refers only to the targetlist entry that GROUP BY + * identified. Other occurrences of an equal expression are not + * references to that grouping column, so they are not candidates for the + * equal() matching below. Replacing them would be wrong anyway: they can + * appear in places where Vars are not allowed, such as the elemexpr of an + * ArrayCoerceExpr. + */ + if (IsA(node, TargetEntry) && + ((TargetEntry *) node)->ressortgroupref != 0 && + context->have_non_var_grouping && + context->sublevels_up == 0) + { + TargetEntry *tle = (TargetEntry *) node; + int attnum = 0; + + foreach(gl, context->groupClauses) + { + TargetEntry *grouptle = (TargetEntry *) lfirst(gl); + + attnum++; + if (grouptle->ressortgroupref != tle->ressortgroupref) + continue; + if (bms_is_member(attnum, context->varfree_grouping)) + { + tle = flatCopyTargetEntry(tle); + tle->expr = (Expr *) buildGroupedVar(attnum, + tle->ressortgroupref, + context); + return (Node *) tle; + } + break; + } + } + /* * If we have any GROUP BY items that are not simple Vars, check to see if * subexpression as a whole matches any GROUP BY item. We need to do this @@ -1448,6 +1506,11 @@ substitute_grouped_columns_mutator(Node *node, TargetEntry *tle = (TargetEntry *) lfirst(gl); attnum++; + + /* Var-free items are matched only by targetlist entry, above */ + if (bms_is_member(attnum, context->varfree_grouping)) + continue; + if (equal(node, tle->expr)) { /* acceptable, replace it with a GROUP Var */ @@ -1458,16 +1521,6 @@ substitute_grouped_columns_mutator(Node *node, } } - /* - * Constants are always acceptable. We have to do this after we checked - * the subexpression as a whole for a match, because it is possible that - * we have GROUP BY items that are constants, and the constants would - * become not so constant after the grouping step. - */ - if (IsA(node, Const) || - IsA(node, Param)) - return node; - /* * If we have an ungrouped Var of the original query level, we have a * failure. Vars below the original query level are not a problem, and @@ -1624,6 +1677,7 @@ finalize_grouping_exprs(Node *node, ParseState *pstate, Query *qry, context.groupClauseSubLevels = NIL; context.gset_common = NIL; context.have_non_var_grouping = have_non_var_grouping; + context.varfree_grouping = NULL; context.func_grouped_rels = NULL; context.sublevels_up = 0; context.in_agg_direct_args = false; diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out index 7d07619956f..6c0209800e9 100644 --- a/src/test/regress/expected/aggregates.out +++ b/src/test/regress/expected/aggregates.out @@ -3958,3 +3958,16 @@ drop table agg_hash_1; drop table agg_hash_2; drop table agg_hash_3; drop table agg_hash_4; + +-- check that a grouped constant does not match an equal Const elsewhere +select array[]::varchar(500)[] as a group by cast('true' as boolean); + a +---- + {} +(1 row) + +select array[]::varchar(500)[] as a group by 504::int4; + a +---- + {} +(1 row) diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out index 08d5ce3156d..0707efa5c37 100644 --- a/src/test/regress/expected/groupingsets.out +++ b/src/test/regress/expected/groupingsets.out @@ -2667,6 +2667,50 @@ select 1 as one group by rollup(one) order by one nulls first; 1 (2 rows) +-- A grouping expression containing no Vars of the query level refers only +-- to the targetlist entry GROUP BY identified. An equal expression elsewhere +-- must not be replaced by a grouped Var. +select (select array[]::varchar(500)[]) as a +group by rollup(cast('true' as boolean)); + a +---- + {} + {} +(2 rows) + +explain (verbose, costs off) +select 1 as a, 1 as b, 1+2 as c, (1+2)*10 as d +group by rollup(a, c); + QUERY PLAN +--------------------------- + MixedAggregate + Output: (1), 1, (3), 30 + Hash Key: 1, 3 + Hash Key: 1 + Group Key: () + -> Result + Output: 1, 3 +(7 rows) + +select 1 as a, 1 as b, 1+2 as c, (1+2)*10 as d +group by rollup(a, c) +order by a nulls first, c nulls first; + a | b | c | d +---+---+---+---- + | 1 | | 30 + 1 | 1 | | 30 + 1 | 1 | 3 | 30 +(3 rows) + +-- likewise for an outer reference used as a grouping expression +select array(select o.a as k from (values (1)) as i (x) + group by rollup(o.a, x+0) order by 1 nulls first) +from (values (10)) as o (a); + array +-------------- + {NULL,10,10} +(1 row) + explain (costs off) select a, b, row_number() over (order by a, b nulls first) from (values (1, 1), (2, 2)) as t (a, b) where a = b diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql index 91f8342166f..3f3d3cb7604 100644 --- a/src/test/regress/sql/aggregates.sql +++ b/src/test/regress/sql/aggregates.sql @@ -1786,3 +1786,7 @@ drop table agg_hash_1; drop table agg_hash_2; drop table agg_hash_3; drop table agg_hash_4; + +-- check that a grouped constant does not match an equal Const elsewhere +select array[]::varchar(500)[] as a group by cast('true' as boolean); +select array[]::varchar(500)[] as a group by 504::int4; diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql index c5d4ed2eb57..bc3be53be5d 100644 --- a/src/test/regress/sql/groupingsets.sql +++ b/src/test/regress/sql/groupingsets.sql @@ -771,6 +771,24 @@ explain (costs off) select 1 as one group by rollup(one) order by one nulls first; select 1 as one group by rollup(one) order by one nulls first; +-- A grouping expression containing no Vars of the query level refers only +-- to the targetlist entry GROUP BY identified. An equal expression elsewhere +-- must not be replaced by a grouped Var. +select (select array[]::varchar(500)[]) as a +group by rollup(cast('true' as boolean)); + +explain (verbose, costs off) +select 1 as a, 1 as b, 1+2 as c, (1+2)*10 as d +group by rollup(a, c); +select 1 as a, 1 as b, 1+2 as c, (1+2)*10 as d +group by rollup(a, c) +order by a nulls first, c nulls first; + +-- likewise for an outer reference used as a grouping expression +select array(select o.a as k from (values (1)) as i (x) + group by rollup(o.a, x+0) order by 1 nulls first) +from (values (10)) as o (a); + explain (costs off) select a, b, row_number() over (order by a, b nulls first) from (values (1, 1), (2, 2)) as t (a, b) where a = b -- 2.53.0