pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedFix GROUP BY ALL handling of ORDER BY operator semantics
14+ messages / 6 participants
[nested] [flat]
Commits from this thread:
2026-07-17 a32733d8f1 372b8d1adb Revert "Add GROUP BY ALL".
* Fix GROUP BY ALL handling of ORDER BY operator semantics
@ 2026-06-29 07:20 Chao Li <li.evan.chao@gmail.com>
0 siblings, 3 replies; 14+ messages in thread
From: Chao Li @ 2026-06-29 07:20 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Tom Lane <tgl@sss.pgh.pa.us>
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.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Fix GROUP BY ALL handling of ORDER BY operator semantics
@ 2026-06-29 09:09 Chao Li <li.evan.chao@gmail.com>
parent: Chao Li <li.evan.chao@gmail.com>
2 siblings, 0 replies; 14+ messages in thread
From: Chao Li @ 2026-06-29 09:09 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Tom Lane <tgl@sss.pgh.pa.us>
> 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/
Attachments:
v1-0001-Fix-GROUP-BY-ALL-handling-of-ORDER-BY-operator-se.patch (application/octet-stream, 8.1K) download
From b6e19f5c8eea6a17a06a2d836f6f1f5a62c149c6 Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <lic@highgo.com>
Date: Mon, 29 Jun 2026 14:22:36 +0800
Subject: [PATCH v1] Fix GROUP BY ALL handling of ORDER BY operator semantics
GROUP BY ALL builds its grouping list directly from the query target
list. In doing so, it used addTargetToGroupList() and skipped the
explicit GROUP BY path's handling for target entries that also appear
in ORDER BY.
That matters when ORDER BY ... USING selects non-default operator
semantics. An explicit GROUP BY item copies the matching ORDER BY
SortGroupClause, so grouping uses the equality semantics implied by
the ordering operator. GROUP BY ALL instead used the default grouping
semantics, making it not equivalent to spelling out the inferred
grouping expressions.
Fix by factoring the "ORDER BY"-aware grouping-list construction into a
helper and using it for both explicit GROUP BY items and GROUP BY ALL
inferred items. Add regression coverage with record_image_ops, where
default record equality and image equality distinguish row(1.0) and
row(1.00) differently.
Author: Chao Li <lic@highgo.com>
---
src/backend/parser/parse_clause.c | 76 +++++++++++++++++-------
src/test/regress/expected/aggregates.out | 27 +++++++++
src/test/regress/sql/aggregates.sql | 18 ++++++
3 files changed, 98 insertions(+), 23 deletions(-)
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 5fe5257b019..68e0049c04c 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -96,6 +96,13 @@ static List *resolve_unique_index_expr(ParseState *pstate, InferClause *infer,
Relation heapRel);
static List *addTargetToGroupList(ParseState *pstate, TargetEntry *tle,
List *grouplist, List *targetlist, int location);
+static List *addTargetToGroupListWithSortClause(ParseState *pstate,
+ TargetEntry *tle,
+ List *grouplist,
+ List *targetlist,
+ List *sortClause,
+ int location,
+ bool toplevel);
static WindowClause *findWindowClause(List *wclist, const char *name);
static Node *transformFrameOffset(ParseState *pstate, int frameOptions,
Oid rangeopfamily, Oid rangeopcintype, Oid *inRangeFunc,
@@ -2514,7 +2521,6 @@ transformGroupClauseExpr(List **flatresult, Bitmapset *seen_local,
ParseExprKind exprKind, bool useSQL99, bool toplevel)
{
TargetEntry *tle;
- bool found = false;
if (useSQL99)
tle = findTargetlistEntrySQL99(pstate, gexpr,
@@ -2525,8 +2531,6 @@ transformGroupClauseExpr(List **flatresult, Bitmapset *seen_local,
if (tle->ressortgroupref > 0)
{
- ListCell *sl;
-
/*
* Eliminate duplicates (GROUP BY x, x) but only at local level.
* (Duplicates in grouping sets can affect the number of returned
@@ -2537,14 +2541,44 @@ transformGroupClauseExpr(List **flatresult, Bitmapset *seen_local,
*/
if (bms_is_member(tle->ressortgroupref, seen_local))
return 0;
+ }
+
+ *flatresult = addTargetToGroupListWithSortClause(pstate, tle,
+ *flatresult, *targetlist,
+ sortClause,
+ exprLocation(gexpr),
+ toplevel);
+
+ /*
+ * _something_ must have assigned us a sortgroupref by now...
+ */
+
+ return tle->ressortgroupref;
+}
+
+/*
+ * Add a targetlist entry to the GROUP BY list, copying matching ORDER BY
+ * operator information if available.
+ */
+static List *
+addTargetToGroupListWithSortClause(ParseState *pstate, TargetEntry *tle,
+ List *grouplist, List *targetlist,
+ List *sortClause, int location,
+ bool toplevel)
+{
+ bool found = false;
+
+ if (tle->ressortgroupref > 0)
+ {
+ ListCell *sl;
/*
* If we're already in the flat clause list, we don't need to consider
* adding ourselves again.
*/
- found = targetIsInSortList(tle, InvalidOid, *flatresult);
+ found = targetIsInSortList(tle, InvalidOid, grouplist);
if (found)
- return tle->ressortgroupref;
+ return grouplist;
/*
* If the GROUP BY tlist entry also appears in ORDER BY, copy operator
@@ -2575,7 +2609,7 @@ transformGroupClauseExpr(List **flatresult, Bitmapset *seen_local,
if (!toplevel)
grpc->nulls_first = false;
- *flatresult = lappend(*flatresult, grpc);
+ grouplist = lappend(grouplist, grpc);
found = true;
break;
}
@@ -2587,15 +2621,10 @@ transformGroupClauseExpr(List **flatresult, Bitmapset *seen_local,
* sort/group semantics.
*/
if (!found)
- *flatresult = addTargetToGroupList(pstate, tle,
- *flatresult, *targetlist,
- exprLocation(gexpr));
+ grouplist = addTargetToGroupList(pstate, tle, grouplist,
+ targetlist, location);
- /*
- * _something_ must have assigned us a sortgroupref by now...
- */
-
- return tle->ressortgroupref;
+ return grouplist;
}
/*
@@ -2822,16 +2851,17 @@ transformGroupClause(ParseState *pstate, List *grouplist, bool groupByAll,
continue;
/*
- * Otherwise, add the TLE to the result using default sort/group
- * semantics. We specify the parse location as the TLE's
- * location, despite the comment for addTargetToGroupList
- * discouraging that. The only other thing we could point to is
- * the ALL keyword, which seems unhelpful when there are multiple
- * TLEs.
+ * Otherwise, add the TLE to the result. We specify the parse
+ * location as the TLE's location, despite the comment for
+ * addTargetToGroupList discouraging that. The only other thing
+ * we could point to is the ALL keyword, which seems unhelpful
+ * when there are multiple TLEs.
*/
- result = addTargetToGroupList(pstate, tle,
- result, *targetlist,
- exprLocation((Node *) tle->expr));
+ result = addTargetToGroupListWithSortClause(pstate, tle,
+ result, *targetlist,
+ sortClause,
+ exprLocation((Node *) tle->expr),
+ true);
}
/* If we found any acceptable targets, we're done */
diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index 89e051ee824..8863d5f9d0b 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -1834,6 +1834,33 @@ SELECT pg_get_viewdef('v1'::regclass);
DROP VIEW v1;
DROP TABLE t1;
+-- GROUP BY ALL should use ORDER BY's equality semantics, just as an
+-- equivalent explicit GROUP BY clause would.
+CREATE TYPE t_rec AS (x numeric);
+CREATE TEMP TABLE t_rec_ops (a t_rec);
+INSERT INTO t_rec_ops VALUES (row(1.0)::t_rec), (row(1.00)::t_rec);
+SET enable_hashagg = off;
+SELECT count(*) AS groups FROM
+ (SELECT a, count(*) FROM t_rec_ops
+ GROUP BY a
+ ORDER BY a USING OPERATOR(pg_catalog.*<)) s;
+ groups
+--------
+ 2
+(1 row)
+
+SELECT count(*) AS groups FROM
+ (SELECT a, count(*) FROM t_rec_ops
+ GROUP BY ALL
+ ORDER BY a USING OPERATOR(pg_catalog.*<)) s;
+ groups
+--------
+ 2
+(1 row)
+
+RESET enable_hashagg;
+DROP TABLE t_rec_ops;
+DROP TYPE t_rec;
--
-- Test GROUP BY matching of join columns that are type-coerced due to USING
--
diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql
index 916383db927..a2a8a4e5fa5 100644
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -664,6 +664,24 @@ SELECT pg_get_viewdef('v1'::regclass);
DROP VIEW v1;
DROP TABLE t1;
+-- GROUP BY ALL should use ORDER BY's equality semantics, just as an
+-- equivalent explicit GROUP BY clause would.
+CREATE TYPE t_rec AS (x numeric);
+CREATE TEMP TABLE t_rec_ops (a t_rec);
+INSERT INTO t_rec_ops VALUES (row(1.0)::t_rec), (row(1.00)::t_rec);
+SET enable_hashagg = off;
+SELECT count(*) AS groups FROM
+ (SELECT a, count(*) FROM t_rec_ops
+ GROUP BY a
+ ORDER BY a USING OPERATOR(pg_catalog.*<)) s;
+SELECT count(*) AS groups FROM
+ (SELECT a, count(*) FROM t_rec_ops
+ GROUP BY ALL
+ ORDER BY a USING OPERATOR(pg_catalog.*<)) s;
+RESET enable_hashagg;
+DROP TABLE t_rec_ops;
+DROP TYPE t_rec;
+
--
-- Test GROUP BY matching of join columns that are type-coerced due to USING
--
--
2.50.1 (Apple Git-155)
=
* Re: Fix GROUP BY ALL handling of ORDER BY operator semantics
@ 2026-07-07 20:00 Miłosz Bieniek <bieniek.milosz@proton.me>
parent: Chao Li <li.evan.chao@gmail.com>
2 siblings, 0 replies; 14+ messages in thread
From: Miłosz Bieniek @ 2026-07-07 20:00 UTC (permalink / raw)
To: Chao Li <li.evan.chao@gmail.com>; +Cc: pgsql-hackers; Tom Lane <tgl@sss.pgh.pa.us>
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
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Fix GROUP BY ALL handling of ORDER BY operator semantics
@ 2026-07-13 20:10 Daniel Gustafsson <daniel@yesql.se>
parent: Chao Li <li.evan.chao@gmail.com>
2 siblings, 1 reply; 14+ messages in thread
From: Daniel Gustafsson @ 2026-07-13 20:10 UTC (permalink / raw)
To: Chao Li <li.evan.chao@gmail.com>; +Cc: pgsql-hackers; Tom Lane <tgl@sss.pgh.pa.us>
> 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
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Fix GROUP BY ALL handling of ORDER BY operator semantics
@ 2026-07-14 00:44 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 14+ messages in thread
From: Tom Lane @ 2026-07-14 00:44 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Chao Li <li.evan.chao@gmail.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Fix GROUP BY ALL handling of ORDER BY operator semantics
@ 2026-07-14 14:01 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 14+ messages in thread
From: Daniel Gustafsson @ 2026-07-14 14:01 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Chao Li <li.evan.chao@gmail.com>; pgsql-hackers
> 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
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Fix GROUP BY ALL handling of ORDER BY operator semantics
@ 2026-07-14 16:08 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 2 replies; 14+ messages in thread
From: Tom Lane @ 2026-07-14 16:08 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Chao Li <li.evan.chao@gmail.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Fix GROUP BY ALL handling of ORDER BY operator semantics
@ 2026-07-14 16:33 Chao Li <li.evan.chao@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 14+ messages in thread
From: Chao Li @ 2026-07-14 16:33 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Daniel Gustafsson <daniel@yesql.se>; pgsql-hackers
> 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/
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Fix GROUP BY ALL handling of ORDER BY operator semantics
@ 2026-07-15 08:01 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 14+ messages in thread
From: Daniel Gustafsson @ 2026-07-15 08:01 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Chao Li <li.evan.chao@gmail.com>; pgsql-hackers
> 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
Attachments:
0001-Revert-Add-GROUP-BY-ALL.patch (application/octet-stream, 19.9K) download
From 15da199cf5d27e82b6e14d1e48df3fa4445f2085 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Wed, 15 Jul 2026 09:38:06 +0200
Subject: [PATCH] Revert Add GROUP BY ALL
This reverts commit ef38a4d9756db9ae1d20f40aa39f3cf76059b81a which
implemented the GROUP BY ALL syntax. A postcommit review discovered
that GROUP BY ALL missed target list handling of entries which also
appear in an ORDER BY in the query. This caused the query to return
wrong results when the ordering used non-default equality semantics.
It was deemed too late in the release cycle to perform the required
refactoring to properly fix this.
The reverted commit also included some additional wordsmithing and
docs cleanup, all of which are retained as weren't connected to the
reverted feature.
Discussion: https://postgr.es/m/5243308F-8E5C-45AA-828C-FAD96C4F34DA@gmail.com
Discussion: https://postgr.es/m/CAHM0NXjz0kDwtzoe-fnHAqPB1qA8_VJN0XAmCgUZ+iPnvP5LbA@mail.gmail.com
---
doc/src/sgml/queries.sgml | 25 -----
doc/src/sgml/ref/select.sgml | 13 +--
doc/src/sgml/ref/select_into.sgml | 2 +-
src/backend/parser/analyze.c | 2 -
src/backend/parser/gram.y | 14 ---
src/backend/parser/parse_clause.c | 63 +-----------
src/backend/utils/adt/ruleutils.c | 4 +-
src/include/nodes/parsenodes.h | 2 -
src/include/parser/parse_clause.h | 1 -
src/test/regress/expected/aggregates.out | 123 -----------------------
src/test/regress/sql/aggregates.sql | 54 ----------
11 files changed, 5 insertions(+), 298 deletions(-)
diff --git a/doc/src/sgml/queries.sgml b/doc/src/sgml/queries.sgml
index b3cfbc93582..3d729f983b5 100644
--- a/doc/src/sgml/queries.sgml
+++ b/doc/src/sgml/queries.sgml
@@ -1159,31 +1159,6 @@ SELECT product_id, p.name, (sum(s.units) * p.price) AS sales
expressions cannot contain aggregate functions or window functions).
</para>
- <para>
- PostgreSQL also supports the syntax <literal>GROUP BY ALL</literal>,
- which is equivalent to explicitly writing all select-list entries that
- do not contain either an aggregate function referring to the same query level or a window function.
- This can greatly simplify ad-hoc exploration of data.
- As an example, these queries are equivalent:
-<screen>
-<prompt>=></prompt> <userinput>SELECT a, b, a + b, sum(c) FROM test1 GROUP BY ALL;</userinput>
- a | b | ?column? | sum
----+---+----------+----
- 1 | 4 | 5 | 9
- 2 | 5 | 7 | 12
- 3 | 6 | 9 | 15
-(3 rows)
-
-<prompt>=></prompt> <userinput>SELECT a, b, a + b, sum(c) FROM test1 GROUP BY a, b, a + b;</userinput>
- a | b | ?column? | sum
----+---+----------+----
- 1 | 4 | 5 | 9
- 2 | 5 | 7 | 12
- 3 | 6 | 9 | 15
-(3 rows)
-</screen>
- </para>
-
<indexterm>
<primary>HAVING</primary>
</indexterm>
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 8d5a751af7e..837bf6d7832 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -37,7 +37,7 @@ SELECT [ ALL | DISTINCT [ ON ( <replaceable class="parameter">expression</replac
[ { * | <replaceable class="parameter">expression</replaceable> [ [ AS ] <replaceable class="parameter">output_name</replaceable> ] } [, ...] ]
[ FROM <replaceable class="parameter">from_item</replaceable> [, ...] ]
[ WHERE <replaceable class="parameter">condition</replaceable> ]
- [ GROUP BY { ALL | [ ALL | DISTINCT ] <replaceable class="parameter">grouping_element</replaceable> [, ...] } ]
+ [ GROUP BY [ ALL | DISTINCT ] <replaceable class="parameter">grouping_element</replaceable> [, ...] ]
[ HAVING <replaceable class="parameter">condition</replaceable> ]
[ WINDOW <replaceable class="parameter">window_name</replaceable> AS ( <replaceable class="parameter">window_definition</replaceable> ) [, ...] ]
[ { UNION | INTERSECT | EXCEPT } [ ALL | DISTINCT ] <replaceable class="parameter">select</replaceable> ]
@@ -839,7 +839,7 @@ WHERE <replaceable class="parameter">condition</replaceable>
<para>
The optional <literal>GROUP BY</literal> clause has the general form
<synopsis>
-GROUP BY { ALL | [ ALL | DISTINCT ] <replaceable class="parameter">grouping_element</replaceable> [, ...] }
+GROUP BY [ ALL | DISTINCT ] <replaceable class="parameter">grouping_element</replaceable> [, ...]
</synopsis>
</para>
@@ -857,15 +857,6 @@ GROUP BY { ALL | [ ALL | DISTINCT ] <replaceable class="parameter">grouping_elem
input-column name rather than an output column name.
</para>
- <para>
- The form <literal>GROUP BY ALL</literal> with no explicit
- <replaceable class="parameter">grouping_elements</replaceable>
- provided is equivalent to writing <literal>GROUP BY</literal> with the
- numbers of all <command>SELECT</command> output columns that do not
- contain either an aggregate function referring to the same query level or
- a window function.
- </para>
-
<para>
If any of <literal>GROUPING SETS</literal>, <literal>ROLLUP</literal> or
<literal>CUBE</literal> are present as grouping elements, then the
diff --git a/doc/src/sgml/ref/select_into.sgml b/doc/src/sgml/ref/select_into.sgml
index cbf865ff838..550ba69d5d1 100644
--- a/doc/src/sgml/ref/select_into.sgml
+++ b/doc/src/sgml/ref/select_into.sgml
@@ -27,7 +27,7 @@ SELECT [ ALL | DISTINCT [ ON ( <replaceable class="parameter">expression</replac
INTO [ TEMPORARY | TEMP | UNLOGGED ] [ TABLE ] <replaceable class="parameter">new_table</replaceable>
[ FROM <replaceable class="parameter">from_item</replaceable> [, ...] ]
[ WHERE <replaceable class="parameter">condition</replaceable> ]
- [ GROUP BY { ALL | [ ALL | DISTINCT ] <replaceable class="parameter">grouping_element</replaceable> [, ...] } ]
+ [ GROUP BY [ ALL | DISTINCT ] <replaceable class="parameter">grouping_element</replaceable> [, ...] ]
[ HAVING <replaceable class="parameter">condition</replaceable> ]
[ WINDOW <replaceable class="parameter">window_name</replaceable> AS ( <replaceable class="parameter">window_definition</replaceable> ) [, ...] ]
[ { UNION | INTERSECT | EXCEPT } [ ALL | DISTINCT ] <replaceable class="parameter">select</replaceable> ]
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index ea97d236ea8..562e4facd74 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -1811,14 +1811,12 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt,
qry->groupClause = transformGroupClause(pstate,
stmt->groupClause,
- stmt->groupByAll,
&qry->groupingSets,
&qry->targetList,
qry->sortClause,
EXPR_KIND_GROUP_BY,
false /* allow SQL92 rules */ );
qry->groupDistinct = stmt->groupDistinct;
- qry->groupByAll = stmt->groupByAll;
if (stmt->distinctClause == NIL)
{
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 9e05a314707..d27abe554cc 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -120,7 +120,6 @@ typedef struct SelectLimit
typedef struct GroupClause
{
bool distinct;
- bool all;
List *list;
} GroupClause;
@@ -13762,7 +13761,6 @@ simple_select:
n->whereClause = $6;
n->groupClause = ($7)->list;
n->groupDistinct = ($7)->distinct;
- n->groupByAll = ($7)->all;
n->havingClause = $8;
n->windowClause = $9;
$$ = (Node *) n;
@@ -13780,7 +13778,6 @@ simple_select:
n->whereClause = $6;
n->groupClause = ($7)->list;
n->groupDistinct = ($7)->distinct;
- n->groupByAll = ($7)->all;
n->havingClause = $8;
n->windowClause = $9;
$$ = (Node *) n;
@@ -14278,24 +14275,14 @@ group_clause:
GroupClause *n = palloc_object(GroupClause);
n->distinct = $3 == SET_QUANTIFIER_DISTINCT;
- n->all = false;
n->list = $4;
$$ = n;
}
- | GROUP_P BY ALL
- {
- GroupClause *n = palloc_object(GroupClause);
- n->distinct = false;
- n->all = true;
- n->list = NIL;
- $$ = n;
- }
| /*EMPTY*/
{
GroupClause *n = palloc_object(GroupClause);
n->distinct = false;
- n->all = false;
n->list = NIL;
$$ = n;
}
@@ -18796,7 +18783,6 @@ PLpgSQL_Expr: opt_distinct_clause opt_target_list
n->whereClause = $4;
n->groupClause = ($5)->list;
n->groupDistinct = ($5)->distinct;
- n->groupByAll = ($5)->all;
n->havingClause = $6;
n->windowClause = $7;
n->sortClause = $8;
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 2f7333e6786..485e33b9e5a 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -2746,9 +2746,6 @@ transformGroupingSet(List **flatresult,
* GROUP BY items will be added to the targetlist (as resjunk columns)
* if not already present, so the targetlist must be passed by reference.
*
- * If GROUP BY ALL is specified, the groupClause will be inferred to be all
- * non-aggregate, non-window expressions in the targetlist.
- *
* This is also used for window PARTITION BY clauses (which act almost the
* same, but are always interpreted per SQL99 rules).
*
@@ -2773,7 +2770,6 @@ transformGroupingSet(List **flatresult,
*
* pstate ParseState
* grouplist clause to transform
- * groupByAll is this a GROUP BY ALL statement?
* groupingSets reference to list to contain the grouping set tree
* targetlist reference to TargetEntry list
* sortClause ORDER BY clause (SortGroupClause nodes)
@@ -2781,8 +2777,7 @@ transformGroupingSet(List **flatresult,
* useSQL99 SQL99 rather than SQL92 syntax
*/
List *
-transformGroupClause(ParseState *pstate, List *grouplist, bool groupByAll,
- List **groupingSets,
+transformGroupClause(ParseState *pstate, List *grouplist, List **groupingSets,
List **targetlist, List *sortClause,
ParseExprKind exprKind, bool useSQL99)
{
@@ -2793,61 +2788,6 @@ transformGroupClause(ParseState *pstate, List *grouplist, bool groupByAll,
bool hasGroupingSets = false;
Bitmapset *seen_local = NULL;
- /* Handle GROUP BY ALL */
- if (groupByAll)
- {
- /* There cannot have been any explicit grouplist items */
- Assert(grouplist == NIL);
-
- /* Iterate over targets, adding acceptable ones to the result list */
- foreach_ptr(TargetEntry, tle, *targetlist)
- {
- /* Ignore junk TLEs */
- if (tle->resjunk)
- continue;
-
- /*
- * TLEs containing aggregates are not okay to add to GROUP BY
- * (compare checkTargetlistEntrySQL92). But the SQL standard
- * directs us to skip them, so it's fine.
- */
- if (pstate->p_hasAggs &&
- contain_aggs_of_level((Node *) tle->expr, 0))
- continue;
-
- /*
- * Likewise, TLEs containing window functions are not okay to add
- * to GROUP BY, and the SQL standard directs us to skip them.
- */
- if (pstate->p_hasWindowFuncs &&
- contain_windowfuncs((Node *) tle->expr))
- continue;
-
- /*
- * Otherwise, add the TLE to the result using default sort/group
- * semantics. We specify the parse location as the TLE's
- * location, despite the comment for addTargetToGroupList
- * discouraging that. The only other thing we could point to is
- * the ALL keyword, which seems unhelpful when there are multiple
- * TLEs.
- */
- result = addTargetToGroupList(pstate, tle,
- result, *targetlist,
- exprLocation((Node *) tle->expr));
- }
-
- /* If we found any acceptable targets, we're done */
- if (result != NIL)
- return result;
-
- /*
- * 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));
- }
-
/*
* Recursively flatten implicit RowExprs. (Technically this is only needed
* for GROUP BY, per the syntax rules for grouping sets, but we do it
@@ -3026,7 +2966,6 @@ transformWindowDefinitions(ParseState *pstate,
true /* force SQL99 rules */ );
partitionClause = transformGroupClause(pstate,
windef->partitionClause,
- false /* not GROUP BY ALL */ ,
NULL,
targetlist,
orderClause,
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 0ea38c18ca4..1b44b7a78d2 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -6555,9 +6555,7 @@ get_basic_select_query(Query *query, deparse_context *context)
save_ingroupby = context->inGroupBy;
context->inGroupBy = true;
- if (query->groupByAll)
- appendStringInfoString(buf, "ALL");
- else if (query->groupingSets == NIL)
+ if (query->groupingSets == NIL)
{
sep = "";
foreach(l, query->groupClause)
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 77f69f94607..5c8f9a07b62 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -220,7 +220,6 @@ typedef struct Query
List *groupClause; /* a list of SortGroupClause's */
bool groupDistinct; /* was GROUP BY DISTINCT used? */
- bool groupByAll; /* was GROUP BY ALL used? */
List *groupingSets; /* a list of GroupingSet's if present */
@@ -2341,7 +2340,6 @@ typedef struct SelectStmt
Node *whereClause; /* WHERE qualification */
List *groupClause; /* GROUP BY clauses */
bool groupDistinct; /* Is this GROUP BY DISTINCT? */
- bool groupByAll; /* Is this GROUP BY ALL? */
Node *havingClause; /* HAVING conditional-expression */
List *windowClause; /* WINDOW window_name AS (...), ... */
diff --git a/src/include/parser/parse_clause.h b/src/include/parser/parse_clause.h
index fe234611007..ca815a9d1bb 100644
--- a/src/include/parser/parse_clause.h
+++ b/src/include/parser/parse_clause.h
@@ -26,7 +26,6 @@ extern Node *transformLimitClause(ParseState *pstate, Node *clause,
ParseExprKind exprKind, const char *constructName,
LimitOption limitOption);
extern List *transformGroupClause(ParseState *pstate, List *grouplist,
- bool groupByAll,
List **groupingSets,
List **targetlist, List *sortClause,
ParseExprKind exprKind, bool useSQL99);
diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index f21f4e225da..5f0668382ed 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -1774,129 +1774,6 @@ select a, count(*) from t_having group by a having a = row(1.0)::avg_rec;
drop table t_having;
drop type avg_rec;
--
--- Test GROUP BY ALL
---
--- We don't care about the data here, just the proper transformation of the
--- GROUP BY clause, so test some queries and verify the EXPLAIN plans.
---
-CREATE TEMP TABLE t1 (
- a int,
- b int,
- c int
-);
--- basic example
-EXPLAIN (COSTS OFF) SELECT b, COUNT(*) FROM t1 GROUP BY ALL;
- QUERY PLAN
-----------------------
- HashAggregate
- Group Key: b
- -> Seq Scan on t1
-(3 rows)
-
--- multiple columns, non-consecutive order
-EXPLAIN (COSTS OFF) SELECT a, SUM(b), b FROM t1 GROUP BY ALL;
- QUERY PLAN
-----------------------
- HashAggregate
- Group Key: a, b
- -> Seq Scan on t1
-(3 rows)
-
--- multi columns, no aggregate
-EXPLAIN (COSTS OFF) SELECT a + b FROM t1 GROUP BY ALL;
- QUERY PLAN
-----------------------
- HashAggregate
- Group Key: (a + b)
- -> Seq Scan on t1
-(3 rows)
-
--- check we detect a non-top-level aggregate
-EXPLAIN (COSTS OFF) SELECT a, SUM(b) + 4 FROM t1 GROUP BY ALL;
- QUERY PLAN
-----------------------
- HashAggregate
- Group Key: a
- -> Seq Scan on t1
-(3 rows)
-
--- including grouped column is okay
-EXPLAIN (COSTS OFF) SELECT a, SUM(b) + a FROM t1 GROUP BY ALL;
- QUERY PLAN
-----------------------
- HashAggregate
- Group Key: a
- -> Seq Scan on t1
-(3 rows)
-
--- including non-grouped column, not so much
-EXPLAIN (COSTS OFF) SELECT a, SUM(b) + c FROM t1 GROUP BY ALL;
-ERROR: column "t1.c" must appear in the GROUP BY clause or be used in an aggregate function
-LINE 1: EXPLAIN (COSTS OFF) SELECT a, SUM(b) + c FROM t1 GROUP BY AL...
- ^
--- all aggregates, should reduce to GROUP BY ()
-EXPLAIN (COSTS OFF) SELECT COUNT(a), SUM(b) FROM t1 GROUP BY ALL;
- QUERY PLAN
-----------------------
- Aggregate
- Group Key: ()
- -> Seq Scan on t1
-(3 rows)
-
--- likewise with empty target list
-EXPLAIN (COSTS OFF) SELECT FROM t1 GROUP BY ALL;
- QUERY PLAN
------------------------
- Result
- Replaces: Aggregate
-(2 rows)
-
--- window functions are not to be included in GROUP BY, either
-EXPLAIN (COSTS OFF) SELECT a, COUNT(a) OVER (PARTITION BY a) FROM t1 GROUP BY ALL;
- QUERY PLAN
-----------------------------------
- WindowAgg
- Window: w1 AS (PARTITION BY a)
- -> Sort
- Sort Key: a
- -> HashAggregate
- Group Key: a
- -> Seq Scan on t1
-(7 rows)
-
--- all cols
-EXPLAIN (COSTS OFF) SELECT *, count(*) FROM t1 GROUP BY ALL;
- QUERY PLAN
-----------------------
- HashAggregate
- Group Key: a, b, c
- -> Seq Scan on t1
-(3 rows)
-
--- group by all with grouping element(s) (equivalent to GROUP BY's
--- default behavior, explicit antithesis to GROUP BY DISTINCT)
-EXPLAIN (COSTS OFF) SELECT a, count(*) FROM t1 GROUP BY ALL a;
- QUERY PLAN
-----------------------
- HashAggregate
- Group Key: a
- -> Seq Scan on t1
-(3 rows)
-
--- verify deparsing of GROUP BY ALL
-CREATE TEMP VIEW v1 AS SELECT b, COUNT(*) FROM t1 GROUP BY ALL;
-SELECT pg_get_viewdef('v1'::regclass);
- pg_get_viewdef
------------------------
- SELECT b, +
- count(*) AS count+
- FROM t1 +
- GROUP BY ALL;
-(1 row)
-
-DROP VIEW v1;
-DROP TABLE t1;
---
-- Test GROUP BY matching of join columns that are type-coerced due to USING
--
create temp table t1(f1 int, f2 int);
diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql
index 5cb9a9dc1be..b788152f0c2 100644
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -640,60 +640,6 @@ select a, count(*) from t_having group by a having a = row(1.0)::avg_rec;
drop table t_having;
drop type avg_rec;
---
--- Test GROUP BY ALL
---
--- We don't care about the data here, just the proper transformation of the
--- GROUP BY clause, so test some queries and verify the EXPLAIN plans.
---
-
-CREATE TEMP TABLE t1 (
- a int,
- b int,
- c int
-);
-
--- basic example
-EXPLAIN (COSTS OFF) SELECT b, COUNT(*) FROM t1 GROUP BY ALL;
-
--- multiple columns, non-consecutive order
-EXPLAIN (COSTS OFF) SELECT a, SUM(b), b FROM t1 GROUP BY ALL;
-
--- multi columns, no aggregate
-EXPLAIN (COSTS OFF) SELECT a + b FROM t1 GROUP BY ALL;
-
--- check we detect a non-top-level aggregate
-EXPLAIN (COSTS OFF) SELECT a, SUM(b) + 4 FROM t1 GROUP BY ALL;
-
--- including grouped column is okay
-EXPLAIN (COSTS OFF) SELECT a, SUM(b) + a FROM t1 GROUP BY ALL;
-
--- including non-grouped column, not so much
-EXPLAIN (COSTS OFF) SELECT a, SUM(b) + c FROM t1 GROUP BY ALL;
-
--- all aggregates, should reduce to GROUP BY ()
-EXPLAIN (COSTS OFF) SELECT COUNT(a), SUM(b) FROM t1 GROUP BY ALL;
-
--- likewise with empty target list
-EXPLAIN (COSTS OFF) SELECT FROM t1 GROUP BY ALL;
-
--- window functions are not to be included in GROUP BY, either
-EXPLAIN (COSTS OFF) SELECT a, COUNT(a) OVER (PARTITION BY a) FROM t1 GROUP BY ALL;
-
--- all cols
-EXPLAIN (COSTS OFF) SELECT *, count(*) FROM t1 GROUP BY ALL;
-
--- group by all with grouping element(s) (equivalent to GROUP BY's
--- default behavior, explicit antithesis to GROUP BY DISTINCT)
-EXPLAIN (COSTS OFF) SELECT a, count(*) FROM t1 GROUP BY ALL a;
-
--- verify deparsing of GROUP BY ALL
-CREATE TEMP VIEW v1 AS SELECT b, COUNT(*) FROM t1 GROUP BY ALL;
-SELECT pg_get_viewdef('v1'::regclass);
-
-DROP VIEW v1;
-DROP TABLE t1;
-
--
-- Test GROUP BY matching of join columns that are type-coerced due to USING
--
--
2.39.3 (Apple Git-146)
=
* Re: Fix GROUP BY ALL handling of ORDER BY operator semantics
@ 2026-07-15 14:53 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 14+ messages in thread
From: Tom Lane @ 2026-07-15 14:53 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Chao Li <li.evan.chao@gmail.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Fix GROUP BY ALL handling of ORDER BY operator semantics
@ 2026-07-17 20:15 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 14+ messages in thread
From: Tom Lane @ 2026-07-17 20:15 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Chao Li <li.evan.chao@gmail.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Fix GROUP BY ALL handling of ORDER BY operator semantics
@ 2026-07-17 21:59 Chao Li <li.evan.chao@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 14+ messages in thread
From: Chao Li @ 2026-07-17 21:59 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Daniel Gustafsson <daniel@yesql.se>; pgsql-hackers
> 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/
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Fix GROUP BY ALL handling of ORDER BY operator semantics
@ 2026-08-20 02:12 Bruce Momjian <bruce@momjian.us>
parent: Chao Li <li.evan.chao@gmail.com>
0 siblings, 0 replies; 14+ messages in thread
From: Bruce Momjian @ 2026-08-20 02:12 UTC (permalink / raw)
To: Chao Li <li.evan.chao@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Daniel Gustafsson <daniel@yesql.se>; pgsql-hackers
On Sat, Jul 18, 2026 at 05:59:10AM +0800, Chao Li wrote:
>
>
> > 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.
Yes, I removed it in my July update, thanks.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Do not let urgent matters crowd out time for investment in the future.
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Fix GROUP BY ALL handling of ORDER BY operator semantics
@ 2026-10-02 14:55 David Christensen <david+pg@pgguru.net>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 14+ messages in thread
From: David Christensen @ 2026-10-02 14:55 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Daniel Gustafsson <daniel@yesql.se>; Chao Li <li.evan.chao@gmail.com>; pgsql-hackers
Just FYI, refactored/revisited this patch for 20 in this thread:
https://www.postgresql.org/message-id/CAHM0NXia7YNC0nUvMfm4oD5FngopB8xeLsaZ7FFa5wMJ8ctk9Q%40mail.gma...
^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2026-10-02 14:55 UTC | newest]
Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-06-29 07:20 Fix GROUP BY ALL handling of ORDER BY operator semantics Chao Li <li.evan.chao@gmail.com>
2026-06-29 09:09 ` Chao Li <li.evan.chao@gmail.com>
2026-07-07 20:00 ` Miłosz Bieniek <bieniek.milosz@proton.me>
2026-07-13 20:10 ` Daniel Gustafsson <daniel@yesql.se>
2026-07-14 00:44 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-07-14 14:01 ` Daniel Gustafsson <daniel@yesql.se>
2026-07-14 16:08 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-07-14 16:33 ` Chao Li <li.evan.chao@gmail.com>
2026-07-15 08:01 ` Daniel Gustafsson <daniel@yesql.se>
2026-07-15 14:53 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-07-17 20:15 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-07-17 21:59 ` Chao Li <li.evan.chao@gmail.com>
2026-08-20 02:12 ` Bruce Momjian <bruce@momjian.us>
2026-10-02 14:55 ` David Christensen <david+pg@pgguru.net>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox