Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3776.700.51.11.12\))
Subject: Re: Fix GROUP BY ALL handling of ORDER BY operator semantics
From: Daniel Gustafsson <daniel@yesql.se>
In-Reply-To: <5243308F-8E5C-45AA-828C-FAD96C4F34DA@gmail.com>
Date: Mon, 13 Jul 2026 22:10:22 +0200
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
 Tom Lane <tgl@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Message-Id: <A0B7C417-5710-47B3-8FA2-C0DDA518DA07@yesql.se>
References: <5243308F-8E5C-45AA-828C-FAD96C4F34DA@gmail.com>
To: Chao Li <li.evan.chao@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/A0B7C417-5710-47B3-8FA2-C0DDA518DA07%40yesql.se>
Precedence: bulk

> 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 =3D lappend(*flatresult, grpc);
+	grouplist =3D lappend(grouplist, grpc);
 	found =3D 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