Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nWPHy-0007hL-Kv for pgsql-bugs@arkaria.postgresql.org; Mon, 21 Mar 2022 21:10:06 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1nWPHv-00084E-SX for pgsql-bugs@arkaria.postgresql.org; Mon, 21 Mar 2022 21:10:03 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nWPHv-000844-G9 for pgsql-bugs@lists.postgresql.org; Mon, 21 Mar 2022 21:10:03 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nWPHq-0006vh-Gp for pgsql-bugs@lists.postgresql.org; Mon, 21 Mar 2022 21:10:02 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 22LL9siR2319371; Mon, 21 Mar 2022 17:09:54 -0400 From: Tom Lane To: Kyotaro Horiguchi cc: guofenglinux@gmail.com, andrew@tao11.riddles.org.uk, michael@paquier.xyz, cyg0810@gmail.com, pgsql-bugs@lists.postgresql.org Subject: Re: BUG #17088: FailedAssertion in prepagg.c In-reply-to: <20220127.171052.1642938123495588521.horikyota.ntt@gmail.com> References: <20220127.171052.1642938123495588521.horikyota.ntt@gmail.com> Comments: In-reply-to Kyotaro Horiguchi message dated "Thu, 27 Jan 2022 17:10:52 +0900" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <2319330.1647896960.0@sss.pgh.pa.us> Date: Mon, 21 Mar 2022 17:09:54 -0400 Message-ID: <2319370.1647896994@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <2319330.1647896960.1@sss.pgh.pa.us> Content-Transfer-Encoding: quoted-printable Kyotaro Horiguchi writes: > At Fri, 9 Jul 2021 14:54:02 +0800, Richard Guo = 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="v2-0001-fix-oversights-in-GroupingFunc-handling.patch"; charset="us-ascii" Content-ID: <2319330.1647896960.2@sss.pgh.pa.us> Content-Description: v2-0001-fix-oversights-in-GroupingFunc-handling.patch Content-Transfer-Encoding: quoted-printable 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 *contex= t) /* 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_co= ntext *context) */ return false; /* don't recurse into children */ } + else if (IsA(node, GroupingFunc)) + { + /* Treat this as having cost 1 */ + context->total.per_tuple +=3D cpu_operator_cost; + return false; /* don't recurse into children */ + } else if (IsA(node, CoerceViaIO)) { CoerceViaIO *iocoerce =3D (CoerceViaIO *) node; diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimize= r/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, Planner= Info *subroot, Node *arg =3D 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 =3D SS_process_sublinks(root, arg, false); = splan->parParam =3D lappend_int(splan->parParam, pitem->paramId); @@ -1957,10 +1959,11 @@ process_sublinks_mutator(Node *node, process_subli= nks_context *context) } = /* - * Don't recurse into the arguments of an outer PHV or aggregate here. A= ny - * 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 Aggr= ef - * 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 wi= th + * 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_sublin= ks_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 =3D NULL; for (proot =3D root->parent_root; proot !=3D NULL; proot =3D proot->pare= nt_root) { - /* Include ordinary Var/PHV/Aggref params */ + /* Include ordinary Var/PHV/Aggref/GroupingFunc params */ foreach(l, proot->plan_params) { PlannerParamItem *pitem =3D (PlannerParamItem *) lfirst(l); diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/rul= eutils.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 *conte= xt) context->varprefix =3D 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 =3D !(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 prett= yFlags) 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 prett= yFlags) 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 prett= yFlags) 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 cu= be(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 cu= be(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 cu= be(v1); +select (select grouping(v1)) from (values ((select 1))) v(v1) group by cu= be(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 ------- =_aaaaaaaaaa0--