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

Commitf13e2d1

Browse files
committed
Fix failure with initplans used conditionally during EvalPlanQual rechecks.
The EvalPlanQual machinery assumes that any initplans (that is,uncorrelated sub-selects) used during an EPQ recheck would have alreadybeen evaluated during the main query; this is implicit in the fact thatexecPlan pointers are not copied into the EPQ estate's es_param_exec_vals.But it's possible for that assumption to fail, if the initplan is onlyreached conditionally. For example, a sub-select inside a CASE expressioncould be reached during a recheck when it had not been previously, if theCASE test depends on a column that was just updated.This bug is old, appearing to date back to my rewrite of EvalPlanQual incommit9f2ee8f, but was not detected until Kyle Samson reported a case.To fix, force all not-yet-evaluated initplans used within the EPQ plansubtree to be evaluated at the start of the recheck, before entering theEPQ environment. This could be inefficient, if such an initplan isexpensive and goes unused again during the recheck --- but that's pilingone layer of improbability atop another. It doesn't seem worth addingmore complexity to prevent that, at least not in the back branches.It was convenient to use the new-in-v11 ExecEvalParamExecParams functionto implement this, but I didn't like either its name or the specifics ofits API, so revise that.Back-patch all the way. Rather than rewrite the patch to avoid dependingon bms_next_member() in the oldest branches, I chose to back-patch thatfunction into 9.4 and 9.3. (This isn't the first time back-patches haveneeded that, and it exhausted my patience.) I also chose to back-patchsome test cases added by commits71404af and342a1ff into 9.4 and 9.3,so that the 9.x versions of eval-plan-qual.spec are all the same.Andrew Gierth diagnosed the problem and contributed the added test cases,though the actual code changes are by me.Discussion:https://postgr.es/m/A033A40A-B234-4324-BE37-272279F7B627@tripadvisor.com
1 parent444455c commitf13e2d1

File tree

8 files changed

+147
-37
lines changed

8 files changed

+147
-37
lines changed

‎src/backend/executor/execExprInterp.c

Lines changed: 0 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -2252,33 +2252,6 @@ ExecEvalParamExec(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
22522252
*op->resnull=prm->isnull;
22532253
}
22542254

2255-
/*
2256-
* ExecEvalParamExecParams
2257-
*
2258-
* Execute the subplan stored in PARAM_EXEC initplans params, if not executed
2259-
* till now.
2260-
*/
2261-
void
2262-
ExecEvalParamExecParams(Bitmapset*params,EState*estate)
2263-
{
2264-
ParamExecData*prm;
2265-
intparamid;
2266-
2267-
paramid=-1;
2268-
while ((paramid=bms_next_member(params,paramid)) >=0)
2269-
{
2270-
prm=&(estate->es_param_exec_vals[paramid]);
2271-
2272-
if (prm->execPlan!=NULL)
2273-
{
2274-
/* Parameter not evaluated yet, so go do it */
2275-
ExecSetParamPlan(prm->execPlan,GetPerTupleExprContext(estate));
2276-
/* ExecSetParamPlan should have processed this param... */
2277-
Assert(prm->execPlan==NULL);
2278-
}
2279-
}
2280-
}
2281-
22822255
/*
22832256
* Evaluate a PARAM_EXTERN parameter.
22842257
*

‎src/backend/executor/execMain.c

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
#include"commands/matview.h"
4747
#include"commands/trigger.h"
4848
#include"executor/execdebug.h"
49+
#include"executor/nodeSubplan.h"
4950
#include"foreign/fdwapi.h"
5051
#include"mb/pg_wchar.h"
5152
#include"miscadmin.h"
@@ -1727,8 +1728,8 @@ ExecutePlan(EState *estate,
17271728
if (TupIsNull(slot))
17281729
{
17291730
/*
1730-
* If we know we won't need to back up, we can release
1731-
*resourcesat this point.
1731+
* If we know we won't need to back up, we can release resources
1732+
* at this point.
17321733
*/
17331734
if (!(estate->es_top_eflags&EXEC_FLAG_BACKWARD))
17341735
(void)ExecShutdownNode(planstate);
@@ -1778,8 +1779,8 @@ ExecutePlan(EState *estate,
17781779
if (numberTuples&&numberTuples==current_tuple_count)
17791780
{
17801781
/*
1781-
* If we know we won't need to back up, we can release
1782-
*resourcesat this point.
1782+
* If we know we won't need to back up, we can release resources
1783+
* at this point.
17831784
*/
17841785
if (!(estate->es_top_eflags&EXEC_FLAG_BACKWARD))
17851786
(void)ExecShutdownNode(planstate);
@@ -3078,6 +3079,14 @@ EvalPlanQualBegin(EPQState *epqstate, EState *parentestate)
30783079
{
30793080
inti;
30803081

3082+
/*
3083+
* Force evaluation of any InitPlan outputs that could be needed
3084+
* by the subplan, just in case they got reset since
3085+
* EvalPlanQualStart (see comments therein).
3086+
*/
3087+
ExecSetParamPlanMulti(planstate->plan->extParam,
3088+
GetPerTupleExprContext(parentestate));
3089+
30813090
i=list_length(parentestate->es_plannedstmt->paramExecTypes);
30823091

30833092
while (--i >=0)
@@ -3170,9 +3179,32 @@ EvalPlanQualStart(EPQState *epqstate, EState *parentestate, Plan *planTree)
31703179
{
31713180
inti;
31723181

3182+
/*
3183+
* Force evaluation of any InitPlan outputs that could be needed by
3184+
* the subplan. (With more complexity, maybe we could postpone this
3185+
* till the subplan actually demands them, but it doesn't seem worth
3186+
* the trouble; this is a corner case already, since usually the
3187+
* InitPlans would have been evaluated before reaching EvalPlanQual.)
3188+
*
3189+
* This will not touch output params of InitPlans that occur somewhere
3190+
* within the subplan tree, only those that are attached to the
3191+
* ModifyTable node or above it and are referenced within the subplan.
3192+
* That's OK though, because the planner would only attach such
3193+
* InitPlans to a lower-level SubqueryScan node, and EPQ execution
3194+
* will not descend into a SubqueryScan.
3195+
*
3196+
* The EState's per-output-tuple econtext is sufficiently short-lived
3197+
* for this, since it should get reset before there is any chance of
3198+
* doing EvalPlanQual again.
3199+
*/
3200+
ExecSetParamPlanMulti(planTree->extParam,
3201+
GetPerTupleExprContext(parentestate));
3202+
3203+
/* now make the internal param workspace ... */
31733204
i=list_length(parentestate->es_plannedstmt->paramExecTypes);
31743205
estate->es_param_exec_vals= (ParamExecData*)
31753206
palloc0(i*sizeof(ParamExecData));
3207+
/* ... and copy down all values, whether really needed or not */
31763208
while (--i >=0)
31773209
{
31783210
/* copy value if any, but not execPlan link */

‎src/backend/executor/execParallel.c

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323

2424
#include"postgres.h"
2525

26-
#include"executor/execExpr.h"
2726
#include"executor/execParallel.h"
2827
#include"executor/executor.h"
2928
#include"executor/nodeAppend.h"
@@ -36,6 +35,7 @@
3635
#include"executor/nodeIndexonlyscan.h"
3736
#include"executor/nodeSeqscan.h"
3837
#include"executor/nodeSort.h"
38+
#include"executor/nodeSubplan.h"
3939
#include"executor/tqueue.h"
4040
#include"nodes/nodeFuncs.h"
4141
#include"optimizer/planmain.h"
@@ -581,8 +581,18 @@ ExecInitParallelPlan(PlanState *planstate, EState *estate,
581581
char*query_string;
582582
intquery_len;
583583

584-
/* Force parameters we're going to pass to workers to be evaluated. */
585-
ExecEvalParamExecParams(sendParams,estate);
584+
/*
585+
* Force any initplan outputs that we're going to pass to workers to be
586+
* evaluated, if they weren't already.
587+
*
588+
* For simplicity, we use the EState's per-output-tuple ExprContext here.
589+
* That risks intra-query memory leakage, since we might pass through here
590+
* many times before that ExprContext gets reset; but ExecSetParamPlan
591+
* doesn't normally leak any memory in the context (see its comments), so
592+
* it doesn't seem worth complicating this function's API to pass it a
593+
* shorter-lived ExprContext. This might need to change someday.
594+
*/
595+
ExecSetParamPlanMulti(sendParams,GetPerTupleExprContext(estate));
586596

587597
/* Allocate object for return value. */
588598
pei=palloc0(sizeof(ParallelExecutorInfo));
@@ -831,8 +841,12 @@ ExecParallelReinitialize(PlanState *planstate,
831841
/* Old workers must already be shut down */
832842
Assert(pei->finished);
833843

834-
/* Force parameters we're going to pass to workers to be evaluated. */
835-
ExecEvalParamExecParams(sendParams,estate);
844+
/*
845+
* Force any initplan outputs that we're going to pass to workers to be
846+
* evaluated, if they weren't already (see comments in
847+
* ExecInitParallelPlan).
848+
*/
849+
ExecSetParamPlanMulti(sendParams,GetPerTupleExprContext(estate));
836850

837851
ReinitializeParallelDSM(pei->pcxt);
838852
pei->tqueue=ExecParallelSetupTupleQueues(pei->pcxt, true);

‎src/backend/executor/nodeSubplan.c

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1009,6 +1009,17 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
10091009
* of initplans: we don't run the subplan until/unless we need its output.
10101010
* Note that this routine MUST clear the execPlan fields of the plan's
10111011
* output parameters after evaluating them!
1012+
*
1013+
* The results of this function are stored in the EState associated with the
1014+
* ExprContext (particularly, its ecxt_param_exec_vals); any pass-by-ref
1015+
* result Datums are allocated in the EState's per-query memory. The passed
1016+
* econtext can be any ExprContext belonging to that EState; which one is
1017+
* important only to the extent that the ExprContext's per-tuple memory
1018+
* context is used to evaluate any parameters passed down to the subplan.
1019+
* (Thus in principle, the shorter-lived the ExprContext the better, since
1020+
* that data isn't needed after we return. In practice, because initplan
1021+
* parameters are never more complex than Vars, Aggrefs, etc, evaluating them
1022+
* currently never leaks any memory anyway.)
10121023
* ----------------------------------------------------------------
10131024
*/
10141025
void
@@ -1195,6 +1206,37 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
11951206
estate->es_direction=dir;
11961207
}
11971208

1209+
/*
1210+
* ExecSetParamPlanMulti
1211+
*
1212+
* Apply ExecSetParamPlan to evaluate any not-yet-evaluated initplan output
1213+
* parameters whose ParamIDs are listed in "params". Any listed params that
1214+
* are not initplan outputs are ignored.
1215+
*
1216+
* As with ExecSetParamPlan, any ExprContext belonging to the current EState
1217+
* can be used, but in principle a shorter-lived ExprContext is better than a
1218+
* longer-lived one.
1219+
*/
1220+
void
1221+
ExecSetParamPlanMulti(constBitmapset*params,ExprContext*econtext)
1222+
{
1223+
intparamid;
1224+
1225+
paramid=-1;
1226+
while ((paramid=bms_next_member(params,paramid)) >=0)
1227+
{
1228+
ParamExecData*prm=&(econtext->ecxt_param_exec_vals[paramid]);
1229+
1230+
if (prm->execPlan!=NULL)
1231+
{
1232+
/* Parameter not evaluated yet, so go do it */
1233+
ExecSetParamPlan(prm->execPlan,econtext);
1234+
/* ExecSetParamPlan should have processed this param... */
1235+
Assert(prm->execPlan==NULL);
1236+
}
1237+
}
1238+
}
1239+
11981240
/*
11991241
* Mark an initplan as needing recalculation
12001242
*/

‎src/include/executor/execExpr.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -697,7 +697,6 @@ extern void ExecEvalFuncExprStrictFusage(ExprState *state, ExprEvalStep *op,
697697
ExprContext*econtext);
698698
externvoidExecEvalParamExec(ExprState*state,ExprEvalStep*op,
699699
ExprContext*econtext);
700-
externvoidExecEvalParamExecParams(Bitmapset*params,EState*estate);
701700
externvoidExecEvalParamExtern(ExprState*state,ExprEvalStep*op,
702701
ExprContext*econtext);
703702
externvoidExecEvalSQLValueFunction(ExprState*state,ExprEvalStep*op);

‎src/include/executor/nodeSubplan.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,4 +28,6 @@ extern void ExecReScanSetParamPlan(SubPlanState *node, PlanState *parent);
2828

2929
externvoidExecSetParamPlan(SubPlanState*node,ExprContext*econtext);
3030

31+
externvoidExecSetParamPlanMulti(constBitmapset*params,ExprContext*econtext);
32+
3133
#endif/* NODESUBPLAN_H */

‎src/test/isolation/expected/eval-plan-qual.out

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,37 @@ ta_id ta_value tb_row
184184
1 newTableAValue (1,tableBValue)
185185
step c2: COMMIT;
186186

187+
starting permutation: updateforcip updateforcip2 c1 c2 read_a
188+
step updateforcip:
189+
UPDATE table_a SET value = NULL WHERE id = 1;
190+
191+
step updateforcip2:
192+
UPDATE table_a SET value = COALESCE(value, (SELECT text 'newValue')) WHERE id = 1;
193+
<waiting ...>
194+
step c1: COMMIT;
195+
step updateforcip2: <... completed>
196+
step c2: COMMIT;
197+
step read_a: SELECT * FROM table_a ORDER BY id;
198+
id value
199+
200+
1 newValue
201+
202+
starting permutation: updateforcip updateforcip3 c1 c2 read_a
203+
step updateforcip:
204+
UPDATE table_a SET value = NULL WHERE id = 1;
205+
206+
step updateforcip3:
207+
WITH d(val) AS (SELECT text 'newValue' FROM generate_series(1,1))
208+
UPDATE table_a SET value = COALESCE(value, (SELECT val FROM d)) WHERE id = 1;
209+
<waiting ...>
210+
step c1: COMMIT;
211+
step updateforcip3: <... completed>
212+
step c2: COMMIT;
213+
step read_a: SELECT * FROM table_a ORDER BY id;
214+
id value
215+
216+
1 newValue
217+
187218
starting permutation: wrtwcte readwcte c1 c2
188219
step wrtwcte: UPDATE table_a SET value = 'tableAValue2' WHERE id = 1;
189220
step readwcte:

‎src/test/isolation/specs/eval-plan-qual.spec

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,13 @@ step "updateforss"{
9292
UPDATEtable_bSETvalue='newTableBValue'WHEREid=1;
9393
}
9494

95+
# these tests exercise EvalPlanQual with conditional InitPlans which
96+
# have not been executed prior to the EPQ
97+
98+
step"updateforcip"{
99+
UPDATEtable_aSETvalue=NULLWHEREid=1;
100+
}
101+
95102
# these tests exercise mark/restore during EPQ recheck, cf bug #15032
96103

97104
step"selectjoinforupdate"{
@@ -129,6 +136,13 @@ step "readforss"{
129136
FROMtable_ata
130137
WHEREta.id=1FORUPDATEOFta;
131138
}
139+
step"updateforcip2"{
140+
UPDATEtable_aSETvalue=COALESCE(value, (SELECTtext'newValue'))WHEREid=1;
141+
}
142+
step"updateforcip3"{
143+
WITHd(val)AS (SELECTtext'newValue'FROMgenerate_series(1,1))
144+
UPDATEtable_aSETvalue=COALESCE(value, (SELECTvalFROMd))WHEREid=1;
145+
}
132146
step"wrtwcte"{UPDATEtable_aSETvalue='tableAValue2'WHEREid=1; }
133147
step"wrjt"{UPDATEjointestSETdata=42WHEREid=7; }
134148
step"c2"{COMMIT; }
@@ -137,6 +151,7 @@ session "s3"
137151
setup{BEGINISOLATIONLEVELREADCOMMITTED; }
138152
step"read"{SELECT*FROMaccountsORDERBYaccountid; }
139153
step"read_ext"{SELECT*FROMaccounts_extORDERBYaccountid; }
154+
step"read_a"{SELECT*FROMtable_aORDERBYid; }
140155

141156
# this test exercises EvalPlanQual with a CTE, cf bug #14328
142157
step"readwcte"{
@@ -171,6 +186,8 @@ permutation "wx2" "partiallock" "c2" "c1" "read"
171186
permutation"wx2""lockwithvalues""c2""c1""read"
172187
permutation"wx2_ext""partiallock_ext""c2""c1""read_ext"
173188
permutation"updateforss""readforss""c1""c2"
189+
permutation"updateforcip""updateforcip2""c1""c2""read_a"
190+
permutation"updateforcip""updateforcip3""c1""c2""read_a"
174191
permutation"wrtwcte""readwcte""c1""c2"
175192
permutation"wrjt""selectjoinforupdate""c2""c1"
176193
permutation"wrtwcte""multireadwcte""c1""c2"

0 commit comments

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