public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tom Lane <[email protected]>
To: David Christensen <[email protected]>
Cc: jian he <[email protected]>
Cc: Andrey Borodin <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: David G. Johnston <[email protected]>
Cc: Jelte Fennema-Nio <[email protected]>
Subject: Re: [PATCH] GROUP BY ALL
Date: Fri, 26 Sep 2025 13:46:19 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAHM0NXgB7hm-JH8c1o=o-4P7LYn_EoE=XZAW0ENdtP-nso60YQ@mail.gmail.com>
References: <[email protected]>
	<[email protected]>
	<CAHM0NXj1Gv_96WxZkiLKhxJVaYOGcmdZ2hApNcvWQ5HxV84Q=A@mail.gmail.com>
	<CACJufxEThXCX1QWyNE1gw--uJ4JQJ8f16MFH-kN+Ym3_02azTA@mail.gmail.com>
	<CAHM0NXg=yGzhn=H0_HiZ9u=Xq-40g1f+Bb=2e2LDxgd6a11bWw@mail.gmail.com>
	<CAHM0NXgB7hm-JH8c1o=o-4P7LYn_EoE=XZAW0ENdtP-nso60YQ@mail.gmail.com>

David Christensen <[email protected]> writes:
> Version 3 of this patch, incorporating some of the feedback thus far:

Some random comments:

I don't love where you put the parsing code.  Instead of
exposing addTargetToGroupList, I think you should have given
transformGroupClause the responsibility of expanding GROUP BY ALL.
One reason for that is that GROUP BY processing depends on the
results of transformSortClause.  This means that in v3, the
behavior of

	 SELECT x FROM ... GROUP BY x ORDER BY x;

will be subtly different from

	 SELECT x FROM ... GROUP BY ALL ORDER BY x;

which seems like a bug, or at least not desirable.  (You might need a
DESC or USING decoration in the ORDER BY to expose this clearly.)

The parsing code itself is not great:

+			TargetEntry *n = (TargetEntry*)lfirst(l1);
+			if (!contain_aggs_of_level((Node *)n->expr, 0))
+				qry->groupClause = addTargetToGroupList(pstate, n, qry->groupClause, qry->targetList, 0);

You should be skipping resjunk entries, and please use "tle" or some
such name less generic than "n", and "0" is not the correct location
to pass to addTargetToGroupList.  Probably the location of the ALL
keyword would be the ideal thing, but if we don't have that, the
notation for "no location known" is -1 not 0.  We could also
consider using exprLocation(tle->expr), despite the comment on
addTargetToGroupList that that's not the right thing.  This might
actually be better than pointing at the ALL anyway, since that would
give no hint which targetentry caused the error.

The test cases seem poorly designed, because it's very hard to be
sure whether the code expanded the ALL as-expected.  I think you
could improve them by not executing the queries but just EXPLAINing
them, so that the expanded group-by list is directly visible.

			regards, tom lane

view thread (49+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [PATCH] GROUP BY ALL
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox