From 2a97a0ad34daefde117cc9001c97ad3c0ef82dfb Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 15 Sep 2018 13:42:34 -0400 Subject: [PATCH] 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 9f2ee8f28, 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 71404af2a and 342a1ffa2 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 --- src/backend/executor/execMain.c | 43 +++++++++++++++++-- src/backend/executor/nodeSubplan.c | 42 ++++++++++++++++++ src/include/executor/nodeSubplan.h | 2 + .../isolation/expected/eval-plan-qual.out | 31 +++++++++++++ src/test/isolation/specs/eval-plan-qual.spec | 17 ++++++++ 5 files changed, 131 insertions(+), 4 deletions(-) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 9c70cb7c17..7d57f8c63c 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -45,6 +45,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" @@ -1572,8 +1573,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); @@ -2718,7 +2719,17 @@ EvalPlanQualBegin(EPQState *epqstate, EState *parentestate) /* Recopy current values of parent parameters */ if (parentestate->es_plannedstmt->nParamExec > 0) { - int i = parentestate->es_plannedstmt->nParamExec; + 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 = parentestate->es_plannedstmt->nParamExec; while (--i >= 0) { @@ -2808,10 +2819,34 @@ EvalPlanQualStart(EPQState *epqstate, EState *parentestate, Plan *planTree) estate->es_param_list_info = parentestate->es_param_list_info; if (parentestate->es_plannedstmt->nParamExec > 0) { - int i = parentestate->es_plannedstmt->nParamExec; + 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 = parentestate->es_plannedstmt->nParamExec; 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/nodeSubplan.c b/src/backend/executor/nodeSubplan.c index b4883c4f8d..01518df8b2 100644 --- a/src/backend/executor/nodeSubplan.c +++ b/src/backend/executor/nodeSubplan.c @@ -947,6 +947,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 @@ -1134,6 +1145,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/nodeSubplan.h b/src/include/executor/nodeSubplan.h index f05a47419d..34b18c1022 100644 --- a/src/include/executor/nodeSubplan.h +++ b/src/include/executor/nodeSubplan.h @@ -24,4 +24,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 d960e83147..235026cb72 100644 --- a/src/test/isolation/expected/eval-plan-qual.out +++ b/src/test/isolation/expected/eval-plan-qual.out @@ -164,6 +164,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 de8ed39b1e..3825dd22f6 100644 --- a/src/test/isolation/specs/eval-plan-qual.spec +++ b/src/test/isolation/specs/eval-plan-qual.spec @@ -78,6 +78,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; +} + session "s2" setup { BEGIN ISOLATION LEVEL READ COMMITTED; } @@ -103,12 +110,20 @@ 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 "c2" { COMMIT; } session "s3" setup { BEGIN ISOLATION LEVEL READ COMMITTED; } step "read" { SELECT * FROM accounts 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" { @@ -142,5 +157,7 @@ permutation "writep2" "returningp1" "c1" "c2" permutation "wx2" "partiallock" "c2" "c1" "read" permutation "wx2" "lockwithvalues" "c2" "c1" "read" 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 "wrtwcte" "multireadwcte" "c1" "c2"