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.
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.
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.
regards, tom lane
