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

От:

Chao Li <li.evan.chao@gmail.com>

Дата:

14 июля в 19:33:41



> On Jul 15, 2026, at 01:08, Tom Lane  wrote:
> 
> Daniel Gustafsson  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/

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

От:

Tom Lane <tgl@sss.pgh.pa.us>

Дата:

14 июля в 03:44:16

Daniel Gustafsson  writes:
>> On 29 Jun 2026, at 09:20, Chao Li  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

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

От:

Tom Lane <tgl@sss.pgh.pa.us>

Дата:

15 июля в 17:53:40

Daniel Gustafsson  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

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

От:

Tom Lane <tgl@sss.pgh.pa.us>

Дата:

17 июля в 23:15:44

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

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

От:

Daniel Gustafsson <daniel@yesql.se>

Дата:

13 июля в 23:10:22

> On 29 Jun 2026, at 09:20, Chao Li  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

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

От:

Daniel Gustafsson <daniel@yesql.se>

Дата:

14 июля в 17:01:34

> On 14 Jul 2026, at 02:44, Tom Lane  wrote:
> 
> Daniel Gustafsson  writes:
>>> On 29 Jun 2026, at 09:20, Chao Li  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

Re: RegisterShmemCallbacks() does nothing in single-user mode

От:

Nathan Bossart <nathandbossart@gmail.com>

Дата:

25 августа в 23:00:59

Does this one deserve a mention on the open items wiki [0]?

[0] https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items

-- 
nathan

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

От:

Chao Li <li.evan.chao@gmail.com>

Дата:

18 июля в 00:59:10



> On Jul 18, 2026, at 04:15, Tom Lane  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/

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

От:

Daniel Gustafsson <daniel@yesql.se>

Дата:

15 июля в 11:01:50

> On 14 Jul 2026, at 18:08, Tom Lane  wrote:
> Daniel Gustafsson  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