From: Tom Lane <tgl@sss.pgh.pa.us>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: guofenglinux@gmail.com
Cc: andrew@tao11.riddles.org.uk
Cc: michael@paquier.xyz
Cc: cyg0810@gmail.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17088: FailedAssertion in prepagg.c
Date: Mon, 21 Mar 2022 17:09:54 -0400
Message-ID: <2319370.1647896994@sss.pgh.pa.us> (raw)
In-Reply-To: <20220127.171052.1642938123495588521.horikyota.ntt@gmail.com>
References: <CAMbWs4_eg=c=pUwaeTgcEHDhe_wkqV7Uy=E4BymjqAXANWAqDw@mail.gmail.com>
<CAMbWs4_1m1Xd2JfCHFjGk9=Pj1gTiicYwG1JEpKLaxLub19RXA@mail.gmail.com>
<CAMbWs4_zpvPa1+qGBU78FhQqiQo_RNDK2vwP3uBxs-S-mABxJQ@mail.gmail.com>
<20220127.171052.1642938123495588521.horikyota.ntt@gmail.com>
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
Attachments:
[text/x-diff] v2-0001-fix-oversights-in-GroupingFunc-handling.patch (7.7K, ../2319370.1647896994@sss.pgh.pa.us/2-v2-0001-fix-oversights-in-GroupingFunc-handling.patch)
download | inline diff:diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.cindex 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.cindex 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.cindex 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.cindex 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.outindex 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.sqlindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, horikyota.ntt@gmail.com, 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: <2319370.1647896994@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox