diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 315a555dcff..e482f47b5dc 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -44,6 +44,7 @@ #include "catalog/namespace.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" @@ -2395,7 +2396,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) { @@ -2485,10 +2496,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 b74d3b1a195..30d5074342d 100644 --- a/src/backend/executor/nodeSubplan.c +++ b/src/backend/executor/nodeSubplan.c @@ -909,6 +909,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 @@ -1072,6 +1083,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/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.c index 8f0877ccaae..f6a9367234d 100644 --- a/src/backend/nodes/bitmapset.c +++ b/src/backend/nodes/bitmapset.c @@ -794,7 +794,7 @@ bms_join(Bitmapset *a, Bitmapset *b) return result; } -/*---------- +/* * bms_first_member - find and remove first member of a set * * Returns -1 if set is empty. NB: set is destructively modified! @@ -802,11 +802,11 @@ bms_join(Bitmapset *a, Bitmapset *b) * This is intended as support for iterating through the members of a set. * The typical pattern is * - * tmpset = bms_copy(inputset); - * while ((x = bms_first_member(tmpset)) >= 0) + * while ((x = bms_first_member(inputset)) >= 0) * process member x; - * bms_free(tmpset); - *---------- + * + * CAUTION: this destroys the content of "inputset". If the set must + * not be modified, use bms_next_member instead. */ int bms_first_member(Bitmapset *a) @@ -841,6 +841,64 @@ bms_first_member(Bitmapset *a) return -1; } +/* + * bms_next_member - find next member of a set + * + * Returns smallest member greater than "prevbit", or -2 if there is none. + * "prevbit" must NOT be less than -1, or the behavior is unpredictable. + * + * This is intended as support for iterating through the members of a set. + * The typical pattern is + * + * x = -1; + * while ((x = bms_next_member(inputset, x)) >= 0) + * process member x; + * + * Notice that when there are no more members, we return -2, not -1 as you + * might expect. The rationale for that is to allow distinguishing the + * loop-not-started state (x == -1) from the loop-completed state (x == -2). + * It makes no difference in simple loop usage, but complex iteration logic + * might need such an ability. + */ +int +bms_next_member(const Bitmapset *a, int prevbit) +{ + int nwords; + int wordnum; + bitmapword mask; + + if (a == NULL) + return -2; + nwords = a->nwords; + prevbit++; + mask = (~(bitmapword) 0) << BITNUM(prevbit); + for (wordnum = WORDNUM(prevbit); wordnum < nwords; wordnum++) + { + bitmapword w = a->words[wordnum]; + + /* ignore bits before prevbit */ + w &= mask; + + if (w != 0) + { + int result; + + result = wordnum * BITS_PER_BITMAPWORD; + while ((w & 255) == 0) + { + w >>= 8; + result += 8; + } + result += rightmost_one_pos[w & 255]; + return result; + } + + /* in subsequent words, consider all bits */ + mask = (~(bitmapword) 0); + } + return -2; +} + /* * bms_hash_value - compute a hash key for a Bitmapset * diff --git a/src/include/executor/nodeSubplan.h b/src/include/executor/nodeSubplan.h index 892e583b659..1068b556912 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/include/nodes/bitmapset.h b/src/include/nodes/bitmapset.h index 2a4b41d805e..a8ebb01e68a 100644 --- a/src/include/nodes/bitmapset.h +++ b/src/include/nodes/bitmapset.h @@ -89,6 +89,7 @@ extern Bitmapset *bms_join(Bitmapset *a, Bitmapset *b); /* support for iterating through the integer elements of a set: */ extern int bms_first_member(Bitmapset *a); +extern int bms_next_member(const Bitmapset *a, int prevbit); /* support for hashtables using Bitmapsets as keys: */ extern uint32 bms_hash_value(const Bitmapset *a); diff --git a/src/test/isolation/expected/eval-plan-qual.out b/src/test/isolation/expected/eval-plan-qual.out index be87d1b0c27..235026cb726 100644 --- a/src/test/isolation/expected/eval-plan-qual.out +++ b/src/test/isolation/expected/eval-plan-qual.out @@ -105,6 +105,96 @@ a b c 2 3 0 step c2: COMMIT; +starting permutation: wx2 partiallock c2 c1 read +step wx2: UPDATE accounts SET balance = balance + 450 WHERE accountid = 'checking'; +step partiallock: + SELECT * FROM accounts a1, accounts a2 + WHERE a1.accountid = a2.accountid + FOR UPDATE OF a1; + +step c2: COMMIT; +step partiallock: <... completed> +accountid balance accountid balance + +checking 1050 checking 600 +savings 600 savings 600 +step c1: COMMIT; +step read: SELECT * FROM accounts ORDER BY accountid; +accountid balance + +checking 1050 +savings 600 + +starting permutation: wx2 lockwithvalues c2 c1 read +step wx2: UPDATE accounts SET balance = balance + 450 WHERE accountid = 'checking'; +step lockwithvalues: + SELECT * FROM accounts a1, (values('checking'),('savings')) v(id) + WHERE a1.accountid = v.id + FOR UPDATE OF a1; + +step c2: COMMIT; +step lockwithvalues: <... completed> +accountid balance id + +checking 1050 checking +savings 600 savings +step c1: COMMIT; +step read: SELECT * FROM accounts ORDER BY accountid; +accountid balance + +checking 1050 +savings 600 + +starting permutation: updateforss readforss c1 c2 +step updateforss: + UPDATE table_a SET value = 'newTableAValue' WHERE id = 1; + UPDATE table_b SET value = 'newTableBValue' WHERE id = 1; + +step readforss: + SELECT ta.id AS ta_id, ta.value AS ta_value, + (SELECT ROW(tb.id, tb.value) + FROM table_b tb WHERE ta.id = tb.id) AS tb_row + FROM table_a ta + WHERE ta.id = 1 FOR UPDATE OF ta; + +step c1: COMMIT; +step readforss: <... completed> +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 749c6405796..3825dd22f6e 100644 --- a/src/test/isolation/specs/eval-plan-qual.spec +++ b/src/test/isolation/specs/eval-plan-qual.spec @@ -56,6 +56,36 @@ step "writep1" { UPDATE p SET b = -1 WHERE a = 1 AND b = 1 AND c = 0; } step "writep2" { UPDATE p SET b = -b WHERE a = 1 AND c = 0; } step "c1" { COMMIT; } +# these tests are meant to exercise EvalPlanQualFetchRowMarks, +# ie, handling non-locked tables in an EvalPlanQual recheck + +step "partiallock" { + SELECT * FROM accounts a1, accounts a2 + WHERE a1.accountid = a2.accountid + FOR UPDATE OF a1; +} +step "lockwithvalues" { + SELECT * FROM accounts a1, (values('checking'),('savings')) v(id) + WHERE a1.accountid = v.id + FOR UPDATE OF a1; +} + +# these tests exercise EvalPlanQual with a SubLink sub-select (which should be +# unaffected by any EPQ recheck behavior in the outer query); cf bug #14034 + +step "updateforss" { + UPDATE table_a SET value = 'newTableAValue' WHERE id = 1; + 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; } step "wx2" { UPDATE accounts SET balance = balance + 450 WHERE accountid = 'checking'; } @@ -73,12 +103,27 @@ step "returningp1" { WITH u AS ( UPDATE p SET b = b WHERE a > 0 RETURNING * ) SELECT * FROM u; } +step "readforss" { + SELECT ta.id AS ta_id, ta.value AS ta_value, + (SELECT ROW(tb.id, tb.value) + FROM table_b tb WHERE ta.id = tb.id) AS tb_row + 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" { @@ -109,5 +154,10 @@ permutation "wy1" "wy2" "c1" "c2" "read" permutation "upsert1" "upsert2" "c1" "c2" "read" permutation "readp1" "writep1" "readp2" "c1" "c2" 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"