Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit7f90a5d

Browse files
committed
Account for optimized MinMax aggregates during SS_finalize_plan.
We are capable of optimizing MIN() and MAX() aggregates on indexedcolumns into subqueries that exploit the index, rather than the normalthing of scanning the whole table. When we do this, we replace theAggref node(s) with Params referencing subquery outputs. Such Paramsreally ought to be included in the per-plan-node extParam/allParamsets computed by SS_finalize_plan. However, we've never done soup to now because of an ancient implementation choice to performthat substitution during set_plan_references, which runs afterSS_finalize_plan, so that SS_finalize_plan never sees these Params.The cleanest fix would be to perform a separate tree walk to dothese substitutions before SS_finalize_plan runs. That seemsunattractive, first because a whole-tree mutation pass is expensive,and second because we lack infrastructure for visiting expressionsubtrees in a Plan tree, so that we'd need a new function knowingas much as SS_finalize_plan knows about that. I also consideredswapping 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 itselfto check for such Aggrefs. I refactored things a bit in setrefs.cto avoid having three copies of the code that does that.Back-patch of v17 commitsd0d4404 and779ac2c. Whend0d4404went 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 supportedbranches.Discussion:https://postgr.es/m/18465-2fae927718976b22@postgresql.orgDiscussion:https://postgr.es/m/2391880.1689025003@sss.pgh.pa.us
1 parentb030697 commit7f90a5d

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
@@ -1745,22 +1745,14 @@ fix_scan_expr_mutator(Node *node, fix_scan_expr_context *context)
17451745
if (IsA(node,Aggref))
17461746
{
17471747
Aggref*aggref= (Aggref*)node;
1748+
Param*aggparam;
17481749

17491750
/* See if the Aggref should be replaced by a Param */
1750-
if(context->root->minmax_aggs!=NIL&&
1751-
list_length(aggref->args)==1)
1751+
aggparam=find_minmax_agg_replacement_param(context->root,aggref);
1752+
if (aggparam!=NULL)
17521753
{
1753-
TargetEntry*curTarget= (TargetEntry*)linitial(aggref->args);
1754-
ListCell*lc;
1755-
1756-
foreach(lc,context->root->minmax_aggs)
1757-
{
1758-
MinMaxAggInfo*mminfo= (MinMaxAggInfo*)lfirst(lc);
1759-
1760-
if (mminfo->aggfnoid==aggref->aggfnoid&&
1761-
equal(mminfo->target,curTarget->expr))
1762-
return (Node*)copyObject(mminfo->param);
1763-
}
1754+
/* Make a copy of the Param for paranoia's sake */
1755+
return (Node*)copyObject(aggparam);
17641756
}
17651757
/* If no match, just fall through to process it normally */
17661758
}
@@ -2656,22 +2648,14 @@ fix_upper_expr_mutator(Node *node, fix_upper_expr_context *context)
26562648
if (IsA(node,Aggref))
26572649
{
26582650
Aggref*aggref= (Aggref*)node;
2651+
Param*aggparam;
26592652

26602653
/* See if the Aggref should be replaced by a Param */
2661-
if(context->root->minmax_aggs!=NIL&&
2662-
list_length(aggref->args)==1)
2654+
aggparam=find_minmax_agg_replacement_param(context->root,aggref);
2655+
if (aggparam!=NULL)
26632656
{
2664-
TargetEntry*curTarget= (TargetEntry*)linitial(aggref->args);
2665-
ListCell*lc;
2666-
2667-
foreach(lc,context->root->minmax_aggs)
2668-
{
2669-
MinMaxAggInfo*mminfo= (MinMaxAggInfo*)lfirst(lc);
2670-
2671-
if (mminfo->aggfnoid==aggref->aggfnoid&&
2672-
equal(mminfo->target,curTarget->expr))
2673-
return (Node*)copyObject(mminfo->param);
2674-
}
2657+
/* Make a copy of the Param for paranoia's sake */
2658+
return (Node*)copyObject(aggparam);
26752659
}
26762660
/* If no match, just fall through to process it normally */
26772661
}
@@ -2747,6 +2731,38 @@ set_returning_clause_references(PlannerInfo *root,
27472731
}
27482732

27492733

2734+
/*
2735+
* find_minmax_agg_replacement_param
2736+
*If the given Aggref is one that we are optimizing into a subquery
2737+
*(cf. planagg.c), then return the Param that should replace it.
2738+
*Else return NULL.
2739+
*
2740+
* This is exported so that SS_finalize_plan can use it before setrefs.c runs.
2741+
* Note that it will not find anything until we have built a Plan from a
2742+
* MinMaxAggPath, as root->minmax_aggs will never be filled otherwise.
2743+
*/
2744+
Param*
2745+
find_minmax_agg_replacement_param(PlannerInfo*root,Aggref*aggref)
2746+
{
2747+
if (root->minmax_aggs!=NIL&&
2748+
list_length(aggref->args)==1)
2749+
{
2750+
TargetEntry*curTarget= (TargetEntry*)linitial(aggref->args);
2751+
ListCell*lc;
2752+
2753+
foreach(lc,root->minmax_aggs)
2754+
{
2755+
MinMaxAggInfo*mminfo= (MinMaxAggInfo*)lfirst(lc);
2756+
2757+
if (mminfo->aggfnoid==aggref->aggfnoid&&
2758+
equal(mminfo->target,curTarget->expr))
2759+
returnmminfo->param;
2760+
}
2761+
}
2762+
returnNULL;
2763+
}
2764+
2765+
27502766
/*****************************************************************************
27512767
*QUERY DEPENDENCY MANAGEMENT
27522768
*****************************************************************************/

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

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

28252825
/*
2826-
* finalize_primnode: add IDs of all PARAM_EXEC paramsappearing in the given
2827-
* expression tree to the result set.
2826+
* finalize_primnode: add IDs of all PARAM_EXEC paramsthat appear (or will
2827+
*appear) in the givenexpression tree to the result set.
28282828
*/
28292829
staticbool
28302830
finalize_primnode(Node*node,finalize_primnode_context*context)
@@ -2841,7 +2841,26 @@ finalize_primnode(Node *node, finalize_primnode_context *context)
28412841
}
28422842
return false;/* no more to do here */
28432843
}
2844-
if (IsA(node,SubPlan))
2844+
elseif (IsA(node,Aggref))
2845+
{
2846+
/*
2847+
* Check to see if the aggregate will be replaced by a Param
2848+
* referencing a subquery output during setrefs.c. If so, we must
2849+
* account for that Param here. (For various reasons, it's not
2850+
* convenient to perform that substitution earlier than setrefs.c, nor
2851+
* to perform this processing after setrefs.c. Thus we need a wart
2852+
* here.)
2853+
*/
2854+
Aggref*aggref= (Aggref*)node;
2855+
Param*aggparam;
2856+
2857+
aggparam=find_minmax_agg_replacement_param(context->root,aggref);
2858+
if (aggparam!=NULL)
2859+
context->paramids=bms_add_member(context->paramids,
2860+
aggparam->paramid);
2861+
/* Fall through to examine the agg's arguments */
2862+
}
2863+
elseif (IsA(node,SubPlan))
28452864
{
28462865
SubPlan*subplan= (SubPlan*)node;
28472866
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
@@ -112,6 +112,8 @@ extern bool innerrel_is_unique(PlannerInfo *root,
112112
* prototypes for plan/setrefs.c
113113
*/
114114
externPlan*set_plan_references(PlannerInfo*root,Plan*plan);
115+
externParam*find_minmax_agg_replacement_param(PlannerInfo*root,
116+
Aggref*aggref);
115117
externvoidrecord_plan_function_dependency(PlannerInfo*root,Oidfuncid);
116118
externvoidrecord_plan_type_dependency(PlannerInfo*root,Oidtypid);
117119
externboolextract_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
@@ -1178,6 +1178,37 @@ NOTICE: drop cascades to 3 other objects
11781178
DETAIL: drop cascades to table minmaxtest1
11791179
drop cascades to table minmaxtest2
11801180
drop cascades to table minmaxtest3
1181+
-- DISTINCT can also trigger wrong answers with hash aggregation (bug #18465)
1182+
begin;
1183+
set local enable_sort = off;
1184+
explain (costs off)
1185+
select f1, (select distinct min(t1.f1) from int4_tbl t1 where t1.f1 = t0.f1)
1186+
from int4_tbl t0;
1187+
QUERY PLAN
1188+
---------------------------------------------------------------------
1189+
Seq Scan on int4_tbl t0
1190+
SubPlan 2
1191+
-> HashAggregate
1192+
Group Key: $1
1193+
InitPlan 1 (returns $1)
1194+
-> Limit
1195+
-> Seq Scan on int4_tbl t1
1196+
Filter: ((f1 IS NOT NULL) AND (f1 = t0.f1))
1197+
-> Result
1198+
(9 rows)
1199+
1200+
select f1, (select distinct min(t1.f1) from int4_tbl t1 where t1.f1 = t0.f1)
1201+
from int4_tbl t0;
1202+
f1 | min
1203+
-------------+-------------
1204+
0 | 0
1205+
123456 | 123456
1206+
-123456 | -123456
1207+
2147483647 | 2147483647
1208+
-2147483647 | -2147483647
1209+
(5 rows)
1210+
1211+
rollback;
11811212
-- check for correct detection of nested-aggregate errors
11821213
select max(min(unique1)) from tenk1;
11831214
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
@@ -394,6 +394,16 @@ select distinct min(f1), max(f1) from minmaxtest;
394394

395395
droptable minmaxtest cascade;
396396

397+
-- DISTINCT can also trigger wrong answers with hash aggregation (bug #18465)
398+
begin;
399+
set local enable_sort= off;
400+
explain (costs off)
401+
select f1, (select distinctmin(t1.f1)from int4_tbl t1wheret1.f1=t0.f1)
402+
from int4_tbl t0;
403+
select f1, (select distinctmin(t1.f1)from int4_tbl t1wheret1.f1=t0.f1)
404+
from int4_tbl t0;
405+
rollback;
406+
397407
-- check for correct detection of nested-aggregate errors
398408
selectmax(min(unique1))from tenk1;
399409
select (selectmax(min(unique1))from int8_tbl)from tenk1;

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp