v1-0001-Fix-GROUP-BY-ALL-handling-of-ORDER-BY-operator-se.patch

application/octet-stream

Filename: v1-0001-Fix-GROUP-BY-ALL-handling-of-ORDER-BY-operator-se.patch
Type: application/octet-stream
Part: 0
Message: Re: Fix GROUP BY ALL handling of ORDER BY operator semantics

Patch

Same data as JSON: GET /api/v1/attachments/:id/patch the parsed metadata as JSON — format, series position, per-file stats; never the diff bytes. API reference →
Format: format-patch
Series: patch v1-0001
Subject: Fix GROUP BY ALL handling of ORDER BY operator semantics
File+
src/backend/parser/parse_clause.c 53 23
src/test/regress/expected/aggregates.out 27 0
src/test/regress/sql/aggregates.sql 18 0
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)