Re: Inconsistent Behavior of GROUP BY ROLLUP in v17 vs master

I wrote:
> The proposed patch tries to close the hole by checking whether
> the condition is degenerate, but that feels subtly wrong to me:
> what we ought to check is whether there is any empty grouping set.
> As proposed, I think we miss optimization opportunities for
> degenerate HAVING because we will not try the trick of copying
> it to WHERE.

Concretely, I think we could do the attached.  This has the same
test query as in v1, but the generated plan is better because it
realizes it can copy the constant-false HAVING clause into WHERE,
resulting in a dummy scan of the table.

I'm not sure if planner.c is the best place to put
has_empty_grouping_set().  I couldn't find any existing code doing the
same thing, but maybe someday we'd want the functionality elsewhere.

            regards, tom lane

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 342d782c74b..2e2e7acf195 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -147,6 +147,7 @@ static double preprocess_limit(PlannerInfo *root,
                                double tuple_fraction,
                                int64 *offset_est, int64 *count_est);
 static List *preprocess_groupclause(PlannerInfo *root, List *force);
+static bool has_empty_grouping_set(List *groupingSets);
 static List *extract_rollup_sets(List *groupingSets);
 static List *reorder_grouping_sets(List *groupingSets, List *sortclause);
 static void standard_qp_callback(PlannerInfo *root, void *extra);
@@ -1131,11 +1132,12 @@ subquery_planner(PlannerGlobal *glob, Query *parse, char *plan_name,
      * In some cases we may want to transfer a HAVING clause into WHERE. We
      * 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.)
+     * only once per group).  We also can't do this if there are any grouping
+     * sets and the clause references any columns that are nullable by the
+     * grouping sets; the nulled values of those columns are not available
+     * before the grouping step.  (The test on groupClause might seem wrong,
+     * but it's okay: it's just an optimization to avoid running pull_varnos
+     * when there cannot be any Vars in the HAVING clause.)
      *
      * 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
@@ -1145,19 +1147,19 @@ subquery_planner(PlannerGlobal *glob, Query *parse, char *plan_name,
      * clause into WHERE, in hopes of eliminating tuples before aggregation
      * instead of after.
      *
-     * If the query has explicit grouping then we can simply move such a
+     * If the query has no empty grouping set then we can simply move such a
      * clause into WHERE; any group that fails the clause will not be in the
      * output because none of its tuples will reach the grouping or
-     * aggregation stage.  Otherwise we must have a degenerate (variable-free)
-     * HAVING clause, which we put in WHERE so that query_planner() can use it
-     * in a gating Result node, but also keep in HAVING to ensure that we
-     * don't emit a bogus aggregated row. (This could be done better, but it
-     * seems not worth optimizing.)
+     * aggregation stage.  Otherwise we have to keep the clause in HAVING to
+     * ensure that we don't emit a bogus aggregated row.  But then the HAVING
+     * clause must be degenerate (variable-free), so we can copy it into WHERE
+     * so that query_planner() can use it in a gating Result node. (This could
+     * be done better, but it seems not worth optimizing.)
      *
      * Note that a HAVING clause may contain expressions that are not fully
      * preprocessed.  This can happen if these expressions are part of
      * grouping items.  In such cases, they are replaced with GROUP Vars in
-     * the parser and then replaced back after we've done with expression
+     * the parser and then replaced back after we're done with expression
      * preprocessing on havingQual.  This is not an issue if the clause
      * remains in HAVING, because these expressions will be matched to lower
      * target items in setrefs.c.  However, if the clause is moved or copied
@@ -1182,7 +1184,8 @@ subquery_planner(PlannerGlobal *glob, Query *parse, char *plan_name,
             /* keep it in HAVING */
             newHaving = lappend(newHaving, havingclause);
         }
-        else if (parse->groupClause)
+        else if (parse->groupClause &&
+                 !has_empty_grouping_set(parse->groupingSets))
         {
             Node       *whereclause;

@@ -2910,6 +2913,39 @@ preprocess_groupclause(PlannerInfo *root, List *force)
     return new_groupclause;
 }

+/*
+ * Check for empty grouping sets within a list of GroupingSets.
+ */
+static bool
+has_empty_grouping_set(List *groupingSets)
+{
+    ListCell   *lc;
+
+    foreach(lc, groupingSets)
+    {
+        GroupingSet *gset = castNode(GroupingSet, lfirst(lc));
+
+        switch (gset->kind)
+        {
+            case GROUPING_SET_EMPTY:
+                return true;
+            case GROUPING_SET_SIMPLE:
+                /* keep looking */
+                break;
+            case GROUPING_SET_ROLLUP:
+            case GROUPING_SET_CUBE:
+                /* these necessarily include an empty set */
+                return true;
+            case GROUPING_SET_SETS:
+                /* recurse */
+                if (has_empty_grouping_set(gset->content))
+                    return true;
+                break;
+        }
+    }
+    return false;
+}
+
 /*
  * Extract lists of grouping sets that can be implemented using a single
  * rollup-type aggregate pass each. Returns a list of lists of grouping sets.
diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out
index 991121545c5..a480b4749a8 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,44 @@ 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
+         ->  Result
+               Replaces: Scan on gstest2
+               One-Time Filter: false
+(9 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