Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add GROUP BY ALL.

  2. Refactor to avoid code duplication in transformPLAssignStmt.

  3. Fix missed copying of groupDistinct in transformPLAssignStmt.

  1. [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2024-07-22T20:55:20Z

    I see that there'd been some chatter but not a lot of discussion about
a GROUP BY ALL feature/functionality.  There certainly is utility in
such a construct IMHO.

The grammar is unambiguous, so can support this construct in lieu of
the traditional GROUP BY clause.  Enclosed is a patch which adds this
via just scanning the TargetEntry list and adding anything that is not
an aggregate function call to the groupList.

Still need some docs; just throwing this out there and getting some feedback.

Thanks,

David

  2. Re: [PATCH] GROUP BY ALL

    David G. Johnston <david.g.johnston@gmail.com> — 2024-07-22T21:33:57Z

    On Mon, Jul 22, 2024 at 1:55 PM David Christensen <david@pgguru.net> wrote:

> I see that there'd been some chatter but not a lot of discussion about
> a GROUP BY ALL feature/functionality.  There certainly is utility in
> such a construct IMHO.
>
> Still need some docs; just throwing this out there and getting some
> feedback.
>
>
I strongly dislike adding this feature.  I'd only consider supporting it if
it was part of the SQL standard.

Code is written once and read many times.  This feature caters to
the writer, not the reader.  And furthermore usage of this is prone to be
to the writer's detriment as well.

David J.

  3. Re: [PATCH] GROUP BY ALL

    Isaac Morland <isaac.morland@gmail.com> — 2024-07-22T21:40:55Z

    On Mon, 22 Jul 2024 at 17:34, David G. Johnston <david.g.johnston@gmail.com>
wrote:

> On Mon, Jul 22, 2024 at 1:55 PM David Christensen <david@pgguru.net>
> wrote:
>
>> I see that there'd been some chatter but not a lot of discussion about
>> a GROUP BY ALL feature/functionality.  There certainly is utility in
>> such a construct IMHO.
>>
>> Still need some docs; just throwing this out there and getting some
>> feedback.
>>
>>
> I strongly dislike adding this feature.  I'd only consider supporting it
> if it was part of the SQL standard.
>
> Code is written once and read many times.  This feature caters to
> the writer, not the reader.  And furthermore usage of this is prone to be
> to the writer's detriment as well.
>

And for when this might be useful, the syntax for it already exists,
although a spurious error message is generated:

odyssey=> select (uw_term).*, count(*) from uw_term group by uw_term;
ERROR:  column "uw_term.term_id" must appear in the GROUP BY clause or be
used in an aggregate function
LINE 1: select (uw_term).*, count(*) from uw_term group by uw_term;
                ^

I'm not sure exactly what's going on here — it's like it's still seeing the
table name in the field list as only a table name and not the value
corresponding to the whole table as a row value (But in general I'm not
happy with the system's ability to figure out that a column's value has
only one possibility given the grouping columns). You can work around:

odyssey=> with t as (select uw_term, count(*) from uw_term group by
uw_term) select (uw_term).*, count from t;

This query works.

  4. Re: [PATCH] GROUP BY ALL

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-22T22:29:35Z

    "David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Mon, Jul 22, 2024 at 1:55 PM David Christensen <david@pgguru.net> wrote:
>> I see that there'd been some chatter but not a lot of discussion about
>> a GROUP BY ALL feature/functionality.  There certainly is utility in
>> such a construct IMHO.

> I strongly dislike adding this feature.  I'd only consider supporting it if
> it was part of the SQL standard.

Yeah ... my recollection is that we already rejected this idea.
If you want to re-litigate that, "throwing this out there" is
not a sufficient argument.

(Personally, I'd wonder exactly what ALL is quantified over: the
whole output of the FROM clause, or only columns mentioned in the
SELECT tlist, or what? And why that choice rather than another?)

			regards, tom lane

  5. Re: [PATCH] GROUP BY ALL

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-22T22:43:41Z

    Isaac Morland <isaac.morland@gmail.com> writes:
> And for when this might be useful, the syntax for it already exists,
> although a spurious error message is generated:

> odyssey=> select (uw_term).*, count(*) from uw_term group by uw_term;
> ERROR:  column "uw_term.term_id" must appear in the GROUP BY clause or be
> used in an aggregate function
> LINE 1: select (uw_term).*, count(*) from uw_term group by uw_term;
>                 ^

> I'm not sure exactly what's going on here

The SELECT entry is expanded into "uw_term.col1, uw_term.col2,
uw_term.col3, ...", and those single-column Vars don't match the
whole-row Var appearing in the GROUP BY list.  I guess if we
think this is important, we could add a proof rule saying that
a per-column Var is functionally dependent on a whole-row Var
of the same relation.  Odd that the point hasn't come up before
(though I guess that suggests that few people try this).

			regards, tom lane

  6. Re: [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2024-07-23T13:22:55Z

    On Mon, Jul 22, 2024 at 4:34 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> On Mon, Jul 22, 2024 at 1:55 PM David Christensen <david@pgguru.net> wrote:
>>
>> I see that there'd been some chatter but not a lot of discussion about
>> a GROUP BY ALL feature/functionality.  There certainly is utility in
>> such a construct IMHO.
>>
>> Still need some docs; just throwing this out there and getting some feedback.
>>
>
> I strongly dislike adding this feature.  I'd only consider supporting it if it was part of the SQL standard.
>
> Code is written once and read many times.  This feature caters to the writer, not the reader.  And furthermore usage of this is prone to be to the writer's detriment as well.

I'd say this feature (at least for me) caters to the investigator;
someone who is interactively looking at data hence why it would cater
to the writer.  Consider acquainting yourself with a large table that
has a large number of annoying-named fields where you want to look at
how different data is correlated or broken-down.  Something along the
lines of:

SELECT last_name, substring(first_name,1,1) as first_initial,
income_range, count(*) FROM census_data GROUP BY ALL;

If you are iteratively looking at things, adding or removing fields
from your breakdown, you only need to change it in a single place, the
tlist.  Additionally, expressions can be used transparently without
needing to repeat them.  (Yes, in practice, I'd often use GROUP BY 1,
2, say, but if you add more fields to this you need to edit in
multiple places.)

David

  7. Re: [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2024-07-23T13:37:02Z

    On Mon, Jul 22, 2024 at 5:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> "David G. Johnston" <david.g.johnston@gmail.com> writes:
> > On Mon, Jul 22, 2024 at 1:55 PM David Christensen <david@pgguru.net> wrote:
> >> I see that there'd been some chatter but not a lot of discussion about
> >> a GROUP BY ALL feature/functionality.  There certainly is utility in
> >> such a construct IMHO.
>
> > I strongly dislike adding this feature.  I'd only consider supporting it if
> > it was part of the SQL standard.
>
> Yeah ... my recollection is that we already rejected this idea.
> If you want to re-litigate that, "throwing this out there" is
> not a sufficient argument.

Heh, fair enough.  I actually wrote the patch after encountering the
syntax in DuckDB and it seemed easy enough to add to Postgres while
providing some utility, then ended up seeing a thread about it later.
I did not get the sense that this had been globally rejected; though
there were definitely voices against it, it seemed to trail off rather
than coming to a conclusion.

> (Personally, I'd wonder exactly what ALL is quantified over: the
> whole output of the FROM clause, or only columns mentioned in the
> SELECT tlist, or what? And why that choice rather than another?)

My intention here was to basically be a shorthand for "group by
specified non-aggregate fields in the select list".  Perhaps I'm not
being creative enough, but what is the interpretation/use case for
anything else? :-)

While there are other ways to accomplish these things, making an easy
way to GROUP BY with aggregate queries would be useful in the field,
particularly when doing iterative discovery work would save a lot of
time with a situation that is both detectable and hits users with
errors all the time.

I'm not married to the exact syntax of this feature; anything else
short and consistent could work if `ALL` is considered to potentially
gain a different interpretation in the future.

David

  8. Re: [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2024-07-23T13:42:07Z

    On Mon, Jul 22, 2024 at 4:41 PM Isaac Morland <isaac.morland@gmail.com> wrote:

> And for when this might be useful, the syntax for it already exists, although a spurious error message is generated:
>
> odyssey=> select (uw_term).*, count(*) from uw_term group by uw_term;
> ERROR:  column "uw_term.term_id" must appear in the GROUP BY clause or be used in an aggregate function
> LINE 1: select (uw_term).*, count(*) from uw_term group by uw_term;
>                 ^

This is with my patch, or existing postgres?  Grouping by record is
not actually what this patch is trying to do, so perhaps there is some
ambiguity; this is intended to GROUP BY any select item that is not an
aggregate function.

  9. Re: [PATCH] GROUP BY ALL

    Laurenz Albe <laurenz.albe@cybertec.at> — 2024-07-23T15:57:51Z

    On Tue, 2024-07-23 at 08:37 -0500, David Christensen wrote:
> My intention here was to basically be a shorthand for "group by
> specified non-aggregate fields in the select list".  Perhaps I'm not
> being creative enough, but what is the interpretation/use case for
> anything else? :-)

I am somewhat against this feature.
It is too much magic for my taste.

It might be handy for interactive use, but I would frown at an application
that uses code like that, much like I'd frown at "SELECT *" in application code.

Yours,
Laurenz Albe

  10. Re: [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2024-07-23T16:48:35Z

    On Tue, Jul 23, 2024 at 10:57 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> On Tue, 2024-07-23 at 08:37 -0500, David Christensen wrote:
> > My intention here was to basically be a shorthand for "group by
> > specified non-aggregate fields in the select list".  Perhaps I'm not
> > being creative enough, but what is the interpretation/use case for
> > anything else? :-)
>
> I am somewhat against this feature.
> It is too much magic for my taste.
>
> It might be handy for interactive use, but I would frown at an application
> that uses code like that, much like I'd frown at "SELECT *" in application code.

Sure, not everything that makes things easier is strictly necessary;
we could require `CAST(field AS text)` instead of `::text`, make
subqueries required for transforming oids into specific system tables
instead of `::regfoo` casts,  any number of other choices, remove
`SELECT *` as a parse option, but making it easier to do common things
interactively as a DBA has value as well.

David

  11. Re: [PATCH] GROUP BY ALL

    David G. Johnston <david.g.johnston@gmail.com> — 2024-07-23T16:59:29Z

    On Tue, Jul 23, 2024 at 9:48 AM David Christensen <david@pgguru.net> wrote:

>
> Sure, not everything that makes things easier is strictly necessary;
> we could require `CAST(field AS text)` instead of `::text`,


Probably should have...being standard and all.  Though syntactic sugar is
quite different from new behavior - transforming :: to CAST is
straight-forward.

make
> subqueries required for transforming oids into specific system tables
> instead of `::regfoo` casts,


Since OID is non-standard this falls within our purview.

  any number of other choices, remove
> `SELECT *` as a parse option,


Again, standard dictated.

but making it easier to do common things
> interactively as a DBA has value as well.
>
>
Agreed, but this isn't a clear-cut win, and doesn't have standard
conformance to tip the scale over fully.

Also, there are so many better tools for data exploration.  Removing this
quirk only marginally closes that gap.

David J.

  12. Re: [PATCH] GROUP BY ALL

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2024-07-23T17:02:36Z

    On 7/22/24 15:43, Tom Lane wrote:
> Isaac Morland <isaac.morland@gmail.com> writes:
>> And for when this might be useful, the syntax for it already exists,
>> although a spurious error message is generated:
> 
>> odyssey=> select (uw_term).*, count(*) from uw_term group by uw_term;
>> ERROR:  column "uw_term.term_id" must appear in the GROUP BY clause or be
>> used in an aggregate function
>> LINE 1: select (uw_term).*, count(*) from uw_term group by uw_term;
>>                  ^
> 
>> I'm not sure exactly what's going on here
> 
> The SELECT entry is expanded into "uw_term.col1, uw_term.col2,
> uw_term.col3, ...", and those single-column Vars don't match the
> whole-row Var appearing in the GROUP BY list.  I guess if we
> think this is important, we could add a proof rule saying that
> a per-column Var is functionally dependent on a whole-row Var
> of the same relation.  Odd that the point hasn't come up before
> (though I guess that suggests that few people try this).

I was just using this group-by-row feature last week to implement a temporal outer join in a way 
that would work for arbitrary tables. Here is some example SQL:

https://github.com/pjungwir/temporal_ops/blob/b10d65323749faa6c47956db2e8f95441e508fce/sql/outer_join.sql#L48-L66

That does `GROUP BY a` then `SELECT (x.a).*`.[1]

It is very useful for writing queries that don't want to know about the structure of the row.

I noticed the same error as Isaac. I worked around the problem by wrapping it in a subquery and 
decomposing the row outside. It's already an obscure feature, and an easy workaround might be why 
you haven't heard complaints before. I wouldn't mind writing a patch for that rule when I get a 
chance (if no one else gets to it first.)

[1] Actually I see it does `GROUP BY a, a.valid_at`, but that is surely more than I need. I think 
that `a.valid_at` is leftover from a previous version of the query.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  13. Re: [PATCH] GROUP BY ALL

    Peter Eisentraut <peter@eisentraut.org> — 2024-07-23T20:02:15Z

    On 23.07.24 00:29, Tom Lane wrote:
> (Personally, I'd wonder exactly what ALL is quantified over: the
> whole output of the FROM clause, or only columns mentioned in the
> SELECT tlist, or what? And why that choice rather than another?)

Looks like the main existing implementations take it to mean all entries 
in the SELECT list that are not aggregate functions.

https://duckdb.org/docs/sql/query_syntax/groupby.html#group-by-all
https://docs.databricks.com/en/sql/language-manual/sql-ref-syntax-qry-select-groupby.html#parameters
https://docs.snowflake.com/en/sql-reference/constructs/group-by#parameters

  14. Re: [PATCH] GROUP BY ALL

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-07-24T08:56:43Z

    On Mon, 22 Jul 2024 at 22:55, David Christensen <david@pgguru.net> wrote:
> I see that there'd been some chatter but not a lot of discussion about
> a GROUP BY ALL feature/functionality.  There certainly is utility in
> such a construct IMHO.

+1 from me. When exploring data, this is extremely useful because you
don't have to update the GROUP BY clause every time

Regarding the arguments against this:
1. I don't think this is any more unreadable than being able to GROUP
BY 1, 2, 3. Or being able to use column aliases from the SELECT in the
GROUP BY clause. Again this is already allowed. Personally I actually
think it's more readable than specifying e.g. 5 columns in the group
by, because then I have to cross-reference with columns in the SELECT
clause to find out if they are the same. With ALL I instantly know
it's grouped by all
2. This is indeed not part of the standard. But we have many things
that are not part of the standard. I think as long as we use the same
syntax as snowflake, databricks and duckdb I personally don't see a
big problem. Then we could try and make this be part of the standard
in the next version of the standard.

  15. Re: [PATCH] GROUP BY ALL

    Pavel Stehule <pavel.stehule@gmail.com> — 2024-07-24T09:07:21Z

    Hi

st 24. 7. 2024 v 10:57 odesílatel Jelte Fennema-Nio <postgres@jeltef.nl>
napsal:

> On Mon, 22 Jul 2024 at 22:55, David Christensen <david@pgguru.net> wrote:
> > I see that there'd been some chatter but not a lot of discussion about
> > a GROUP BY ALL feature/functionality.  There certainly is utility in
> > such a construct IMHO.
>
> +1 from me. When exploring data, this is extremely useful because you
> don't have to update the GROUP BY clause every time
>
> Regarding the arguments against this:
> 1. I don't think this is any more unreadable than being able to GROUP
> BY 1, 2, 3. Or being able to use column aliases from the SELECT in the
> GROUP BY clause. Again this is already allowed. Personally I actually
> think it's more readable than specifying e.g. 5 columns in the group
> by, because then I have to cross-reference with columns in the SELECT
> clause to find out if they are the same. With ALL I instantly know
> it's grouped by all
> 2. This is indeed not part of the standard. But we have many things
> that are not part of the standard. I think as long as we use the same
> syntax as snowflake, databricks and duckdb I personally don't see a
> big problem. Then we could try and make this be part of the standard
> in the next version of the standard.
>

 Aggregation against more columns is pretty slow and memory expensive in
Postgres.

DuckDB is an analytic database with different storage, different executors.
I like it very much, but I am not sure if I want to see these features in
Postgres.

Lot of developers are not very smart, and with proposed feature, then
instead to write correct and effective query, then they use just GROUP BY
ALL. Slow query should look like a slow query :-)

Regards

Pavel

  16. Re: [PATCH] GROUP BY ALL

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-07-24T14:12:01Z

    On Tue, Jul 23, 2024 at 6:53 PM David Christensen <david@pgguru.net> wrote:
>
> On Mon, Jul 22, 2024 at 4:34 PM David G. Johnston
> <david.g.johnston@gmail.com> wrote:
> >
> > On Mon, Jul 22, 2024 at 1:55 PM David Christensen <david@pgguru.net> wrote:
> >>
> >> I see that there'd been some chatter but not a lot of discussion about
> >> a GROUP BY ALL feature/functionality.  There certainly is utility in
> >> such a construct IMHO.
> >>
> >> Still need some docs; just throwing this out there and getting some feedback.
> >>
> >
> > I strongly dislike adding this feature.  I'd only consider supporting it if it was part of the SQL standard.
> >
> > Code is written once and read many times.  This feature caters to the writer, not the reader.  And furthermore usage of this is prone to be to the writer's detriment as well.
>
> I'd say this feature (at least for me) caters to the investigator;
> someone who is interactively looking at data hence why it would cater
> to the writer.  Consider acquainting yourself with a large table that
> has a large number of annoying-named fields where you want to look at
> how different data is correlated or broken-down.  Something along the
> lines of:

To me this looks like a feature that a data exploration tool may
implement instead of being part of the server. It would then provide
more statistics about each correlation/column set etc.

-- 
Best Wishes,
Ashutosh Bapat

  17. Re: [PATCH] GROUP BY ALL

    Greg Sabino Mullane <htamfids@gmail.com> — 2024-08-13T14:57:31Z

    On Tue, Jul 23, 2024 at 9:37 AM David Christensen <david@pgguru.net> wrote:

> I'm not married to the exact syntax of this feature; anything else short
> and consistent could work if `ALL` is considered to potentially
> gain a different interpretation in the future.
>

GROUP BY *

Just kidding. But a big +1 to the whole concept. It would have been
extraordinarily useful over the years.

Cheers,
Greg

  18. Re: [PATCH] GROUP BY ALL

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-08-17T17:12:51Z

    On Tue, 23 Jul 2024 at 22:02, Peter Eisentraut <peter@eisentraut.org> wrote:
> Looks like the main existing implementations take it to mean all entries
> in the SELECT list that are not aggregate functions.
>
> https://duckdb.org/docs/sql/query_syntax/groupby.html#group-by-all
> https://docs.databricks.com/en/sql/language-manual/sql-ref-syntax-qry-select-groupby.html#parameters
> https://docs.snowflake.com/en/sql-reference/constructs/group-by#parameters

Oracle added support for GROUP BY ALL too now:
https://danischnider.wordpress.com/2025/08/05/oracle-23-9-supports-group-by-all/

  19. Re: [PATCH] GROUP BY ALL

    Peter Eisentraut <peter@eisentraut.org> — 2025-09-26T07:02:08Z

    On 17.08.25 19:12, Jelte Fennema-Nio wrote:
> On Tue, 23 Jul 2024 at 22:02, Peter Eisentraut <peter@eisentraut.org> wrote:
>> Looks like the main existing implementations take it to mean all entries
>> in the SELECT list that are not aggregate functions.
>>
>> https://duckdb.org/docs/sql/query_syntax/groupby.html#group-by-all
>> https://docs.databricks.com/en/sql/language-manual/sql-ref-syntax-qry-select-groupby.html#parameters
>> https://docs.snowflake.com/en/sql-reference/constructs/group-by#parameters
> 
> Oracle added support for GROUP BY ALL too now:
> https://danischnider.wordpress.com/2025/08/05/oracle-23-9-supports-group-by-all/

The proposal for GROUP BY ALL was accepted into the SQL standard draft 
yesterday.  So maybe someone wants to take this up again.

The initially proposed patch appears to have the right idea overall. 
But it does not handle more complex cases like

     SELECT a, SUM(b)+a FROM t1 GROUP BY ALL;

correctly.  The piece of code that does

     if (!IsA(n->expr,Aggref))

should be generalized to check for aggregates not only at the top level.

(For explanation:  GROUP BY ALL expands to all select list entries that 
do not contain aggregates.  So the above would expand to

     SELECT a, SUM(b)+a FROM t1 GROUP BY a;

which should then be rejected based on the existing rules.)

  20. Re: [PATCH] GROUP BY ALL

    Andrey Borodin <x4mmm@yandex-team.ru> — 2025-09-26T08:45:01Z

    
> On 26 Sep 2025, at 12:02, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
> maybe someone wants to take this up again.

I've compared my old patch with David's, and his version looks better.
If David is not going to work on this in nearest future - I'l like to pick up the work.
Either way I'll join as reviewer.

Thanks!


Best regards, Andrey Borodin.

  21. Re: [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2025-09-26T11:16:15Z

    
> On Sep 26, 2025, at 3:45 AM, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> 
> 
> 
>> On 26 Sep 2025, at 12:02, Peter Eisentraut <peter@eisentraut.org> wrote:
>> 
>> maybe someone wants to take this up again.
> 
> I've compared my old patch with David's, and his version looks better.
> If David is not going to work on this in nearest future - I'l like to pick up the work.
> Either way I'll join as reviewer.
> 
> Thanks!

I’m interested in picking it up again but would appreciate the review. 

Thanks,

David

  22. Re: [PATCH] GROUP BY ALL

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-26T14:11:55Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> The initially proposed patch appears to have the right idea overall. 
> But it does not handle more complex cases like
>      SELECT a, SUM(b)+a FROM t1 GROUP BY ALL;

> (For explanation:  GROUP BY ALL expands to all select list entries that 
> do not contain aggregates.  So the above would expand to
>      SELECT a, SUM(b)+a FROM t1 GROUP BY a;
> which should then be rejected based on the existing rules.)

I thought I understood this definition, up till your last
comment.  What's invalid about that expanded query?

regression=# create table t1 (a int, b int);
CREATE TABLE
regression=# SELECT a, SUM(b)+a FROM t1 GROUP BY a;
 a | ?column? 
---+----------
(0 rows)


			regards, tom lane

  23. Re: [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2025-09-26T15:45:53Z

    On Fri, Sep 26, 2025 at 6:16 AM David Christensen <david@pgguru.net> wrote:
>
>
>
> > On Sep 26, 2025, at 3:45 AM, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> >
> > 
> >
> >> On 26 Sep 2025, at 12:02, Peter Eisentraut <peter@eisentraut.org> wrote:
> >>
> >> maybe someone wants to take this up again.
> >
> > I've compared my old patch with David's, and his version looks better.
> > If David is not going to work on this in nearest future - I'l like to pick up the work.
> > Either way I'll join as reviewer.
> >
> > Thanks!
>
> I’m interested in picking it up again but would appreciate the review.

Here is a rebased version with a few more tests.  I also changed the
main check here to using `!contain_agg_clause` instead of
`!IsA(Aggref))` directly.  (This was defined in `optimizer/clauses.h`,
but we already are pulling in `optimizer.h`, so it felt valid to me.)

David

  24. Re: [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2025-09-26T15:49:25Z

    Added to commitfest as https://commitfest.postgresql.org/patch/6085/

  25. Re: [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2025-09-26T15:52:36Z

    On Fri, Sep 26, 2025 at 9:12 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Eisentraut <peter@eisentraut.org> writes:
> > The initially proposed patch appears to have the right idea overall.
> > But it does not handle more complex cases like
> >      SELECT a, SUM(b)+a FROM t1 GROUP BY ALL;
>
> > (For explanation:  GROUP BY ALL expands to all select list entries that
> > do not contain aggregates.  So the above would expand to
> >      SELECT a, SUM(b)+a FROM t1 GROUP BY a;
> > which should then be rejected based on the existing rules.)
>
> I thought I understood this definition, up till your last
> comment.  What's invalid about that expanded query?
>
> regression=# create table t1 (a int, b int);
> CREATE TABLE
> regression=# SELECT a, SUM(b)+a FROM t1 GROUP BY a;
>  a | ?column?
> ---+----------
> (0 rows)

Agreed that this shouldn't be an error; added a similar test case to
v2 of this patch.

David

  26. Re: [PATCH] GROUP BY ALL

    Peter Eisentraut <peter@eisentraut.org> — 2025-09-26T15:54:18Z

    On 26.09.25 16:11, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> The initially proposed patch appears to have the right idea overall.
>> But it does not handle more complex cases like
>>       SELECT a, SUM(b)+a FROM t1 GROUP BY ALL;
> 
>> (For explanation:  GROUP BY ALL expands to all select list entries that
>> do not contain aggregates.  So the above would expand to
>>       SELECT a, SUM(b)+a FROM t1 GROUP BY a;
>> which should then be rejected based on the existing rules.)
> 
> I thought I understood this definition, up till your last
> comment.  What's invalid about that expanded query?
> 
> regression=# create table t1 (a int, b int);
> CREATE TABLE
> regression=# SELECT a, SUM(b)+a FROM t1 GROUP BY a;
>   a | ?column?
> ---+----------
> (0 rows)

This was a sloppy example.  Here is a better one:

     create table t1 (a int, b int, c int);

     select a, sum(b)+c from t1 group by all;

This is equivalent to

     select a, sum(b)+c from t1 group by a;

which would be rejected as

     ERROR:  column "t1.c" must appear in the GROUP BY clause or be used
     in an aggregate function

  27. Re: [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2025-09-26T15:59:26Z

    On Fri, Sep 26, 2025 at 10:54 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 26.09.25 16:11, Tom Lane wrote:
> > Peter Eisentraut <peter@eisentraut.org> writes:
> >> The initially proposed patch appears to have the right idea overall.
> >> But it does not handle more complex cases like
> >>       SELECT a, SUM(b)+a FROM t1 GROUP BY ALL;
> >
> >> (For explanation:  GROUP BY ALL expands to all select list entries that
> >> do not contain aggregates.  So the above would expand to
> >>       SELECT a, SUM(b)+a FROM t1 GROUP BY a;
> >> which should then be rejected based on the existing rules.)
> >
> > I thought I understood this definition, up till your last
> > comment.  What's invalid about that expanded query?
> >
> > regression=# create table t1 (a int, b int);
> > CREATE TABLE
> > regression=# SELECT a, SUM(b)+a FROM t1 GROUP BY a;
> >   a | ?column?
> > ---+----------
> > (0 rows)
>
> This was a sloppy example.  Here is a better one:
>
>      create table t1 (a int, b int, c int);
>
>      select a, sum(b)+c from t1 group by all;
>
> This is equivalent to
>
>      select a, sum(b)+c from t1 group by a;
>
> which would be rejected as
>
>      ERROR:  column "t1.c" must appear in the GROUP BY clause or be used
>      in an aggregate function

Verified that with v2 that this is what happens in this case; will
include this and whatever other feedback there is in a v3.

  28. Re: [PATCH] GROUP BY ALL

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-26T16:04:54Z

    David Christensen <david@pgguru.net> writes:
> Here is a rebased version with a few more tests.  I also changed the
> main check here to using `!contain_agg_clause` instead of
> `!IsA(Aggref))` directly.  (This was defined in `optimizer/clauses.h`,
> but we already are pulling in `optimizer.h`, so it felt valid to me.)

contain_agg_clause will blow up on a SubLink, so I doubt this is
gonna be robust.

			regards, tom lane

  29. Re: [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2025-09-26T16:13:04Z

    On Fri, Sep 26, 2025 at 11:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Christensen <david@pgguru.net> writes:
> > Here is a rebased version with a few more tests.  I also changed the
> > main check here to using `!contain_agg_clause` instead of
> > `!IsA(Aggref))` directly.  (This was defined in `optimizer/clauses.h`,
> > but we already are pulling in `optimizer.h`, so it felt valid to me.)
>
> contain_agg_clause will blow up on a SubLink, so I doubt this is
> gonna be robust.

Fair enough, see that Assert now; easy enough to make a new
expression_tree_walker that only looks for Aggref and short-circuits
SubLink (which I assume is the right behavior here, but might have to
add some more tests/play around with subqueries in the GROUP BY ALL
part).

David

  30. Re: [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2025-09-26T16:17:29Z

    On Fri, Sep 26, 2025 at 11:13 AM David Christensen <david@pgguru.net> wrote:
>
> On Fri, Sep 26, 2025 at 11:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > David Christensen <david@pgguru.net> writes:
> > > Here is a rebased version with a few more tests.  I also changed the
> > > main check here to using `!contain_agg_clause` instead of
> > > `!IsA(Aggref))` directly.  (This was defined in `optimizer/clauses.h`,
> > > but we already are pulling in `optimizer.h`, so it felt valid to me.)
> >
> > contain_agg_clause will blow up on a SubLink, so I doubt this is
> > gonna be robust.
>
> Fair enough, see that Assert now; easy enough to make a new
> expression_tree_walker that only looks for Aggref and short-circuits
> SubLink (which I assume is the right behavior here, but might have to
> add some more tests/play around with subqueries in the GROUP BY ALL
> part).

Or contain_aggs_of_level(), assuming I can suss out how to get the
current level... :D  Anyway, will mess with this for a bit.

David

  31. Re: [PATCH] GROUP BY ALL

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-26T16:23:35Z

    David Christensen <david@pgguru.net> writes:
> On Fri, Sep 26, 2025 at 11:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> contain_agg_clause will blow up on a SubLink, so I doubt this is
>> gonna be robust.

> Fair enough, see that Assert now; easy enough to make a new
> expression_tree_walker that only looks for Aggref and short-circuits
> SubLink (which I assume is the right behavior here, but might have to
> add some more tests/play around with subqueries in the GROUP BY ALL
> part).

No, I think the correct behavior would have to be to descend into
SubLinks to see if they contain any aggregates belonging to the
outer query level.

However (looks around) we do already have that code.
See contain_aggs_of_level.  (contain_agg_clause is essentially
a simplified version that is okay to use in the planner because
it's already gotten rid of sublinks.)

What mainly concerns me at this point is whether we've identified
aggregate levels at the point in parsing where you want to run this.
I have a bit of a worry that that might interact with grouping.
Presumably the SQL committee thought about that, so it's probably
soluble, but ...

			regards, tom lane

  32. Re: [PATCH] GROUP BY ALL

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-26T16:24:37Z

    David Christensen <david@pgguru.net> writes:
> Or contain_aggs_of_level(), assuming I can suss out how to get the
> current level... :D  Anyway, will mess with this for a bit.

Current level is zero by definition ...

			regards, tom lane

  33. Re: [PATCH] GROUP BY ALL

    jian he <jian.universality@gmail.com> — 2025-09-26T16:37:29Z

    On Fri, Sep 26, 2025 at 11:46 PM David Christensen <david@pgguru.net> wrote:
>
> >
> > I’m interested in picking it up again but would appreciate the review.
>
> Here is a rebased version with a few more tests.  I also changed the
> main check here to using `!contain_agg_clause` instead of
> `!IsA(Aggref))` directly.  (This was defined in `optimizer/clauses.h`,
> but we already are pulling in `optimizer.h`, so it felt valid to me.)
>

hi.
I only briefly browse the patch text file, so forgive me.
seems missing deparse regress tests

i think you may need one test like:

create view v1 as SELECT b, COUNT(*) FROM t1 GROUP BY ALL;
\sv v1

  34. Re: [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2025-09-26T16:56:49Z

    On Fri, Sep 26, 2025 at 11:38 AM jian he <jian.universality@gmail.com> wrote:
>
> On Fri, Sep 26, 2025 at 11:46 PM David Christensen <david@pgguru.net> wrote:
> >
> > >
> > > I’m interested in picking it up again but would appreciate the review.
> >
> > Here is a rebased version with a few more tests.  I also changed the
> > main check here to using `!contain_agg_clause` instead of
> > `!IsA(Aggref))` directly.  (This was defined in `optimizer/clauses.h`,
> > but we already are pulling in `optimizer.h`, so it felt valid to me.)
> >
>
> hi.
> I only briefly browse the patch text file, so forgive me.
> seems missing deparse regress tests
>
> i think you may need one test like:
>
> create view v1 as SELECT b, COUNT(*) FROM t1 GROUP BY ALL;
> \sv v1

Thanks, good suggestion; not sure the appropriate final location for
this, but it did show a bug in the current patch.

David

  35. Re: [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2025-09-26T17:00:01Z

    Version 3 of this patch, incorporating some of the feedback thus far:


On Fri, Sep 26, 2025 at 11:56 AM David Christensen <david@pgguru.net> wrote:
>
> On Fri, Sep 26, 2025 at 11:38 AM jian he <jian.universality@gmail.com> wrote:
> >
> > On Fri, Sep 26, 2025 at 11:46 PM David Christensen <david@pgguru.net> wrote:
> > >
> > > >
> > > > I’m interested in picking it up again but would appreciate the review.
> > >
> > > Here is a rebased version with a few more tests.  I also changed the
> > > main check here to using `!contain_agg_clause` instead of
> > > `!IsA(Aggref))` directly.  (This was defined in `optimizer/clauses.h`,
> > > but we already are pulling in `optimizer.h`, so it felt valid to me.)
> > >
> >
> > hi.
> > I only briefly browse the patch text file, so forgive me.
> > seems missing deparse regress tests
> >
> > i think you may need one test like:
> >
> > create view v1 as SELECT b, COUNT(*) FROM t1 GROUP BY ALL;
> > \sv v1
>
> Thanks, good suggestion; not sure the appropriate final location for
> this, but it did show a bug in the current patch.
>
> David

  36. Re: [PATCH] GROUP BY ALL

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-26T17:46:19Z

    David Christensen <david@pgguru.net> 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

  37. Re: [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2025-09-26T18:50:27Z

    On Fri, Sep 26, 2025 at 12:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Some random comments:

[snip]

Great feedback, thanks; attached is v4 which should address these comments.

David

  38. Re: [PATCH] GROUP BY ALL

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-26T20:18:32Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> On 26.09.25 16:11, Tom Lane wrote:
>> I thought I understood this definition, up till your last
>> comment.  What's invalid about that expanded query?

> This was a sloppy example.  Here is a better one:
>      create table t1 (a int, b int, c int);
>      select a, sum(b)+c from t1 group by all;
> This is equivalent to
>      select a, sum(b)+c from t1 group by a;
> which would be rejected as
>      ERROR:  column "t1.c" must appear in the GROUP BY clause or be used
>      in an aggregate function

Got it, mostly.  There is an edge case, though: what if there are no
candidate grouping items?  I see these test cases in David's patch:

+-- oops all aggregates
+EXPLAIN (COSTS OFF) SELECT COUNT(a), SUM(b) FROM t1 GROUP BY ALL;
+      QUERY PLAN      
+----------------------
+ Aggregate
+   ->  Seq Scan on t1
+(2 rows)
+
+-- empty column list
+EXPLAIN (COSTS OFF) SELECT FROM t1 GROUP BY ALL;
+   QUERY PLAN   
+----------------
+ Seq Scan on t1
+(1 row)

That is, in such cases the patch behaves as if there were no GROUP BY
clause at all, which seems kinda dubious.  Should this be an error,
and if not what's it supposed to do?  The second case is outside the
SQL spec so I'm not expecting guidance on that, but surely the
committee thought about the first case.

Also, what about window functions in the tlist?  If you do

regression=# explain select sum(q1) over(partition by q2) from int8_tbl group by 1;

you get 

ERROR:  window functions are not allowed in GROUP BY
LINE 1: explain select sum(q1) over(partition by q2) from int8_tbl g...
                       ^

but that's not what is happening with

regression=# explain select sum(q1) over(partition by q2) from int8_tbl group by all;
ERROR:  column "int8_tbl.q2" must appear in the GROUP BY clause or be used in an aggregate function
LINE 1: explain select sum(q1) over(partition by q2) from int8_tbl g...
                                                 ^

(I didn't stop to figure out why this isn't giving the same error, but
maybe it's an order-of-checks thing.)  In any case: should this give
"window functions are not allowed in GROUP BY", or should the
window-function-containing tlist item be silently skipped by GROUP BY
ALL?  Trying to make it work is surely not the right answer.

			regards, tom lane

  39. Re: [PATCH] GROUP BY ALL

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-26T20:37:58Z

    [ trimming the cc: list because gmail decided my last was spam ]

David Christensen <david@pgguru.net> writes:
> Great feedback, thanks; attached is v4 which should address these comments.

I did a pass of cleanup over this --- mostly cosmetic, but not
entirely.  Along the way I discovered a pre-existing bug:
transformSelectStmt does
 	qry->groupDistinct = stmt->groupDistinct;
but transformPLAssignStmt fails to, with the result that GROUP BY
DISTINCT would misbehave if used in a plpgsql "expression" context.
I'm not hugely surprised that no one has reported that from the
field, but nonetheless it's broken.  In the attached v5 I just
quickly added the missing line and moved on, but we'll need to
back-patch that bit.  (Maybe we'd be well advised to refactor to
reduce the amount of duplicated code that needs to be kept in sync?)

I have not attempted to address the definitional issues I just
queried Peter about.  Other open items:

* Documentation

* The test cases deserve reconsideration now that we think their
charter only goes as far as EXPLAIN'ing the results; some of them
seem pretty redundant in this context.

			regards, tom lane

  40. Re: [PATCH] GROUP BY ALL

    Peter Eisentraut <peter@eisentraut.org> — 2025-09-27T13:30:29Z

    On 26.09.25 22:18, Tom Lane wrote:
> Got it, mostly.  There is an edge case, though: what if there are no
> candidate grouping items?  I see these test cases in David's patch:
> 
> +-- oops all aggregates
> +EXPLAIN (COSTS OFF) SELECT COUNT(a), SUM(b) FROM t1 GROUP BY ALL;
> +      QUERY PLAN
> +----------------------
> + Aggregate
> +   ->  Seq Scan on t1
> +(2 rows)
> +
> +-- empty column list
> +EXPLAIN (COSTS OFF) SELECT FROM t1 GROUP BY ALL;
> +   QUERY PLAN
> +----------------
> + Seq Scan on t1
> +(1 row)
> 
> That is, in such cases the patch behaves as if there were no GROUP BY
> clause at all, which seems kinda dubious.  Should this be an error,
> and if not what's it supposed to do?

These should resolve to GROUP BY ().

> Also, what about window functions in the tlist?

> (I didn't stop to figure out why this isn't giving the same error, but
> maybe it's an order-of-checks thing.)  In any case: should this give
> "window functions are not allowed in GROUP BY", or should the
> window-function-containing tlist item be silently skipped by GROUP BY
> ALL?  Trying to make it work is surely not the right answer.

Hmm, I don't know.  The syntactic transformation talks about select list 
elements that "do not directly contain an <aggregate function>", but 
that can also appear as part of <window function>, so the syntactic 
transformation might appear to apply only to some types of window 
functions, which doesn't make sense to me.

I don't know what a sensible behavior should be here.  Maybe in this 
first patch version just reject use of GROUP BY ALL if you find any 
window functions in the select list.

  41. Re: [PATCH] GROUP BY ALL

    Peter Eisentraut <peter@eisentraut.org> — 2025-09-27T13:40:05Z

    On 26.09.25 18:23, Tom Lane wrote:
> No, I think the correct behavior would have to be to descend into
> SubLinks to see if they contain any aggregates belonging to the
> outer query level.
> 
> However (looks around) we do already have that code.
> See contain_aggs_of_level.  (contain_agg_clause is essentially
> a simplified version that is okay to use in the planner because
> it's already gotten rid of sublinks.)
> 
> What mainly concerns me at this point is whether we've identified
> aggregate levels at the point in parsing where you want to run this.
> I have a bit of a worry that that might interact with grouping.
> Presumably the SQL committee thought about that, so it's probably
> soluble, but ...

The language used in the standard at the moment is the select list 
elements that "do not directly contain an <aggregate function>", where 
"directly contain" is a term of art that means "contains without an 
intervening instance of <subquery>, <within group specification>, or 
<set function specification> that is not an <ordered set function>".  So 
it means not to look into subqueries.

Note that in standard SQL, the GROUP BY clause can only contain plain 
column references, not expressions, so this question is kind of moot in 
that context, because the query would be invalid no matter whether you 
transform the GROUP BY ALL to group by the subquery or not.

For this first patch version, I suggest you reject the use of GROUP BY 
ALL if you find a subquery in the select list, unless you have an 
unambiguous better solution.

(It was discussed to relax this restriction, so this discussion is 
useful to collect some questions related to that.)

  42. Re: [PATCH] GROUP BY ALL

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-27T16:03:10Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> The language used in the standard at the moment is the select list 
> elements that "do not directly contain an <aggregate function>", where 
> "directly contain" is a term of art that means "contains without an 
> intervening instance of <subquery>, <within group specification>, or 
> <set function specification> that is not an <ordered set function>".  So 
> it means not to look into subqueries.

TBH, that is obvious nonsense.  A subquery could contain an aggregate
function that we've already identified as being of the current query
level.  Putting such a construct into the GROUP BY list would create
an invalid query (cf. checkTargetlistEntrySQL92).  Similarly, putting
a window function into the GROUP BY list would create an invalid
query.

> Note that in standard SQL, the GROUP BY clause can only contain plain 
> column references, not expressions, so this question is kind of moot in 
> that context, because the query would be invalid no matter whether you 
> transform the GROUP BY ALL to group by the subquery or not.

So according to the standard, this:

	select a+b, count(*) from ... group by all;

would be invalid because a+b couldn't be written directly in
GROUP BY?  I can't see us rejecting that though, since we do
allow a+b in GROUP BY.

Seems like we're getting very little help from the standard as to
what this construct actually means.  I suggest that we ignore the
current draft as not having been thought through quite enough yet,
and make ALL skip any tlist entries that contain_aggs_of_level
zero or contain_windowfuncs.  If that means we're extending the
standard, so be it --- we've already extended GROUP BY quite a lot,
it seems.

			regards, tom lane

  43. Re: [PATCH] GROUP BY ALL

    Vik Fearing <vik@postgresfriends.org> — 2025-09-27T18:14:58Z

    On 27/09/2025 18:03, Tom Lane wrote:
> So according to the standard, this:
>
> 	select a+b, count(*) from ... group by all;
>
> would be invalid because a+b couldn't be written directly in
> GROUP BY?


Correct.


> I can't see us rejecting that though, since we do
> allow a+b in GROUP BY.


No, nor do I.  Also, there were rumors about adding expressions to group 
by in the standard but no formal proposal yet.


FWIW, I opposed adding this GROUP BY ALL feature, but I was 
outnumbered.  I hope to never see it in production.

-- 

Vik Fearing

  44. Re: [PATCH] GROUP BY ALL

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-27T22:23:16Z

    Here's a v6 that's rebased up to HEAD and contains fixes for the
semantic issues we discussed.  It still lacks documentation, but
otherwise I think it's about ready to go.

			regards, tom lane

  45. Re: [PATCH] GROUP BY ALL

    Chao Li <li.evan.chao@gmail.com> — 2025-09-28T09:05:04Z

    
> On Sep 28, 2025, at 06:23, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> +		/* Iterate over targets, adding acceptable ones to the result list */
> +		foreach_ptr(TargetEntry, tle, *targetlist)
> +		{
> +			/* Ignore junk TLEs */
> +			if (tle->resjunk)
> +				continue;


Do we want to specifically check “ctid”?

If a user does:

```
select ctid, col1, col2, ... from t group by all;
```

It would be equivalent to no group by. Combing “select ctid” with “group by all” seems totally useless, but when users do such things, we can optimize that.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  46. Re: [PATCH] GROUP BY ALL

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-28T14:20:29Z

    Chao Li <li.evan.chao@gmail.com> writes:
> Do we want to specifically check “ctid”?

No.  Complete waste of effort.

			regards, tom lane

  47. Re: [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2025-09-28T19:18:48Z

    On Sat, Sep 27, 2025 at 5:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Here's a v6 that's rebased up to HEAD and contains fixes for the
> semantic issues we discussed.  It still lacks documentation, but
> otherwise I think it's about ready to go.

Here is v7 with a stab at docs; fairly minimal at this point, but
touching the two areas that are likely to need adjusting.  When
adjusting the docs for sql-select, I noticed that the grammar also
supports `GROUP BY ALL <grouping_elements>`, so I also added a test to
ensure that this syntax is explicitly supported. (It seems like it
works as-is without further grammar adjustments, but I was a little
worried when I first saw that fact... :D)  Not sure that
aggregates.sql is still the right place for all of these bits, but it
does seem like having all things `GROUP BY ALL`-related tested in the
same place is a nice property, so leaving there for now.

David

  48. Re: [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2025-09-28T20:34:28Z

    On Sun, Sep 28, 2025 at 2:18 PM David Christensen <david@pgguru.net> wrote:
>
> On Sat, Sep 27, 2025 at 5:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Here's a v6 that's rebased up to HEAD and contains fixes for the
> > semantic issues we discussed.  It still lacks documentation, but
> > otherwise I think it's about ready to go.
>
> Here is v7 with a stab at docs; fairly minimal at this point, but
> touching the two areas that are likely to need adjusting.  When
> adjusting the docs for sql-select, I noticed that the grammar also
> supports `GROUP BY ALL <grouping_elements>`, so I also added a test to
> ensure that this syntax is explicitly supported. (It seems like it
> works as-is without further grammar adjustments, but I was a little
> worried when I first saw that fact... :D)  Not sure that
> aggregates.sql is still the right place for all of these bits, but it
> does seem like having all things `GROUP BY ALL`-related tested in the
> same place is a nice property, so leaving there for now.

This time with attachment!

  49. Re: [PATCH] GROUP BY ALL

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-29T20:58:37Z

    David Christensen <david@pgguru.net> writes:
>> Here is v7 with a stab at docs; fairly minimal at this point, but
>> touching the two areas that are likely to need adjusting.

I did some more word-smithing on the docs and pushed it.

>> When
>> adjusting the docs for sql-select, I noticed that the grammar also
>> supports `GROUP BY ALL <grouping_elements>`, so I also added a test to
>> ensure that this syntax is explicitly supported.

+1, can't hurt.

>> (It seems like it
>> works as-is without further grammar adjustments, but I was a little
>> worried when I first saw that fact... :D)

Bison would have been vocal about it if you'd introduced any
ambiguity.  Still, I didn't feel like looking around to see if we
already covered this syntax, and I agree it's close enough to being
an issue to be worth covering.

Thanks for the patch!

			regards, tom lane

  50. Re: [PATCH] GROUP BY ALL

    David Christensen <david@pgguru.net> — 2025-09-29T21:13:11Z

    > On Sep 29, 2025, at 3:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> David Christensen <david@pgguru.net> writes:
>>> Here is v7 with a stab at docs; fairly minimal at this point, but
>>> touching the two areas that are likely to need adjusting.
> 
> I did some more word-smithing on the docs and pushed it.
> 
>>> When
>>> adjusting the docs for sql-select, I noticed that the grammar also
>>> supports `GROUP BY ALL <grouping_elements>`, so I also added a test to
>>> ensure that this syntax is explicitly supported.
> 
> +1, can't hurt.
> 
>>> (It seems like it
>>> works as-is without further grammar adjustments, but I was a little
>>> worried when I first saw that fact... :D)
> 
> Bison would have been vocal about it if you'd introduced any
> ambiguity.  Still, I didn't feel like looking around to see if we
> already covered this syntax, and I agree it's close enough to being
> an issue to be worth covering.
> 
> Thanks for the patch!

Great, thank you!

David

  51. Re: [PATCH] GROUP BY ALL

    Chao Li <li.evan.chao@gmail.com> — 2025-09-29T23:46:25Z

    
> On Sep 29, 2025, at 04:34, David Christensen <david@pgguru.net> wrote:
> 
> On Sun, Sep 28, 2025 at 2:18 PM David Christensen <david@pgguru.net <mailto:david@pgguru.net>> wrote:
>> 
>> On Sat, Sep 27, 2025 at 5:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> 
>>> Here's a v6 that's rebased up to HEAD and contains fixes for the
>>> semantic issues we discussed.  It still lacks documentation, but
>>> otherwise I think it's about ready to go.
>> 
>> Here is v7 with a stab at docs; fairly minimal at this point, but
>> touching the two areas that are likely to need adjusting.  When
>> adjusting the docs for sql-select, I noticed that the grammar also
>> supports `GROUP BY ALL <grouping_elements>`, so I also added a test to
>> ensure that this syntax is explicitly supported. (It seems like it
>> works as-is without further grammar adjustments, but I was a little
>> worried when I first saw that fact... :D)  Not sure that
>> aggregates.sql is still the right place for all of these bits, but it
>> does seem like having all things `GROUP BY ALL`-related tested in the
>> same place is a nice property, so leaving there for now.
> 
> This time with attachment!
> <v7-0001-Add-GROUP-BY-ALL.patch>


A nit comment for the doc:

```
+    not contain either an aggregate function or a window function in their
+    expression list.  This can greatly simplify ad-hoc exploration of data.
+   </para>
```

“In their expression list” => “in the <literal>SELECT</literal> list"

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/