Fix GROUP BY ALL handling of ORDER BY operator semantics

On Jun 29, 2026, at 15:20, Chao Li <li.evan.chao@gmail.com> wrote:

Hi,

While testing “[ef38a4d97] Add GROUP BY ALL”, I traced the code and found a suspicious difference. In theory, GROUP BY ALL should behave the same as spelling out all inferred grouping expressions, but I found that they go through different code paths.

After entering transformGroupClause(), if this is GROUP BY ALL, it immediately enters a separate branch, loops over the target list, calls addTargetToGroupList() for every TLE, and then returns.

For non-ALL, it calls transformGroupClauseExpr() for every group clause. Inside transformGroupClauseExpr(), there is logic that the ALL path misses:
```
/*
* If the GROUP BY tlist entry also appears in ORDER BY, copy operator
* info from the (first) matching ORDER BY item. This means that if
* you write something like "GROUP BY foo ORDER BY foo USING <<<", the
* GROUP BY operation silently takes on the equality semantics implied
* by the ORDER BY. There are two reasons to do this: it improves the
* odds that we can implement both GROUP BY and ORDER BY with a single
* sort step, and it allows the user to choose the equality semantics
* used by GROUP BY, should she be working with a datatype that has
* more than one equality operator.
*
* If we're in a grouping set, though, we force our requested ordering
* to be NULLS LAST, because if we have any hope of using a sorted agg
* for the job, we're going to be tacking on generated NULL values
* after the corresponding groups. If the user demands nulls first,
* another sort step is going to be inevitable, but that's the
* planner's problem.
*/

foreach(sl, sortClause)
{
SortGroupClause *sc = (SortGroupClause *) lfirst(sl);

if (sc->tleSortGroupRef == tle->ressortgroupref)
{
SortGroupClause *grpc = copyObject(sc);

if (!toplevel)
grpc->nulls_first = false;
*flatresult = lappend(*flatresult, grpc);
found = true;
break;
}
}
```

Based on this finding, I built a test using record_image_ops, where row(1.0) and row(1.00) compare as distinct under record-image equality:
```
evantest=# CREATE TYPE t_rec AS (x numeric);
CREATE TYPE
evantest=#
evantest=# SELECT row(1.0)::t_rec OPERATOR(pg_catalog.*=) row(1.00)::t_rec;
?column?
----------
f
(1 row)
```

Here is the repro:
```
evantest=# create table t (a t_rec);
CREATE TABLE
evantest=# set enable_hashagg = 0;
SET
evantest=# insert into t values(row(1.0)::t_rec), (row(1.00)::t_rec);
INSERT 0 2
evantest=# select a, count(a) from t group by a order by a using operator(pg_catalog.*<);
a | count
--------+-------
(1.00) | 1
(1.0) | 1
(2 rows)

evantest=# select a, count(a) from t group by all order by a using operator(pg_catalog.*<);
a | count
-------+-------
(1.0) | 2
(1 row)
```

As we can see, "GROUP BY a" distinguishes the two rows because it uses the equality semantics implied by ORDER BY ... USING, but "GROUP BY ALL" groups the two rows together because it uses the default grouping semantics instead.

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.

Oops! Once again, forgot attachment.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Hello,
sorry Chao Li for spam, but I forgot to add pgsql-hackers to CC in the previous response.

On Sunday, 5 July 2026 at 11:43, Chao Li <li.evan.chao@gmail.com> wrote:

Hi,

While testing “[ef38a4d97] Add GROUP BY ALL”, I traced the code and found a suspicious difference. In theory, GROUP BY ALL should behave the same as spelling out all inferred grouping expressions, but I found that they go through different code paths.

After entering transformGroupClause(), if this is GROUP BY ALL, it immediately enters a separate branch, loops over the target list, calls addTargetToGroupList() for every TLE, and then returns.

For non-ALL, it calls transformGroupClauseExpr() for every group clause. Inside transformGroupClauseExpr(), there is logic that the ALL path misses:

I can confirm that this path is skipped for GROUP BY ALL.

Here is the repro:
```
evantest=# create table t (a t_rec);
CREATE TABLE
evantest=# set enable_hashagg = 0;
SET
evantest=# insert into t values(row(1.0)::t_rec), (row(1.00)::t_rec);
INSERT 0 2
evantest=# select a, count(a) from t group by a order by a using operator(pg_catalog.*<);
a | count
--------+-------
(1.00) | 1
(1.0) | 1
(2 rows)

evantest=# select a, count(a) from t group by all order by a using operator(pg_catalog.*<);
a | count
-------+-------
(1.0) | 2
(1 row)
```

Reproduced locally, both cases now return consistent results with the patch applied.

As we can see, "GROUP BY a" distinguishes the two rows because it uses the equality semantics implied by ORDER BY ... USING, but "GROUP BY ALL" groups the two rows together because it uses the default grouping semantics instead.

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.

The patch looks correct to me.

Best regards,
Miłosz Bieniek

On 29 Jun 2026, at 09:20, Chao Li <li.evan.chao@gmail.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.

+/*
+ * Add a targetlist entry to the GROUP BY list, copying matching ORDER BY
+ * operator information if available.
+ */

This comment is a bit light for such an important routine. The foreach has a
really great comment, I think we should move some (most?) of it to the function
comment.

-	*flatresult = lappend(*flatresult, grpc);
+	grouplist = lappend(grouplist, grpc);
 	found = true;
 	break;

Now that we are in an external function, we could just return grouplist here
instead of breaking with a flag. The code outside the foreach can then call
addTargetToGroupList directly since we know that if we reach thus far there was
no match.

--
Daniel Gustafsson

Daniel Gustafsson <daniel@yesql.se> writes:

On 29 Jun 2026, at 09:20, Chao Li <li.evan.chao@gmail.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.
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

On 14 Jul 2026, at 02:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Daniel Gustafsson <daniel@yesql.se> writes:

On 29 Jun 2026, at 09:20, Chao Li <li.evan.chao@gmail.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

On Jul 15, 2026, at 01:08, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Daniel Gustafsson <daniel@yesql.se> writes:

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?

I might be undercaffeinated still, but I think those are both correct.
"GROUP BY ()" has similar effects to use of an aggregate or HAVING
clause: it forces the table scan's results to be combined into a
single grouped row. But you get a grouped row even if the table is
empty. The second case is equivalent to "select 1 as x from t group
by x", and this is different because it will produce a grouped row
only if the table isn't empty. (The fact that the grouping expression
is a constant doesn't change the rule.) Not one of SQL's more
consistent behaviors perhaps, but I believe it's all per spec.

regards, tom lane

Thanks for all the comments. I was trying to target the bug fix for v19, so I kept the scope narrow. It looks like I don’t need to revise the current patch for now.

I have no objection to reverting GROUP BY ALL from v19. If the original feature author wants to rework the feature following Tom suggested direction, please go ahead. Otherwise, if people think it would be useful, I can try to work on it for v20.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On 14 Jul 2026, at 18:08, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:

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

I might be undercaffeinated still, but I think those are both correct.

I think you are right, I confused myself on the cases where group by all is
rewritten into group by ().

FWIW, I had a look at what a revert of the commit would look like. The
original commit included some while-in-there docs/comment cleanups which carry
value on their own, so I think we should leave those in. The attached reverts
GROUP BY ALL but retains the cleanups. If we decide to revert, the v19 back
patch would need to remove the entry from the release notes as well, but I only
did a master revert patch for now.

--
Daniel Gustafsson

Daniel Gustafsson <daniel@yesql.se> writes:

FWIW, I had a look at what a revert of the commit would look like. The
original commit included some while-in-there docs/comment cleanups which carry
value on their own, so I think we should leave those in. The attached reverts
GROUP BY ALL but retains the cleanups. If we decide to revert, the v19 back
patch would need to remove the entry from the release notes as well, but I only
did a master revert patch for now.

Thanks for doing that! I'll wait another day or so to see if anyone
wants to push back on reverting this.

regards, tom lane

On Jul 18, 2026, at 04:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

Thanks for doing that! I'll wait another day or so to see if anyone
wants to push back on reverting this.

Hearing nothing, pushed. Thanks for doing the legwork on that.

regards, tom lane

We need to notify Bruce to remove this feature from the PG19 release note. If that’s not done yet, I can do it.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/