Skip to content

Commit 8494755

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 3cac7c2 commit 8494755

File tree

7 files changed

+285
-7
lines changed

7 files changed

+285
-7
lines changed

‎src/backend/executor/execMain.c

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
#include "commands/matview.h"
4646
#include "commands/trigger.h"
4747
#include "executor/execdebug.h"
48+
#include "executor/nodeSubplan.h"
4849
#include "foreign/fdwapi.h"
4950
#include "mb/pg_wchar.h"
5051
#include "miscadmin.h"
@@ -2467,7 +2468,17 @@ EvalPlanQualBegin(EPQState *epqstate, EState *parentestate)
24672468
/* Recopy current values of parent parameters */
24682469
if (parentestate->es_plannedstmt->nParamExec > 0)
24692470
{
2470-
int i = parentestate->es_plannedstmt->nParamExec;
2471+
int i;
2472+
2473+
/*
2474+
* Force evaluation of any InitPlan outputs that could be needed
2475+
* by the subplan, just in case they got reset since
2476+
* EvalPlanQualStart (see comments therein).
2477+
*/
2478+
ExecSetParamPlanMulti(planstate->plan->extParam,
2479+
GetPerTupleExprContext(parentestate));
2480+
2481+
i = parentestate->es_plannedstmt->nParamExec;
24712482

24722483
while (--i >= 0)
24732484
{
@@ -2557,10 +2568,34 @@ EvalPlanQualStart(EPQState *epqstate, EState *parentestate, Plan *planTree)
25572568
estate->es_param_list_info = parentestate->es_param_list_info;
25582569
if (parentestate->es_plannedstmt->nParamExec > 0)
25592570
{
2560-
int i = parentestate->es_plannedstmt->nParamExec;
2571+
int i;
2572+
2573+
/*
2574+
* Force evaluation of any InitPlan outputs that could be needed by
2575+
* the subplan. (With more complexity, maybe we could postpone this
2576+
* till the subplan actually demands them, but it doesn't seem worth
2577+
* the trouble; this is a corner case already, since usually the
2578+
* InitPlans would have been evaluated before reaching EvalPlanQual.)
2579+
*
2580+
* This will not touch output params of InitPlans that occur somewhere
2581+
* within the subplan tree, only those that are attached to the
2582+
* ModifyTable node or above it and are referenced within the subplan.
2583+
* That's OK though, because the planner would only attach such
2584+
* InitPlans to a lower-level SubqueryScan node, and EPQ execution
2585+
* will not descend into a SubqueryScan.
2586+
*
2587+
* The EState's per-output-tuple econtext is sufficiently short-lived
2588+
* for this, since it should get reset before there is any chance of
2589+
* doing EvalPlanQual again.
2590+
*/
2591+
ExecSetParamPlanMulti(planTree->extParam,
2592+
GetPerTupleExprContext(parentestate));
25612593

2594+
/* now make the internal param workspace ... */
2595+
i = parentestate->es_plannedstmt->nParamExec;
25622596
estate->es_param_exec_vals = (ParamExecData *)
25632597
palloc0(i * sizeof(ParamExecData));
2598+
/* ... and copy down all values, whether really needed or not */
25642599
while (--i >= 0)
25652600
{
25662601
/* copy value if any, but not execPlan link */

‎src/backend/executor/nodeSubplan.c

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -909,6 +909,17 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
909909
* of initplans: we don't run the subplan until/unless we need its output.
910910
* Note that this routine MUST clear the execPlan fields of the plan's
911911
* output parameters after evaluating them!
912+
*
913+
* The results of this function are stored in the EState associated with the
914+
* ExprContext (particularly, its ecxt_param_exec_vals); any pass-by-ref
915+
* result Datums are allocated in the EState's per-query memory. The passed
916+
* econtext can be any ExprContext belonging to that EState; which one is
917+
* important only to the extent that the ExprContext's per-tuple memory
918+
* context is used to evaluate any parameters passed down to the subplan.
919+
* (Thus in principle, the shorter-lived the ExprContext the better, since
920+
* that data isn't needed after we return. In practice, because initplan
921+
* parameters are never more complex than Vars, Aggrefs, etc, evaluating them
922+
* currently never leaks any memory anyway.)
912923
* ----------------------------------------------------------------
913924
*/
914925
void
@@ -1072,6 +1083,37 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
10721083
estate->es_direction = dir;
10731084
}
10741085

1086+
/*
1087+
* ExecSetParamPlanMulti
1088+
*
1089+
* Apply ExecSetParamPlan to evaluate any not-yet-evaluated initplan output
1090+
* parameters whose ParamIDs are listed in "params". Any listed params that
1091+
* are not initplan outputs are ignored.
1092+
*
1093+
* As with ExecSetParamPlan, any ExprContext belonging to the current EState
1094+
* can be used, but in principle a shorter-lived ExprContext is better than a
1095+
* longer-lived one.
1096+
*/
1097+
void
1098+
ExecSetParamPlanMulti(const Bitmapset *params, ExprContext *econtext)
1099+
{
1100+
int paramid;
1101+
1102+
paramid = -1;
1103+
while ((paramid = bms_next_member(params, paramid)) >= 0)
1104+
{
1105+
ParamExecData *prm = &(econtext->ecxt_param_exec_vals[paramid]);
1106+
1107+
if (prm->execPlan != NULL)
1108+
{
1109+
/* Parameter not evaluated yet, so go do it */
1110+
ExecSetParamPlan(prm->execPlan, econtext);
1111+
/* ExecSetParamPlan should have processed this param... */
1112+
Assert(prm->execPlan == NULL);
1113+
}
1114+
}
1115+
}
1116+
10751117
/*
10761118
* Mark an initplan as needing recalculation
10771119
*/

‎src/backend/nodes/bitmapset.c

Lines changed: 63 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -793,19 +793,19 @@ bms_join(Bitmapset *a, Bitmapset *b)
793793
return result;
794794
}
795795

796-
/*----------
796+
/*
797797
* bms_first_member - find and remove first member of a set
798798
*
799799
* Returns -1 if set is empty. NB: set is destructively modified!
800800
*
801801
* This is intended as support for iterating through the members of a set.
802802
* The typical pattern is
803803
*
804-
* tmpset = bms_copy(inputset);
805-
* while ((x = bms_first_member(tmpset)) >= 0)
804+
* while ((x = bms_first_member(inputset)) >= 0)
806805
* process member x;
807-
* bms_free(tmpset);
808-
*----------
806+
*
807+
* CAUTION: this destroys the content of "inputset". If the set must
808+
* not be modified, use bms_next_member instead.
809809
*/
810810
int
811811
bms_first_member(Bitmapset *a)
@@ -840,6 +840,64 @@ bms_first_member(Bitmapset *a)
840840
return -1;
841841
}
842842

843+
/*
844+
* bms_next_member - find next member of a set
845+
*
846+
* Returns smallest member greater than "prevbit", or -2 if there is none.
847+
* "prevbit" must NOT be less than -1, or the behavior is unpredictable.
848+
*
849+
* This is intended as support for iterating through the members of a set.
850+
* The typical pattern is
851+
*
852+
* x = -1;
853+
* while ((x = bms_next_member(inputset, x)) >= 0)
854+
* process member x;
855+
*
856+
* Notice that when there are no more members, we return -2, not -1 as you
857+
* might expect. The rationale for that is to allow distinguishing the
858+
* loop-not-started state (x == -1) from the loop-completed state (x == -2).
859+
* It makes no difference in simple loop usage, but complex iteration logic
860+
* might need such an ability.
861+
*/
862+
int
863+
bms_next_member(const Bitmapset *a, int prevbit)
864+
{
865+
int nwords;
866+
int wordnum;
867+
bitmapword mask;
868+
869+
if (a == NULL)
870+
return -2;
871+
nwords = a->nwords;
872+
prevbit++;
873+
mask = (~(bitmapword) 0) << BITNUM(prevbit);
874+
for (wordnum = WORDNUM(prevbit); wordnum < nwords; wordnum++)
875+
{
876+
bitmapword w = a->words[wordnum];
877+
878+
/* ignore bits before prevbit */
879+
w &= mask;
880+
881+
if (w != 0)
882+
{
883+
int result;
884+
885+
result = wordnum * BITS_PER_BITMAPWORD;
886+
while ((w & 255) == 0)
887+
{
888+
w >>= 8;
889+
result += 8;
890+
}
891+
result += rightmost_one_pos[w & 255];
892+
return result;
893+
}
894+
895+
/* in subsequent words, consider all bits */
896+
mask = (~(bitmapword) 0);
897+
}
898+
return -2;
899+
}
900+
843901
/*
844902
* bms_hash_value - compute a hash key for a Bitmapset
845903
*

‎src/include/executor/nodeSubplan.h

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

2525
extern void ExecSetParamPlan(SubPlanState *node, ExprContext *econtext);
2626

27+
extern void ExecSetParamPlanMulti(const Bitmapset *params, ExprContext *econtext);
28+
2729
#endif /* NODESUBPLAN_H */

‎src/include/nodes/bitmapset.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ extern Bitmapset *bms_join(Bitmapset *a, Bitmapset *b);
8989

9090
/* support for iterating through the integer elements of a set: */
9191
extern int bms_first_member(Bitmapset *a);
92+
extern int bms_next_member(const Bitmapset *a, int prevbit);
9293

9394
/* support for hashtables using Bitmapsets as keys: */
9495
extern uint32 bms_hash_value(const Bitmapset *a);

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

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,96 @@ a b c
105105
2 3 0
106106
step c2: COMMIT;
107107

108+
starting permutation: wx2 partiallock c2 c1 read
109+
step wx2: UPDATE accounts SET balance = balance + 450 WHERE accountid = 'checking';
110+
step partiallock:
111+
SELECT * FROM accounts a1, accounts a2
112+
WHERE a1.accountid = a2.accountid
113+
FOR UPDATE OF a1;
114+
<waiting ...>
115+
step c2: COMMIT;
116+
step partiallock: <... completed>
117+
accountid balance accountid balance
118+
119+
checking 1050 checking 600
120+
savings 600 savings 600
121+
step c1: COMMIT;
122+
step read: SELECT * FROM accounts ORDER BY accountid;
123+
accountid balance
124+
125+
checking 1050
126+
savings 600
127+
128+
starting permutation: wx2 lockwithvalues c2 c1 read
129+
step wx2: UPDATE accounts SET balance = balance + 450 WHERE accountid = 'checking';
130+
step lockwithvalues:
131+
SELECT * FROM accounts a1, (values('checking'),('savings')) v(id)
132+
WHERE a1.accountid = v.id
133+
FOR UPDATE OF a1;
134+
<waiting ...>
135+
step c2: COMMIT;
136+
step lockwithvalues: <... completed>
137+
accountid balance id
138+
139+
checking 1050 checking
140+
savings 600 savings
141+
step c1: COMMIT;
142+
step read: SELECT * FROM accounts ORDER BY accountid;
143+
accountid balance
144+
145+
checking 1050
146+
savings 600
147+
148+
starting permutation: updateforss readforss c1 c2
149+
step updateforss:
150+
UPDATE table_a SET value = 'newTableAValue' WHERE id = 1;
151+
UPDATE table_b SET value = 'newTableBValue' WHERE id = 1;
152+
153+
step readforss:
154+
SELECT ta.id AS ta_id, ta.value AS ta_value,
155+
(SELECT ROW(tb.id, tb.value)
156+
FROM table_b tb WHERE ta.id = tb.id) AS tb_row
157+
FROM table_a ta
158+
WHERE ta.id = 1 FOR UPDATE OF ta;
159+
<waiting ...>
160+
step c1: COMMIT;
161+
step readforss: <... completed>
162+
ta_id ta_value tb_row
163+
164+
1 newTableAValue (1,tableBValue)
165+
step c2: COMMIT;
166+
167+
starting permutation: updateforcip updateforcip2 c1 c2 read_a
168+
step updateforcip:
169+
UPDATE table_a SET value = NULL WHERE id = 1;
170+
171+
step updateforcip2:
172+
UPDATE table_a SET value = COALESCE(value, (SELECT text 'newValue')) WHERE id = 1;
173+
<waiting ...>
174+
step c1: COMMIT;
175+
step updateforcip2: <... completed>
176+
step c2: COMMIT;
177+
step read_a: SELECT * FROM table_a ORDER BY id;
178+
id value
179+
180+
1 newValue
181+
182+
starting permutation: updateforcip updateforcip3 c1 c2 read_a
183+
step updateforcip:
184+
UPDATE table_a SET value = NULL WHERE id = 1;
185+
186+
step updateforcip3:
187+
WITH d(val) AS (SELECT text 'newValue' FROM generate_series(1,1))
188+
UPDATE table_a SET value = COALESCE(value, (SELECT val FROM d)) WHERE id = 1;
189+
<waiting ...>
190+
step c1: COMMIT;
191+
step updateforcip3: <... completed>
192+
step c2: COMMIT;
193+
step read_a: SELECT * FROM table_a ORDER BY id;
194+
id value
195+
196+
1 newValue
197+
108198
starting permutation: wrtwcte readwcte c1 c2
109199
step wrtwcte: UPDATE table_a SET value = 'tableAValue2' WHERE id = 1;
110200
step readwcte:

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

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,36 @@ step "writep1" { UPDATE p SET b = -1 WHERE a = 1 AND b = 1 AND c = 0; }
5656
step "writep2" { UPDATE p SET b = -b WHERE a = 1 AND c = 0; }
5757
step "c1" { COMMIT; }
5858

59+
# these tests are meant to exercise EvalPlanQualFetchRowMarks,
60+
# ie, handling non-locked tables in an EvalPlanQual recheck
61+
62+
step "partiallock" {
63+
SELECT * FROM accounts a1, accounts a2
64+
WHERE a1.accountid = a2.accountid
65+
FOR UPDATE OF a1;
66+
}
67+
step "lockwithvalues" {
68+
SELECT * FROM accounts a1, (values('checking'),('savings')) v(id)
69+
WHERE a1.accountid = v.id
70+
FOR UPDATE OF a1;
71+
}
72+
73+
# these tests exercise EvalPlanQual with a SubLink sub-select (which should be
74+
# unaffected by any EPQ recheck behavior in the outer query); cf bug #14034
75+
76+
step "updateforss" {
77+
UPDATE table_a SET value = 'newTableAValue' WHERE id = 1;
78+
UPDATE table_b SET value = 'newTableBValue' WHERE id = 1;
79+
}
80+
81+
# these tests exercise EvalPlanQual with conditional InitPlans which
82+
# have not been executed prior to the EPQ
83+
84+
step "updateforcip" {
85+
UPDATE table_a SET value = NULL WHERE id = 1;
86+
}
87+
88+
5989
session "s2"
6090
setup { BEGIN ISOLATION LEVEL READ COMMITTED; }
6191
step "wx2" { UPDATE accounts SET balance = balance + 450 WHERE accountid = 'checking'; }
@@ -73,12 +103,27 @@ step "returningp1" {
73103
WITH u AS ( UPDATE p SET b = b WHERE a > 0 RETURNING * )
74104
SELECT * FROM u;
75105
}
106+
step "readforss" {
107+
SELECT ta.id AS ta_id, ta.value AS ta_value,
108+
(SELECT ROW(tb.id, tb.value)
109+
FROM table_b tb WHERE ta.id = tb.id) AS tb_row
110+
FROM table_a ta
111+
WHERE ta.id = 1 FOR UPDATE OF ta;
112+
}
113+
step "updateforcip2" {
114+
UPDATE table_a SET value = COALESCE(value, (SELECT text 'newValue')) WHERE id = 1;
115+
}
116+
step "updateforcip3" {
117+
WITH d(val) AS (SELECT text 'newValue' FROM generate_series(1,1))
118+
UPDATE table_a SET value = COALESCE(value, (SELECT val FROM d)) WHERE id = 1;
119+
}
76120
step "wrtwcte" { UPDATE table_a SET value = 'tableAValue2' WHERE id = 1; }
77121
step "c2" { COMMIT; }
78122

79123
session "s3"
80124
setup { BEGIN ISOLATION LEVEL READ COMMITTED; }
81125
step "read" { SELECT * FROM accounts ORDER BY accountid; }
126+
step "read_a" { SELECT * FROM table_a ORDER BY id; }
82127

83128
# this test exercises EvalPlanQual with a CTE, cf bug #14328
84129
step "readwcte" {
@@ -109,5 +154,10 @@ permutation "wy1" "wy2" "c1" "c2" "read"
109154
permutation "upsert1" "upsert2" "c1" "c2" "read"
110155
permutation "readp1" "writep1" "readp2" "c1" "c2"
111156
permutation "writep2" "returningp1" "c1" "c2"
157+
permutation "wx2" "partiallock" "c2" "c1" "read"
158+
permutation "wx2" "lockwithvalues" "c2" "c1" "read"
159+
permutation "updateforss" "readforss" "c1" "c2"
160+
permutation "updateforcip" "updateforcip2" "c1" "c2" "read_a"
161+
permutation "updateforcip" "updateforcip3" "c1" "c2" "read_a"
112162
permutation "wrtwcte" "readwcte" "c1" "c2"
113163
permutation "wrtwcte" "multireadwcte" "c1" "c2"

0 commit comments

Comments
 (0)