diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c index 9d6e25aae5..f597363eb1 100644 --- a/src/backend/executor/execExprInterp.c +++ b/src/backend/executor/execExprInterp.c @@ -2252,33 +2252,6 @@ ExecEvalParamExec(ExprState *state, ExprEvalStep *op, ExprContext *econtext) *op->resnull = prm->isnull; } -/* - * ExecEvalParamExecParams - * - * Execute the subplan stored in PARAM_EXEC initplans params, if not executed - * till now. - */ -void -ExecEvalParamExecParams(Bitmapset *params, EState *estate) -{ - ParamExecData *prm; - int paramid; - - paramid = -1; - while ((paramid = bms_next_member(params, paramid)) >= 0) - { - prm = &(estate->es_param_exec_vals[paramid]); - - if (prm->execPlan != NULL) - { - /* Parameter not evaluated yet, so go do it */ - ExecSetParamPlan(prm->execPlan, GetPerTupleExprContext(estate)); - /* ExecSetParamPlan should have processed this param... */ - Assert(prm->execPlan == NULL); - } - } -} - /* * Evaluate a PARAM_EXTERN parameter. * diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index c583e020a0..85d980356b 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -46,6 +46,7 @@ #include "commands/matview.h" #include "commands/trigger.h" #include "executor/execdebug.h" +#include "executor/nodeSubplan.h" #include "foreign/fdwapi.h" #include "mb/pg_wchar.h" #include "miscadmin.h" @@ -1727,8 +1728,8 @@ ExecutePlan(EState *estate, if (TupIsNull(slot)) { /* - * If we know we won't need to back up, we can release - * resources at this point. + * If we know we won't need to back up, we can release resources + * at this point. */ if (!(estate->es_top_eflags & EXEC_FLAG_BACKWARD)) (void) ExecShutdownNode(planstate); @@ -1778,8 +1779,8 @@ ExecutePlan(EState *estate, if (numberTuples && numberTuples == current_tuple_count) { /* - * If we know we won't need to back up, we can release - * resources at this point. + * If we know we won't need to back up, we can release resources + * at this point. */ if (!(estate->es_top_eflags & EXEC_FLAG_BACKWARD)) (void) ExecShutdownNode(planstate); @@ -3078,6 +3079,14 @@ EvalPlanQualBegin(EPQState *epqstate, EState *parentestate) { int i; + /* + * Force evaluation of any InitPlan outputs that could be needed + * by the subplan, just in case they got reset since + * EvalPlanQualStart (see comments therein). + */ + ExecSetParamPlanMulti(planstate->plan->extParam, + GetPerTupleExprContext(parentestate)); + i = list_length(parentestate->es_plannedstmt->paramExecTypes); while (--i >= 0) @@ -3170,9 +3179,32 @@ EvalPlanQualStart(EPQState *epqstate, EState *parentestate, Plan *planTree) { int i; + /* + * Force evaluation of any InitPlan outputs that could be needed by + * the subplan. (With more complexity, maybe we could postpone this + * till the subplan actually demands them, but it doesn't seem worth + * the trouble; this is a corner case already, since usually the + * InitPlans would have been evaluated before reaching EvalPlanQual.) + * + * This will not touch output params of InitPlans that occur somewhere + * within the subplan tree, only those that are attached to the + * ModifyTable node or above it and are referenced within the subplan. + * That's OK though, because the planner would only attach such + * InitPlans to a lower-level SubqueryScan node, and EPQ execution + * will not descend into a SubqueryScan. + * + * The EState's per-output-tuple econtext is sufficiently short-lived + * for this, since it should get reset before there is any chance of + * doing EvalPlanQual again. + */ + ExecSetParamPlanMulti(planTree->extParam, + GetPerTupleExprContext(parentestate)); + + /* now make the internal param workspace ... */ i = list_length(parentestate->es_plannedstmt->paramExecTypes); estate->es_param_exec_vals = (ParamExecData *) palloc0(i * sizeof(ParamExecData)); + /* ... and copy down all values, whether really needed or not */ while (--i >= 0) { /* copy value if any, but not execPlan link */ diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c index ee0f07a81e..c93084e4d2 100644 --- a/src/backend/executor/execParallel.c +++ b/src/backend/executor/execParallel.c @@ -23,7 +23,6 @@ #include "postgres.h" -#include "executor/execExpr.h" #include "executor/execParallel.h" #include "executor/executor.h" #include "executor/nodeAppend.h" @@ -36,6 +35,7 @@ #include "executor/nodeIndexonlyscan.h" #include "executor/nodeSeqscan.h" #include "executor/nodeSort.h" +#include "executor/nodeSubplan.h" #include "executor/tqueue.h" #include "nodes/nodeFuncs.h" #include "optimizer/planmain.h" @@ -581,8 +581,18 @@ ExecInitParallelPlan(PlanState *planstate, EState *estate, char *query_string; int query_len; - /* Force parameters we're going to pass to workers to be evaluated. */ - ExecEvalParamExecParams(sendParams, estate); + /* + * Force any initplan outputs that we're going to pass to workers to be + * evaluated, if they weren't already. + * + * For simplicity, we use the EState's per-output-tuple ExprContext here. + * That risks intra-query memory leakage, since we might pass through here + * many times before that ExprContext gets reset; but ExecSetParamPlan + * doesn't normally leak any memory in the context (see its comments), so + * it doesn't seem worth complicating this function's API to pass it a + * shorter-lived ExprContext. This might need to change someday. + */ + ExecSetParamPlanMulti(sendParams, GetPerTupleExprContext(estate)); /* Allocate object for return value. */ pei = palloc0(sizeof(ParallelExecutorInfo)); @@ -831,8 +841,12 @@ ExecParallelReinitialize(PlanState *planstate, /* Old workers must already be shut down */ Assert(pei->finished); - /* Force parameters we're going to pass to workers to be evaluated. */ - ExecEvalParamExecParams(sendParams, estate); + /* + * Force any initplan outputs that we're going to pass to workers to be + * evaluated, if they weren't already (see comments in + * ExecInitParallelPlan). + */ + ExecSetParamPlanMulti(sendParams, GetPerTupleExprContext(estate)); ReinitializeParallelDSM(pei->pcxt); pei->tqueue = ExecParallelSetupTupleQueues(pei->pcxt, true); diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c index 6b370750c5..63de981034 100644 --- a/src/backend/executor/nodeSubplan.c +++ b/src/backend/executor/nodeSubplan.c @@ -1009,6 +1009,17 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent) * of initplans: we don't run the subplan until/unless we need its output. * Note that this routine MUST clear the execPlan fields of the plan's * output parameters after evaluating them! + * + * The results of this function are stored in the EState associated with the + * ExprContext (particularly, its ecxt_param_exec_vals); any pass-by-ref + * result Datums are allocated in the EState's per-query memory. The passed + * econtext can be any ExprContext belonging to that EState; which one is + * important only to the extent that the ExprContext's per-tuple memory + * context is used to evaluate any parameters passed down to the subplan. + * (Thus in principle, the shorter-lived the ExprContext the better, since + * that data isn't needed after we return. In practice, because initplan + * parameters are never more complex than Vars, Aggrefs, etc, evaluating them + * currently never leaks any memory anyway.) * ---------------------------------------------------------------- */ void @@ -1195,6 +1206,37 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext) estate->es_direction = dir; } +/* + * ExecSetParamPlanMulti + * + * Apply ExecSetParamPlan to evaluate any not-yet-evaluated initplan output + * parameters whose ParamIDs are listed in "params". Any listed params that + * are not initplan outputs are ignored. + * + * As with ExecSetParamPlan, any ExprContext belonging to the current EState + * can be used, but in principle a shorter-lived ExprContext is better than a + * longer-lived one. + */ +void +ExecSetParamPlanMulti(const Bitmapset *params, ExprContext *econtext) +{ + int paramid; + + paramid = -1; + while ((paramid = bms_next_member(params, paramid)) >= 0) + { + ParamExecData *prm = &(econtext->ecxt_param_exec_vals[paramid]); + + if (prm->execPlan != NULL) + { + /* Parameter not evaluated yet, so go do it */ + ExecSetParamPlan(prm->execPlan, econtext); + /* ExecSetParamPlan should have processed this param... */ + Assert(prm->execPlan == NULL); + } + } +} + /* * Mark an initplan as needing recalculation */ diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h index f7b1f77616..02af68f2c2 100644 --- a/src/include/executor/execExpr.h +++ b/src/include/executor/execExpr.h @@ -697,7 +697,6 @@ extern void ExecEvalFuncExprStrictFusage(ExprState *state, ExprEvalStep *op, ExprContext *econtext); extern void ExecEvalParamExec(ExprState *state, ExprEvalStep *op, ExprContext *econtext); -extern void ExecEvalParamExecParams(Bitmapset *params, EState *estate); extern void ExecEvalParamExtern(ExprState *state, ExprEvalStep *op, ExprContext *econtext); extern void ExecEvalSQLValueFunction(ExprState *state, ExprEvalStep *op); diff --git a/src/include/executor/nodeSubplan.h b/src/include/executor/nodeSubplan.h index d9784a2b71..fd21b5df8f 100644 --- a/src/include/executor/nodeSubplan.h +++ b/src/include/executor/nodeSubplan.h @@ -28,4 +28,6 @@ extern void ExecReScanSetParamPlan(SubPlanState *node, PlanState *parent); extern void ExecSetParamPlan(SubPlanState *node, ExprContext *econtext); +extern void ExecSetParamPlanMulti(const Bitmapset *params, ExprContext *econtext); + #endif /* NODESUBPLAN_H */ diff --git a/src/test/isolation/expected/eval-plan-qual.out b/src/test/isolation/expected/eval-plan-qual.out index 9bbfdc1b5d..49b3fb3446 100644 --- a/src/test/isolation/expected/eval-plan-qual.out +++ b/src/test/isolation/expected/eval-plan-qual.out @@ -184,6 +184,37 @@ ta_id ta_value tb_row 1 newTableAValue (1,tableBValue) step c2: COMMIT; +starting permutation: updateforcip updateforcip2 c1 c2 read_a +step updateforcip: + UPDATE table_a SET value = NULL WHERE id = 1; + +step updateforcip2: + UPDATE table_a SET value = COALESCE(value, (SELECT text 'newValue')) WHERE id = 1; + +step c1: COMMIT; +step updateforcip2: <... completed> +step c2: COMMIT; +step read_a: SELECT * FROM table_a ORDER BY id; +id value + +1 newValue + +starting permutation: updateforcip updateforcip3 c1 c2 read_a +step updateforcip: + UPDATE table_a SET value = NULL WHERE id = 1; + +step updateforcip3: + WITH d(val) AS (SELECT text 'newValue' FROM generate_series(1,1)) + UPDATE table_a SET value = COALESCE(value, (SELECT val FROM d)) WHERE id = 1; + +step c1: COMMIT; +step updateforcip3: <... completed> +step c2: COMMIT; +step read_a: SELECT * FROM table_a ORDER BY id; +id value + +1 newValue + starting permutation: wrtwcte readwcte c1 c2 step wrtwcte: UPDATE table_a SET value = 'tableAValue2' WHERE id = 1; step readwcte: diff --git a/src/test/isolation/specs/eval-plan-qual.spec b/src/test/isolation/specs/eval-plan-qual.spec index 0b70ad55ba..367922de75 100644 --- a/src/test/isolation/specs/eval-plan-qual.spec +++ b/src/test/isolation/specs/eval-plan-qual.spec @@ -92,6 +92,13 @@ step "updateforss" { UPDATE table_b SET value = 'newTableBValue' WHERE id = 1; } +# these tests exercise EvalPlanQual with conditional InitPlans which +# have not been executed prior to the EPQ + +step "updateforcip" { + UPDATE table_a SET value = NULL WHERE id = 1; +} + # these tests exercise mark/restore during EPQ recheck, cf bug #15032 step "selectjoinforupdate" { @@ -129,6 +136,13 @@ step "readforss" { FROM table_a ta WHERE ta.id = 1 FOR UPDATE OF ta; } +step "updateforcip2" { + UPDATE table_a SET value = COALESCE(value, (SELECT text 'newValue')) WHERE id = 1; +} +step "updateforcip3" { + WITH d(val) AS (SELECT text 'newValue' FROM generate_series(1,1)) + UPDATE table_a SET value = COALESCE(value, (SELECT val FROM d)) WHERE id = 1; +} step "wrtwcte" { UPDATE table_a SET value = 'tableAValue2' WHERE id = 1; } step "wrjt" { UPDATE jointest SET data = 42 WHERE id = 7; } step "c2" { COMMIT; } @@ -137,6 +151,7 @@ session "s3" setup { BEGIN ISOLATION LEVEL READ COMMITTED; } step "read" { SELECT * FROM accounts ORDER BY accountid; } step "read_ext" { SELECT * FROM accounts_ext ORDER BY accountid; } +step "read_a" { SELECT * FROM table_a ORDER BY id; } # this test exercises EvalPlanQual with a CTE, cf bug #14328 step "readwcte" { @@ -171,6 +186,8 @@ permutation "wx2" "partiallock" "c2" "c1" "read" permutation "wx2" "lockwithvalues" "c2" "c1" "read" permutation "wx2_ext" "partiallock_ext" "c2" "c1" "read_ext" permutation "updateforss" "readforss" "c1" "c2" +permutation "updateforcip" "updateforcip2" "c1" "c2" "read_a" +permutation "updateforcip" "updateforcip3" "c1" "c2" "read_a" permutation "wrtwcte" "readwcte" "c1" "c2" permutation "wrjt" "selectjoinforupdate" "c2" "c1" permutation "wrtwcte" "multireadwcte" "c1" "c2"