Skip to content

Commit 591d0ac

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 6884491 commit 591d0ac

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
@@ -44,6 +44,7 @@
4444
#include "catalog/namespace.h"
4545
#include "commands/trigger.h"
4646
#include "executor/execdebug.h"
47+
#include "executor/nodeSubplan.h"
4748
#include "foreign/fdwapi.h"
4849
#include "mb/pg_wchar.h"
4950
#include "miscadmin.h"
@@ -2395,7 +2396,17 @@ EvalPlanQualBegin(EPQState *epqstate, EState *parentestate)
23952396
/* Recopy current values of parent parameters */
23962397
if (parentestate->es_plannedstmt->nParamExec > 0)
23972398
{
2398-
int i = parentestate->es_plannedstmt->nParamExec;
2399+
int i;
2400+
2401+
/*
2402+
* Force evaluation of any InitPlan outputs that could be needed
2403+
* by the subplan, just in case they got reset since
2404+
* EvalPlanQualStart (see comments therein).
2405+
*/
2406+
ExecSetParamPlanMulti(planstate->plan->extParam,
2407+
GetPerTupleExprContext(parentestate));
2408+
2409+
i = parentestate->es_plannedstmt->nParamExec;
23992410

24002411
while (--i >= 0)
24012412
{
@@ -2485,10 +2496,34 @@ EvalPlanQualStart(EPQState *epqstate, EState *parentestate, Plan *planTree)
24852496
estate->es_param_list_info = parentestate->es_param_list_info;
24862497
if (parentestate->es_plannedstmt->nParamExec > 0)
24872498
{
2488-
int i = parentestate->es_plannedstmt->nParamExec;
2499+
int i;
2500+
2501+
/*
2502+
* Force evaluation of any InitPlan outputs that could be needed by
2503+
* the subplan. (With more complexity, maybe we could postpone this
2504+
* till the subplan actually demands them, but it doesn't seem worth
2505+
* the trouble; this is a corner case already, since usually the
2506+
* InitPlans would have been evaluated before reaching EvalPlanQual.)
2507+
*
2508+
* This will not touch output params of InitPlans that occur somewhere
2509+
* within the subplan tree, only those that are attached to the
2510+
* ModifyTable node or above it and are referenced within the subplan.
2511+
* That's OK though, because the planner would only attach such
2512+
* InitPlans to a lower-level SubqueryScan node, and EPQ execution
2513+
* will not descend into a SubqueryScan.
2514+
*
2515+
* The EState's per-output-tuple econtext is sufficiently short-lived
2516+
* for this, since it should get reset before there is any chance of
2517+
* doing EvalPlanQual again.
2518+
*/
2519+
ExecSetParamPlanMulti(planTree->extParam,
2520+
GetPerTupleExprContext(parentestate));
24892521

2522+
/* now make the internal param workspace ... */
2523+
i = parentestate->es_plannedstmt->nParamExec;
24902524
estate->es_param_exec_vals = (ParamExecData *)
24912525
palloc0(i * sizeof(ParamExecData));
2526+
/* ... and copy down all values, whether really needed or not */
24922527
while (--i >= 0)
24932528
{
24942529
/* 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
@@ -794,19 +794,19 @@ bms_join(Bitmapset *a, Bitmapset *b)
794794
return result;
795795
}
796796

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

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

‎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)