> On 14 Jul 2026, at 02:44, Tom Lane <[email protected]> wrote: > > Daniel Gustafsson <[email protected]> writes: >>> On 29 Jun 2026, at 09:20, Chao Li <[email protected]> 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
