Skip to content

Commit 91ad1bd

Browse files
deanrasheeddeanrasheed
committed
Fix concurrent update trigger issues with MERGE in a CTE.
If a MERGE inside a CTE attempts an UPDATE or DELETE on a table with BEFORE ROW triggers, and a concurrent UPDATE or DELETE happens, the merge code would fail (crashing in the case of an UPDATE action, and potentially executing the wrong action for a DELETE action). This is the same issue that 9321c79 attempted to fix, except now for a MERGE inside a CTE. As noted in 9321c79, what needs to happen is for the trigger code to exit early, returning the TM_Result and TM_FailureData information to the merge code, if a concurrent modification is detected, rather than attempting to do an EPQ recheck. The merge code will then do its own rechecking, and rescan the action list, potentially executing a different action in light of the concurrent update. In particular, the trigger code must never call ExecGetUpdateNewTuple() for MERGE, since that is bound to fail because MERGE has its own per-action projection information. Commit 9321c79 did this using estate->es_plannedstmt->commandType in the trigger code to detect that a MERGE was being executed, which is fine for a plain MERGE command, but does not work for a MERGE inside a CTE. Fix by passing that information to the trigger code as an additional parameter passed to ExecBRUpdateTriggers() and ExecBRDeleteTriggers(). Back-patch as far as v17 only, since MERGE cannot appear inside a CTE prior to that. Additionally, take care to preserve the trigger ABI in v17 (though not in v18, which is still in beta). Bug: #18986 Reported-by: Yaroslav Syrytsia <me@ys.lc> Author: Dean Rasheed <dean.a.rasheed@gmail.com> Reviewed-by: Michael Paquier <michael@paquier.xyz> Discussion: https://postgr.es/m/18986-e7a8aac3d339fa47@postgresql.org Backpatch-through: 17
1 parent 3f10d2b commit 91ad1bd

File tree

5 files changed

+153
-62
lines changed

5 files changed

+153
-62
lines changed

‎src/backend/commands/trigger.c

Lines changed: 96 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ static bool GetTupleForTrigger(EState *estate,
7979
ItemPointer tid,
8080
LockTupleMode lockmode,
8181
TupleTableSlot *oldslot,
82+
bool do_epq_recheck,
8283
TupleTableSlot **epqslot,
8384
TM_Result *tmresultp,
8485
TM_FailureData *tmfdp);
@@ -2679,13 +2680,14 @@ ExecASDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
26792680
* back the concurrently updated tuple if any.
26802681
*/
26812682
bool
2682-
ExecBRDeleteTriggers(EState *estate, EPQState *epqstate,
2683-
ResultRelInfo *relinfo,
2684-
ItemPointer tupleid,
2685-
HeapTuple fdw_trigtuple,
2686-
TupleTableSlot **epqslot,
2687-
TM_Result *tmresult,
2688-
TM_FailureData *tmfd)
2683+
ExecBRDeleteTriggersNew(EState *estate, EPQState *epqstate,
2684+
ResultRelInfo *relinfo,
2685+
ItemPointer tupleid,
2686+
HeapTuple fdw_trigtuple,
2687+
TupleTableSlot **epqslot,
2688+
TM_Result *tmresult,
2689+
TM_FailureData *tmfd,
2690+
bool is_merge_delete)
26892691
{
26902692
TupleTableSlot *slot = ExecGetTriggerOldSlot(estate, relinfo);
26912693
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
@@ -2700,9 +2702,17 @@ ExecBRDeleteTriggers(EState *estate, EPQState *epqstate,
27002702
{
27012703
TupleTableSlot *epqslot_candidate = NULL;
27022704

2705+
/*
2706+
* Get a copy of the on-disk tuple we are planning to delete. In
2707+
* general, if the tuple has been concurrently updated, we should
2708+
* recheck it using EPQ. However, if this is a MERGE DELETE action,
2709+
* we skip this EPQ recheck and leave it to the caller (it must do
2710+
* additional rechecking, and might end up executing a different
2711+
* action entirely).
2712+
*/
27032713
if (!GetTupleForTrigger(estate, epqstate, relinfo, tupleid,
2704-
LockTupleExclusive, slot, &epqslot_candidate,
2705-
tmresult, tmfd))
2714+
LockTupleExclusive, slot, !is_merge_delete,
2715+
&epqslot_candidate, tmresult, tmfd))
27062716
return false;
27072717

27082718
/*
@@ -2765,6 +2775,24 @@ ExecBRDeleteTriggers(EState *estate, EPQState *epqstate,
27652775
return result;
27662776
}
27672777

2778+
/*
2779+
* ABI-compatible wrapper to emulate old version of the above function.
2780+
* Do not call this version in new code.
2781+
*/
2782+
bool
2783+
ExecBRDeleteTriggers(EState *estate, EPQState *epqstate,
2784+
ResultRelInfo *relinfo,
2785+
ItemPointer tupleid,
2786+
HeapTuple fdw_trigtuple,
2787+
TupleTableSlot **epqslot,
2788+
TM_Result *tmresult,
2789+
TM_FailureData *tmfd)
2790+
{
2791+
return ExecBRDeleteTriggersNew(estate, epqstate, relinfo, tupleid,
2792+
fdw_trigtuple, epqslot, tmresult, tmfd,
2793+
false);
2794+
}
2795+
27682796
/*
27692797
* Note: is_crosspart_update must be true if the DELETE is being performed
27702798
* as part of a cross-partition update.
@@ -2792,6 +2820,7 @@ ExecARDeleteTriggers(EState *estate,
27922820
tupleid,
27932821
LockTupleExclusive,
27942822
slot,
2823+
false,
27952824
NULL,
27962825
NULL,
27972826
NULL);
@@ -2930,13 +2959,14 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
29302959
}
29312960

29322961
bool
2933-
ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
2934-
ResultRelInfo *relinfo,
2935-
ItemPointer tupleid,
2936-
HeapTuple fdw_trigtuple,
2937-
TupleTableSlot *newslot,
2938-
TM_Result *tmresult,
2939-
TM_FailureData *tmfd)
2962+
ExecBRUpdateTriggersNew(EState *estate, EPQState *epqstate,
2963+
ResultRelInfo *relinfo,
2964+
ItemPointer tupleid,
2965+
HeapTuple fdw_trigtuple,
2966+
TupleTableSlot *newslot,
2967+
TM_Result *tmresult,
2968+
TM_FailureData *tmfd,
2969+
bool is_merge_update)
29402970
{
29412971
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
29422972
TupleTableSlot *oldslot = ExecGetTriggerOldSlot(estate, relinfo);
@@ -2957,10 +2987,17 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
29572987
{
29582988
TupleTableSlot *epqslot_candidate = NULL;
29592989

2960-
/* get a copy of the on-disk tuple we are planning to update */
2990+
/*
2991+
* Get a copy of the on-disk tuple we are planning to update. In
2992+
* general, if the tuple has been concurrently updated, we should
2993+
* recheck it using EPQ. However, if this is a MERGE UPDATE action,
2994+
* we skip this EPQ recheck and leave it to the caller (it must do
2995+
* additional rechecking, and might end up executing a different
2996+
* action entirely).
2997+
*/
29612998
if (!GetTupleForTrigger(estate, epqstate, relinfo, tupleid,
2962-
lockmode, oldslot, &epqslot_candidate,
2963-
tmresult, tmfd))
2999+
lockmode, oldslot, !is_merge_update,
3000+
&epqslot_candidate, tmresult, tmfd))
29643001
return false; /* cancel the update action */
29653002

29663003
/*
@@ -3082,6 +3119,24 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
30823119
return true;
30833120
}
30843121

3122+
/*
3123+
* ABI-compatible wrapper to emulate old version of the above function.
3124+
* Do not call this version in new code.
3125+
*/
3126+
bool
3127+
ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
3128+
ResultRelInfo *relinfo,
3129+
ItemPointer tupleid,
3130+
HeapTuple fdw_trigtuple,
3131+
TupleTableSlot *newslot,
3132+
TM_Result *tmresult,
3133+
TM_FailureData *tmfd)
3134+
{
3135+
return ExecBRUpdateTriggersNew(estate, epqstate, relinfo, tupleid,
3136+
fdw_trigtuple, newslot, tmresult, tmfd,
3137+
false);
3138+
}
3139+
30853140
/*
30863141
* Note: 'src_partinfo' and 'dst_partinfo', when non-NULL, refer to the source
30873142
* and destination partitions, respectively, of a cross-partition update of
@@ -3132,6 +3187,7 @@ ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
31323187
tupleid,
31333188
LockTupleExclusive,
31343189
oldslot,
3190+
false,
31353191
NULL,
31363192
NULL,
31373193
NULL);
@@ -3288,6 +3344,7 @@ GetTupleForTrigger(EState *estate,
32883344
ItemPointer tid,
32893345
LockTupleMode lockmode,
32903346
TupleTableSlot *oldslot,
3347+
bool do_epq_recheck,
32913348
TupleTableSlot **epqslot,
32923349
TM_Result *tmresultp,
32933350
TM_FailureData *tmfdp)
@@ -3347,29 +3404,30 @@ GetTupleForTrigger(EState *estate,
33473404
if (tmfd.traversed)
33483405
{
33493406
/*
3350-
* Recheck the tuple using EPQ. For MERGE, we leave this
3351-
* to the caller (it must do additional rechecking, and
3352-
* might end up executing a different action entirely).
3407+
* Recheck the tuple using EPQ, if requested. Otherwise,
3408+
* just return that it was concurrently updated.
33533409
*/
3354-
if (estate->es_plannedstmt->commandType == CMD_MERGE)
3410+
if (do_epq_recheck)
33553411
{
3356-
if (tmresultp)
3357-
*tmresultp = TM_Updated;
3358-
return false;
3412+
*epqslot = EvalPlanQual(epqstate,
3413+
relation,
3414+
relinfo->ri_RangeTableIndex,
3415+
oldslot);
3416+
3417+
/*
3418+
* If PlanQual failed for updated tuple - we must not
3419+
* process this tuple!
3420+
*/
3421+
if (TupIsNull(*epqslot))
3422+
{
3423+
*epqslot = NULL;
3424+
return false;
3425+
}
33593426
}
3360-
3361-
*epqslot = EvalPlanQual(epqstate,
3362-
relation,
3363-
relinfo->ri_RangeTableIndex,
3364-
oldslot);
3365-
3366-
/*
3367-
* If PlanQual failed for updated tuple - we must not
3368-
* process this tuple!
3369-
*/
3370-
if (TupIsNull(*epqslot))
3427+
else
33713428
{
3372-
*epqslot = NULL;
3429+
if (tmresultp)
3430+
*tmresultp = TM_Updated;
33733431
return false;
33743432
}
33753433
}

‎src/backend/executor/nodeModifyTable.c

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1349,9 +1349,10 @@ ExecDeletePrologue(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
13491349
if (context->estate->es_insert_pending_result_relations != NIL)
13501350
ExecPendingInserts(context->estate);
13511351

1352-
return ExecBRDeleteTriggers(context->estate, context->epqstate,
1353-
resultRelInfo, tupleid, oldtuple,
1354-
epqreturnslot, result, &context->tmfd);
1352+
return ExecBRDeleteTriggersNew(context->estate, context->epqstate,
1353+
resultRelInfo, tupleid, oldtuple,
1354+
epqreturnslot, result, &context->tmfd,
1355+
context->mtstate->operation == CMD_MERGE);
13551356
}
13561357

13571358
return true;
@@ -1947,9 +1948,10 @@ ExecUpdatePrologue(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
19471948
if (context->estate->es_insert_pending_result_relations != NIL)
19481949
ExecPendingInserts(context->estate);
19491950

1950-
return ExecBRUpdateTriggers(context->estate, context->epqstate,
1951-
resultRelInfo, tupleid, oldtuple, slot,
1952-
result, &context->tmfd);
1951+
return ExecBRUpdateTriggersNew(context->estate, context->epqstate,
1952+
resultRelInfo, tupleid, oldtuple, slot,
1953+
result, &context->tmfd,
1954+
context->mtstate->operation == CMD_MERGE);
19531955
}
19541956

19551957
return true;

‎src/include/commands/trigger.h

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,15 @@ extern void ExecBSDeleteTriggers(EState *estate,
206206
extern void ExecASDeleteTriggers(EState *estate,
207207
ResultRelInfo *relinfo,
208208
TransitionCaptureState *transition_capture);
209+
extern bool ExecBRDeleteTriggersNew(EState *estate,
210+
EPQState *epqstate,
211+
ResultRelInfo *relinfo,
212+
ItemPointer tupleid,
213+
HeapTuple fdw_trigtuple,
214+
TupleTableSlot **epqslot,
215+
TM_Result *tmresult,
216+
TM_FailureData *tmfd,
217+
bool is_merge_delete);
209218
extern bool ExecBRDeleteTriggers(EState *estate,
210219
EPQState *epqstate,
211220
ResultRelInfo *relinfo,
@@ -228,6 +237,15 @@ extern void ExecBSUpdateTriggers(EState *estate,
228237
extern void ExecASUpdateTriggers(EState *estate,
229238
ResultRelInfo *relinfo,
230239
TransitionCaptureState *transition_capture);
240+
extern bool ExecBRUpdateTriggersNew(EState *estate,
241+
EPQState *epqstate,
242+
ResultRelInfo *relinfo,
243+
ItemPointer tupleid,
244+
HeapTuple fdw_trigtuple,
245+
TupleTableSlot *newslot,
246+
TM_Result *tmresult,
247+
TM_FailureData *tmfd,
248+
bool is_merge_update);
231249
extern bool ExecBRUpdateTriggers(EState *estate,
232250
EPQState *epqstate,
233251
ResultRelInfo *relinfo,

‎src/test/isolation/expected/merge-match-recheck.out

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -241,19 +241,28 @@ starting permutation: update_bal1_tg merge_bal_tg c2 select1_tg c1
241241
s2: NOTICE: Update: (1,160,s1,setup) -> (1,50,s1,"setup updated by update_bal1_tg")
242242
step update_bal1_tg: UPDATE target_tg t SET balance = 50, val = t.val || ' updated by update_bal1_tg' WHERE t.key = 1;
243243
step merge_bal_tg:
244-
MERGE INTO target_tg t
245-
USING (SELECT 1 as key) s
246-
ON s.key = t.key
247-
WHEN MATCHED AND balance < 100 THEN
248-
UPDATE SET balance = balance * 2, val = t.val || ' when1'
249-
WHEN MATCHED AND balance < 200 THEN
250-
UPDATE SET balance = balance * 4, val = t.val || ' when2'
251-
WHEN MATCHED AND balance < 300 THEN
252-
UPDATE SET balance = balance * 8, val = t.val || ' when3';
244+
WITH t AS (
245+
MERGE INTO target_tg t
246+
USING (SELECT 1 as key) s
247+
ON s.key = t.key
248+
WHEN MATCHED AND balance < 100 THEN
249+
UPDATE SET balance = balance * 2, val = t.val || ' when1'
250+
WHEN MATCHED AND balance < 200 THEN
251+
UPDATE SET balance = balance * 4, val = t.val || ' when2'
252+
WHEN MATCHED AND balance < 300 THEN
253+
UPDATE SET balance = balance * 8, val = t.val || ' when3'
254+
RETURNING t.*
255+
)
256+
SELECT * FROM t;
253257
<waiting ...>
254258
step c2: COMMIT;
255259
s1: NOTICE: Update: (1,50,s1,"setup updated by update_bal1_tg") -> (1,100,s1,"setup updated by update_bal1_tg when1")
256260
step merge_bal_tg: <... completed>
261+
key|balance|status|val
262+
---+-------+------+-------------------------------------
263+
1| 100|s1 |setup updated by update_bal1_tg when1
264+
(1 row)
265+
257266
step select1_tg: SELECT * FROM target_tg;
258267
key|balance|status|val
259268
---+-------+------+-------------------------------------

‎src/test/isolation/specs/merge-match-recheck.spec

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -99,15 +99,19 @@ step "merge_bal_pa"
9999
}
100100
step "merge_bal_tg"
101101
{
102-
MERGE INTO target_tg t
103-
USING (SELECT 1 as key) s
104-
ON s.key = t.key
105-
WHEN MATCHED AND balance < 100 THEN
106-
UPDATE SET balance = balance * 2, val = t.val || ' when1'
107-
WHEN MATCHED AND balance < 200 THEN
108-
UPDATE SET balance = balance * 4, val = t.val || ' when2'
109-
WHEN MATCHED AND balance < 300 THEN
110-
UPDATE SET balance = balance * 8, val = t.val || ' when3';
102+
WITH t AS (
103+
MERGE INTO target_tg t
104+
USING (SELECT 1 as key) s
105+
ON s.key = t.key
106+
WHEN MATCHED AND balance < 100 THEN
107+
UPDATE SET balance = balance * 2, val = t.val || ' when1'
108+
WHEN MATCHED AND balance < 200 THEN
109+
UPDATE SET balance = balance * 4, val = t.val || ' when2'
110+
WHEN MATCHED AND balance < 300 THEN
111+
UPDATE SET balance = balance * 8, val = t.val || ' when3'
112+
RETURNING t.*
113+
)
114+
SELECT * FROM t;
111115
}
112116

113117
step "merge_delete"

0 commit comments

Comments
 (0)