Skip to content

Commit 99cbbbb

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 1ceb103 commit 99cbbbb

File tree

5 files changed

+133
-6
lines changed

5 files changed

+133
-6
lines changed

‎src/backend/executor/execMain.c

Lines changed: 41 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
#include "commands/matview.h"
4949
#include "commands/trigger.h"
5050
#include "executor/execdebug.h"
51+
#include "executor/nodeSubplan.h"
5152
#include "foreign/fdwapi.h"
5253
#include "mb/pg_wchar.h"
5354
#include "miscadmin.h"
@@ -1728,8 +1729,8 @@ ExecutePlan(EState *estate,
17281729
if (TupIsNull(slot))
17291730
{
17301731
/*
1731-
* If we know we won't need to back up, we can release
1732-
* resources at this point.
1732+
* If we know we won't need to back up, we can release resources
1733+
* at this point.
17331734
*/
17341735
if (!(estate->es_top_eflags & EXEC_FLAG_BACKWARD))
17351736
(void) ExecShutdownNode(planstate);
@@ -1779,8 +1780,8 @@ ExecutePlan(EState *estate,
17791780
if (numberTuples && numberTuples == current_tuple_count)
17801781
{
17811782
/*
1782-
* If we know we won't need to back up, we can release
1783-
* resources at this point.
1783+
* If we know we won't need to back up, we can release resources
1784+
* at this point.
17841785
*/
17851786
if (!(estate->es_top_eflags & EXEC_FLAG_BACKWARD))
17861787
(void) ExecShutdownNode(planstate);
@@ -3042,7 +3043,17 @@ EvalPlanQualBegin(EPQState *epqstate, EState *parentestate)
30423043
/* Recopy current values of parent parameters */
30433044
if (parentestate->es_plannedstmt->nParamExec > 0)
30443045
{
3045-
int i = parentestate->es_plannedstmt->nParamExec;
3046+
int i;
3047+
3048+
/*
3049+
* Force evaluation of any InitPlan outputs that could be needed
3050+
* by the subplan, just in case they got reset since
3051+
* EvalPlanQualStart (see comments therein).
3052+
*/
3053+
ExecSetParamPlanMulti(planstate->plan->extParam,
3054+
GetPerTupleExprContext(parentestate));
3055+
3056+
i = parentestate->es_plannedstmt->nParamExec;
30463057

30473058
while (--i >= 0)
30483059
{
@@ -3132,10 +3143,34 @@ EvalPlanQualStart(EPQState *epqstate, EState *parentestate, Plan *planTree)
31323143
estate->es_param_list_info = parentestate->es_param_list_info;
31333144
if (parentestate->es_plannedstmt->nParamExec > 0)
31343145
{
3135-
int i = parentestate->es_plannedstmt->nParamExec;
3146+
int i;
3147+
3148+
/*
3149+
* Force evaluation of any InitPlan outputs that could be needed by
3150+
* the subplan. (With more complexity, maybe we could postpone this
3151+
* till the subplan actually demands them, but it doesn't seem worth
3152+
* the trouble; this is a corner case already, since usually the
3153+
* InitPlans would have been evaluated before reaching EvalPlanQual.)
3154+
*
3155+
* This will not touch output params of InitPlans that occur somewhere
3156+
* within the subplan tree, only those that are attached to the
3157+
* ModifyTable node or above it and are referenced within the subplan.
3158+
* That's OK though, because the planner would only attach such
3159+
* InitPlans to a lower-level SubqueryScan node, and EPQ execution
3160+
* will not descend into a SubqueryScan.
3161+
*
3162+
* The EState's per-output-tuple econtext is sufficiently short-lived
3163+
* for this, since it should get reset before there is any chance of
3164+
* doing EvalPlanQual again.
3165+
*/
3166+
ExecSetParamPlanMulti(planTree->extParam,
3167+
GetPerTupleExprContext(parentestate));
31363168

3169+
/* now make the internal param workspace ... */
3170+
i = parentestate->es_plannedstmt->nParamExec;
31373171
estate->es_param_exec_vals = (ParamExecData *)
31383172
palloc0(i * sizeof(ParamExecData));
3173+
/* ... and copy down all values, whether really needed or not */
31393174
while (--i >= 0)
31403175
{
31413176
/* 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
@@ -921,6 +921,17 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
921921
* of initplans: we don't run the subplan until/unless we need its output.
922922
* Note that this routine MUST clear the execPlan fields of the plan's
923923
* output parameters after evaluating them!
924+
*
925+
* The results of this function are stored in the EState associated with the
926+
* ExprContext (particularly, its ecxt_param_exec_vals); any pass-by-ref
927+
* result Datums are allocated in the EState's per-query memory. The passed
928+
* econtext can be any ExprContext belonging to that EState; which one is
929+
* important only to the extent that the ExprContext's per-tuple memory
930+
* context is used to evaluate any parameters passed down to the subplan.
931+
* (Thus in principle, the shorter-lived the ExprContext the better, since
932+
* that data isn't needed after we return. In practice, because initplan
933+
* parameters are never more complex than Vars, Aggrefs, etc, evaluating them
934+
* currently never leaks any memory anyway.)
924935
* ----------------------------------------------------------------
925936
*/
926937
void
@@ -1107,6 +1118,37 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
11071118
estate->es_direction = dir;
11081119
}
11091120

1121+
/*
1122+
* ExecSetParamPlanMulti
1123+
*
1124+
* Apply ExecSetParamPlan to evaluate any not-yet-evaluated initplan output
1125+
* parameters whose ParamIDs are listed in "params". Any listed params that
1126+
* are not initplan outputs are ignored.
1127+
*
1128+
* As with ExecSetParamPlan, any ExprContext belonging to the current EState
1129+
* can be used, but in principle a shorter-lived ExprContext is better than a
1130+
* longer-lived one.
1131+
*/
1132+
void
1133+
ExecSetParamPlanMulti(const Bitmapset *params, ExprContext *econtext)
1134+
{
1135+
int paramid;
1136+
1137+
paramid = -1;
1138+
while ((paramid = bms_next_member(params, paramid)) >= 0)
1139+
{
1140+
ParamExecData *prm = &(econtext->ecxt_param_exec_vals[paramid]);
1141+
1142+
if (prm->execPlan != NULL)
1143+
{
1144+
/* Parameter not evaluated yet, so go do it */
1145+
ExecSetParamPlan(prm->execPlan, econtext);
1146+
/* ExecSetParamPlan should have processed this param... */
1147+
Assert(prm->execPlan == NULL);
1148+
}
1149+
}
1150+
}
1151+
11101152
/*
11111153
* Mark an initplan as needing recalculation
11121154
*/

‎src/include/executor/nodeSubplan.h

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

2929
extern void ExecSetParamPlan(SubPlanState *node, ExprContext *econtext);
3030

31+
extern void ExecSetParamPlanMulti(const Bitmapset *params, ExprContext *econtext);
32+
3133
#endif /* NODESUBPLAN_H */

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,37 @@ ta_id ta_value tb_row
164164
1 newTableAValue (1,tableBValue)
165165
step c2: COMMIT;
166166

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+
167198
starting permutation: wrtwcte readwcte c1 c2
168199
step wrtwcte: UPDATE table_a SET value = 'tableAValue2' WHERE id = 1;
169200
step readwcte:

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,13 @@ step "updateforss" {
8181
UPDATE table_b SET value = 'newTableBValue' WHERE id = 1;
8282
}
8383

84+
# these tests exercise EvalPlanQual with conditional InitPlans which
85+
# have not been executed prior to the EPQ
86+
87+
step "updateforcip" {
88+
UPDATE table_a SET value = NULL WHERE id = 1;
89+
}
90+
8491
# these tests exercise mark/restore during EPQ recheck, cf bug #15032
8592

8693
step "selectjoinforupdate" {
@@ -117,13 +124,21 @@ step "readforss" {
117124
FROM table_a ta
118125
WHERE ta.id = 1 FOR UPDATE OF ta;
119126
}
127+
step "updateforcip2" {
128+
UPDATE table_a SET value = COALESCE(value, (SELECT text 'newValue')) WHERE id = 1;
129+
}
130+
step "updateforcip3" {
131+
WITH d(val) AS (SELECT text 'newValue' FROM generate_series(1,1))
132+
UPDATE table_a SET value = COALESCE(value, (SELECT val FROM d)) WHERE id = 1;
133+
}
120134
step "wrtwcte" { UPDATE table_a SET value = 'tableAValue2' WHERE id = 1; }
121135
step "wrjt" { UPDATE jointest SET data = 42 WHERE id = 7; }
122136
step "c2" { COMMIT; }
123137

124138
session "s3"
125139
setup { BEGIN ISOLATION LEVEL READ COMMITTED; }
126140
step "read" { SELECT * FROM accounts ORDER BY accountid; }
141+
step "read_a" { SELECT * FROM table_a ORDER BY id; }
127142

128143
# this test exercises EvalPlanQual with a CTE, cf bug #14328
129144
step "readwcte" {
@@ -157,6 +172,8 @@ permutation "writep2" "returningp1" "c1" "c2"
157172
permutation "wx2" "partiallock" "c2" "c1" "read"
158173
permutation "wx2" "lockwithvalues" "c2" "c1" "read"
159174
permutation "updateforss" "readforss" "c1" "c2"
175+
permutation "updateforcip" "updateforcip2" "c1" "c2" "read_a"
176+
permutation "updateforcip" "updateforcip3" "c1" "c2" "read_a"
160177
permutation "wrtwcte" "readwcte" "c1" "c2"
161178
permutation "wrjt" "selectjoinforupdate" "c2" "c1"
162179
permutation "wrtwcte" "multireadwcte" "c1" "c2"

0 commit comments

Comments
 (0)