Re: BUG #19783: Sorted GROUPING SETS return a grouping column's value in a set that leaves it out

Hi,

On Tue, 27 Oct 2026 at 09:42, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> On Thu, 22 Oct 2026 at 19:25, PG Bug reporting form
> <noreply(at)postgresql(dot)org> wrote:
> > With a sorted grouping-sets plan (GroupAggregate), a column that is not
> > in the current grouping set can show its real value instead of NULL,
> > while GROUPING() for the same row says it is not grouped. It happens
> > when the row has a NULL somewhere and the target list reads a column
> > that comes after the grouping columns in the Agg's input:
> >
> > CREATE TABLE t (id int PRIMARY KEY, a int, b int, c int);
> > INSERT INTO t VALUES (1, 10, NULL, 100);
> > SET enable_hashagg = off;
> > SELECT a, b, c, GROUPING(a, b) AS g
> > FROM t GROUP BY GROUPING SETS ((id, a), (id, b));
> >
> > a | b | c | g
> > ----+---+-----+---
> > 10 | | 100 | 1
> > 10 | | 100 | 2
> > (2 rows)
>
> Many thanks for reporting this.
>
> What's happening here is due to the change c456e3911 made, as it now
> uses a sort of slice-by-eight method of deforming the tuple's NULL
> bitmap into the slot's tts_values array. When working on that, I
> considered if the incremental deform should be careful to pick up
> where it left off or not. I opted not, as the masking out of previous
> bits or tts_isnull elements would, in most cases, cost more than it
> would save.
>
> The problem is that prepare_projection_slot() overwrites the slot's
> tts_isnull elements to represent the rollup rows, but if any other
> column is required from the tuple, in this case "c", then that will
> overwrite the manually adjusted NULL when performing additional
> deforming on that slot.
>
> The reported case allows "c" to be in the SELECT list because it's
> functionally dependent on the primary key. Ordinarily, it would be in
> the GROUP BY clause or in an aggregate parameter, in which case we'd
> have deformed it already. The heap tuple also needs to have at least 1
> NULL value, else we wouldn't be deforming the NULL bitmask. The
> deforming code would just "isnull[attnum] = false" each element as
> they're deformed, and that wouldn't overwrite any previously deformed
> attribute's null property.
>
> I thought of a few ways to fix this:
>
> 1. Modify populate_isnull_array() to pick up where it left off. Likely
> by adjusting the loop start byte, combined with adjusting the memcpy()
> to not overwrite bytes that are previously deformed.
> 2. Modify ExprState code to remember the last outer varattno required,
> then deform to there in prepare_projection_slot().
>
> One problem with #2 is that if some very high varattno is needed for
> the projection, but some lesser varattno is needed for the HAVING
> qual, then we might deform more of the tuple than we need to when the
> HAVING qual filters out the tuple. Ultimately, we'd not perform the
> projection step in that case, therefore not deform the remaining
> attributes. I was less concerned about that when I saw the hashed
> version does this anyway. I understand it's possible to craft a case
> where this might make a difference, but I suspect in practice that
> it's not going to be an issue.
>
> I found another way of doing #2 (attached) without the mods to
> ExprState by reusing some existing code in nodeAgg.c which walks the
> targetlist and Aggrefs looking for columns. That was only being
> executed for the hashing code path, so I added some code to execute
> this for the non-hash version.

Thanks for the patch, LGTM.

Is there a reason though for not using else if (node->groupingSets)
for the new block instead of just else?

Regards,
Ayush