Re: Fix GROUP BY ALL handling of ORDER BY operator semantics

> On 14 Jul 2026, at 02:44, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
>>> On 29 Jun 2026, at 09:20, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>>> The fix mostly refactors the existing logic so the GROUP BY ALL path also handles the ORDER BY sort clause. See the attached patch for details.
>
>> Thanks for the report and the patch, I am going to study it a bit more but I
>> have a few small comments on the patch.
>
> I'd been too busy to look at this right away, but after glancing at it
> briefly: on the one hand, I'd rather not engage in refactoring as part
> of a bug fix, but I think I agree that we have to here. On the other
> hand, the handling of these clauses was already messy, and I now see
> that the addition of grouping sets made the mess quite a lot worse.

Ack. Another case I am a bit confused by is:

postgres=# create table t (a integer);
CREATE TABLE
postgres=# select 1 as x from t group by ();
x
---
1
(1 row)

postgres=# select 1 as x from t group by all;
x
---
(0 rows)

Shouldn't those two queries yield the same result, or am I missing something
obvious?

> The division of labor is confusing and seemingly redundant, the
> commenting is poor, and there are even visibly-falsified comments
> such as this one for transformGroupClauseExpr:
>
> * Returns the ressortgroupref of the expression.
>
> which doesn't mention that oh no, we might just return zero instead
> (let alone explain why).
>
> Chao's patch as proposed doesn't clean any of that up, but just adds
> another layer of impenetrability to the logic. I don't have a lot of
> faith that there aren't other comparable bugs lurking. I think we
> really ought to take a step back and redesign this code, after first
> figuring out which operations need to happen for which cases (plain
> GROUP BY, grouping sets, GROUP BY ALL, SQL92 vs SQL99 behavior, etc).
> Then we need a less-baroque layering, IMO.

As an excercise, I started to pull apart with the GROUP BY ALL feature in mind
to see if that isolated part could be refactored out into a neater layering
combined with the rest of the code.

On a related noted, the GROUP BY ALL code scribbles on the SelectStmt as part
of its processing, is that really guaranteed to always be Ok?

/*
* Otherwise, the SQL standard says to treat it like "GROUP BY ()".
* Build a representation of that, and let the rest of this function
* handle it.
*/
grouplist = list_make1(makeGroupingSet(GROUPING_SET_EMPTY, NIL, -1));

> That is a large change to take on post-beta2, though. Maybe the path
> of prudence is to revert GROUP BY ALL for v19 and try again for v20.

Maybe so, it's a bit of a shame since this is a really neat bit of syntax but
fixing incorrect query results post beta2 does carry the smell of most things
potentially lurking in the shadows.

--
Daniel Gustafsson