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

Commit772a6d4

Browse files
committed
Fix mis-calculation of extParam/allParam sets for plan nodes, as seen in
bug #4290. The fundamental bug is that masking extParam by outer_params,as finalize_plan had been doing, caused us to lose the information thatan initPlan depended on the output of a sibling initPlan. On reflectionthe best thing to do seemed to be not to try to adjust outer_params forthis case but get rid of it entirely. The only thing it was really doingfor us was to filter out param IDs associated with SubPlan nodes, and thatcan be done (with greater accuracy) while processing individual SubPlannodes in finalize_primnode. This approach was vindicated by the discoverythat the masking method was hiding a second bug: SS_finalize_plan failed toremove extParam bits for initPlan output params that were referenced in themain plan tree (it only got rid of those referenced by other initPlans).It's not clear that this caused any real problems, given the limited useof extParam by the executor, but it's certainly not what was intended.I originally thought that there was also a problem with needing to includeindirect dependencies on external params in initPlans' param sets, but itturns out that the executor handles this correctly so long as the depended-oninitPlan is earlier in the initPlans list than the one using its output.That seems a bit of a fragile assumption, but it is true at the moment,so I just documented it in some code comments rather than making what wouldbe rather invasive changes to remove the assumption.Back-patch to 8.1. Previous versions don't have the case of initPlansreferring to other initPlans' outputs, so while the existing logic is stillquestionable for them, there are not any known bugs to be fixed. So I'llrefrain from changing them for now.
1 parent6b7eebc commit772a6d4

File tree

5 files changed

+145
-48
lines changed

5 files changed

+145
-48
lines changed

‎src/backend/executor/execAmi.c

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
77
* Portions Copyright (c) 1994, Regents of the University of California
88
*
9-
*$PostgreSQL: pgsql/src/backend/executor/execAmi.c,v 1.94 2008/01/01 19:45:49 momjian Exp $
9+
*$PostgreSQL: pgsql/src/backend/executor/execAmi.c,v 1.95 2008/07/10 01:17:29 tgl Exp $
1010
*
1111
*-------------------------------------------------------------------------
1212
*/
@@ -63,7 +63,19 @@ ExecReScan(PlanState *node, ExprContext *exprCtxt)
6363
if (node->instrument)
6464
InstrEndLoop(node->instrument);
6565

66-
/* If we have changed parameters, propagate that info */
66+
/*
67+
* If we have changed parameters, propagate that info.
68+
*
69+
* Note: ExecReScanSetParamPlan() can add bits to node->chgParam,
70+
* corresponding to the output param(s) that the InitPlan will update.
71+
* Since we make only one pass over the list, that means that an InitPlan
72+
* can depend on the output param(s) of a sibling InitPlan only if that
73+
* sibling appears earlier in the list. This is workable for now given
74+
* the limited ways in which one InitPlan could depend on another, but
75+
* eventually we might need to work harder (or else make the planner
76+
* enlarge the extParam/allParam sets to include the params of depended-on
77+
* InitPlans).
78+
*/
6779
if (node->chgParam!=NULL)
6880
{
6981
ListCell*l;

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/optimizer/plan/planagg.c,v 1.39 2008/06/19 00:46:04 alvherre Exp $
11+
* $PostgreSQL: pgsql/src/backend/optimizer/plan/planagg.c,v 1.40 2008/07/10 01:17:29 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -563,7 +563,11 @@ make_agg_subplan(PlannerInfo *root, MinMaxAggInfo *info)
563563
exprType((Node*)tle->expr),
564564
-1);
565565

566-
/* Make sure the InitPlan gets into the outer list */
566+
/*
567+
* Make sure the InitPlan gets into the outer list. It has to appear
568+
* after any other InitPlans it might depend on, too (see comments in
569+
* ExecReScan).
570+
*/
567571
root->init_plans=list_concat(root->init_plans,subroot.init_plans);
568572
}
569573

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

Lines changed: 72 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1994, Regents of the University of California
88
*
99
* IDENTIFICATION
10-
* $PostgreSQL: pgsql/src/backend/optimizer/plan/subselect.c,v 1.130 2008/04/21 20:54:15 tgl Exp $
10+
* $PostgreSQL: pgsql/src/backend/optimizer/plan/subselect.c,v 1.131 2008/07/10 01:17:29 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -47,8 +47,7 @@ typedef struct process_sublinks_context
4747
typedefstructfinalize_primnode_context
4848
{
4949
PlannerInfo*root;
50-
Bitmapset*paramids;/* Set of PARAM_EXEC paramids found */
51-
Bitmapset*outer_params;/* Set of accessible outer paramids */
50+
Bitmapset*paramids;/* Non-local PARAM_EXEC paramids found */
5251
}finalize_primnode_context;
5352

5453

@@ -68,7 +67,6 @@ static Node *process_sublinks_mutator(Node *node,
6867
process_sublinks_context*context);
6968
staticBitmapset*finalize_plan(PlannerInfo*root,
7069
Plan*plan,
71-
Bitmapset*outer_params,
7270
Bitmapset*valid_params);
7371
staticboolfinalize_primnode(Node*node,finalize_primnode_context*context);
7472

@@ -394,17 +392,21 @@ make_subplan(PlannerInfo *root, SubLink *slink, Node *testexpr, bool isTopQual)
394392
}
395393
else
396394
{
397-
List*params;
398395
List*args;
399396
ListCell*l;
400397

401-
/* Adjust the Params */
402-
params=generate_subquery_params(root,
403-
plan->targetlist,
404-
&splan->paramIds);
405-
splan->testexpr=convert_testexpr(root,
406-
testexpr,
407-
params);
398+
if (testexpr)
399+
{
400+
List*params;
401+
402+
/* Adjust the Params in the testexpr */
403+
params=generate_subquery_params(root,
404+
plan->targetlist,
405+
&splan->paramIds);
406+
splan->testexpr=convert_testexpr(root,
407+
testexpr,
408+
params);
409+
}
408410

409411
/*
410412
* We can't convert subplans of ALL_SUBLINK or ANY_SUBLINK types to
@@ -1031,8 +1033,7 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context)
10311033
void
10321034
SS_finalize_plan(PlannerInfo*root,Plan*plan)
10331035
{
1034-
Bitmapset*outer_params,
1035-
*valid_params,
1036+
Bitmapset*valid_params,
10361037
*initExtParam,
10371038
*initSetParam;
10381039
Costinitplan_cost;
@@ -1042,9 +1043,12 @@ SS_finalize_plan(PlannerInfo *root, Plan *plan)
10421043
/*
10431044
* First, scan the param list to discover the sets of params that are
10441045
* available from outer query levels and my own query level. We do this
1045-
* once to save time in the per-plan recursion steps.
1046+
* once to save time in the per-plan recursion steps. (This calculation
1047+
* is overly generous: it can include a lot of params that actually
1048+
* shouldn't be referenced here. However, valid_params is just used as
1049+
* a debugging crosscheck, so it's not worth trying to be exact.)
10461050
*/
1047-
outer_params=valid_params=NULL;
1051+
valid_params=NULL;
10481052
paramid=0;
10491053
foreach(l,root->glob->paramlist)
10501054
{
@@ -1053,7 +1057,6 @@ SS_finalize_plan(PlannerInfo *root, Plan *plan)
10531057
if (pitem->abslevel<root->query_level)
10541058
{
10551059
/* valid outer-level parameter */
1056-
outer_params=bms_add_member(outer_params,paramid);
10571060
valid_params=bms_add_member(valid_params,paramid);
10581061
}
10591062
elseif (pitem->abslevel==root->query_level&&
@@ -1069,9 +1072,8 @@ SS_finalize_plan(PlannerInfo *root, Plan *plan)
10691072
/*
10701073
* Now recurse through plan tree.
10711074
*/
1072-
(void)finalize_plan(root,plan,outer_params,valid_params);
1075+
(void)finalize_plan(root,plan,valid_params);
10731076

1074-
bms_free(outer_params);
10751077
bms_free(valid_params);
10761078

10771079
/*
@@ -1107,11 +1109,13 @@ SS_finalize_plan(PlannerInfo *root, Plan *plan)
11071109
/* allParam must include all these params */
11081110
plan->allParam=bms_add_members(plan->allParam,initExtParam);
11091111
plan->allParam=bms_add_members(plan->allParam,initSetParam);
1112+
/* extParam must include any child extParam */
1113+
plan->extParam=bms_add_members(plan->extParam,initExtParam);
11101114
/* but extParam shouldn't include any setParams */
1111-
initExtParam=bms_del_members(initExtParam,initSetParam);
1112-
/*empty test ensures extParam is exactly NULL if it's empty */
1113-
if (!bms_is_empty(initExtParam))
1114-
plan->extParam=bms_join(plan->extParam,initExtParam);
1115+
plan->extParam=bms_del_members(plan->extParam,initSetParam);
1116+
/*ensure extParam is exactly NULL if it's empty */
1117+
if (bms_is_empty(plan->extParam))
1118+
plan->extParam=NULL;
11151119

11161120
plan->startup_cost+=initplan_cost;
11171121
plan->total_cost+=initplan_cost;
@@ -1124,8 +1128,7 @@ SS_finalize_plan(PlannerInfo *root, Plan *plan)
11241128
* This is just an internal notational convenience.
11251129
*/
11261130
staticBitmapset*
1127-
finalize_plan(PlannerInfo*root,Plan*plan,
1128-
Bitmapset*outer_params,Bitmapset*valid_params)
1131+
finalize_plan(PlannerInfo*root,Plan*plan,Bitmapset*valid_params)
11291132
{
11301133
finalize_primnode_contextcontext;
11311134

@@ -1134,7 +1137,6 @@ finalize_plan(PlannerInfo *root, Plan *plan,
11341137

11351138
context.root=root;
11361139
context.paramids=NULL;/* initialize set to empty */
1137-
context.outer_params=outer_params;
11381140

11391141
/*
11401142
* When we call finalize_primnode, context.paramids sets are automatically
@@ -1218,7 +1220,6 @@ finalize_plan(PlannerInfo *root, Plan *plan,
12181220
bms_add_members(context.paramids,
12191221
finalize_plan(root,
12201222
(Plan*)lfirst(l),
1221-
outer_params,
12221223
valid_params));
12231224
}
12241225
}
@@ -1234,7 +1235,6 @@ finalize_plan(PlannerInfo *root, Plan *plan,
12341235
bms_add_members(context.paramids,
12351236
finalize_plan(root,
12361237
(Plan*)lfirst(l),
1237-
outer_params,
12381238
valid_params));
12391239
}
12401240
}
@@ -1250,7 +1250,6 @@ finalize_plan(PlannerInfo *root, Plan *plan,
12501250
bms_add_members(context.paramids,
12511251
finalize_plan(root,
12521252
(Plan*)lfirst(l),
1253-
outer_params,
12541253
valid_params));
12551254
}
12561255
}
@@ -1301,36 +1300,36 @@ finalize_plan(PlannerInfo *root, Plan *plan,
13011300
context.paramids=bms_add_members(context.paramids,
13021301
finalize_plan(root,
13031302
plan->lefttree,
1304-
outer_params,
13051303
valid_params));
13061304

13071305
context.paramids=bms_add_members(context.paramids,
13081306
finalize_plan(root,
13091307
plan->righttree,
1310-
outer_params,
13111308
valid_params));
13121309

13131310
/* Now we have all the paramids */
13141311

13151312
if (!bms_is_subset(context.paramids,valid_params))
13161313
elog(ERROR,"plan should not reference subplan's variable");
13171314

1318-
plan->extParam=bms_intersect(context.paramids,outer_params);
1319-
plan->allParam=context.paramids;
1320-
13211315
/*
1316+
* Note: by definition, extParam and allParam should have the same value
1317+
* in any plan node that doesn't have child initPlans. We set them
1318+
* equal here, and later SS_finalize_plan will update them properly
1319+
* in node(s) that it attaches initPlans to.
1320+
*
13221321
* For speed at execution time, make sure extParam/allParam are actually
13231322
* NULL if they are empty sets.
13241323
*/
1325-
if (bms_is_empty(plan->extParam))
1324+
if (bms_is_empty(context.paramids))
13261325
{
1327-
bms_free(plan->extParam);
13281326
plan->extParam=NULL;
1327+
plan->allParam=NULL;
13291328
}
1330-
if (bms_is_empty(plan->allParam))
1329+
else
13311330
{
1332-
bms_free(plan->allParam);
1333-
plan->allParam=NULL;
1331+
plan->extParam=context.paramids;
1332+
plan->allParam=bms_copy(context.paramids);
13341333
}
13351334

13361335
returnplan->allParam;
@@ -1359,12 +1358,41 @@ finalize_primnode(Node *node, finalize_primnode_context *context)
13591358
{
13601359
SubPlan*subplan= (SubPlan*)node;
13611360
Plan*plan=planner_subplan_get_plan(context->root,subplan);
1361+
ListCell*lc;
1362+
Bitmapset*subparamids;
1363+
1364+
/* Recurse into the testexpr, but not into the Plan */
1365+
finalize_primnode(subplan->testexpr,context);
1366+
1367+
/*
1368+
* Remove any param IDs of output parameters of the subplan that were
1369+
* referenced in the testexpr. These are not interesting for
1370+
* parameter change signaling since we always re-evaluate the subplan.
1371+
* Note that this wouldn't work too well if there might be uses of the
1372+
* same param IDs elsewhere in the plan, but that can't happen because
1373+
* generate_new_param never tries to merge params.
1374+
*/
1375+
foreach(lc,subplan->paramIds)
1376+
{
1377+
context->paramids=bms_del_member(context->paramids,
1378+
lfirst_int(lc));
1379+
}
13621380

1363-
/* Add outer-level params needed by the subplan to paramids */
1364-
context->paramids=bms_join(context->paramids,
1365-
bms_intersect(plan->extParam,
1366-
context->outer_params));
1367-
/* fall through to recurse into subplan args */
1381+
/* Also examine args list */
1382+
finalize_primnode((Node*)subplan->args,context);
1383+
1384+
/*
1385+
* Add params needed by the subplan to paramids, but excluding those
1386+
* we will pass down to it.
1387+
*/
1388+
subparamids=bms_copy(plan->extParam);
1389+
foreach(lc,subplan->parParam)
1390+
{
1391+
subparamids=bms_del_member(subparamids,lfirst_int(lc));
1392+
}
1393+
context->paramids=bms_join(context->paramids,subparamids);
1394+
1395+
return false;/* no more to do here */
13681396
}
13691397
returnexpression_tree_walker(node,finalize_primnode,
13701398
(void*)context);

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -438,3 +438,30 @@ select * from numeric_table
438438
3
439439
(4 rows)
440440

441+
--
442+
-- Test case for bug #4290: bogus calculation of subplan param sets
443+
--
444+
create temp table ta (id int primary key, val int);
445+
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "ta_pkey" for table "ta"
446+
insert into ta values(1,1);
447+
insert into ta values(2,2);
448+
create temp table tb (id int primary key, aval int);
449+
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "tb_pkey" for table "tb"
450+
insert into tb values(1,1);
451+
insert into tb values(2,1);
452+
insert into tb values(3,2);
453+
insert into tb values(4,2);
454+
create temp table tc (id int primary key, aid int);
455+
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "tc_pkey" for table "tc"
456+
insert into tc values(1,1);
457+
insert into tc values(2,2);
458+
select
459+
( select min(tb.id) from tb
460+
where tb.aval = (select ta.val from ta where ta.id = tc.aid) ) as min_tb_id
461+
from tc;
462+
min_tb_id
463+
-----------
464+
1
465+
3
466+
(2 rows)
467+

‎src/test/regress/sql/subselect.sql

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,3 +272,29 @@ select * from float_table
272272

273273
select*from numeric_table
274274
where num_colin (select float_colfrom float_table);
275+
276+
--
277+
-- Test case for bug #4290: bogus calculation of subplan param sets
278+
--
279+
280+
create temp table ta (idintprimary key, valint);
281+
282+
insert into tavalues(1,1);
283+
insert into tavalues(2,2);
284+
285+
create temp table tb (idintprimary key, avalint);
286+
287+
insert into tbvalues(1,1);
288+
insert into tbvalues(2,1);
289+
insert into tbvalues(3,2);
290+
insert into tbvalues(4,2);
291+
292+
create temp table tc (idintprimary key, aidint);
293+
294+
insert into tcvalues(1,1);
295+
insert into tcvalues(2,2);
296+
297+
select
298+
(selectmin(tb.id)from tb
299+
wheretb.aval= (selectta.valfrom tawhereta.id=tc.aid) )as min_tb_id
300+
from tc;

0 commit comments

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