Skip to content

Commit 4ac385a

Browse files
tglsfdctglsfdc
committed
Account for optimized MinMax aggregates during SS_finalize_plan.
We are capable of optimizing MIN() and MAX() aggregates on indexed columns into subqueries that exploit the index, rather than the normal thing of scanning the whole table. When we do this, we replace the Aggref node(s) with Params referencing subquery outputs. Such Params really ought to be included in the per-plan-node extParam/allParam sets computed by SS_finalize_plan. However, we've never done so up to now because of an ancient implementation choice to perform that substitution during set_plan_references, which runs after SS_finalize_plan, so that SS_finalize_plan never sees these Params. The cleanest fix would be to perform a separate tree walk to do these substitutions before SS_finalize_plan runs. That seems unattractive, first because a whole-tree mutation pass is expensive, and second because we lack infrastructure for visiting expression subtrees in a Plan tree, so that we'd need a new function knowing as much as SS_finalize_plan knows about that. I also considered swapping the order of SS_finalize_plan and set_plan_references, but that fell foul of various assumptions that seem tricky to fix. So the approach adopted here is to teach SS_finalize_plan itself to check for such Aggrefs. I refactored things a bit in setrefs.c to avoid having three copies of the code that does that. Back-patch of v17 commits d0d4404 and 779ac2c. When d0d4404 went in, there was no evidence that it was fixing a reachable bug, so I refrained from back-patching. Now we have such evidence. Per bug #18465 from Hal Takahara. Back-patch to all supported branches. Discussion: https://postgr.es/m/18465-2fae927718976b22@postgresql.org Discussion: https://postgr.es/m/2391880.1689025003@sss.pgh.pa.us
1 parent 484b958 commit 4ac385a

File tree

5 files changed

+107
-29
lines changed

5 files changed

+107
-29
lines changed

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

Lines changed: 42 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2088,22 +2088,14 @@ fix_scan_expr_mutator(Node *node, fix_scan_expr_context *context)
20882088
if (IsA(node, Aggref))
20892089
{
20902090
Aggref *aggref = (Aggref *) node;
2091+
Param *aggparam;
20912092

20922093
/* See if the Aggref should be replaced by a Param */
2093-
if (context->root->minmax_aggs != NIL &&
2094-
list_length(aggref->args) == 1)
2094+
aggparam = find_minmax_agg_replacement_param(context->root, aggref);
2095+
if (aggparam != NULL)
20952096
{
2096-
TargetEntry *curTarget = (TargetEntry *) linitial(aggref->args);
2097-
ListCell *lc;
2098-
2099-
foreach(lc, context->root->minmax_aggs)
2100-
{
2101-
MinMaxAggInfo *mminfo = (MinMaxAggInfo *) lfirst(lc);
2102-
2103-
if (mminfo->aggfnoid == aggref->aggfnoid &&
2104-
equal(mminfo->target, curTarget->expr))
2105-
return (Node *) copyObject(mminfo->param);
2106-
}
2097+
/* Make a copy of the Param for paranoia's sake */
2098+
return (Node *) copyObject(aggparam);
21072099
}
21082100
/* If no match, just fall through to process it normally */
21092101
}
@@ -3030,22 +3022,14 @@ fix_upper_expr_mutator(Node *node, fix_upper_expr_context *context)
30303022
if (IsA(node, Aggref))
30313023
{
30323024
Aggref *aggref = (Aggref *) node;
3025+
Param *aggparam;
30333026

30343027
/* See if the Aggref should be replaced by a Param */
3035-
if (context->root->minmax_aggs != NIL &&
3036-
list_length(aggref->args) == 1)
3028+
aggparam = find_minmax_agg_replacement_param(context->root, aggref);
3029+
if (aggparam != NULL)
30373030
{
3038-
TargetEntry *curTarget = (TargetEntry *) linitial(aggref->args);
3039-
ListCell *lc;
3040-
3041-
foreach(lc, context->root->minmax_aggs)
3042-
{
3043-
MinMaxAggInfo *mminfo = (MinMaxAggInfo *) lfirst(lc);
3044-
3045-
if (mminfo->aggfnoid == aggref->aggfnoid &&
3046-
equal(mminfo->target, curTarget->expr))
3047-
return (Node *) copyObject(mminfo->param);
3048-
}
3031+
/* Make a copy of the Param for paranoia's sake */
3032+
return (Node *) copyObject(aggparam);
30493033
}
30503034
/* If no match, just fall through to process it normally */
30513035
}
@@ -3199,6 +3183,38 @@ set_windowagg_runcondition_references(PlannerInfo *root,
31993183
return newlist;
32003184
}
32013185

3186+
/*
3187+
* find_minmax_agg_replacement_param
3188+
* If the given Aggref is one that we are optimizing into a subquery
3189+
* (cf. planagg.c), then return the Param that should replace it.
3190+
* Else return NULL.
3191+
*
3192+
* This is exported so that SS_finalize_plan can use it before setrefs.c runs.
3193+
* Note that it will not find anything until we have built a Plan from a
3194+
* MinMaxAggPath, as root->minmax_aggs will never be filled otherwise.
3195+
*/
3196+
Param *
3197+
find_minmax_agg_replacement_param(PlannerInfo *root, Aggref *aggref)
3198+
{
3199+
if (root->minmax_aggs != NIL &&
3200+
list_length(aggref->args) == 1)
3201+
{
3202+
TargetEntry *curTarget = (TargetEntry *) linitial(aggref->args);
3203+
ListCell *lc;
3204+
3205+
foreach(lc, root->minmax_aggs)
3206+
{
3207+
MinMaxAggInfo *mminfo = (MinMaxAggInfo *) lfirst(lc);
3208+
3209+
if (mminfo->aggfnoid == aggref->aggfnoid &&
3210+
equal(mminfo->target, curTarget->expr))
3211+
return mminfo->param;
3212+
}
3213+
}
3214+
return NULL;
3215+
}
3216+
3217+
32023218
/*****************************************************************************
32033219
* QUERY DEPENDENCY MANAGEMENT
32043220
*****************************************************************************/

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

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2849,8 +2849,8 @@ finalize_plan(PlannerInfo *root, Plan *plan,
28492849
}
28502850

28512851
/*
2852-
* finalize_primnode: add IDs of all PARAM_EXEC params appearing in the given
2853-
* expression tree to the result set.
2852+
* finalize_primnode: add IDs of all PARAM_EXEC params that appear (or will
2853+
* appear) in the given expression tree to the result set.
28542854
*/
28552855
static bool
28562856
finalize_primnode(Node *node, finalize_primnode_context *context)
@@ -2867,7 +2867,26 @@ finalize_primnode(Node *node, finalize_primnode_context *context)
28672867
}
28682868
return false; /* no more to do here */
28692869
}
2870-
if (IsA(node, SubPlan))
2870+
else if (IsA(node, Aggref))
2871+
{
2872+
/*
2873+
* Check to see if the aggregate will be replaced by a Param
2874+
* referencing a subquery output during setrefs.c. If so, we must
2875+
* account for that Param here. (For various reasons, it's not
2876+
* convenient to perform that substitution earlier than setrefs.c, nor
2877+
* to perform this processing after setrefs.c. Thus we need a wart
2878+
* here.)
2879+
*/
2880+
Aggref *aggref = (Aggref *) node;
2881+
Param *aggparam;
2882+
2883+
aggparam = find_minmax_agg_replacement_param(context->root, aggref);
2884+
if (aggparam != NULL)
2885+
context->paramids = bms_add_member(context->paramids,
2886+
aggparam->paramid);
2887+
/* Fall through to examine the agg's arguments */
2888+
}
2889+
else if (IsA(node, SubPlan))
28712890
{
28722891
SubPlan *subplan = (SubPlan *) node;
28732892
Plan *plan = planner_subplan_get_plan(context->root, subplan);

‎src/include/optimizer/planmain.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,8 @@ extern bool innerrel_is_unique(PlannerInfo *root,
113113
*/
114114
extern Plan *set_plan_references(PlannerInfo *root, Plan *plan);
115115
extern bool trivial_subqueryscan(SubqueryScan *plan);
116+
extern Param *find_minmax_agg_replacement_param(PlannerInfo *root,
117+
Aggref *aggref);
116118
extern void record_plan_function_dependency(PlannerInfo *root, Oid funcid);
117119
extern void record_plan_type_dependency(PlannerInfo *root, Oid typid);
118120
extern bool extract_query_dependencies_walker(Node *node, PlannerInfo *root);

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1249,6 +1249,37 @@ NOTICE: drop cascades to 3 other objects
12491249
DETAIL: drop cascades to table minmaxtest1
12501250
drop cascades to table minmaxtest2
12511251
drop cascades to table minmaxtest3
1252+
-- DISTINCT can also trigger wrong answers with hash aggregation (bug #18465)
1253+
begin;
1254+
set local enable_sort = off;
1255+
explain (costs off)
1256+
select f1, (select distinct min(t1.f1) from int4_tbl t1 where t1.f1 = t0.f1)
1257+
from int4_tbl t0;
1258+
QUERY PLAN
1259+
---------------------------------------------------------------------
1260+
Seq Scan on int4_tbl t0
1261+
SubPlan 2
1262+
-> HashAggregate
1263+
Group Key: $1
1264+
InitPlan 1 (returns $1)
1265+
-> Limit
1266+
-> Seq Scan on int4_tbl t1
1267+
Filter: ((f1 IS NOT NULL) AND (f1 = t0.f1))
1268+
-> Result
1269+
(9 rows)
1270+
1271+
select f1, (select distinct min(t1.f1) from int4_tbl t1 where t1.f1 = t0.f1)
1272+
from int4_tbl t0;
1273+
f1 | min
1274+
-------------+-------------
1275+
0 | 0
1276+
123456 | 123456
1277+
-123456 | -123456
1278+
2147483647 | 2147483647
1279+
-2147483647 | -2147483647
1280+
(5 rows)
1281+
1282+
rollback;
12521283
-- check for correct detection of nested-aggregate errors
12531284
select max(min(unique1)) from tenk1;
12541285
ERROR: aggregate function calls cannot be nested

‎src/test/regress/sql/aggregates.sql

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,16 @@ select distinct min(f1), max(f1) from minmaxtest;
431431

432432
drop table minmaxtest cascade;
433433

434+
-- DISTINCT can also trigger wrong answers with hash aggregation (bug #18465)
435+
begin;
436+
set local enable_sort = off;
437+
explain (costs off)
438+
select f1, (select distinct min(t1.f1) from int4_tbl t1 where t1.f1 = t0.f1)
439+
from int4_tbl t0;
440+
select f1, (select distinct min(t1.f1) from int4_tbl t1 where t1.f1 = t0.f1)
441+
from int4_tbl t0;
442+
rollback;
443+
434444
-- check for correct detection of nested-aggregate errors
435445
select max(min(unique1)) from tenk1;
436446
select (select max(min(unique1)) from int8_tbl) from tenk1;

0 commit comments

Comments
 (0)