Skip to content

Commit 193e6d1

Browse files
akorotkovakorotkov
committed
Revert: Allow locking updated tuples in tuple_update() and tuple_delete()
This commit reverts 87985cc and 818861e per review by Andres Freund. Discussion: https://postgr.es/m/20240410165236.rwyrny7ihi4ddxw4%40awork3.anarazel.de
1 parent da841aa commit 193e6d1

File tree

9 files changed

+346
-501
lines changed

9 files changed

+346
-501
lines changed

‎src/backend/access/heap/heapam.c

Lines changed: 50 additions & 155 deletions
Large diffs are not rendered by default.

‎src/backend/access/heap/heapam_handler.c

Lines changed: 18 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -46,11 +46,6 @@
4646
#include "utils/builtins.h"
4747
#include "utils/rel.h"
4848

49-
static TM_Result heapam_tuple_lock(Relation relation, ItemPointer tid,
50-
Snapshot snapshot, TupleTableSlot *slot,
51-
CommandId cid, LockTupleMode mode,
52-
LockWaitPolicy wait_policy, uint8 flags,
53-
TM_FailureData *tmfd);
5449
static void reform_and_rewrite_tuple(HeapTuple tuple,
5550
Relation OldHeap, Relation NewHeap,
5651
Datum *values, bool *isnull, RewriteState rwstate);
@@ -306,55 +301,23 @@ heapam_tuple_complete_speculative(Relation relation, TupleTableSlot *slot,
306301

307302
static TM_Result
308303
heapam_tuple_delete(Relation relation, ItemPointer tid, CommandId cid,
309-
Snapshot snapshot, Snapshot crosscheck, int options,
310-
TM_FailureData *tmfd, bool changingPart,
311-
TupleTableSlot *oldSlot)
304+
Snapshot snapshot, Snapshot crosscheck, bool wait,
305+
TM_FailureData *tmfd, bool changingPart)
312306
{
313-
TM_Result result;
314-
315307
/*
316308
* Currently Deleting of index tuples are handled at vacuum, in case if
317309
* the storage itself is cleaning the dead tuples by itself, it is the
318310
* time to call the index tuple deletion also.
319311
*/
320-
result = heap_delete(relation, tid, cid, crosscheck, options,
321-
tmfd, changingPart, oldSlot);
322-
323-
/*
324-
* If the tuple has been concurrently updated, then get the lock on it.
325-
* (Do only if caller asked for this by setting the
326-
* TABLE_MODIFY_LOCK_UPDATED option) With the lock held retry of the
327-
* delete should succeed even if there are more concurrent update
328-
* attempts.
329-
*/
330-
if (result == TM_Updated && (options & TABLE_MODIFY_LOCK_UPDATED))
331-
{
332-
/*
333-
* heapam_tuple_lock() will take advantage of tuple loaded into
334-
* oldSlot by heap_delete().
335-
*/
336-
result = heapam_tuple_lock(relation, tid, snapshot,
337-
oldSlot, cid, LockTupleExclusive,
338-
(options & TABLE_MODIFY_WAIT) ?
339-
LockWaitBlock :
340-
LockWaitSkip,
341-
TUPLE_LOCK_FLAG_FIND_LAST_VERSION,
342-
tmfd);
343-
344-
if (result == TM_Ok)
345-
return TM_Updated;
346-
}
347-
348-
return result;
312+
return heap_delete(relation, tid, cid, crosscheck, wait, tmfd, changingPart);
349313
}
350314

351315

352316
static TM_Result
353317
heapam_tuple_update(Relation relation, ItemPointer otid, TupleTableSlot *slot,
354318
CommandId cid, Snapshot snapshot, Snapshot crosscheck,
355-
int options, TM_FailureData *tmfd,
356-
LockTupleMode *lockmode, TU_UpdateIndexes *update_indexes,
357-
TupleTableSlot *oldSlot)
319+
bool wait, TM_FailureData *tmfd,
320+
LockTupleMode *lockmode, TU_UpdateIndexes *update_indexes)
358321
{
359322
bool shouldFree = true;
360323
HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true, &shouldFree);
@@ -364,8 +327,8 @@ heapam_tuple_update(Relation relation, ItemPointer otid, TupleTableSlot *slot,
364327
slot->tts_tableOid = RelationGetRelid(relation);
365328
tuple->t_tableOid = slot->tts_tableOid;
366329

367-
result = heap_update(relation, otid, tuple, cid, crosscheck, options,
368-
tmfd, lockmode, update_indexes, oldSlot);
330+
result = heap_update(relation, otid, tuple, cid, crosscheck, wait,
331+
tmfd, lockmode, update_indexes);
369332
ItemPointerCopy(&tuple->t_self, &slot->tts_tid);
370333

371334
/*
@@ -392,31 +355,6 @@ heapam_tuple_update(Relation relation, ItemPointer otid, TupleTableSlot *slot,
392355
if (shouldFree)
393356
pfree(tuple);
394357

395-
/*
396-
* If the tuple has been concurrently updated, then get the lock on it.
397-
* (Do only if caller asked for this by setting the
398-
* TABLE_MODIFY_LOCK_UPDATED option) With the lock held retry of the
399-
* update should succeed even if there are more concurrent update
400-
* attempts.
401-
*/
402-
if (result == TM_Updated && (options & TABLE_MODIFY_LOCK_UPDATED))
403-
{
404-
/*
405-
* heapam_tuple_lock() will take advantage of tuple loaded into
406-
* oldSlot by heap_update().
407-
*/
408-
result = heapam_tuple_lock(relation, otid, snapshot,
409-
oldSlot, cid, *lockmode,
410-
(options & TABLE_MODIFY_WAIT) ?
411-
LockWaitBlock :
412-
LockWaitSkip,
413-
TUPLE_LOCK_FLAG_FIND_LAST_VERSION,
414-
tmfd);
415-
416-
if (result == TM_Ok)
417-
return TM_Updated;
418-
}
419-
420358
return result;
421359
}
422360

@@ -428,6 +366,7 @@ heapam_tuple_lock(Relation relation, ItemPointer tid, Snapshot snapshot,
428366
{
429367
BufferHeapTupleTableSlot *bslot = (BufferHeapTupleTableSlot *) slot;
430368
TM_Result result;
369+
Buffer buffer;
431370
HeapTuple tuple = &bslot->base.tupdata;
432371
bool follow_updates;
433372

@@ -437,15 +376,18 @@ heapam_tuple_lock(Relation relation, ItemPointer tid, Snapshot snapshot,
437376
Assert(TTS_IS_BUFFERTUPLE(slot));
438377

439378
tuple_lock_retry:
440-
result = heap_lock_tuple(relation, tid, slot, cid, mode, wait_policy,
441-
follow_updates, tmfd);
379+
tuple->t_self = *tid;
380+
result = heap_lock_tuple(relation, tuple, cid, mode, wait_policy,
381+
follow_updates, &buffer, tmfd);
442382

443383
if (result == TM_Updated &&
444384
(flags & TUPLE_LOCK_FLAG_FIND_LAST_VERSION))
445385
{
446386
/* Should not encounter speculative tuple on recheck */
447387
Assert(!HeapTupleHeaderIsSpeculative(tuple->t_data));
448388

389+
ReleaseBuffer(buffer);
390+
449391
if (!ItemPointerEquals(&tmfd->ctid, &tuple->t_self))
450392
{
451393
SnapshotData SnapshotDirty;
@@ -467,8 +409,6 @@ heapam_tuple_lock(Relation relation, ItemPointer tid, Snapshot snapshot,
467409
InitDirtySnapshot(SnapshotDirty);
468410
for (;;)
469411
{
470-
Buffer buffer = InvalidBuffer;
471-
472412
if (ItemPointerIndicatesMovedPartitions(tid))
473413
ereport(ERROR,
474414
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
@@ -563,7 +503,7 @@ heapam_tuple_lock(Relation relation, ItemPointer tid, Snapshot snapshot,
563503
/*
564504
* This is a live tuple, so try to lock it again.
565505
*/
566-
ExecStorePinnedBufferHeapTuple(tuple, slot, buffer);
506+
ReleaseBuffer(buffer);
567507
goto tuple_lock_retry;
568508
}
569509

@@ -574,7 +514,7 @@ heapam_tuple_lock(Relation relation, ItemPointer tid, Snapshot snapshot,
574514
*/
575515
if (tuple->t_data == NULL)
576516
{
577-
ReleaseBuffer(buffer);
517+
Assert(!BufferIsValid(buffer));
578518
return TM_Deleted;
579519
}
580520

@@ -627,6 +567,9 @@ heapam_tuple_lock(Relation relation, ItemPointer tid, Snapshot snapshot,
627567
slot->tts_tableOid = RelationGetRelid(relation);
628568
tuple->t_tableOid = slot->tts_tableOid;
629569

570+
/* store in slot, transferring existing pin */
571+
ExecStorePinnedBufferHeapTuple(tuple, slot, buffer);
572+
630573
return result;
631574
}
632575

‎src/backend/access/table/tableam.c

Lines changed: 6 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -287,23 +287,16 @@ simple_table_tuple_insert(Relation rel, TupleTableSlot *slot)
287287
* via ereport().
288288
*/
289289
void
290-
simple_table_tuple_delete(Relation rel, ItemPointer tid, Snapshot snapshot,
291-
TupleTableSlot *oldSlot)
290+
simple_table_tuple_delete(Relation rel, ItemPointer tid, Snapshot snapshot)
292291
{
293292
TM_Result result;
294293
TM_FailureData tmfd;
295-
int options = TABLE_MODIFY_WAIT; /* wait for commit */
296-
297-
/* Fetch old tuple if the relevant slot is provided */
298-
if (oldSlot)
299-
options |= TABLE_MODIFY_FETCH_OLD_TUPLE;
300294

301295
result = table_tuple_delete(rel, tid,
302296
GetCurrentCommandId(true),
303297
snapshot, InvalidSnapshot,
304-
options,
305-
&tmfd, false /* changingPart */ ,
306-
oldSlot);
298+
true /* wait for commit */ ,
299+
&tmfd, false /* changingPart */ );
307300

308301
switch (result)
309302
{
@@ -342,24 +335,17 @@ void
342335
simple_table_tuple_update(Relation rel, ItemPointer otid,
343336
TupleTableSlot *slot,
344337
Snapshot snapshot,
345-
TU_UpdateIndexes *update_indexes,
346-
TupleTableSlot *oldSlot)
338+
TU_UpdateIndexes *update_indexes)
347339
{
348340
TM_Result result;
349341
TM_FailureData tmfd;
350342
LockTupleMode lockmode;
351-
int options = TABLE_MODIFY_WAIT; /* wait for commit */
352-
353-
/* Fetch old tuple if the relevant slot is provided */
354-
if (oldSlot)
355-
options |= TABLE_MODIFY_FETCH_OLD_TUPLE;
356343

357344
result = table_tuple_update(rel, otid, slot,
358345
GetCurrentCommandId(true),
359346
snapshot, InvalidSnapshot,
360-
options,
361-
&tmfd, &lockmode, update_indexes,
362-
oldSlot);
347+
true /* wait for commit */ ,
348+
&tmfd, &lockmode, update_indexes);
363349

364350
switch (result)
365351
{

‎src/backend/commands/trigger.c

Lines changed: 40 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2773,8 +2773,8 @@ ExecBRDeleteTriggers(EState *estate, EPQState *epqstate,
27732773
void
27742774
ExecARDeleteTriggers(EState *estate,
27752775
ResultRelInfo *relinfo,
2776+
ItemPointer tupleid,
27762777
HeapTuple fdw_trigtuple,
2777-
TupleTableSlot *slot,
27782778
TransitionCaptureState *transition_capture,
27792779
bool is_crosspart_update)
27802780
{
@@ -2783,11 +2783,20 @@ ExecARDeleteTriggers(EState *estate,
27832783
if ((trigdesc && trigdesc->trig_delete_after_row) ||
27842784
(transition_capture && transition_capture->tcs_delete_old_table))
27852785
{
2786-
/*
2787-
* Put the FDW old tuple to the slot. Otherwise, the caller is
2788-
* expected to have an old tuple already fetched to the slot.
2789-
*/
2790-
if (fdw_trigtuple != NULL)
2786+
TupleTableSlot *slot = ExecGetTriggerOldSlot(estate, relinfo);
2787+
2788+
Assert(HeapTupleIsValid(fdw_trigtuple) ^ ItemPointerIsValid(tupleid));
2789+
if (fdw_trigtuple == NULL)
2790+
GetTupleForTrigger(estate,
2791+
NULL,
2792+
relinfo,
2793+
tupleid,
2794+
LockTupleExclusive,
2795+
slot,
2796+
NULL,
2797+
NULL,
2798+
NULL);
2799+
else
27912800
ExecForceStoreHeapTuple(fdw_trigtuple, slot, false);
27922801

27932802
AfterTriggerSaveEvent(estate, relinfo, NULL, NULL,
@@ -3078,17 +3087,18 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
30783087
* Note: 'src_partinfo' and 'dst_partinfo', when non-NULL, refer to the source
30793088
* and destination partitions, respectively, of a cross-partition update of
30803089
* the root partitioned table mentioned in the query, given by 'relinfo'.
3081-
* 'oldslot' contains the "old" tuple in the source partition, and 'newslot'
3082-
* contains the "new" tuple in the destination partition. This interface
3083-
* allows to support the requirements of ExecCrossPartitionUpdateForeignKey();
3084-
* is_crosspart_update must be true in that case.
3090+
* 'tupleid' in that case refers to the ctid of the "old" tuple in the source
3091+
* partition, and 'newslot' contains the "new" tuple in the destination
3092+
* partition. This interface allows to support the requirements of
3093+
* ExecCrossPartitionUpdateForeignKey(); is_crosspart_update must be true in
3094+
* that case.
30853095
*/
30863096
void
30873097
ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
30883098
ResultRelInfo *src_partinfo,
30893099
ResultRelInfo *dst_partinfo,
3100+
ItemPointer tupleid,
30903101
HeapTuple fdw_trigtuple,
3091-
TupleTableSlot *oldslot,
30923102
TupleTableSlot *newslot,
30933103
List *recheckIndexes,
30943104
TransitionCaptureState *transition_capture,
@@ -3107,14 +3117,29 @@ ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
31073117
* separately for DELETE and INSERT to capture transition table rows.
31083118
* In such case, either old tuple or new tuple can be NULL.
31093119
*/
3120+
TupleTableSlot *oldslot;
3121+
ResultRelInfo *tupsrc;
3122+
31103123
Assert((src_partinfo != NULL && dst_partinfo != NULL) ||
31113124
!is_crosspart_update);
31123125

3113-
if (fdw_trigtuple != NULL)
3114-
{
3115-
Assert(oldslot);
3126+
tupsrc = src_partinfo ? src_partinfo : relinfo;
3127+
oldslot = ExecGetTriggerOldSlot(estate, tupsrc);
3128+
3129+
if (fdw_trigtuple == NULL && ItemPointerIsValid(tupleid))
3130+
GetTupleForTrigger(estate,
3131+
NULL,
3132+
tupsrc,
3133+
tupleid,
3134+
LockTupleExclusive,
3135+
oldslot,
3136+
NULL,
3137+
NULL,
3138+
NULL);
3139+
else if (fdw_trigtuple != NULL)
31163140
ExecForceStoreHeapTuple(fdw_trigtuple, oldslot, false);
3117-
}
3141+
else
3142+
ExecClearTuple(oldslot);
31183143

31193144
AfterTriggerSaveEvent(estate, relinfo,
31203145
src_partinfo, dst_partinfo,

‎src/backend/executor/execReplication.c

Lines changed: 4 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -577,7 +577,6 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo,
577577
{
578578
List *recheckIndexes = NIL;
579579
TU_UpdateIndexes update_indexes;
580-
TupleTableSlot *oldSlot = NULL;
581580

582581
/* Compute stored generated columns */
583582
if (rel->rd_att->constr &&
@@ -591,12 +590,8 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo,
591590
if (rel->rd_rel->relispartition)
592591
ExecPartitionCheck(resultRelInfo, slot, estate, true);
593592

594-
if (resultRelInfo->ri_TrigDesc &&
595-
resultRelInfo->ri_TrigDesc->trig_update_after_row)
596-
oldSlot = ExecGetTriggerOldSlot(estate, resultRelInfo);
597-
598593
simple_table_tuple_update(rel, tid, slot, estate->es_snapshot,
599-
&update_indexes, oldSlot);
594+
&update_indexes);
600595

601596
if (resultRelInfo->ri_NumIndices > 0 && (update_indexes != TU_None))
602597
recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
@@ -607,7 +602,7 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo,
607602
/* AFTER ROW UPDATE Triggers */
608603
ExecARUpdateTriggers(estate, resultRelInfo,
609604
NULL, NULL,
610-
NULL, oldSlot, slot,
605+
tid, NULL, slot,
611606
recheckIndexes, NULL, false);
612607

613608
list_free(recheckIndexes);
@@ -641,18 +636,12 @@ ExecSimpleRelationDelete(ResultRelInfo *resultRelInfo,
641636

642637
if (!skip_tuple)
643638
{
644-
TupleTableSlot *oldSlot = NULL;
645-
646-
if (resultRelInfo->ri_TrigDesc &&
647-
resultRelInfo->ri_TrigDesc->trig_delete_after_row)
648-
oldSlot = ExecGetTriggerOldSlot(estate, resultRelInfo);
649-
650639
/* OK, delete the tuple */
651-
simple_table_tuple_delete(rel, tid, estate->es_snapshot, oldSlot);
640+
simple_table_tuple_delete(rel, tid, estate->es_snapshot);
652641

653642
/* AFTER ROW DELETE Triggers */
654643
ExecARDeleteTriggers(estate, resultRelInfo,
655-
NULL, oldSlot, NULL, false);
644+
tid, NULL, NULL, false);
656645
}
657646
}
658647

0 commit comments

Comments
 (0)