Skip to content

Commit 1f4a920

Browse files
tglsfdctglsfdc
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 already been evaluated during the main query; this is implicit in the fact that execPlan 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 only reached conditionally. For example, a sub-select inside a CASE expression could be reached during a recheck when it had not been previously, if the CASE test depends on a column that was just updated. This bug is old, appearing to date back to my rewrite of EvalPlanQual in commit 9f2ee8f, but was not detected until Kyle Samson reported a case. To fix, force all not-yet-evaluated initplans used within the EPQ plan subtree to be evaluated at the start of the recheck, before entering the EPQ environment. This could be inefficient, if such an initplan is expensive and goes unused again during the recheck --- but that's piling one layer of improbability atop another. It doesn't seem worth adding more complexity to prevent that, at least not in the back branches. It was convenient to use the new-in-v11 ExecEvalParamExecParams function to implement this, but I didn't like either its name or the specifics of its API, so revise that. Back-patch all the way. Rather than rewrite the patch to avoid depending on bms_next_member() in the oldest branches, I chose to back-patch that function into 9.4 and 9.3. (This isn't the first time back-patches have needed that, and it exhausted my patience.) I also chose to back-patch some test cases added by commits 71404af and 342a1ff 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 parent 6b78231 commit 1f4a920

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-
int paramid;
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-
* resources at 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-
* resources at 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
int i;
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
int i;
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
int query_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(const Bitmapset *params, ExprContext *econtext)
1222+
{
1223+
int paramid;
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
extern void ExecEvalParamExec(ExprState *state, ExprEvalStep *op,
699699
ExprContext *econtext);
700-
extern void ExecEvalParamExecParams(Bitmapset *params, EState *estate);
701700
extern void ExecEvalParamExtern(ExprState *state, ExprEvalStep *op,
702701
ExprContext *econtext);
703702
extern void ExecEvalSQLValueFunction(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
extern void ExecSetParamPlan(SubPlanState *node, ExprContext *econtext);
3030

31+
extern void ExecSetParamPlanMulti(const Bitmapset *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
UPDATE table_b SET value = 'newTableBValue' WHERE id = 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+
UPDATE table_a SET value = NULL WHERE id = 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
FROM table_a ta
130137
WHERE ta.id = 1 FOR UPDATE OF ta;
131138
}
139+
step "updateforcip2" {
140+
UPDATE table_a SET value = COALESCE(value, (SELECT text 'newValue')) WHERE id = 1;
141+
}
142+
step "updateforcip3" {
143+
WITH d(val) AS (SELECT text 'newValue' FROM generate_series(1,1))
144+
UPDATE table_a SET value = COALESCE(value, (SELECT val FROM d)) WHERE id = 1;
145+
}
132146
step "wrtwcte" { UPDATE table_a SET value = 'tableAValue2' WHERE id = 1; }
133147
step "wrjt" { UPDATE jointest SET data = 42 WHERE id = 7; }
134148
step "c2" { COMMIT; }
@@ -137,6 +151,7 @@ session "s3"
137151
setup { BEGIN ISOLATION LEVEL READ COMMITTED; }
138152
step "read" { SELECT * FROM accounts ORDER BY accountid; }
139153
step "read_ext" { SELECT * FROM accounts_ext ORDER BY accountid; }
154+
step "read_a" { SELECT * FROM table_a ORDER BY id; }
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)