]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
Fix assorted missing logic for GroupingFunc nodes.
authorTom Lane <tgl@sss.pgh.pa.us>
Mon, 21 Mar 2022 21:44:29 +0000 (17:44 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Mon, 21 Mar 2022 21:44:29 +0000 (17:44 -0400)
The planner needs to treat GroupingFunc like Aggref for many purposes,
in particular with respect to processing of the argument expressions,
which are not to be evaluated at runtime.  A few places hadn't gotten
that memo, notably including subselect.c's processing of outer-level
aggregates.  This resulted in assertion failures or wrong plans for
cases in which a GROUPING() construct references an outer aggregation
level.

Also fix missing special cases for GroupingFunc in cost_qual_eval
(resulting in wrong cost estimates for GROUPING(), although it's
not clear that that would affect plan shapes in practice) and in
ruleutils.c (resulting in excess parentheses in pretty-print mode).

Per bug #17088 from Yaoguang Chen.  Back-patch to all supported
branches.

Richard Guo, Tom Lane

Discussion: https://postgr.es/m/17088-e33882b387de7f5c@postgresql.org

src/backend/nodes/nodeFuncs.c
src/backend/optimizer/path/costsize.c
src/backend/optimizer/plan/subselect.c
src/backend/utils/adt/ruleutils.c
src/test/regress/expected/groupingsets.out
src/test/regress/sql/groupingsets.sql

index 64c7eca95b08fb6bf785c91830292f8e5819ce88..229a8a9ae27d6023e7ba3efa39321fbb114efed9 100644 (file)
@@ -746,6 +746,8 @@ expression_returns_set_walker(Node *node, void *context)
        /* Avoid recursion for some cases that parser checks not to return a set */
        if (IsA(node, Aggref))
                return false;
+       if (IsA(node, GroupingFunc))
+               return false;
        if (IsA(node, WindowFunc))
                return false;
 
index 8d2a0166564b4b7c54a4f004b7a72f0d6911f997..a4522a0ff9733eae00d3ea13ab28f3d7ebacdeca 100644 (file)
@@ -3976,6 +3976,12 @@ cost_qual_eval_walker(Node *node, cost_qual_eval_context *context)
                 */
                return false;                   /* don't recurse into children */
        }
+       else if (IsA(node, GroupingFunc))
+       {
+               /* Treat this as having cost 1 */
+               context->total.per_tuple += cpu_operator_cost;
+               return false;                   /* don't recurse into children */
+       }
        else if (IsA(node, CoerceViaIO))
        {
                CoerceViaIO *iocoerce = (CoerceViaIO *) node;
index 9b9f5a9d45f3b348b9d3f7a3b86ce7409a756f76..5eb40a05f8b52864d4ea07b6ff5fb4e6fe1f5241 100644 (file)
@@ -357,15 +357,17 @@ build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot,
                Node       *arg = pitem->item;
 
                /*
-                * The Var, PlaceHolderVar, or Aggref has already been adjusted to
-                * have the correct varlevelsup, phlevelsup, or agglevelsup.
+                * The Var, PlaceHolderVar, Aggref or GroupingFunc has already been
+                * adjusted to have the correct varlevelsup, phlevelsup, or
+                * agglevelsup.
                 *
-                * If it's a PlaceHolderVar or Aggref, its arguments might contain
-                * SubLinks, which have not yet been processed (see the comments for
-                * SS_replace_correlation_vars).  Do that now.
+                * If it's a PlaceHolderVar, Aggref or GroupingFunc, its arguments
+                * might contain SubLinks, which have not yet been processed (see the
+                * comments for SS_replace_correlation_vars).  Do that now.
                 */
                if (IsA(arg, PlaceHolderVar) ||
-                       IsA(arg, Aggref))
+                       IsA(arg, Aggref) ||
+                       IsA(arg, GroupingFunc))
                        arg = SS_process_sublinks(root, arg, false);
 
                splan->parParam = lappend_int(splan->parParam, pitem->paramId);
@@ -1927,10 +1929,11 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context)
        }
 
        /*
-        * Don't recurse into the arguments of an outer PHV or aggregate here. Any
-        * SubLinks in the arguments have to be dealt with at the outer query
-        * level; they'll be handled when build_subplan collects the PHV or Aggref
-        * into the arguments to be passed down to the current subplan.
+        * Don't recurse into the arguments of an outer PHV, Aggref or
+        * GroupingFunc here.  Any SubLinks in the arguments have to be dealt with
+        * at the outer query level; they'll be handled when build_subplan
+        * collects the PHV, Aggref or GroupingFunc into the arguments to be
+        * passed down to the current subplan.
         */
        if (IsA(node, PlaceHolderVar))
        {
@@ -1942,6 +1945,11 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context)
                if (((Aggref *) node)->agglevelsup > 0)
                        return node;
        }
+       else if (IsA(node, GroupingFunc))
+       {
+               if (((GroupingFunc *) node)->agglevelsup > 0)
+                       return node;
+       }
 
        /*
         * We should never see a SubPlan expression in the input (since this is
@@ -2054,7 +2062,7 @@ SS_identify_outer_params(PlannerInfo *root)
        outer_params = NULL;
        for (proot = root->parent_root; proot != NULL; proot = proot->parent_root)
        {
-               /* Include ordinary Var/PHV/Aggref params */
+               /* Include ordinary Var/PHV/Aggref/GroupingFunc params */
                foreach(l, proot->plan_params)
                {
                        PlannerParamItem *pitem = (PlannerParamItem *) lfirst(l);
index 829b1f0332b3caf4fa09d0ee1452a58af76bfda6..65b53a110fe0f97593b08f39b13dfcfb32d3a814 100644 (file)
@@ -7466,12 +7466,13 @@ get_parameter(Param *param, deparse_context *context)
                context->varprefix = true;
 
                /*
-                * A Param's expansion is typically a Var, Aggref, or upper-level
-                * Param, which wouldn't need extra parentheses.  Otherwise, insert
-                * parens to ensure the expression looks atomic.
+                * A Param's expansion is typically a Var, Aggref, GroupingFunc, or
+                * upper-level Param, which wouldn't need extra parentheses.
+                * Otherwise, insert parens to ensure the expression looks atomic.
                 */
                need_paren = !(IsA(expr, Var) ||
                                           IsA(expr, Aggref) ||
+                                          IsA(expr, GroupingFunc) ||
                                           IsA(expr, Param));
                if (need_paren)
                        appendStringInfoChar(context->buf, '(');
@@ -7553,6 +7554,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
                case T_NextValueExpr:
                case T_NullIfExpr:
                case T_Aggref:
+               case T_GroupingFunc:
                case T_WindowFunc:
                case T_FuncExpr:
                        /* function-like: name(..) or name[..] */
@@ -7669,6 +7671,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
                                case T_XmlExpr: /* own parentheses */
                                case T_NullIfExpr:      /* other separators */
                                case T_Aggref:  /* own parentheses */
+                               case T_GroupingFunc:    /* own parentheses */
                                case T_WindowFunc:      /* own parentheses */
                                case T_CaseExpr:        /* other separators */
                                        return true;
@@ -7719,6 +7722,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
                                case T_XmlExpr: /* own parentheses */
                                case T_NullIfExpr:      /* other separators */
                                case T_Aggref:  /* own parentheses */
+                               case T_GroupingFunc:    /* own parentheses */
                                case T_WindowFunc:      /* own parentheses */
                                case T_CaseExpr:        /* other separators */
                                        return true;
index cd62af95e09c2c722fe02346e8f495fa2b55d5a2..1b01f0af82946c2912eb2a8752117905ab86ecf9 100644 (file)
@@ -1665,4 +1665,49 @@ select v||'a', case when grouping(v||'a') = 1 then 1 else 0 end, count(*)
           |    1 |     2
 (4 rows)
 
+-- test handling of outer GroupingFunc within subqueries
+explain (costs off)
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
+        QUERY PLAN         
+---------------------------
+ MixedAggregate
+   Hash Key: $2
+   Group Key: ()
+   InitPlan 1 (returns $1)
+     ->  Result
+   InitPlan 3 (returns $2)
+     ->  Result
+   ->  Result
+   SubPlan 2
+     ->  Result
+(10 rows)
+
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
+ grouping 
+----------
+        1
+        0
+(2 rows)
+
+explain (costs off)
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
+        QUERY PLAN         
+---------------------------
+ GroupAggregate
+   Group Key: $2
+   InitPlan 1 (returns $1)
+     ->  Result
+   InitPlan 3 (returns $2)
+     ->  Result
+   ->  Result
+   SubPlan 2
+     ->  Result
+(9 rows)
+
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
+ grouping 
+----------
+        0
+(1 row)
+
 -- end
index d62edf535cee38f3fca8fd140b089a685fe6bdcf..e5baa6a2a35cd6fd3e47b9cc3c9c2cfec6b6d85a 100644 (file)
@@ -457,4 +457,13 @@ select v||'a', case when grouping(v||'a') = 1 then 1 else 0 end, count(*)
   from unnest(array[1,1], array['a','b']) u(i,v)
  group by rollup(i, v||'a') order by 1,3;
 
+-- test handling of outer GroupingFunc within subqueries
+explain (costs off)
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
+
+explain (costs off)
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
+
 -- end