From 6dcaed11d7521f22ec924b342c5668a2a7fb855a Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Wed, 24 Sep 2025 16:49:24 +0900 Subject: [PATCH v1] Fix pushdown of degenerate HAVING clauses Commit 67a54b9e8 taught the planner to push down HAVING clauses even when nonempty grouping sets are present, as long as the clause does not reference any columns that are nullable by the grouping sets. However, there was an oversight: if any empty grouping sets are also present, the aggregation node can produce a row that did not come from the input, and pushing down a HAVING clause in this case may cause us to fail to filter out that row. Currently, non-degenerate HAVING clauses are not pushed down when empty grouping sets are present, since the grouping set would nullify the vars they reference. However, degenerate (variable-free) HAVING clauses are not subject to this restriction and may be incorrectly pushed down. To fix, the patch adds a check that prevents degenerate HAVING clauses from being pushed down when any nonempty grouping sets are present. This could be done more precisely, for example by restricting the prevention only to cases where empty grouping sets are also present, but the added complexity does not seem justified. Back-patch to v18 where this issue was introduced. --- src/backend/optimizer/plan/planner.c | 13 +++++--- src/test/regress/expected/groupingsets.out | 39 +++++++++++++++++++++- src/test/regress/sql/groupingsets.sql | 12 ++++++- 3 files changed, 57 insertions(+), 7 deletions(-) diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 41bd8353430..8d331aa34bd 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -1097,10 +1097,11 @@ subquery_planner(PlannerGlobal *glob, Query *parse, PlannerInfo *parent_root, * cannot do so if the HAVING clause contains aggregates (obviously) or * volatile functions (since a HAVING clause is supposed to be executed * only once per group). We also can't do this if there are any nonempty - * grouping sets and the clause references any columns that are nullable - * by the grouping sets; moving such a clause into WHERE would potentially - * change the results. (If there are only empty grouping sets, then the - * HAVING clause must be degenerate as discussed below.) + * grouping sets and the clause either references any columns that are + * nullable by the grouping sets or is degenerate; moving such a clause + * into WHERE would potentially change the results. (If there are only + * empty grouping sets, then the HAVING clause must be degenerate as + * discussed below.) * * Also, it may be that the clause is so expensive to execute that we're * better off doing it only once per group, despite the loss of @@ -1137,12 +1138,14 @@ subquery_planner(PlannerGlobal *glob, Query *parse, PlannerInfo *parent_root, foreach(l, (List *) parse->havingQual) { Node *havingclause = (Node *) lfirst(l); + Relids having_relids; if (contain_agg_clause(havingclause) || contain_volatile_functions(havingclause) || contain_subplans(havingclause) || (parse->groupClause && parse->groupingSets && - bms_is_member(root->group_rtindex, pull_varnos(root, havingclause)))) + ((having_relids = pull_varnos(root, havingclause)) == NULL || + bms_is_member(root->group_rtindex, having_relids)))) { /* keep it in HAVING */ newHaving = lappend(newHaving, havingclause); diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out index 210bbe307a7..7a8533228d6 100644 --- a/src/test/regress/expected/groupingsets.out +++ b/src/test/regress/expected/groupingsets.out @@ -890,7 +890,8 @@ explain (costs off) -> Seq Scan on gstest2 (10 rows) --- test pushdown of HAVING clause that does not reference any columns that are nullable by grouping sets +-- test pushdown of non-degenerate HAVING clause that does not reference any +-- columns that are nullable by grouping sets explain (costs off) select a, b, count(*) from gstest2 group by grouping sets ((a, b), (a)) having a > 1 and b > 1; QUERY PLAN @@ -911,6 +912,42 @@ select a, b, count(*) from gstest2 group by grouping sets ((a, b), (a)) having a 2 | 2 | 1 (1 row) +-- test pushdown of degenerate HAVING clause +explain (costs off) +select count(*) from gstest2 group by grouping sets (()) having false; + QUERY PLAN +----------------------------------- + Aggregate + Group Key: () + Filter: false + -> Result + Replaces: Scan on gstest2 + One-Time Filter: false +(6 rows) + +select count(*) from gstest2 group by grouping sets (()) having false; + count +------- +(0 rows) + +explain (costs off) +select a, count(*) from gstest2 group by grouping sets ((a), ()) having false; + QUERY PLAN +--------------------------------- + GroupAggregate + Group Key: a + Group Key: () + Filter: false + -> Sort + Sort Key: a + -> Seq Scan on gstest2 +(7 rows) + +select a, count(*) from gstest2 group by grouping sets ((a), ()) having false; + a | count +---+------- +(0 rows) + -- HAVING with GROUPING queries select ten, grouping(ten) from onek group by grouping sets(ten) having grouping(ten) >= 0 diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql index 38d3cdd0fd8..dbacc2ffdce 100644 --- a/src/test/regress/sql/groupingsets.sql +++ b/src/test/regress/sql/groupingsets.sql @@ -290,11 +290,21 @@ explain (costs off) select v.c, (select count(*) from gstest2 group by () having v.c) from (values (false),(true)) v(c) order by v.c; --- test pushdown of HAVING clause that does not reference any columns that are nullable by grouping sets +-- test pushdown of non-degenerate HAVING clause that does not reference any +-- columns that are nullable by grouping sets explain (costs off) select a, b, count(*) from gstest2 group by grouping sets ((a, b), (a)) having a > 1 and b > 1; select a, b, count(*) from gstest2 group by grouping sets ((a, b), (a)) having a > 1 and b > 1; +-- test pushdown of degenerate HAVING clause +explain (costs off) +select count(*) from gstest2 group by grouping sets (()) having false; +select count(*) from gstest2 group by grouping sets (()) having false; + +explain (costs off) +select a, count(*) from gstest2 group by grouping sets ((a), ()) having false; +select a, count(*) from gstest2 group by grouping sets ((a), ()) having false; + -- HAVING with GROUPING queries select ten, grouping(ten) from onek group by grouping sets(ten) having grouping(ten) >= 0 -- 2.39.5 (Apple Git-154)