Re: [PATCH] GROUP BY ALL: the regroupening

On Tue Oct 27, 2026 at 5:22 PM -03, David Christensen wrote:
>> This has been on my list to review for a while, but the patch needs a
>> rebase, could you please send a new version rebased?
>
> Hi Matheus, rebased as-of just now.
>

Thanks for the rebased version!

I tested it and the ORDER BY ... USING case reported by Chao is fixed,
both with sorted and hashed aggregation. I also compared GROUP BY ALL
against the equivalent explicit GROUP BY for a bunch of other cases
(SRFs and GROUPING() in the target list, duplicated target entries,
window functions over aggregates, sublinks with outer-level aggregates,
plpgsql assignment and views) and didn't find any wrong results.

I have some comments:

1: The refactoring requested on the revert thread

When GROUP BY ALL was reverted, IIUC, Tom concern was not only the ORDER
BY bug itself, but that the GROUP BY transformation code needed to be
redesigned first [1]. He asked to figure out which operations need to
happen for which cases (plain GROUP BY, grouping sets, GROUP BY ALL,
SQL92 vs SQL99) and then find a "less-baroque layering".

The new addTargetToGroupClause() fixes the bug, but it seems to me that
it's the same kind of change as Chao original fix. The GROUP BY ALL path
is still a separate early-return branch at the top of
transformGroupClause(). So I'm wondering if this patch is in the right
direction based on the previous feedbacks?

2: Changes from the reverted version are missing

Before the revert Peter updated the window function comment, since the
SQL standard now specifies the current behavior [2]. The patch brings
back the old wording:

* Likewise, TLEs containing window functions are not okay to add
* to GROUP BY. At this writing, the SQL standard is silent on
* what to do with them, but by analogy to aggregates we'll just
* skip them.

The reverted version said "and the SQL standard directs us to skip
them". The commit message has the same claim. The docs on queries.sgml
and select.sgml also lost the "referring to the same query level"
wording for aggregate functions.

3: Naming of the new helper

addTargetToGroupClause() and the existing addTargetToGroupList() are
very similar names doing different things, which is a bit confusing
when reading transformGroupClause(). Maybe a different name, or at least
mention in the header comment how it differs from addTargetToGroupList()?

4: Daniel comments on Chao's patch

Daniel comments on the same helper in Chao's patch [3] also apply here:
the function comment could absorb part of the big comment on the
foreach, and since this is now a separate function we could return
directly when a matching ORDER BY item is found instead of using the
"found" flag.

Also, the new helper's comment correctly says that it can return 0, but
transformGroupClauseExpr() still says "Returns the ressortgroupref of
the expression", which was one of the misleading comments Tom pointed
out, and it can also return 0.

5: Tests

- aggregates.sql already has an avg_rec (x numeric) type a few lines
below the new tests, used to test the same record_ops vs
record_image_ops difference. Maybe we can move the new tests after it
and reuse the type instead of creating gba_rec?

- The record_image test doesn't fix the plan type. Chao original repro
used enable_hashagg = off, so I think it would be good to run it with
both sorted and hashed aggregation (or add an EXPLAIN) so we don't
lose the sorted case coverage.

- Maybe add a deparse test for a view with ORDER BY ... USING, since
that's the case that GROUP BY ALL needs to reproduce exactly on
reload.

6: Docs

It may be worth documenting that when all target list entries contain
aggregates (or the target list is empty) GROUP BY ALL behaves like
GROUP BY (), so one row is returned even on an empty table.

[1] https://www.postgresql.org/message-id/3278800.1783989856%40sss.pgh.pa.us
[2] https://www.postgresql.org/message-id/2494010.1782828112%40sss.pgh.pa.us
[3] https://www.postgresql.org/message-id/A0B7C417-5710-47B3-8FA2-C0DDA518DA07%40yesql.se

--
Matheus Alcantara
EDB: https://www.enterprisedb.com