From 0c2497d188028534d6cce57f867abe21e45230f0 Mon Sep 17 00:00:00 2001 From: Haibo Yan Date: Fri, 9 Oct 2026 21:05:25 -0700 Subject: [PATCH] Fix grouping-set nulling of variable-free grouping expressions When flatten_group_exprs() replaces a reference to a grouping expression that can be nulled by grouping sets, and the expression is variable-free, mark_nullable_by_grouping() wraps it in a PlaceHolderVar to carry the nullingrels. However, it made a PlaceHolderVar with a new phid for each reference. PlaceHolderVars are compared by phid, so only the grouping item itself was recognized as the grouping column. Any other reference, such as one nested in another targetlist expression or in the HAVING clause, was computed as a separate input column of the grouping step, which grouping sets do not null. Such references thus yielded the non-null value of the grouping expression in rows where it should be null. For example, SELECT (1 + 1) * 10 FROM t GROUP BY ROLLUP (1 + 1); returned 20 rather than NULL for the grand-total row. This affects constant grouping expressions, stable functions such as now(), and any grouping expression that constant-folds to a variable-free expression. (Branches before v18 return wrong results for such cases too, but they lack the RTE_GROUP machinery and are not addressed here.) In v19 the problem became much easier to hit: commits e2debb64380 and 0aaf0de7fed taught eval_const_expressions() to reduce NullTest and BooleanTest using non-nullability proofs, so that, for example, with "a" declared NOT NULL, SELECT CASE WHEN a IS NOT NULL THEN 'x' ELSE 'y' END FROM t GROUP BY ROLLUP (a IS NOT NULL); now returns 'x' rather than 'y' for the grand-total row. Reducing the grouping expression itself is correct, since it is evaluated on the grouping step's input rows, where "a" cannot be null; what goes wrong is only that the references to the reduced expression are no longer recognized as the grouping column. To fix, remember the phid assigned to each such grouping expression, and use it for all PlaceHolderVars made for references to the same grouping expression, much as pullup_replace_vars_callback() does for references to the same subquery output column. All such references then match the grouping column and are nulled by the grouping step. We track the phids by RTE_GROUP column rather than by expression, since distinct grouping expressions can simplify to equal expressions and must still be nulled independently. The new PlannerInfo field will need to be moved to the end of the struct in back branches. Also, the back-patch to v18 needs its own expected regression output, because v18 does not treat references from within subqueries as references to the grouping expressions (that came with commit 415100aa62b). Backpatch-through: 18 --- src/backend/optimizer/util/var.c | 31 ++++++- src/include/nodes/pathnodes.h | 7 ++ src/test/regress/expected/groupingsets.out | 96 ++++++++++++++++++++++ src/test/regress/sql/groupingsets.sql | 52 ++++++++++++ 4 files changed, 185 insertions(+), 1 deletion(-) diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c index 337e0306752..8a55ba08886 100644 --- a/src/backend/optimizer/util/var.c +++ b/src/backend/optimizer/util/var.c @@ -1210,6 +1210,12 @@ mark_nullable_by_grouping(PlannerInfo *root, Node *newnode, Var *oldvar) * * Aggregate functions and window functions are not allowed in * grouping expressions. + * + * All references to the same grouping expression must share the + * PlaceHolderVar's phid. Otherwise, a reference other than the + * grouping item itself would not be recognized as the grouping + * column, and would be computed as a separate input column of the + * grouping step, which is not nulled by grouping sets. */ Assert(!contain_agg_clause(newnode)); Assert(!contain_window_function(newnode)); @@ -1219,12 +1225,35 @@ mark_nullable_by_grouping(PlannerInfo *root, Node *newnode, Var *oldvar) { PlaceHolderVar *newphv; Relids phrels; + int attidx = oldvar->varattno - 1; phrels = get_relids_in_jointree((Node *) root->parse->jointree, true, false); Assert(!bms_is_empty(phrels)); - newphv = make_placeholder_expr(root, (Expr *) newnode, phrels); + /* group_phids is indexed by column of the RTE_GROUP RTE */ + Assert(oldvar->varno == root->group_rtindex); + if (root->group_phids == NULL) + { + RangeTblEntry *rte = rt_fetch(root->group_rtindex, + root->parse->rtable); + + root->group_phids = palloc0_array(Index, + list_length(rte->groupexprs)); + } + + if (root->group_phids[attidx] == 0) + { + newphv = make_placeholder_expr(root, (Expr *) newnode, phrels); + root->group_phids[attidx] = newphv->phid; + } + else + { + newphv = makeNode(PlaceHolderVar); + newphv->phexpr = (Expr *) newnode; + newphv->phrels = phrels; + newphv->phid = root->group_phids[attidx]; + } /* newphv has zero phlevelsup and NULL phnullingrels; fix it */ newphv->phlevelsup = oldvar->varlevelsup; newphv->phnullingrels = bms_copy(oldvar->varnullingrels); diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index 1c6d1fe3d04..868e2ad6cb9 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -648,6 +648,13 @@ struct PlannerInfo */ int group_rtindex; + /* + * PlaceHolderVar IDs assigned to variable-free grouping expressions that + * are nullable by grouping sets, indexed by RTE_GROUP column number minus + * one, or NULL if none assigned yet. See mark_nullable_by_grouping(). + */ + Index *group_phids pg_node_attr(read_write_ignore); + /* * Information about aggregates. Filled by preprocess_aggrefs(). */ diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out index 08d5ce3156d..b59de454e4e 100644 --- a/src/test/regress/expected/groupingsets.out +++ b/src/test/regress/expected/groupingsets.out @@ -2762,4 +2762,100 @@ order by 1, 2; 2 | (6 rows) +-- test that all references to a variable-free grouping expression that is +-- nullable by grouping sets are recognized as the same grouping column +create temp table gs_nn (id int primary key, a int not null, b int); +insert into gs_nn values (1, 1, null), (2, 2, 2); +-- "a is not null" is reduced to constant true +explain (verbose, costs off) +select case when a is not null then 'has a' else 'no a' end as label, + grouping(a is not null) as g, count(*) +from gs_nn group by rollup(a is not null); + QUERY PLAN +------------------------------------------------------------------------------------------------------- + MixedAggregate + Output: CASE WHEN (true) THEN 'has a'::text ELSE 'no a'::text END, GROUPING(true), count(*), (true) + Hash Key: true + Group Key: () + -> Seq Scan on pg_temp.gs_nn + Output: true +(6 rows) + +select case when a is not null then 'has a' else 'no a' end as label, + grouping(a is not null) as g, count(*) +from gs_nn group by rollup(a is not null) order by g; + label | g | count +-------+---+------- + has a | 0 | 2 + no a | 1 | 2 +(2 rows) + +set enable_hashagg = false; +select case when a is not null then 'has a' else 'no a' end as label, + grouping(a is not null) as g, count(*) +from gs_nn group by rollup(a is not null) order by g; + label | g | count +-------+---+------- + has a | 0 | 2 + no a | 1 | 2 +(2 rows) + +reset enable_hashagg; +select grouping(a is not null) as g, count(*) +from gs_nn group by rollup(a is not null) +having (a is not null) is null; + g | count +---+------- + 1 | 2 +(1 row) + +-- "(b is null) is unknown" is reduced to constant false +select ((b is null) is unknown)::int as x, + grouping((b is null) is unknown) as g, count(*) +from gs_nn group by rollup((b is null) is unknown) order by g; + x | g | count +---+---+------- + 0 | 0 | 2 + | 1 | 2 +(2 rows) + +-- distinct grouping expressions that are reduced to equal constants +select (a is not null)::int as xa, (id is not null)::int as xid, + grouping(a is not null, id is not null) as g +from gs_nn +group by grouping sets ((a is not null), (id is not null), ()) +order by g; + xa | xid | g +----+-----+--- + 1 | | 1 + | 1 | 2 + | | 3 +(3 rows) + +-- constant grouping expression, referenced in ORDER BY and in a subquery +select (1 + 1) * 10 as x, grouping(1 + 1) as g, + (select count(*) from gs_nn where a * 10 < (1 + 1) * 10) as c +from gs_nn group by cube(1 + 1) +order by (1 + 1) * 10 nulls first; + x | g | c +----+---+--- + | 1 | 0 + 20 | 0 | 1 +(2 rows) + +-- grouping expression containing a LATERAL reference +select t1.a, ss.x, ss.count +from gs_nn t1, + lateral (select (t1.a + 1) * 10 as x, count(*) + from gs_nn t2 group by rollup(t1.a + 1)) ss +order by t1.a, ss.x; + a | x | count +---+----+------- + 1 | 20 | 2 + 1 | | 2 + 2 | 30 | 2 + 2 | | 2 +(4 rows) + +drop table gs_nn; -- end diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql index c5d4ed2eb57..a7529585637 100644 --- a/src/test/regress/sql/groupingsets.sql +++ b/src/test/regress/sql/groupingsets.sql @@ -803,4 +803,56 @@ from (values (1, 1), (2, 2)) as t (a, b) group by rollup(a, ab) order by 1, 2; +-- test that all references to a variable-free grouping expression that is +-- nullable by grouping sets are recognized as the same grouping column +create temp table gs_nn (id int primary key, a int not null, b int); +insert into gs_nn values (1, 1, null), (2, 2, 2); + +-- "a is not null" is reduced to constant true +explain (verbose, costs off) +select case when a is not null then 'has a' else 'no a' end as label, + grouping(a is not null) as g, count(*) +from gs_nn group by rollup(a is not null); + +select case when a is not null then 'has a' else 'no a' end as label, + grouping(a is not null) as g, count(*) +from gs_nn group by rollup(a is not null) order by g; + +set enable_hashagg = false; +select case when a is not null then 'has a' else 'no a' end as label, + grouping(a is not null) as g, count(*) +from gs_nn group by rollup(a is not null) order by g; +reset enable_hashagg; + +select grouping(a is not null) as g, count(*) +from gs_nn group by rollup(a is not null) +having (a is not null) is null; + +-- "(b is null) is unknown" is reduced to constant false +select ((b is null) is unknown)::int as x, + grouping((b is null) is unknown) as g, count(*) +from gs_nn group by rollup((b is null) is unknown) order by g; + +-- distinct grouping expressions that are reduced to equal constants +select (a is not null)::int as xa, (id is not null)::int as xid, + grouping(a is not null, id is not null) as g +from gs_nn +group by grouping sets ((a is not null), (id is not null), ()) +order by g; + +-- constant grouping expression, referenced in ORDER BY and in a subquery +select (1 + 1) * 10 as x, grouping(1 + 1) as g, + (select count(*) from gs_nn where a * 10 < (1 + 1) * 10) as c +from gs_nn group by cube(1 + 1) +order by (1 + 1) * 10 nulls first; + +-- grouping expression containing a LATERAL reference +select t1.a, ss.x, ss.count +from gs_nn t1, + lateral (select (t1.a + 1) * 10 as x, count(*) + from gs_nn t2 group by rollup(t1.a + 1)) ss +order by t1.a, ss.x; + +drop table gs_nn; + -- end -- 2.54.0