Horton
Menu

Thread

  1. Fix GROUP BY ALL handling of ORDER BY operator semantics

    Chao Li <li.evan.chao@gmail.com> — 2026-06-29T07:20:24Z

    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/

  2. Re: Fix GROUP BY ALL handling of ORDER BY operator semantics

    Chao Li <li.evan.chao@gmail.com> — 2026-06-29T09:09:59Z

    
> 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/

  3. Re: Fix GROUP BY ALL handling of ORDER BY operator semantics

    Miłosz Bieniek <bieniek.milosz@proton.me> — 2026-07-07T20:00:26Z

    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