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

Hi Tender,

On Wed, Nov 11, 2026 at 12:08 PM Tender Wang <tndrwang(at)gmail(dot)com> wrote:
>
> Skipping SJE query-wide seems too strict; I'm also thinking about
> approach 2. As in the attached v2.
>
> In the attached v2, I collect the relids referenced by the grouping
> expressions and skip only an SJE candidate whose toRemove or toKeep
> relation overlaps that set. The relid set is computed once in
> remove_useless_self_joins(), so the per-candidate check only requires
> bitmap overlap tests.

Thanks for the v2 patch! I like this approach much better than skipping
SJE for the entire query.

While testing it further, I noticed a couple of small things,

1. In remove_useless_self_joins(), I passed NULL as the root to
pull_varnos(NULL, (Node *) tle->expr). When a grouping set contains a
constant (like GROUP BY ROLLUP(1)), mark_nullable_by_grouping() wraps the
constant in a PlaceHolderVar whose phrels includes all relations in the
jointree. Passing NULL tells pull_varnos() to look inside the
PlaceHolderVar's expression rather than returning ph_eval_at, so SJE can
still optimize self-joins in those queries.

2. In remove_self_joins_one_group(), since r and k are base relation IDs,
I moved the check for r to the outer loop using bms_is_member(r,
grouping_relids) and used bms_is_member(k, grouping_relids) in the inner
loop.

3. I also noticed that the underlying issue can cause wrong results even
without ORDER BY: if the second grouping column is used in HAVING (e.g.,
HAVING t2.a IS NULL) or in an expression like COALESCE(t2.a, -1), SJE
rewrites t1.a and t2.a to the same Var and setrefs.c ends up binding both
to the first column. Skipping SJE for relations in grouping_relids fixes
this case as well.

4. Added the commit message and regression tests in join.sql covering
both cases where SJE should be skipped and cases where SJE should still
apply.

Please find the patch with the above changes attached.

Thank you,
Narayanan