public inbox for [email protected]
help / color / mirror / Atom feedFix GROUP BY ALL handling of ORDER BY operator semantics
3+ messages / 2 participants
[nested] [flat]
* Fix GROUP BY ALL handling of ORDER BY operator semantics
@ 2026-06-29 07:20 Chao Li <[email protected]>
0 siblings, 2 replies; 3+ messages in thread
From: Chao Li @ 2026-06-29 07:20 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Tom Lane <[email protected]>
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] 3+ messages in thread
* Re: Fix GROUP BY ALL handling of ORDER BY operator semantics
@ 2026-06-29 09:09 Chao Li <[email protected]>
parent: Chao Li <[email protected]>
1 sibling, 0 replies; 3+ messages in thread
From: Chao Li @ 2026-06-29 09:09 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Tom Lane <[email protected]>
> On Jun 29, 2026, at 15:20, Chao Li <[email protected]> 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:
[application/octet-stream] v1-0001-Fix-GROUP-BY-ALL-handling-of-ORDER-BY-operator-se.patch (8.1K, ../../[email protected]/2-v1-0001-Fix-GROUP-BY-ALL-handling-of-ORDER-BY-operator-se.patch)
download | inline diff:
From b6e19f5c8eea6a17a06a2d836f6f1f5a62c149c6 Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
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 <[email protected]>
---
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)
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Fix GROUP BY ALL handling of ORDER BY operator semantics
@ 2026-07-07 20:00 Miłosz Bieniek <[email protected]>
parent: Chao Li <[email protected]>
1 sibling, 0 replies; 3+ messages in thread
From: Miłosz Bieniek @ 2026-07-07 20:00 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]>
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 <[email protected]> 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] 3+ messages in thread
end of thread, other threads:[~2026-07-07 20:00 UTC | newest]
Thread overview: 3+ 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 <[email protected]>
2026-06-29 09:09 ` Chao Li <[email protected]>
2026-07-07 20:00 ` Miłosz Bieniek <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox