Skip to content

Commit f8320cc

Browse files
tglsfdctglsfdc
committed
Fix misbehavior of EvalPlanQual checks with multiple result relations.
The idea of EvalPlanQual is that we replace the query's scan of the result relation with a single injected tuple, and see if we get a tuple out, thereby implying that the injected tuple still passes the query quals. (In join cases, other relations in the query are still scanned normally.) This logic was not updated when commit 86dc900 made it possible for a single DML query plan to have multiple result relations, when the query target relation has inheritance or partition children. We replaced the output for the current result relation successfully, but other result relations were still scanned normally; thus, if any other result relation contained a tuple satisfying the quals, we'd think the EPQ check passed, even if it did not pass for the injected tuple itself. This would lead to update or delete actions getting performed when they should have been skipped due to a conflicting concurrent update in READ COMMITTED isolation mode. Fix by blocking all sibling result relations from emitting tuples during an EvalPlanQual recheck. In the back branches, the fix is complicated a bit by the need to not change the size of struct EPQState (else we'd have ABI-breaking changes in offsets in struct ModifyTableState). Like the back-patches of 3f7836f and 4b3e379, add a separately palloc'd struct to avoid that. The logic is the same as in HEAD otherwise. This is only a live bug back to v14 where 86dc900 came in. However, I chose to back-patch the test cases further, on the grounds that this whole area is none too well tested. I skipped doing so in v11 though because none of the test applied cleanly, and it didn't quite seem worth extra work for a branch with only six months to live. Per report from Ante Krešić (via Aleksander Alekseev) Discussion: https://postgr.es/m/CAJ7c6TMBTN3rcz4=AjYhLPD_w3FFT0Wq_C15jxCDn8U4tZnH1g@mail.gmail.com
1 parent 4cdda71 commit f8320cc

File tree

7 files changed

+240
-41
lines changed

7 files changed

+240
-41
lines changed

‎src/backend/executor/execMain.c

Lines changed: 64 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2343,7 +2343,7 @@ ExecBuildAuxRowMark(ExecRowMark *erm, List *targetlist)
23432343
* relation - table containing tuple
23442344
* rti - rangetable index of table containing tuple
23452345
* inputslot - tuple for processing - this can be the slot from
2346-
* EvalPlanQualSlot(), for the increased efficiency.
2346+
* EvalPlanQualSlot() for this rel, for increased efficiency.
23472347
*
23482348
* This tests whether the tuple in inputslot still matches the relevant
23492349
* quals. For that result to be useful, typically the input tuple has to be
@@ -2377,6 +2377,14 @@ EvalPlanQual(EPQState *epqstate, Relation relation,
23772377
if (testslot != inputslot)
23782378
ExecCopySlot(testslot, inputslot);
23792379

2380+
/*
2381+
* Mark that an EPQ tuple is available for this relation. (If there is
2382+
* more than one result relation, the others remain marked as having no
2383+
* tuple available.)
2384+
*/
2385+
epqstate->relsubs_done[rti - 1] = false;
2386+
epqstate->epqExtra->relsubs_blocked[rti - 1] = false;
2387+
23802388
/*
23812389
* Run the EPQ query. We assume it will return at most one tuple.
23822390
*/
@@ -2393,11 +2401,12 @@ EvalPlanQual(EPQState *epqstate, Relation relation,
23932401
ExecMaterializeSlot(slot);
23942402

23952403
/*
2396-
* Clear out the test tuple. This is needed in case the EPQ query is
2397-
* re-used to test a tuple for a different relation. (Not clear that can
2398-
* really happen, but let's be safe.)
2404+
* Clear out the test tuple, and mark that no tuple is available here.
2405+
* This is needed in case the EPQ state is re-used to test a tuple for a
2406+
* different target relation.
23992407
*/
24002408
ExecClearTuple(testslot);
2409+
epqstate->epqExtra->relsubs_blocked[rti - 1] = true;
24012410

24022411
return slot;
24032412
}
@@ -2412,12 +2421,34 @@ EvalPlanQual(EPQState *epqstate, Relation relation,
24122421
void
24132422
EvalPlanQualInit(EPQState *epqstate, EState *parentestate,
24142423
Plan *subplan, List *auxrowmarks, int epqParam)
2424+
{
2425+
EvalPlanQualInitExt(epqstate, parentestate,
2426+
subplan, auxrowmarks, epqParam, NIL);
2427+
}
2428+
2429+
/*
2430+
* EvalPlanQualInitExt -- same, but allow specification of resultRelations
2431+
*
2432+
* If the caller intends to use EvalPlanQual(), resultRelations should be
2433+
* a list of RT indexes of potential target relations for EvalPlanQual(),
2434+
* and we will arrange that the other listed relations don't return any
2435+
* tuple during an EvalPlanQual() call. Otherwise resultRelations
2436+
* should be NIL.
2437+
*/
2438+
void
2439+
EvalPlanQualInitExt(EPQState *epqstate, EState *parentestate,
2440+
Plan *subplan, List *auxrowmarks,
2441+
int epqParam, List *resultRelations)
24152442
{
24162443
Index rtsize = parentestate->es_range_table_size;
24172444

2445+
/* create some extra space to avoid ABI break */
2446+
epqstate->epqExtra = (EPQStateExtra *) palloc(sizeof(EPQStateExtra));
2447+
24182448
/* initialize data not changing over EPQState's lifetime */
24192449
epqstate->parentestate = parentestate;
24202450
epqstate->epqParam = epqParam;
2451+
epqstate->epqExtra->resultRelations = resultRelations;
24212452

24222453
/*
24232454
* Allocate space to reference a slot for each potential rti - do so now
@@ -2426,7 +2457,7 @@ EvalPlanQualInit(EPQState *epqstate, EState *parentestate,
24262457
* that *may* need EPQ later, without forcing the overhead of
24272458
* EvalPlanQualBegin().
24282459
*/
2429-
epqstate->tuple_table = NIL;
2460+
epqstate->epqExtra->tuple_table = NIL;
24302461
epqstate->relsubs_slot = (TupleTableSlot **)
24312462
palloc0(rtsize * sizeof(TupleTableSlot *));
24322463

@@ -2440,6 +2471,7 @@ EvalPlanQualInit(EPQState *epqstate, EState *parentestate,
24402471
epqstate->recheckplanstate = NULL;
24412472
epqstate->relsubs_rowmark = NULL;
24422473
epqstate->relsubs_done = NULL;
2474+
epqstate->epqExtra->relsubs_blocked = NULL;
24432475
}
24442476

24452477
/*
@@ -2480,7 +2512,7 @@ EvalPlanQualSlot(EPQState *epqstate,
24802512
MemoryContext oldcontext;
24812513

24822514
oldcontext = MemoryContextSwitchTo(epqstate->parentestate->es_query_cxt);
2483-
*slot = table_slot_create(relation, &epqstate->tuple_table);
2515+
*slot = table_slot_create(relation, &epqstate->epqExtra->tuple_table);
24842516
MemoryContextSwitchTo(oldcontext);
24852517
}
24862518

@@ -2637,7 +2669,13 @@ EvalPlanQualBegin(EPQState *epqstate)
26372669
Index rtsize = parentestate->es_range_table_size;
26382670
PlanState *rcplanstate = epqstate->recheckplanstate;
26392671

2640-
MemSet(epqstate->relsubs_done, 0, rtsize * sizeof(bool));
2672+
/*
2673+
* Reset the relsubs_done[] flags to equal relsubs_blocked[], so that
2674+
* the EPQ run will never attempt to fetch tuples from blocked target
2675+
* relations.
2676+
*/
2677+
memcpy(epqstate->relsubs_done, epqstate->epqExtra->relsubs_blocked,
2678+
rtsize * sizeof(bool));
26412679

26422680
/* Recopy current values of parent parameters */
26432681
if (parentestate->es_plannedstmt->paramExecTypes != NIL)
@@ -2804,10 +2842,22 @@ EvalPlanQualStart(EPQState *epqstate, Plan *planTree)
28042842
}
28052843

28062844
/*
2807-
* Initialize per-relation EPQ tuple states to not-fetched.
2845+
* Initialize per-relation EPQ tuple states. Result relations, if any,
2846+
* get marked as blocked; others as not-fetched.
28082847
*/
2809-
epqstate->relsubs_done = (bool *)
2810-
palloc0(rtsize * sizeof(bool));
2848+
epqstate->relsubs_done = palloc_array(bool, rtsize);
2849+
epqstate->epqExtra->relsubs_blocked = palloc0_array(bool, rtsize);
2850+
2851+
foreach(l, epqstate->epqExtra->resultRelations)
2852+
{
2853+
int rtindex = lfirst_int(l);
2854+
2855+
Assert(rtindex > 0 && rtindex <= rtsize);
2856+
epqstate->epqExtra->relsubs_blocked[rtindex - 1] = true;
2857+
}
2858+
2859+
memcpy(epqstate->relsubs_done, epqstate->epqExtra->relsubs_blocked,
2860+
rtsize * sizeof(bool));
28112861

28122862
/*
28132863
* Initialize the private state information for all the nodes in the part
@@ -2844,12 +2894,12 @@ EvalPlanQualEnd(EPQState *epqstate)
28442894
* We may have a tuple table, even if EPQ wasn't started, because we allow
28452895
* use of EvalPlanQualSlot() without calling EvalPlanQualBegin().
28462896
*/
2847-
if (epqstate->tuple_table != NIL)
2897+
if (epqstate->epqExtra->tuple_table != NIL)
28482898
{
28492899
memset(epqstate->relsubs_slot, 0,
28502900
rtsize * sizeof(TupleTableSlot *));
2851-
ExecResetTupleTable(epqstate->tuple_table, true);
2852-
epqstate->tuple_table = NIL;
2901+
ExecResetTupleTable(epqstate->epqExtra->tuple_table, true);
2902+
epqstate->epqExtra->tuple_table = NIL;
28532903
}
28542904

28552905
/* EPQ wasn't started, nothing further to do */
@@ -2883,4 +2933,5 @@ EvalPlanQualEnd(EPQState *epqstate)
28832933
epqstate->recheckplanstate = NULL;
28842934
epqstate->relsubs_rowmark = NULL;
28852935
epqstate->relsubs_done = NULL;
2936+
epqstate->epqExtra->relsubs_blocked = NULL;
28862937
}

‎src/backend/executor/execScan.c

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -69,13 +69,12 @@ ExecScanFetch(ScanState *node,
6969
else if (epqstate->relsubs_done[scanrelid - 1])
7070
{
7171
/*
72-
* Return empty slot, as we already performed an EPQ substitution
73-
* for this relation.
72+
* Return empty slot, as either there is no EPQ tuple for this rel
73+
* or we already returned it.
7474
*/
7575

7676
TupleTableSlot *slot = node->ss_ScanTupleSlot;
7777

78-
/* Return empty slot, as we already returned a tuple */
7978
return ExecClearTuple(slot);
8079
}
8180
else if (epqstate->relsubs_slot[scanrelid - 1] != NULL)
@@ -88,7 +87,7 @@ ExecScanFetch(ScanState *node,
8887

8988
Assert(epqstate->relsubs_rowmark[scanrelid - 1] == NULL);
9089

91-
/* Mark to remember that we shouldn't return more */
90+
/* Mark to remember that we shouldn't return it again */
9291
epqstate->relsubs_done[scanrelid - 1] = true;
9392

9493
/* Return empty slot if we haven't got a test tuple */
@@ -306,14 +305,18 @@ ExecScanReScan(ScanState *node)
306305
*/
307306
ExecClearTuple(node->ss_ScanTupleSlot);
308307

309-
/* Rescan EvalPlanQual tuple if we're inside an EvalPlanQual recheck */
308+
/*
309+
* Rescan EvalPlanQual tuple(s) if we're inside an EvalPlanQual recheck.
310+
* But don't lose the "blocked" status of blocked target relations.
311+
*/
310312
if (estate->es_epq_active != NULL)
311313
{
312314
EPQState *epqstate = estate->es_epq_active;
313315
Index scanrelid = ((Scan *) node->ps.plan)->scanrelid;
314316

315317
if (scanrelid > 0)
316-
epqstate->relsubs_done[scanrelid - 1] = false;
318+
epqstate->relsubs_done[scanrelid - 1] =
319+
epqstate->epqExtra->relsubs_blocked[scanrelid - 1];
317320
else
318321
{
319322
Bitmapset *relids;
@@ -335,7 +338,8 @@ ExecScanReScan(ScanState *node)
335338
while ((rtindex = bms_next_member(relids, rtindex)) >= 0)
336339
{
337340
Assert(rtindex > 0);
338-
epqstate->relsubs_done[rtindex - 1] = false;
341+
epqstate->relsubs_done[rtindex - 1] =
342+
epqstate->epqExtra->relsubs_blocked[rtindex - 1];
339343
}
340344
}
341345
}

‎src/backend/executor/nodeModifyTable.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2880,7 +2880,8 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
28802880
}
28812881

28822882
/* set up epqstate with dummy subplan data for the moment */
2883-
EvalPlanQualInit(&mtstate->mt_epqstate, estate, NULL, NIL, node->epqParam);
2883+
EvalPlanQualInitExt(&mtstate->mt_epqstate, estate, NULL, NIL,
2884+
node->epqParam, node->resultRelations);
28842885
mtstate->fireBSTriggers = true;
28852886

28862887
/*

‎src/include/executor/executor.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,9 @@ extern TupleTableSlot *EvalPlanQual(EPQState *epqstate, Relation relation,
219219
Index rti, TupleTableSlot *testslot);
220220
extern void EvalPlanQualInit(EPQState *epqstate, EState *parentestate,
221221
Plan *subplan, List *auxrowmarks, int epqParam);
222+
extern void EvalPlanQualInitExt(EPQState *epqstate, EState *parentestate,
223+
Plan *subplan, List *auxrowmarks,
224+
int epqParam, List *resultRelations);
222225
extern void EvalPlanQualSetPlan(EPQState *epqstate,
223226
Plan *subplan, List *auxrowmarks);
224227
extern TupleTableSlot *EvalPlanQualSlot(EPQState *epqstate,

‎src/include/nodes/execnodes.h

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1121,17 +1121,17 @@ typedef struct PlanState
11211121
*/
11221122
typedef struct EPQState
11231123
{
1124-
/* Initialized at EvalPlanQualInit() time: */
1125-
1124+
/* These are initialized by EvalPlanQualInit() and do not change later: */
11261125
EState *parentestate; /* main query's EState */
11271126
int epqParam; /* ID of Param to force scan node re-eval */
1127+
struct EPQStateExtra *epqExtra; /* extension pointer to avoid ABI break */
11281128

11291129
/*
1130-
* Tuples to be substituted by scan nodes. They need to set up, before
1131-
* calling EvalPlanQual()/EvalPlanQualNext(), into the slot returned by
1132-
* EvalPlanQualSlot(scanrelid). The array is indexed by scanrelid - 1.
1130+
* relsubs_slot[scanrelid - 1] holds the EPQ test tuple to be returned by
1131+
* the scan node for the scanrelid'th RT index, in place of performing an
1132+
* actual table scan. Callers should use EvalPlanQualSlot() to fetch
1133+
* these slots.
11331134
*/
1134-
List *tuple_table; /* tuple table for relsubs_slot */
11351135
TupleTableSlot **relsubs_slot;
11361136

11371137
/*
@@ -1163,15 +1163,35 @@ typedef struct EPQState
11631163
ExecAuxRowMark **relsubs_rowmark;
11641164

11651165
/*
1166-
* True if a relation's EPQ tuple has been fetched for relation, indexed
1167-
* by scanrelid - 1.
1166+
* relsubs_done[scanrelid - 1] is true if there is no EPQ tuple for this
1167+
* target relation or it has already been fetched in the current scan of
1168+
* this target relation within the current EvalPlanQual test.
11681169
*/
11691170
bool *relsubs_done;
11701171

11711172
PlanState *recheckplanstate; /* EPQ specific exec nodes, for ->plan */
11721173
} EPQState;
11731174

11741175

1176+
/*
1177+
* To avoid an ABI-breaking change in the size of EPQState in back branches,
1178+
* we create one of these during EvalPlanQualInit.
1179+
*/
1180+
typedef struct EPQStateExtra
1181+
{
1182+
List *resultRelations; /* integer list of RT indexes, or NIL */
1183+
List *tuple_table; /* tuple table for relsubs_slot */
1184+
1185+
/*
1186+
* relsubs_blocked[scanrelid - 1] is true if there is no EPQ tuple for
1187+
* this target relation during the current EvalPlanQual test. We keep
1188+
* these flags set for all relids listed in resultRelations, but
1189+
* transiently clear the one for the relation whose tuple is actually
1190+
* passed to EvalPlanQual().
1191+
*/
1192+
bool *relsubs_blocked;
1193+
} EPQStateExtra;
1194+
11751195
/* ----------------
11761196
* ResultState information
11771197
* ----------------

0 commit comments

Comments
 (0)