From d1e28c5eb8f937b382eab7df0e6869508ffae5fb Mon Sep 17 00:00:00 2001 From: David Rowley Date: Tue, 27 Oct 2026 12:47:08 +1300 Subject: [PATCH v1] Fix issue with code manually adjusting a TupleTableSlot In prepare_projection_slot() when grouped_cols was non-empty, the code would manually put a NULL into grouped columns to represent the rollup row. This relied on the old pre-c456e3911 tuple deforming code which, when performing incremental deforming, would only touch tts_isnull elements for attributes that were yet to be deformed. c456e3911 changed that so that deforming the NULL bitmap was done again on every call as masking out previously deformed attributes would likely be more effort than it would ever save. Due to the changes made in c456e3911, if any other attributes were required from the slot after the slot_getsomeattrs() call in prepare_projection_slot(), then the deformation done for those would have overwritten the manually placed NULL value. Here we fix this by changing the slot_getsomeattrs() call to always deform everything we need for this tuple. That means there will be no additional tuple deforms for this tuple. Bug: #19783 Reported-by: Alperen Keles Author: David Rowley Discussion: https://postgr.es/m/19783-070e4bf0c73c1076@postgresql.org Backpatch-through: 19 --- src/backend/executor/nodeAgg.c | 29 +++++++++++++++++++--- src/test/regress/expected/groupingsets.out | 14 +++++++++++ src/test/regress/sql/groupingsets.sql | 9 +++++++ 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c index 29037cf3122..2d3174dff21 100644 --- a/src/backend/executor/nodeAgg.c +++ b/src/backend/executor/nodeAgg.c @@ -1229,7 +1229,7 @@ prepare_hash_slot(AggStatePerHash perhash, * read as null in the context of the current grouping set. Also stash the * current group bitmap where GroupingExpr can get at it. * - * This relies on three conditions: + * This relies on four conditions: * * 1) Nothing is ever going to try and extract the whole tuple from this slot, * only reference it in evaluations, which will only access individual @@ -1242,6 +1242,12 @@ prepare_hash_slot(AggStatePerHash perhash, * 3) Within a given phase, we never need to recover the value of an attribute * once it has been set to null. * + * 4) We must extract all remaining required attributes from the tuple before + * manually adjusting the tts_isnull elements. This is required to prevent + * further deformations that might be required for the HAVING clause or + * projection step from re-extracting the NULL bitmap into the tts_isnull + * array and overwriting our manual adjustments. + * * Poking into the slot this way is a bit ugly, but the consensus is that the * alternative was worse. */ @@ -1267,8 +1273,12 @@ prepare_projection_slot(AggState *aggstate, TupleTableSlot *slot, int currentSet { ListCell *lc; - /* all_grouped_cols is arranged in desc order */ - slot_getsomeattrs(slot, linitial_int(aggstate->all_grouped_cols)); + /* Sanity check on max_colno_needed value */ + Assert(aggstate->max_colno_needed >= + linitial_int(aggstate->all_grouped_cols)); + + /* deform all remaining columns, per #4 above */ + slot_getsomeattrs(slot, aggstate->max_colno_needed); foreach(lc, aggstate->all_grouped_cols) { @@ -3721,6 +3731,19 @@ ExecInitAgg(Agg *node, EState *estate, int eflags) /* Initialize this to 1, meaning nothing spilled, yet */ aggstate->hash_batches_used = 1; } + else + { + Bitmapset *varattnos; + Bitmapset *aggvarattnos; + int max_varattno; + + /* Figure out the highest outer-side varattno required. */ + find_cols(aggstate, &aggvarattnos, &varattnos); + max_varattno = bms_prev_member(varattnos, -1); + max_varattno = Max(max_varattno, bms_prev_member(aggvarattnos, -1)); + + aggstate->max_colno_needed = max_varattno; + } /* * Initialize current phase-dependent values to initial phase. The initial diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out index 08d5ce3156d..393d63cfb0c 100644 --- a/src/test/regress/expected/groupingsets.out +++ b/src/test/regress/expected/groupingsets.out @@ -1784,6 +1784,20 @@ SELECT a, b, count(*), max(a), max(b) FROM gstest3 GROUP BY GROUPING SETS(a, b,( (5 rows) COMMIT; +-- Verify handling of injecting NULL values for the rollup row works correctly +-- when additional columns need to be deformed from the tuple after the NULL +-- is added. +begin; +insert into gstest3 values(3,4,null,5); +set enable_hashagg = false; +select b, c, d, grouping(b, c) AS g from gstest3 where a = 3 group by grouping sets ((a, b), (a, c)); + b | c | d | g +---+---+---+--- + 4 | | 5 | 1 + | | 5 | 2 +(2 rows) + +rollback; -- More rescan tests select * from (values (1),(2)) v(a) left join lateral (select v.a, four, ten, count(*) from onek group by cube(four,ten)) s on true order by v.a,four,ten; a | a | four | ten | count diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql index c5d4ed2eb57..91c05adf432 100644 --- a/src/test/regress/sql/groupingsets.sql +++ b/src/test/regress/sql/groupingsets.sql @@ -528,6 +528,15 @@ EXPLAIN (COSTS OFF) SELECT a, b, count(*), max(a), max(b) FROM gstest3 GROUP BY SELECT a, b, count(*), max(a), max(b) FROM gstest3 GROUP BY GROUPING SETS(a, b,()) ORDER BY a, b; COMMIT; +-- Verify handling of injecting NULL values for the rollup row works correctly +-- when additional columns need to be deformed from the tuple after the NULL +-- is added. +begin; +insert into gstest3 values(3,4,null,5); +set enable_hashagg = false; +select b, c, d, grouping(b, c) AS g from gstest3 where a = 3 group by grouping sets ((a, b), (a, c)); +rollback; + -- More rescan tests select * from (values (1),(2)) v(a) left join lateral (select v.a, four, ten, count(*) from onek group by cube(four,ten)) s on true order by v.a,four,ten; select array(select row(v.a,s1.*) from (select two,four, count(*) from onek group by cube(two,four) order by two,four) s1) from (values (1),(2)) v(a); -- 2.53.0