Skip to content

Commit 2591ee8

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 1361959 commit 2591ee8

File tree

6 files changed

+88
-14
lines changed

6 files changed

+88
-14
lines changed

‎src/backend/nodes/nodeFuncs.c

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

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

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

+7-3
Original file line numberDiff line numberDiff line change
@@ -7956,12 +7956,13 @@ get_parameter(Param *param, deparse_context *context)
79567956
context->varprefix = true;
79577957

79587958
/*
7959-
* A Param's expansion is typically a Var, Aggref, or upper-level
7960-
* Param, which wouldn't need extra parentheses. Otherwise, insert
7961-
* parens to ensure the expression looks atomic.
7959+
* A Param's expansion is typically a Var, Aggref, GroupingFunc, or
7960+
* upper-level Param, which wouldn't need extra parentheses.
7961+
* Otherwise, insert parens to ensure the expression looks atomic.
79627962
*/
79637963
need_paren = !(IsA(expr, Var) ||
79647964
IsA(expr, Aggref) ||
7965+
IsA(expr, GroupingFunc) ||
79657966
IsA(expr, Param));
79667967
if (need_paren)
79677968
appendStringInfoChar(context->buf, '(');
@@ -8089,6 +8090,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
80898090
case T_NextValueExpr:
80908091
case T_NullIfExpr:
80918092
case T_Aggref:
8093+
case T_GroupingFunc:
80928094
case T_WindowFunc:
80938095
case T_FuncExpr:
80948096
/* function-like: name(..) or name[..] */
@@ -8205,6 +8207,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
82058207
case T_XmlExpr: /* own parentheses */
82068208
case T_NullIfExpr: /* other separators */
82078209
case T_Aggref: /* own parentheses */
8210+
case T_GroupingFunc: /* own parentheses */
82088211
case T_WindowFunc: /* own parentheses */
82098212
case T_CaseExpr: /* other separators */
82108213
return true;
@@ -8255,6 +8258,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
82558258
case T_XmlExpr: /* own parentheses */
82568259
case T_NullIfExpr: /* other separators */
82578260
case T_Aggref: /* own parentheses */
8261+
case T_GroupingFunc: /* own parentheses */
82588262
case T_WindowFunc: /* own parentheses */
82598263
case T_CaseExpr: /* other separators */
82608264
return true;

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

+45
Original file line numberDiff line numberDiff line change
@@ -2042,4 +2042,49 @@ order by a, b, c;
20422042
| |
20432043
(11 rows)
20442044

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

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

+9
Original file line numberDiff line numberDiff line change
@@ -557,4 +557,13 @@ from (values (1, 2, 3), (4, null, 6), (7, 8, 9)) as t (a, b, c)
557557
group by rollup(a, b), rollup(a, c)
558558
order by a, b, c;
559559

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

0 commit comments

Comments
 (0)