Skip to content

Commit 48b6035

Browse files
tglsfdctglsfdc
committed
Fix assorted missing logic for GroupingFunc nodes.
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
1 parent 05ccf97 commit 48b6035

File tree

6 files changed

+88
-14
lines changed

6 files changed

+88
-14
lines changed

‎src/backend/nodes/nodeFuncs.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -736,6 +736,8 @@ expression_returns_set_walker(Node *node, void *context)
736736
/* Avoid recursion for some cases that parser checks not to return a set */
737737
if (IsA(node, Aggref))
738738
return false;
739+
if (IsA(node, GroupingFunc))
740+
return false;
739741
if (IsA(node, WindowFunc))
740742
return false;
741743

‎src/backend/optimizer/path/costsize.c

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4492,6 +4492,12 @@ cost_qual_eval_walker(Node *node, cost_qual_eval_context *context)
44924492
*/
44934493
return false; /* don't recurse into children */
44944494
}
4495+
else if (IsA(node, GroupingFunc))
4496+
{
4497+
/* Treat this as having cost 1 */
4498+
context->total.per_tuple += cpu_operator_cost;
4499+
return false; /* don't recurse into children */
4500+
}
44954501
else if (IsA(node, CoerceViaIO))
44964502
{
44974503
CoerceViaIO *iocoerce = (CoerceViaIO *) node;

‎src/backend/optimizer/plan/subselect.c

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -356,15 +356,17 @@ build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot,
356356
Node *arg = pitem->item;
357357

358358
/*
359-
* The Var, PlaceHolderVar, or Aggref has already been adjusted to
360-
* have the correct varlevelsup, phlevelsup, or agglevelsup.
359+
* The Var, PlaceHolderVar, Aggref or GroupingFunc has already been
360+
* adjusted to have the correct varlevelsup, phlevelsup, or
361+
* agglevelsup.
361362
*
362-
* If it's a PlaceHolderVar or Aggref, its arguments might contain
363-
* SubLinks, which have not yet been processed (see the comments for
364-
* SS_replace_correlation_vars). Do that now.
363+
* If it's a PlaceHolderVar, Aggref or GroupingFunc, its arguments
364+
* might contain SubLinks, which have not yet been processed (see the
365+
* comments for SS_replace_correlation_vars). Do that now.
365366
*/
366367
if (IsA(arg, PlaceHolderVar) ||
367-
IsA(arg, Aggref))
368+
IsA(arg, Aggref) ||
369+
IsA(arg, GroupingFunc))
368370
arg = SS_process_sublinks(root, arg, false);
369371

370372
splan->parParam = lappend_int(splan->parParam, pitem->paramId);
@@ -1957,10 +1959,11 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context)
19571959
}
19581960

19591961
/*
1960-
* Don't recurse into the arguments of an outer PHV or aggregate here. Any
1961-
* SubLinks in the arguments have to be dealt with at the outer query
1962-
* level; they'll be handled when build_subplan collects the PHV or Aggref
1963-
* into the arguments to be passed down to the current subplan.
1962+
* Don't recurse into the arguments of an outer PHV, Aggref or
1963+
* GroupingFunc here. Any SubLinks in the arguments have to be dealt with
1964+
* at the outer query level; they'll be handled when build_subplan
1965+
* collects the PHV, Aggref or GroupingFunc into the arguments to be
1966+
* passed down to the current subplan.
19641967
*/
19651968
if (IsA(node, PlaceHolderVar))
19661969
{
@@ -1972,6 +1975,11 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context)
19721975
if (((Aggref *) node)->agglevelsup > 0)
19731976
return node;
19741977
}
1978+
else if (IsA(node, GroupingFunc))
1979+
{
1980+
if (((GroupingFunc *) node)->agglevelsup > 0)
1981+
return node;
1982+
}
19751983

19761984
/*
19771985
* We should never see a SubPlan expression in the input (since this is
@@ -2084,7 +2092,7 @@ SS_identify_outer_params(PlannerInfo *root)
20842092
outer_params = NULL;
20852093
for (proot = root->parent_root; proot != NULL; proot = proot->parent_root)
20862094
{
2087-
/* Include ordinary Var/PHV/Aggref params */
2095+
/* Include ordinary Var/PHV/Aggref/GroupingFunc params */
20882096
foreach(l, proot->plan_params)
20892097
{
20902098
PlannerParamItem *pitem = (PlannerParamItem *) lfirst(l);

‎src/backend/utils/adt/ruleutils.c

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7895,12 +7895,13 @@ get_parameter(Param *param, deparse_context *context)
78957895
context->varprefix = true;
78967896

78977897
/*
7898-
* A Param's expansion is typically a Var, Aggref, or upper-level
7899-
* Param, which wouldn't need extra parentheses. Otherwise, insert
7900-
* parens to ensure the expression looks atomic.
7898+
* A Param's expansion is typically a Var, Aggref, GroupingFunc, or
7899+
* upper-level Param, which wouldn't need extra parentheses.
7900+
* Otherwise, insert parens to ensure the expression looks atomic.
79017901
*/
79027902
need_paren = !(IsA(expr, Var) ||
79037903
IsA(expr, Aggref) ||
7904+
IsA(expr, GroupingFunc) ||
79047905
IsA(expr, Param));
79057906
if (need_paren)
79067907
appendStringInfoChar(context->buf, '(');
@@ -8028,6 +8029,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
80288029
case T_NextValueExpr:
80298030
case T_NullIfExpr:
80308031
case T_Aggref:
8032+
case T_GroupingFunc:
80318033
case T_WindowFunc:
80328034
case T_FuncExpr:
80338035
/* function-like: name(..) or name[..] */
@@ -8144,6 +8146,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
81448146
case T_XmlExpr: /* own parentheses */
81458147
case T_NullIfExpr: /* other separators */
81468148
case T_Aggref: /* own parentheses */
8149+
case T_GroupingFunc: /* own parentheses */
81478150
case T_WindowFunc: /* own parentheses */
81488151
case T_CaseExpr: /* other separators */
81498152
return true;
@@ -8194,6 +8197,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
81948197
case T_XmlExpr: /* own parentheses */
81958198
case T_NullIfExpr: /* other separators */
81968199
case T_Aggref: /* own parentheses */
8200+
case T_GroupingFunc: /* own parentheses */
81978201
case T_WindowFunc: /* own parentheses */
81988202
case T_CaseExpr: /* other separators */
81998203
return true;

‎src/test/regress/expected/groupingsets.out

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2040,4 +2040,49 @@ order by a, b, c;
20402040
| |
20412041
(11 rows)
20422042

2043+
-- test handling of outer GroupingFunc within subqueries
2044+
explain (costs off)
2045+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
2046+
QUERY PLAN
2047+
---------------------------
2048+
MixedAggregate
2049+
Hash Key: $2
2050+
Group Key: ()
2051+
InitPlan 1 (returns $1)
2052+
-> Result
2053+
InitPlan 3 (returns $2)
2054+
-> Result
2055+
-> Result
2056+
SubPlan 2
2057+
-> Result
2058+
(10 rows)
2059+
2060+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
2061+
grouping
2062+
----------
2063+
1
2064+
0
2065+
(2 rows)
2066+
2067+
explain (costs off)
2068+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
2069+
QUERY PLAN
2070+
---------------------------
2071+
GroupAggregate
2072+
Group Key: $2
2073+
InitPlan 1 (returns $1)
2074+
-> Result
2075+
InitPlan 3 (returns $2)
2076+
-> Result
2077+
-> Result
2078+
SubPlan 2
2079+
-> Result
2080+
(9 rows)
2081+
2082+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
2083+
grouping
2084+
----------
2085+
0
2086+
(1 row)
2087+
20432088
-- end

‎src/test/regress/sql/groupingsets.sql

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -555,4 +555,13 @@ from (values (1, 2, 3), (4, null, 6), (7, 8, 9)) as t (a, b, c)
555555
group by rollup(a, b), rollup(a, c)
556556
order by a, b, c;
557557

558+
-- test handling of outer GroupingFunc within subqueries
559+
explain (costs off)
560+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
561+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
562+
563+
explain (costs off)
564+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
565+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
566+
558567
-- end

0 commit comments

Comments
 (0)