Re: [PATCH] Fix grouping-set nulling of variable-free grouping expressions

On Fri, Oct 16, 2026 at 8:23 PM shihao zhong <zhong950419(at)gmail(dot)com> wrote:
>
> Hi Haibo,
>
> Thanks for your v3. I think it breaks some thing that is right today.
>
> On master with v3:
>
> SELECT 1 AS one, 1 AS uno GROUP BY ROLLUP(one);
>
> gives uno = NULL in the grand-total row. Master, 17 and 18 give 1.
>
> The parser turns the second 1 into the group Var, and with the shared
> phid it now gets nulled. Andrey Rachitskiy's patch for bug 19752 stops
> that [1].
>
> With his patch, v3 is supposed to be ok. So that created some dependency
> on submission.
>
> [1] https://www.postgresql.org/message-id/CAB8bMiv2JLSxs1aYF42G4887y5LdY=1-E8FLj_wGmKFooqWocQ@mail.gmail.com
> Thanks,
> Shihao

Hi hackers,

After Shihao's report about v3, I investigated the interaction
between grouping-expression matching in parse_agg.c and PHV identity
in the planner.

I agree that v3 is not safe as submitted. Sharing a PHV ID by
RTE_GROUP attribute is only correct if the parser has identified
genuine references to the same grouping output. Currently, equal()
can replace independently written expressions with the same group Var.

This seems closely related to Tom's suggestions in the discussion
of bug #19752:

https://www.postgresql.org/message-id/2530849.1791487908@sss.pgh.pa.us
https://www.postgresql.org/message-id/2533535.1791490239@sss.pgh.pa.us

I would like to confirm whether the following matching rules are
reasonable:

A target entry explicitly identified as a grouping item should
reference its own RTE_GROUP attribute, selected by ressortgroupref,
rather than the first structurally equal grouping item.

Other subexpressions should be matched structurally only against
grouping items containing a Var of the current query level.
Grouping items without such a Var would only be referenced through
their explicitly identified target entries.

The second rule would leave independent constants, stable expressions,
Params, and outer-only correlated expressions untouched, while still
allowing expressions involving current-level Vars to be recognized
as grouping references.

For example, consider:

CREATE TABLE t (a int NOT NULL);
INSERT INTO t VALUES (1);

SELECT DISTINCT a IS NOT NULL AS c1,
a IS NOT NULL AS c2
FROM t
GROUP BY CUBE(c1, c2)
ORDER BY 1 NULLS LAST, 2 NULLS LAST;

The two grouping outputs have different ressortgrouprefs, even
though their expressions are equal. They should remain distinct.
The query returns four rows on current master, but only two with v3.

There is also a query-level question:

SELECT s.c1, s.c2
FROM t,
LATERAL (
SELECT t.a + 1 AS c1,
ARRAY[t.a + 1] AS c2
FROM (VALUES (1)) v(x)
GROUP BY ROLLUP(c1)
) s
ORDER BY s.c1 NULLS LAST;

Current master retains {2} in c2 for the grand-total row. I think
this is appropriate because c2 is an independently written expression
depending only on the outer query. It should not become a reference
to the inner grouping output merely because a subtree is equal.

I have experimented with the two parser rules above together with
v3's PHV-sharing mechanism. The results are encouraging: the
original PG19 wrong result, the independent-expression cases, and
the CUBE example behave as expected. Core and isolation regression
tests pass on master and REL_19_STABLE.

Before preparing another patch revision, I'd appreciate opinions on
whether these two matching rules reflect the intended semantics,
particularly the level-zero-Var criterion.

The parser changes also overlap with Andrey's work on bug #19752,
so I would like to resolve the intended behavior before deciding how
best to separate or coordinate the fixes.

Tom, your suggestion in the bug #19752 discussion about restricting
structural matching to grouping items containing a level-zero Var seems
relevant here. Do you think that rule, together with matching explicitly
identified target entries by ressortgroupref, would be the right direction?

Thanks,
Haibo