Skip to content

Commit dfefe38

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 2241e5c commit dfefe38

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

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4213,6 +4213,12 @@ cost_qual_eval_walker(Node *node, cost_qual_eval_context *context)
42134213
*/
42144214
return false; /* don't recurse into children */
42154215
}
4216+
else if (IsA(node, GroupingFunc))
4217+
{
4218+
/* Treat this as having cost 1 */
4219+
context->total.per_tuple += cpu_operator_cost;
4220+
return false; /* don't recurse into children */
4221+
}
42164222
else if (IsA(node, CoerceViaIO))
42174223
{
42184224
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);
@@ -1929,10 +1931,11 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context)
19291931
}
19301932

19311933
/*
1932-
* Don't recurse into the arguments of an outer PHV or aggregate here. Any
1933-
* SubLinks in the arguments have to be dealt with at the outer query
1934-
* level; they'll be handled when build_subplan collects the PHV or Aggref
1935-
* into the arguments to be passed down to the current subplan.
1934+
* Don't recurse into the arguments of an outer PHV, Aggref or
1935+
* GroupingFunc here. Any SubLinks in the arguments have to be dealt with
1936+
* at the outer query level; they'll be handled when build_subplan
1937+
* collects the PHV, Aggref or GroupingFunc into the arguments to be
1938+
* passed down to the current subplan.
19361939
*/
19371940
if (IsA(node, PlaceHolderVar))
19381941
{
@@ -1944,6 +1947,11 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context)
19441947
if (((Aggref *) node)->agglevelsup > 0)
19451948
return node;
19461949
}
1950+
else if (IsA(node, GroupingFunc))
1951+
{
1952+
if (((GroupingFunc *) node)->agglevelsup > 0)
1953+
return node;
1954+
}
19471955

19481956
/*
19491957
* We should never see a SubPlan expression in the input (since this is
@@ -2056,7 +2064,7 @@ SS_identify_outer_params(PlannerInfo *root)
20562064
outer_params = NULL;
20572065
for (proot = root->parent_root; proot != NULL; proot = proot->parent_root)
20582066
{
2059-
/* Include ordinary Var/PHV/Aggref params */
2067+
/* Include ordinary Var/PHV/Aggref/GroupingFunc params */
20602068
foreach(l, proot->plan_params)
20612069
{
20622070
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
@@ -7541,12 +7541,13 @@ get_parameter(Param *param, deparse_context *context)
75417541
context->varprefix = true;
75427542

75437543
/*
7544-
* A Param's expansion is typically a Var, Aggref, or upper-level
7545-
* Param, which wouldn't need extra parentheses. Otherwise, insert
7546-
* parens to ensure the expression looks atomic.
7544+
* A Param's expansion is typically a Var, Aggref, GroupingFunc, or
7545+
* upper-level Param, which wouldn't need extra parentheses.
7546+
* Otherwise, insert parens to ensure the expression looks atomic.
75477547
*/
75487548
need_paren = !(IsA(expr, Var) ||
75497549
IsA(expr, Aggref) ||
7550+
IsA(expr, GroupingFunc) ||
75507551
IsA(expr, Param));
75517552
if (need_paren)
75527553
appendStringInfoChar(context->buf, '(');
@@ -7628,6 +7629,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
76287629
case T_NextValueExpr:
76297630
case T_NullIfExpr:
76307631
case T_Aggref:
7632+
case T_GroupingFunc:
76317633
case T_WindowFunc:
76327634
case T_FuncExpr:
76337635
/* function-like: name(..) or name[..] */
@@ -7744,6 +7746,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
77447746
case T_XmlExpr: /* own parentheses */
77457747
case T_NullIfExpr: /* other separators */
77467748
case T_Aggref: /* own parentheses */
7749+
case T_GroupingFunc: /* own parentheses */
77477750
case T_WindowFunc: /* own parentheses */
77487751
case T_CaseExpr: /* other separators */
77497752
return true;
@@ -7794,6 +7797,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
77947797
case T_XmlExpr: /* own parentheses */
77957798
case T_NullIfExpr: /* other separators */
77967799
case T_Aggref: /* own parentheses */
7800+
case T_GroupingFunc: /* own parentheses */
77977801
case T_WindowFunc: /* own parentheses */
77987802
case T_CaseExpr: /* other separators */
77997803
return true;

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

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1929,4 +1929,49 @@ set work_mem to default;
19291929

19301930
drop table gs_group_1;
19311931
drop table gs_hash_1;
1932+
-- test handling of outer GroupingFunc within subqueries
1933+
explain (costs off)
1934+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
1935+
QUERY PLAN
1936+
---------------------------
1937+
MixedAggregate
1938+
Hash Key: $2
1939+
Group Key: ()
1940+
InitPlan 1 (returns $1)
1941+
-> Result
1942+
InitPlan 3 (returns $2)
1943+
-> Result
1944+
-> Result
1945+
SubPlan 2
1946+
-> Result
1947+
(10 rows)
1948+
1949+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
1950+
grouping
1951+
----------
1952+
1
1953+
0
1954+
(2 rows)
1955+
1956+
explain (costs off)
1957+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
1958+
QUERY PLAN
1959+
---------------------------
1960+
GroupAggregate
1961+
Group Key: $2
1962+
InitPlan 1 (returns $1)
1963+
-> Result
1964+
InitPlan 3 (returns $2)
1965+
-> Result
1966+
-> Result
1967+
SubPlan 2
1968+
-> Result
1969+
(9 rows)
1970+
1971+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
1972+
grouping
1973+
----------
1974+
0
1975+
(1 row)
1976+
19321977
-- end

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -529,4 +529,13 @@ set work_mem to default;
529529
drop table gs_group_1;
530530
drop table gs_hash_1;
531531

532+
-- test handling of outer GroupingFunc within subqueries
533+
explain (costs off)
534+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
535+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
536+
537+
explain (costs off)
538+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
539+
select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
540+
532541
-- end

0 commit comments

Comments
 (0)