Обсуждение: BUG #17088: FailedAssertion in prepagg.c

The following bug has been logged on the website:

Bug reference:      17088
Logged by:          yaoguang chen
Email address:      cyg0810@gmail.com
PostgreSQL version: 14beta2
Operating system:   Linux supersix 5.4.0-39-generic #43-Ubuntu SMP Fri
Description:

run the following sql command through client and the PostgreSQL database
process will crash:

CREATE TEMP TABLE v0 ( v1 INT PRIMARY KEY ) ON COMMIT DELETE ROWS ; 
SELECT FROM ( VALUES ( ( SELECT - - 84 FROM v0 LIMIT - -1 ) ) ) v1 ( v1 )
GROUP BY ROLLUP ( v1 , v1 ) , ROLLUP ( ROW ( ) , ROW ( - - - - -128 ,
6099928.000000 ) , v1 ) ORDER BY v1 = v1 AND v1 = - - ( SELECT GROUPING ( v1
) GROUP BY v1 ) ASC FETCH FIRST ROWS WITH TIES

crash log:

HINT:  Future log output will go to log destination "csvlog".
TRAP: FailedAssertion("!IsA(node, SubLink)", File:
"/home/supersix/fuzz/Squirrel/PostgreSQL/postgres/build/../src/backend/optimizer/prep/prepagg.c",
Line: 341, PID: 2310031)
postgres: supersix x 127.0.0.1(65126)
SELECT(ExceptionalCondition+0xbb)[0x562398112ffb]
postgres: supersix x 127.0.0.1(65126)
SELECT(preprocess_aggrefs+0x0)[0x562397d2da10]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x95)[0x562397c8c755]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x641)[0x562397c8cd01]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x95)[0x562397c8c755]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x7ef)[0x562397c8ceaf]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x95)[0x562397c8c755]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x159)[0x562397c8c819]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x95)[0x562397c8c755]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x159)[0x562397c8c819]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x95)[0x562397c8c755]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x159)[0x562397c8c819]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x95)[0x562397c8c755]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x832)[0x562397c8cef2]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x95)[0x562397c8c755]
postgres: supersix x 127.0.0.1(65126) SELECT(+0x592dd9)[0x562397d1bdd9]
postgres: supersix x 127.0.0.1(65126)
SELECT(subquery_planner+0xf63)[0x562397d1e8e3]
postgres: supersix x 127.0.0.1(65126)
SELECT(standard_planner+0x165)[0x562397d1f535]
postgres: supersix x 127.0.0.1(65126)
SELECT(pg_plan_query+0x6a)[0x562397ebceaa]
postgres: supersix x 127.0.0.1(65126)
SELECT(pg_plan_queries+0x4d)[0x562397ebcffd]
postgres: supersix x 127.0.0.1(65126) SELECT(+0x7359f2)[0x562397ebe9f2]
postgres: supersix x 127.0.0.1(65126)
SELECT(PostgresMain+0x1ae7)[0x562397ec0d57]
postgres: supersix x 127.0.0.1(65126) SELECT(+0x61671f)[0x562397d9f71f]
postgres: supersix x 127.0.0.1(65126)
SELECT(PostmasterMain+0x1182)[0x562397da2672]
postgres: supersix x 127.0.0.1(65126) SELECT(main+0x533)[0x562397852133]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3)[0x7f6aa088f0b3]
postgres: supersix x 127.0.0.1(65126) SELECT(_start+0x2e)[0x56239785228e]

On Wed, Jul 07, 2021 at 06:33:07AM +0000, PG Bug reporting form wrote:
> run the following sql command through client and the PostgreSQL database
> process will crash:
>
> CREATE TEMP TABLE v0 ( v1 INT PRIMARY KEY ) ON COMMIT DELETE ROWS ;
> SELECT FROM ( VALUES ( ( SELECT - - 84 FROM v0 LIMIT - -1 ) ) ) v1 ( v1 )
> GROUP BY ROLLUP ( v1 , v1 ) , ROLLUP ( ROW ( ) , ROW ( - - - - -128 ,
> 6099928.000000 ) , v1 ) ORDER BY v1 = v1 AND v1 = - - ( SELECT GROUPING ( v1
> ) GROUP BY v1 ) ASC FETCH FIRST ROWS WITH TIES

Reproduced here, thanks for the test case.  As far as I can see, this
is not limited to 14.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
>> CREATE TEMP TABLE v0 ( v1 INT PRIMARY KEY ) ON COMMIT DELETE ROWS ;
>> SELECT FROM ( VALUES ( ( SELECT - - 84 FROM v0 LIMIT - -1 ) ) ) v1 ( v1 )
>> GROUP BY ROLLUP ( v1 , v1 ) , ROLLUP ( ROW ( ) , ROW ( - - - - -128 ,
>> 6099928.000000 ) , v1 ) ORDER BY v1 = v1 AND v1 = - - ( SELECT GROUPING ( v1
>> ) GROUP BY v1 ) ASC FETCH FIRST ROWS WITH TIES

> Reproduced here, thanks for the test case.  As far as I can see, this
> is not limited to 14.

Yeah, this looks like it probably dates back to the addition of
GroupingFunc.  The test case can be simplified a good deal:

SELECT (SELECT GROUPING(v1)) FROM (VALUES ((SELECT 1))) v(v1) GROUP BY cube(v1);
server closed the connection unexpectedly

I also found a probably-related variant:

SELECT (SELECT GROUPING(v1)) FROM (VALUES ((SELECT 1))) v(v1) GROUP BY v1;
ERROR:  plan should not reference subplan's variable

These cases don't fail if the GROUPING call isn't inside a sub-select.

The proximate cause of the assertion failure is that preprocess_aggrefs
isn't expecting to find a SubLink, which is reasonable since we should
have removed them already.  However, what it's actually seeing is

   {TARGETENTRY
   :expr
      {SUBPLAN
      ...
      :args (
         {GROUPINGFUNC
         :args (
            {PLACEHOLDERVAR
            :phexpr
               {SUBLINK
               ...
               }
            ...

If we don't put GROUPING(v1) inside a sub-SELECT, it looks like

      {GROUPINGFUNC
      :args (
         {PLACEHOLDERVAR
         :phexpr
            {PARAM
            :paramkind 1
            :paramid 0
            :paramtype 23
            :paramtypmod -1
            :paramcollid 0
            :location -1
            }
         :phrels (b 2)
         :phid 1
         :phlevelsup 0
         }
      )
      :refs (i 1)
      :cols <>
      :agglevelsup 0
      :location 15
      }

which seems a whole lot saner.  So I surmise that somebody is
missing doing something relevant to the "args" list of a SubPlan.

An alternative theory is that we should never have done anything
at all to the argument tree of a GroupingFunc.  Since it's not
supposed to be evaluated, treating it as a target for expression
preprocessing might be a mistake altogether.  I wonder why its
arguments aren't stored as sortgroupref indexes or the like.

            regards, tom lane

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom> An alternative theory is that we should never have done anything
 Tom> at all to the argument tree of a GroupingFunc. Since it's not
 Tom> supposed to be evaluated, treating it as a target for expression
 Tom> preprocessing might be a mistake altogether. I wonder why its
 Tom> arguments aren't stored as sortgroupref indexes or the like.

The arguments are stored as sortgrouprefs (the "refs" list) for actual
use; the "args" list is only kept for the benefit of EXPLAIN and
deparsing.

A number of places in the planner have to explicitly avoid recursing
into GroupingFunc->args when walking trees specifically because they are
not evaluated. It looks to me like some places where that should have
been checked for were missed. Looking into it.

-- 
Andrew (irc:RhodiumToad)

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> A number of places in the planner have to explicitly avoid recursing
> into GroupingFunc->args when walking trees specifically because they are
> not evaluated. It looks to me like some places where that should have
> been checked for were missed. Looking into it.

Hmm.  Maybe it'd be better if the default behavior in
expression_tree_walker/mutator did not include recursing into the args,
then?  I am thinking this might be comparable to SubLinks/SubPlans, where
the walker has to take explicit action if it wants to recurse into the
sub-query.

            regards, tom lane

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 > Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
 >> A number of places in the planner have to explicitly avoid recursing
 >> into GroupingFunc->args when walking trees specifically because they
 >> are not evaluated. It looks to me like some places where that should
 >> have been checked for were missed. Looking into it.

 Tom> Hmm. Maybe it'd be better if the default behavior in
 Tom> expression_tree_walker/mutator did not include recursing into the
 Tom> args, then?

You'd think, but as I recall (I will re-check this to confirm) there
were more places where we _did_ need to recurse (especially during parse
analysis before we've matched up the sortgrouprefs), while most of the
places where recursion needed to be explicitly avoided already needed
special-case handling, so having the default the other way would likely
have required a special-case almost everywhere.

-- 
Andrew.

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>  Tom> Hmm. Maybe it'd be better if the default behavior in
>  Tom> expression_tree_walker/mutator did not include recursing into the
>  Tom> args, then?

> You'd think, but as I recall (I will re-check this to confirm) there
> were more places where we _did_ need to recurse (especially during parse
> analysis before we've matched up the sortgrouprefs), while most of the
> places where recursion needed to be explicitly avoided already needed
> special-case handling, so having the default the other way would likely
> have required a special-case almost everywhere.

Fair enough.  This is the kind of design choice that can be worth
revisiting later; but if the conclusion is still the same, fine with me.

            regards, tom lane

At Fri, 9 Jul 2021 14:54:02 +0800, Richard Guo <guofenglinux@gmail.com> wrote in 
> Update the patch with comments and test cases.

AFAICS the patch looks correct.  It works for the first example and
the two from Tom.  I don't find other place that has the similar
issue.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Fri, 9 Jul 2021 14:54:02 +0800, Richard Guo <guofenglinux@gmail.com> wrote in
>> Update the patch with comments and test cases.

> AFAICS the patch looks correct.  It works for the first example and
> the two from Tom.  I don't find other place that has the similar
> issue.

I'd been expecting Andrew to pick this up, but since he hasn't,
I took a look.

I concur that the core problem is that GroupingFunc has to be treated
almost exactly like Aggref, and here we have a couple of places that
didn't get that memo.  So it occurred to me to look for other places
that special-case Aggref and don't have parallel code for GroupingFunc,
and I found several:

expression_returns_set_walker

This isn't particularly hazardous, since the argument (probably?) can't
contain a SRF, but it still seems like it ought to treat the two node
types the same.

cost_qual_eval_walker

It's defaulting to charging the eval costs of the arguments, which is
flat wrong.  I made it charge one cpu_operator_cost instead.

ruleutils.c

Various places concerned with whether or not we need parens were
making the wrong choice, resulting in excess parens in pretty-printing
mode.  This is also just cosmetic, but still.

This looks good to me now, and I'll set about back-patching.

            regards, tom lane

diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 47d0564fa2..ec25aae6e3 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -736,6 +736,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;

diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 8dc7dd4ca2..4d9f3b4bb6 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -4492,6 +4492,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;
diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
index 41bd1ae7d4..863e0e24a1 100644
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -356,15 +356,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);
@@ -1957,10 +1959,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))
     {
@@ -1972,6 +1975,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
@@ -2084,7 +2092,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);
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index b16526e65e..7f4f3f7369 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -7956,12 +7956,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, '(');
@@ -8089,6 +8090,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[..] */
@@ -8205,6 +8207,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;
@@ -8255,6 +8258,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;
diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out
index 58a25b691a..6a56f0b09c 100644
--- a/src/test/regress/expected/groupingsets.out
+++ b/src/test/regress/expected/groupingsets.out
@@ -2042,4 +2042,49 @@ order by a, b, c;
    |   |
 (11 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
diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql
index 473d21f6b9..8050dbf260 100644
--- a/src/test/regress/sql/groupingsets.sql
+++ b/src/test/regress/sql/groupingsets.sql
@@ -557,4 +557,13 @@ from (values (1, 2, 3), (4, null, 6), (7, 8, 9)) as t (a, b, c)
 group by rollup(a, b), rollup(a, c)
 order by a, b, c;

+-- 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