From 15da199cf5d27e82b6e14d1e48df3fa4445f2085 Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson Date: Wed, 15 Jul 2026 09:38:06 +0200 Subject: [PATCH] Revert Add GROUP BY ALL This reverts commit ef38a4d9756db9ae1d20f40aa39f3cf76059b81a which implemented the GROUP BY ALL syntax. A postcommit review discovered that GROUP BY ALL missed target list handling of entries which also appear in an ORDER BY in the query. This caused the query to return wrong results when the ordering used non-default equality semantics. It was deemed too late in the release cycle to perform the required refactoring to properly fix this. The reverted commit also included some additional wordsmithing and docs cleanup, all of which are retained as weren't connected to the reverted feature. Discussion: https://postgr.es/m/5243308F-8E5C-45AA-828C-FAD96C4F34DA@gmail.com Discussion: https://postgr.es/m/CAHM0NXjz0kDwtzoe-fnHAqPB1qA8_VJN0XAmCgUZ+iPnvP5LbA@mail.gmail.com --- doc/src/sgml/queries.sgml | 25 ----- doc/src/sgml/ref/select.sgml | 13 +-- doc/src/sgml/ref/select_into.sgml | 2 +- src/backend/parser/analyze.c | 2 - src/backend/parser/gram.y | 14 --- src/backend/parser/parse_clause.c | 63 +----------- src/backend/utils/adt/ruleutils.c | 4 +- src/include/nodes/parsenodes.h | 2 - src/include/parser/parse_clause.h | 1 - src/test/regress/expected/aggregates.out | 123 ----------------------- src/test/regress/sql/aggregates.sql | 54 ---------- 11 files changed, 5 insertions(+), 298 deletions(-) diff --git a/doc/src/sgml/queries.sgml b/doc/src/sgml/queries.sgml index b3cfbc93582..3d729f983b5 100644 --- a/doc/src/sgml/queries.sgml +++ b/doc/src/sgml/queries.sgml @@ -1159,31 +1159,6 @@ SELECT product_id, p.name, (sum(s.units) * p.price) AS sales expressions cannot contain aggregate functions or window functions). - - PostgreSQL also supports the syntax GROUP BY ALL, - which is equivalent to explicitly writing all select-list entries that - do not contain either an aggregate function referring to the same query level or a window function. - This can greatly simplify ad-hoc exploration of data. - As an example, these queries are equivalent: - -=> SELECT a, b, a + b, sum(c) FROM test1 GROUP BY ALL; - a | b | ?column? | sum ----+---+----------+---- - 1 | 4 | 5 | 9 - 2 | 5 | 7 | 12 - 3 | 6 | 9 | 15 -(3 rows) - -=> SELECT a, b, a + b, sum(c) FROM test1 GROUP BY a, b, a + b; - a | b | ?column? | sum ----+---+----------+---- - 1 | 4 | 5 | 9 - 2 | 5 | 7 | 12 - 3 | 6 | 9 | 15 -(3 rows) - - - HAVING diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml index 8d5a751af7e..837bf6d7832 100644 --- a/doc/src/sgml/ref/select.sgml +++ b/doc/src/sgml/ref/select.sgml @@ -37,7 +37,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionexpression [ [ AS ] output_name ] } [, ...] ] [ FROM from_item [, ...] ] [ WHERE condition ] - [ GROUP BY { ALL | [ ALL | DISTINCT ] grouping_element [, ...] } ] + [ GROUP BY [ ALL | DISTINCT ] grouping_element [, ...] ] [ HAVING condition ] [ WINDOW window_name AS ( window_definition ) [, ...] ] [ { UNION | INTERSECT | EXCEPT } [ ALL | DISTINCT ] select ] @@ -839,7 +839,7 @@ WHERE condition The optional GROUP BY clause has the general form -GROUP BY { ALL | [ ALL | DISTINCT ] grouping_element [, ...] } +GROUP BY [ ALL | DISTINCT ] grouping_element [, ...] @@ -857,15 +857,6 @@ GROUP BY { ALL | [ ALL | DISTINCT ] grouping_elem input-column name rather than an output column name. - - The form GROUP BY ALL with no explicit - grouping_elements - provided is equivalent to writing GROUP BY with the - numbers of all SELECT output columns that do not - contain either an aggregate function referring to the same query level or - a window function. - - If any of GROUPING SETS, ROLLUP or CUBE are present as grouping elements, then the diff --git a/doc/src/sgml/ref/select_into.sgml b/doc/src/sgml/ref/select_into.sgml index cbf865ff838..550ba69d5d1 100644 --- a/doc/src/sgml/ref/select_into.sgml +++ b/doc/src/sgml/ref/select_into.sgml @@ -27,7 +27,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionnew_table [ FROM from_item [, ...] ] [ WHERE condition ] - [ GROUP BY { ALL | [ ALL | DISTINCT ] grouping_element [, ...] } ] + [ GROUP BY [ ALL | DISTINCT ] grouping_element [, ...] ] [ HAVING condition ] [ WINDOW window_name AS ( window_definition ) [, ...] ] [ { UNION | INTERSECT | EXCEPT } [ ALL | DISTINCT ] select ] diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index ea97d236ea8..562e4facd74 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -1811,14 +1811,12 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt, qry->groupClause = transformGroupClause(pstate, stmt->groupClause, - stmt->groupByAll, &qry->groupingSets, &qry->targetList, qry->sortClause, EXPR_KIND_GROUP_BY, false /* allow SQL92 rules */ ); qry->groupDistinct = stmt->groupDistinct; - qry->groupByAll = stmt->groupByAll; if (stmt->distinctClause == NIL) { diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 9e05a314707..d27abe554cc 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -120,7 +120,6 @@ typedef struct SelectLimit typedef struct GroupClause { bool distinct; - bool all; List *list; } GroupClause; @@ -13762,7 +13761,6 @@ simple_select: n->whereClause = $6; n->groupClause = ($7)->list; n->groupDistinct = ($7)->distinct; - n->groupByAll = ($7)->all; n->havingClause = $8; n->windowClause = $9; $$ = (Node *) n; @@ -13780,7 +13778,6 @@ simple_select: n->whereClause = $6; n->groupClause = ($7)->list; n->groupDistinct = ($7)->distinct; - n->groupByAll = ($7)->all; n->havingClause = $8; n->windowClause = $9; $$ = (Node *) n; @@ -14278,24 +14275,14 @@ group_clause: GroupClause *n = palloc_object(GroupClause); n->distinct = $3 == SET_QUANTIFIER_DISTINCT; - n->all = false; n->list = $4; $$ = n; } - | GROUP_P BY ALL - { - GroupClause *n = palloc_object(GroupClause); - n->distinct = false; - n->all = true; - n->list = NIL; - $$ = n; - } | /*EMPTY*/ { GroupClause *n = palloc_object(GroupClause); n->distinct = false; - n->all = false; n->list = NIL; $$ = n; } @@ -18796,7 +18783,6 @@ PLpgSQL_Expr: opt_distinct_clause opt_target_list n->whereClause = $4; n->groupClause = ($5)->list; n->groupDistinct = ($5)->distinct; - n->groupByAll = ($5)->all; n->havingClause = $6; n->windowClause = $7; n->sortClause = $8; diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c index 2f7333e6786..485e33b9e5a 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -2746,9 +2746,6 @@ transformGroupingSet(List **flatresult, * GROUP BY items will be added to the targetlist (as resjunk columns) * if not already present, so the targetlist must be passed by reference. * - * If GROUP BY ALL is specified, the groupClause will be inferred to be all - * non-aggregate, non-window expressions in the targetlist. - * * This is also used for window PARTITION BY clauses (which act almost the * same, but are always interpreted per SQL99 rules). * @@ -2773,7 +2770,6 @@ transformGroupingSet(List **flatresult, * * pstate ParseState * grouplist clause to transform - * groupByAll is this a GROUP BY ALL statement? * groupingSets reference to list to contain the grouping set tree * targetlist reference to TargetEntry list * sortClause ORDER BY clause (SortGroupClause nodes) @@ -2781,8 +2777,7 @@ transformGroupingSet(List **flatresult, * useSQL99 SQL99 rather than SQL92 syntax */ List * -transformGroupClause(ParseState *pstate, List *grouplist, bool groupByAll, - List **groupingSets, +transformGroupClause(ParseState *pstate, List *grouplist, List **groupingSets, List **targetlist, List *sortClause, ParseExprKind exprKind, bool useSQL99) { @@ -2793,61 +2788,6 @@ transformGroupClause(ParseState *pstate, List *grouplist, bool groupByAll, bool hasGroupingSets = false; Bitmapset *seen_local = NULL; - /* Handle GROUP BY ALL */ - if (groupByAll) - { - /* There cannot have been any explicit grouplist items */ - Assert(grouplist == NIL); - - /* Iterate over targets, adding acceptable ones to the result list */ - foreach_ptr(TargetEntry, tle, *targetlist) - { - /* Ignore junk TLEs */ - if (tle->resjunk) - continue; - - /* - * TLEs containing aggregates are not okay to add to GROUP BY - * (compare checkTargetlistEntrySQL92). But the SQL standard - * directs us to skip them, so it's fine. - */ - if (pstate->p_hasAggs && - contain_aggs_of_level((Node *) tle->expr, 0)) - continue; - - /* - * Likewise, TLEs containing window functions are not okay to add - * to GROUP BY, and the SQL standard directs us to skip them. - */ - if (pstate->p_hasWindowFuncs && - contain_windowfuncs((Node *) tle->expr)) - continue; - - /* - * Otherwise, add the TLE to the result using default sort/group - * semantics. We specify the parse location as the TLE's - * location, despite the comment for addTargetToGroupList - * discouraging that. The only other thing we could point to is - * the ALL keyword, which seems unhelpful when there are multiple - * TLEs. - */ - result = addTargetToGroupList(pstate, tle, - result, *targetlist, - exprLocation((Node *) tle->expr)); - } - - /* If we found any acceptable targets, we're done */ - if (result != NIL) - return result; - - /* - * 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)); - } - /* * Recursively flatten implicit RowExprs. (Technically this is only needed * for GROUP BY, per the syntax rules for grouping sets, but we do it @@ -3026,7 +2966,6 @@ transformWindowDefinitions(ParseState *pstate, true /* force SQL99 rules */ ); partitionClause = transformGroupClause(pstate, windef->partitionClause, - false /* not GROUP BY ALL */ , NULL, targetlist, orderClause, diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 0ea38c18ca4..1b44b7a78d2 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -6555,9 +6555,7 @@ get_basic_select_query(Query *query, deparse_context *context) save_ingroupby = context->inGroupBy; context->inGroupBy = true; - if (query->groupByAll) - appendStringInfoString(buf, "ALL"); - else if (query->groupingSets == NIL) + if (query->groupingSets == NIL) { sep = ""; foreach(l, query->groupClause) diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 77f69f94607..5c8f9a07b62 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -220,7 +220,6 @@ typedef struct Query List *groupClause; /* a list of SortGroupClause's */ bool groupDistinct; /* was GROUP BY DISTINCT used? */ - bool groupByAll; /* was GROUP BY ALL used? */ List *groupingSets; /* a list of GroupingSet's if present */ @@ -2341,7 +2340,6 @@ typedef struct SelectStmt Node *whereClause; /* WHERE qualification */ List *groupClause; /* GROUP BY clauses */ bool groupDistinct; /* Is this GROUP BY DISTINCT? */ - bool groupByAll; /* Is this GROUP BY ALL? */ Node *havingClause; /* HAVING conditional-expression */ List *windowClause; /* WINDOW window_name AS (...), ... */ diff --git a/src/include/parser/parse_clause.h b/src/include/parser/parse_clause.h index fe234611007..ca815a9d1bb 100644 --- a/src/include/parser/parse_clause.h +++ b/src/include/parser/parse_clause.h @@ -26,7 +26,6 @@ extern Node *transformLimitClause(ParseState *pstate, Node *clause, ParseExprKind exprKind, const char *constructName, LimitOption limitOption); extern List *transformGroupClause(ParseState *pstate, List *grouplist, - bool groupByAll, List **groupingSets, List **targetlist, List *sortClause, ParseExprKind exprKind, bool useSQL99); diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out index f21f4e225da..5f0668382ed 100644 --- a/src/test/regress/expected/aggregates.out +++ b/src/test/regress/expected/aggregates.out @@ -1774,129 +1774,6 @@ select a, count(*) from t_having group by a having a = row(1.0)::avg_rec; drop table t_having; drop type avg_rec; -- --- Test GROUP BY ALL --- --- We don't care about the data here, just the proper transformation of the --- GROUP BY clause, so test some queries and verify the EXPLAIN plans. --- -CREATE TEMP TABLE t1 ( - a int, - b int, - c int -); --- basic example -EXPLAIN (COSTS OFF) SELECT b, COUNT(*) FROM t1 GROUP BY ALL; - QUERY PLAN ----------------------- - HashAggregate - Group Key: b - -> Seq Scan on t1 -(3 rows) - --- multiple columns, non-consecutive order -EXPLAIN (COSTS OFF) SELECT a, SUM(b), b FROM t1 GROUP BY ALL; - QUERY PLAN ----------------------- - HashAggregate - Group Key: a, b - -> Seq Scan on t1 -(3 rows) - --- multi columns, no aggregate -EXPLAIN (COSTS OFF) SELECT a + b FROM t1 GROUP BY ALL; - QUERY PLAN ----------------------- - HashAggregate - Group Key: (a + b) - -> Seq Scan on t1 -(3 rows) - --- check we detect a non-top-level aggregate -EXPLAIN (COSTS OFF) SELECT a, SUM(b) + 4 FROM t1 GROUP BY ALL; - QUERY PLAN ----------------------- - HashAggregate - Group Key: a - -> Seq Scan on t1 -(3 rows) - --- including grouped column is okay -EXPLAIN (COSTS OFF) SELECT a, SUM(b) + a FROM t1 GROUP BY ALL; - QUERY PLAN ----------------------- - HashAggregate - Group Key: a - -> Seq Scan on t1 -(3 rows) - --- including non-grouped column, not so much -EXPLAIN (COSTS OFF) SELECT a, SUM(b) + c FROM t1 GROUP BY ALL; -ERROR: column "t1.c" must appear in the GROUP BY clause or be used in an aggregate function -LINE 1: EXPLAIN (COSTS OFF) SELECT a, SUM(b) + c FROM t1 GROUP BY AL... - ^ --- all aggregates, should reduce to GROUP BY () -EXPLAIN (COSTS OFF) SELECT COUNT(a), SUM(b) FROM t1 GROUP BY ALL; - QUERY PLAN ----------------------- - Aggregate - Group Key: () - -> Seq Scan on t1 -(3 rows) - --- likewise with empty target list -EXPLAIN (COSTS OFF) SELECT FROM t1 GROUP BY ALL; - QUERY PLAN ------------------------ - Result - Replaces: Aggregate -(2 rows) - --- window functions are not to be included in GROUP BY, either -EXPLAIN (COSTS OFF) SELECT a, COUNT(a) OVER (PARTITION BY a) FROM t1 GROUP BY ALL; - QUERY PLAN ----------------------------------- - WindowAgg - Window: w1 AS (PARTITION BY a) - -> Sort - Sort Key: a - -> HashAggregate - Group Key: a - -> Seq Scan on t1 -(7 rows) - --- all cols -EXPLAIN (COSTS OFF) SELECT *, count(*) FROM t1 GROUP BY ALL; - QUERY PLAN ----------------------- - HashAggregate - Group Key: a, b, c - -> Seq Scan on t1 -(3 rows) - --- group by all with grouping element(s) (equivalent to GROUP BY's --- default behavior, explicit antithesis to GROUP BY DISTINCT) -EXPLAIN (COSTS OFF) SELECT a, count(*) FROM t1 GROUP BY ALL a; - QUERY PLAN ----------------------- - HashAggregate - Group Key: a - -> Seq Scan on t1 -(3 rows) - --- verify deparsing of GROUP BY ALL -CREATE TEMP VIEW v1 AS SELECT b, COUNT(*) FROM t1 GROUP BY ALL; -SELECT pg_get_viewdef('v1'::regclass); - pg_get_viewdef ------------------------ - SELECT b, + - count(*) AS count+ - FROM t1 + - GROUP BY ALL; -(1 row) - -DROP VIEW v1; -DROP TABLE t1; --- -- Test GROUP BY matching of join columns that are type-coerced due to USING -- create temp table t1(f1 int, f2 int); diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql index 5cb9a9dc1be..b788152f0c2 100644 --- a/src/test/regress/sql/aggregates.sql +++ b/src/test/regress/sql/aggregates.sql @@ -640,60 +640,6 @@ select a, count(*) from t_having group by a having a = row(1.0)::avg_rec; drop table t_having; drop type avg_rec; --- --- Test GROUP BY ALL --- --- We don't care about the data here, just the proper transformation of the --- GROUP BY clause, so test some queries and verify the EXPLAIN plans. --- - -CREATE TEMP TABLE t1 ( - a int, - b int, - c int -); - --- basic example -EXPLAIN (COSTS OFF) SELECT b, COUNT(*) FROM t1 GROUP BY ALL; - --- multiple columns, non-consecutive order -EXPLAIN (COSTS OFF) SELECT a, SUM(b), b FROM t1 GROUP BY ALL; - --- multi columns, no aggregate -EXPLAIN (COSTS OFF) SELECT a + b FROM t1 GROUP BY ALL; - --- check we detect a non-top-level aggregate -EXPLAIN (COSTS OFF) SELECT a, SUM(b) + 4 FROM t1 GROUP BY ALL; - --- including grouped column is okay -EXPLAIN (COSTS OFF) SELECT a, SUM(b) + a FROM t1 GROUP BY ALL; - --- including non-grouped column, not so much -EXPLAIN (COSTS OFF) SELECT a, SUM(b) + c FROM t1 GROUP BY ALL; - --- all aggregates, should reduce to GROUP BY () -EXPLAIN (COSTS OFF) SELECT COUNT(a), SUM(b) FROM t1 GROUP BY ALL; - --- likewise with empty target list -EXPLAIN (COSTS OFF) SELECT FROM t1 GROUP BY ALL; - --- window functions are not to be included in GROUP BY, either -EXPLAIN (COSTS OFF) SELECT a, COUNT(a) OVER (PARTITION BY a) FROM t1 GROUP BY ALL; - --- all cols -EXPLAIN (COSTS OFF) SELECT *, count(*) FROM t1 GROUP BY ALL; - --- group by all with grouping element(s) (equivalent to GROUP BY's --- default behavior, explicit antithesis to GROUP BY DISTINCT) -EXPLAIN (COSTS OFF) SELECT a, count(*) FROM t1 GROUP BY ALL a; - --- verify deparsing of GROUP BY ALL -CREATE TEMP VIEW v1 AS SELECT b, COUNT(*) FROM t1 GROUP BY ALL; -SELECT pg_get_viewdef('v1'::regclass); - -DROP VIEW v1; -DROP TABLE t1; - -- -- Test GROUP BY matching of join columns that are type-coerced due to USING -- -- 2.39.3 (Apple Git-146)