Skip to content

Commit 69c88e2

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 d8d378d commit 69c88e2

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
@@ -746,6 +746,8 @@ expression_returns_set_walker(Node *node, void *context)
746746
/* Avoid recursion for some cases that parser checks not to return a set */
747747
if (IsA(node, Aggref))
748748
return false;
749+
if (IsA(node, GroupingFunc))
750+
return false;
749751
if (IsA(node, WindowFunc))
750752
return false;
751753

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3976,6 +3976,12 @@ cost_qual_eval_walker(Node *node, cost_qual_eval_context *context)
39763976
*/
39773977
return false; /* don't recurse into children */
39783978
}
3979+
else if (IsA(node, GroupingFunc))
3980+
{
3981+
/* Treat this as having cost 1 */
3982+
context->total.per_tuple += cpu_operator_cost;
3983+
return false; /* don't recurse into children */
3984+
}
39793985
else if (IsA(node, CoerceViaIO))
39803986
{
39813987
CoerceViaIO *iocoerce = (CoerceViaIO *) node;

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

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

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

371373
splan->parParam = lappend_int(splan->parParam, pitem->paramId);
@@ -1927,10 +1929,11 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context)
19271929
}
19281930

19291931
/*
1930-
* Don't recurse into the arguments of an outer PHV or aggregate here. Any
1931-
* SubLinks in the arguments have to be dealt with at the outer query
1932-
* level; they'll be handled when build_subplan collects the PHV or Aggref
1933-
* into the arguments to be passed down to the current subplan.
1932+
* Don't recurse into the arguments of an outer PHV, Aggref or
1933+
* GroupingFunc here. Any SubLinks in the arguments have to be dealt with
1934+
* at the outer query level; they'll be handled when build_subplan
1935+
* collects the PHV, Aggref or GroupingFunc into the arguments to be
1936+
* passed down to the current subplan.
19341937
*/
19351938
if (IsA(node, PlaceHolderVar))
19361939
{
@@ -1942,6 +1945,11 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context)
19421945
if (((Aggref *) node)->agglevelsup > 0)
19431946
return node;
19441947
}
1948+
else if (IsA(node, GroupingFunc))
1949+
{
1950+
if (((GroupingFunc *) node)->agglevelsup > 0)
1951+
return node;
1952+
}
19451953

19461954
/*
19471955
* We should never see a SubPlan expression in the input (since this is
@@ -2054,7 +2062,7 @@ SS_identify_outer_params(PlannerInfo *root)
20542062
outer_params = NULL;
20552063
for (proot = root->parent_root; proot != NULL; proot = proot->parent_root)
20562064
{
2057-
/* Include ordinary Var/PHV/Aggref params */
2065+
/* Include ordinary Var/PHV/Aggref/GroupingFunc params */
20582066
foreach(l, proot->plan_params)
20592067
{
20602068
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
@@ -7466,12 +7466,13 @@ get_parameter(Param *param, deparse_context *context)
74667466
context->varprefix = true;
74677467

74687468
/*
7469-
* A Param's expansion is typically a Var, Aggref, or upper-level
7470-
* Param, which wouldn't need extra parentheses. Otherwise, insert
7471-
* parens to ensure the expression looks atomic.
7469+
* A Param's expansion is typically a Var, Aggref, GroupingFunc, or
7470+
* upper-level Param, which wouldn't need extra parentheses.
7471+
* Otherwise, insert parens to ensure the expression looks atomic.
74727472
*/
74737473
need_paren = !(IsA(expr, Var) ||
74747474
IsA(expr, Aggref) ||
7475+
IsA(expr, GroupingFunc) ||
74757476
IsA(expr, Param));
74767477
if (need_paren)
74777478
appendStringInfoChar(context->buf, '(');
@@ -7553,6 +7554,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
75537554
case T_NextValueExpr:
75547555
case T_NullIfExpr:
75557556
case T_Aggref:
7557+
case T_GroupingFunc:
75567558
case T_WindowFunc:
75577559
case T_FuncExpr:
75587560
/* function-like: name(..) or name[..] */
@@ -7669,6 +7671,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
76697671
case T_XmlExpr: /* own parentheses */
76707672
case T_NullIfExpr: /* other separators */
76717673
case T_Aggref: /* own parentheses */
7674+
case T_GroupingFunc: /* own parentheses */
76727675
case T_WindowFunc: /* own parentheses */
76737676
case T_CaseExpr: /* other separators */
76747677
return true;
@@ -7719,6 +7722,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
77197722
case T_XmlExpr: /* own parentheses */
77207723
case T_NullIfExpr: /* other separators */
77217724
case T_Aggref: /* own parentheses */
7725+
case T_GroupingFunc: /* own parentheses */
77227726
case T_WindowFunc: /* own parentheses */
77237727
case T_CaseExpr: /* other separators */
77247728
return true;

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

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1665,4 +1665,49 @@ select v||'a', case when grouping(v||'a') = 1 then 1 else 0 end, count(*)
16651665
| 1 | 2
16661666
(4 rows)
16671667

1668+
-- test handling of outer GroupingFunc within subqueries
1669+
explain (costs off)
1670+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
1671+
QUERY PLAN
1672+
---------------------------
1673+
MixedAggregate
1674+
Hash Key: $2
1675+
Group Key: ()
1676+
InitPlan 1 (returns $1)
1677+
-> Result
1678+
InitPlan 3 (returns $2)
1679+
-> Result
1680+
-> Result
1681+
SubPlan 2
1682+
-> Result
1683+
(10 rows)
1684+
1685+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
1686+
grouping
1687+
----------
1688+
1
1689+
0
1690+
(2 rows)
1691+
1692+
explain (costs off)
1693+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
1694+
QUERY PLAN
1695+
---------------------------
1696+
GroupAggregate
1697+
Group Key: $2
1698+
InitPlan 1 (returns $1)
1699+
-> Result
1700+
InitPlan 3 (returns $2)
1701+
-> Result
1702+
-> Result
1703+
SubPlan 2
1704+
-> Result
1705+
(9 rows)
1706+
1707+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
1708+
grouping
1709+
----------
1710+
0
1711+
(1 row)
1712+
16681713
-- end

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -457,4 +457,13 @@ select v||'a', case when grouping(v||'a') = 1 then 1 else 0 end, count(*)
457457
from unnest(array[1,1], array['a','b']) u(i,v)
458458
group by rollup(i, v||'a') order by 1,3;
459459

460+
-- test handling of outer GroupingFunc within subqueries
461+
explain (costs off)
462+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
463+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
464+
465+
explain (costs off)
466+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
467+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
468+
460469
-- end

0 commit comments

Comments
 (0)