Re: BUG #19800: Wrong results: self-join elimination drops an ORDER BY sort key with GROUPING SETS / CUBE / ROLLUP

Hi,

On Tue, Oct 27, 2026 at 2:58 PM Tender Wang <tndrwang(at)gmail(dot)com> wrote:
>
> PG Bug reporting form <noreply(at)postgresql(dot)org> 于2026年10月27日周二 17:04写道：
> >
> > Tested on:
> > - PostgreSQL 18.6, official Docker image postgres:latest (Debian
> > 18.6-1.pgdg13+2)
> > - PostgreSQL 18.6, build of REL_18_STABLE tip (commit 606f3db)
> > - PostgreSQL 20devel, build of master
> > All three reproduce. enable_hashagg=off does not change the result.
> >
>
> Thanks for the report. I agree that this is a correctness issue.
>
> The special property of grouping sets here is that two grouping items
> whose underlying expressions are equal do not necessarily produce equal
> output values. Each grouping set can independently replace a grouping
> item with NULL.
>
> Before SJE, s1.a and s2.a are distinct grouping items:
>
> GROUP BY CUBE (s1.a, s2.a)
>
> In particular, the expanded grouping sets include both (s1.a) and
> (s2.a), so the output can contain (value, NULL) and (NULL, value).
>
> SJE removes one side of the join and rewrites references to the removed
> relation to reference the kept relation. After that rewrite, the two
> grouping items appear to contain the same Var. Consequently, their
> ORDER BY keys can be considered equivalent and the second one is
> discarded as redundant. That reasoning would be valid for ordinary
> rows, but it is not valid after grouping sets have independently
> nullified the two grouping items.
>

Your analysis is spot on.

When self-join elimination (SJE) removes one side of a self-join on a
unique key,
references to the removed relation are rewritten via ChangeVarNodes().

When both sides of the self-join are referenced in grouping items
under grouping sets
(CUBE, ROLLUP, or GROUPING SETS), the rewrite causes two distinct grouping items
to become the exact same Var expression. Downstream in
make_pathkeys_for_sortclauses(),
pathkey_is_redundant() assumes members of the same Equivalence Class cannot
distinguish tied rows, and silently drops the second ORDER BY key,
resulting in arbitrary row
ordering.

> The simplest fix I can think of for now is to skip SJE when the current
> query level contains grouping sets. For example, in
> remove_useless_self_joins():
>
> if (!enable_self_join_elimination ||
> root->parse->groupingSets != NIL ||
> joinlist == NIL ||
> (list_length(joinlist) == 1 && !IsA(linitial(joinlist), List)))
> return joinlist;

I have implemented and tested the Approach you suggested above. I have
also added comprehensive
unit tests covering different permutations.
>
> This is conservative, since some queries containing grouping sets could
> still safely use SJE. However, it avoids changing the grouping-item
> identity, and the affected combination of grouping sets and a removable
> self join seems uncommon. It may therefore be a reasonable quick fix,
> particularly for back-patching to v18.

I agree with you that this approach looks conservative, although I
think this is the best approach due to
its simplicity and safety.

>
> A more selective fix would need to determine whether the SJE
> substitution makes two distinct grouping items identical while some
> grouping set contains one but not the other. That seems considerably
> more complicated, so I think the conservative check is a useful
> starting point.

I also want to explore the following two approaches and present a
comparative evaluation of alternatives

Approach 2 (Selective SJE check): Instead of skipping SJE query-wide,
check if the candidate relations
(toRemove / toKeep) appear in root->parse->groupClause. If neither
relation is referenced in the grouping
items, SJE can still safely eliminate the join.

Approach 3 (Pathkey level): Relaxing the redundancy assumption in
pathkey_is_redundant() when two
sort keys originate from distinct grouping items (tleSortGroupRef) in
the presence of grouping sets.

I need to deep-dive into whether the above two will have any broader
implications I am missing.

>
>
> --
> Thanks,
> Tender Wang
>

Thank you,
Narayanan

>