Hello, In the table AM world, we provide table_tuple_insert() that gets some behavior-changing options via a bitmask; and then we have table_tuple_update and table_tuple_delete which take a couple of booleans for the same purpose. To me it seems that it would make sense to make these things consistent.
Now I don't want consistency just for the sake of it. The issue is that the REPACK CONCURRENTLY patch wants to add one more option to those two routines. (In reality, the repack patch as posted doesn't do that because it deals with heapam.c routines directly instead of going through table-AM, so there was no need to touch tableam.h; but of course that's not a good way to implement it, because then you can't repack tables that aren't heapam-based. So that patch would get more invasive as we add those bool options everywhere.) We could solve this easily by adding one more boolean to each, but I think this is a good time to instead consolidate the API by using a bitmask; this also allows us not to have changingPart in the wrong place of the heap_delete API. So here's a patch to do that, which shouldn't change any behavior. (This change is vaguely similar to b7271aa1d71a, except I used 'int' instead of 'bits32', to keep the interface consistent with the existing heap_insert() one. Maybe I should make all three take bits64 instead? We don't actually have that type at present, so I'd have to add that too.) While at it, I noticed that the table_insert() and heap_insert() uses one set of value definitions for each half of the interface; that is, in tableam.h we have /* "options" flag bits for table_tuple_insert */ /* TABLE_INSERT_SKIP_WAL was 0x0001; RelationNeedsWAL() now governs */ #define TABLE_INSERT_SKIP_FSM 0x0002 #define TABLE_INSERT_FROZEN 0x0004 #define TABLE_INSERT_NO_LOGICAL 0x0008 and in heapam.h we have /* "options" flag bits for heap_insert */ #define HEAP_INSERT_SKIP_FSM TABLE_INSERT_SKIP_FSM #define HEAP_INSERT_FROZEN TABLE_INSERT_FROZEN #define HEAP_INSERT_NO_LOGICAL TABLE_INSERT_NO_LOGICAL #define HEAP_INSERT_SPECULATIVE 0x0010 This seems rather odd to me -- how could heapam.c have a different set of behaviors than what table AM uses? I find it even more weird that HEAP_INSERT_SPECULATIVE is defined so that as soon as some future patch defines the next "free" tableam.h flag value to do something new, we'll have a conflict. I think this would be cleaner if we removed from heapam.h the flags that correspond to anything in tableam.h, and use heapam.c and all its direct callers use the tableam.h flag definitions instead; and perhaps move HEAP_INSERT_SPECULATIVE to be at the other end of the bitmask (0x1000) -- maybe simply say in tableam.h that the first byte of the options int is reserved for internal use. Anyway, this is the reason I only defined these flags in tableam.h and nothing appears in heapam.h about it. They're just something heapam.c is forced to know about. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ Al principio era UNIX, y UNIX habló y dijo: "Hello world\n". No dijo "Hello New Jersey\n", ni "Hello USA\n".
>From 794f655f33bb89d979a9948810fdd4800a8241ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> Date: Tue, 17 Mar 2026 17:21:14 +0100 Subject: [PATCH v1] table_tuple_update/_delete(): use an options bitmask This replaces a couple of booleans, making the interface more similar to what table_tuple_insert() uses, and better suited for future expansion. Discussion: https://postgr.es/m/[email protected] --- src/backend/access/heap/heapam.c | 15 +++++---- src/backend/access/heap/heapam_handler.c | 10 +++--- src/backend/access/table/tableam.c | 3 +- src/backend/executor/nodeModifyTable.c | 11 ++++--- src/include/access/heapam.h | 6 ++-- src/include/access/tableam.h | 40 ++++++++++++++++-------- 6 files changed, 52 insertions(+), 33 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index e5bd062de77..1cf74ed8c46 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -2852,8 +2852,8 @@ xmax_infomask_changed(uint16 new_infomask, uint16 old_infomask) */ TM_Result heap_delete(Relation relation, const ItemPointerData *tid, - CommandId cid, Snapshot crosscheck, bool wait, - TM_FailureData *tmfd, bool changingPart) + CommandId cid, Snapshot crosscheck, int options, + TM_FailureData *tmfd) { TM_Result result; TransactionId xid = GetCurrentTransactionId(); @@ -2863,6 +2863,8 @@ heap_delete(Relation relation, const ItemPointerData *tid, BlockNumber block; Buffer buffer; Buffer vmbuffer = InvalidBuffer; + bool wait = (options & TABLE_DELETE_WAIT) != 0; + bool changingPart = (options & TABLE_DELETE_CHANGING_PART) != 0; TransactionId new_xmax; uint16 new_infomask, new_infomask2; @@ -3281,8 +3283,8 @@ simple_heap_delete(Relation relation, const ItemPointerData *tid) result = heap_delete(relation, tid, GetCurrentCommandId(true), InvalidSnapshot, - true /* wait for commit */ , - &tmfd, false /* changingPart */ ); + TABLE_DELETE_WAIT, + &tmfd); switch (result) { case TM_SelfModified: @@ -3321,7 +3323,7 @@ simple_heap_delete(Relation relation, const ItemPointerData *tid) */ TM_Result heap_update(Relation relation, const ItemPointerData *otid, HeapTuple newtup, - CommandId cid, Snapshot crosscheck, bool wait, + CommandId cid, Snapshot crosscheck, int options, TM_FailureData *tmfd, LockTupleMode *lockmode, TU_UpdateIndexes *update_indexes) { @@ -3340,6 +3342,7 @@ heap_update(Relation relation, const ItemPointerData *otid, HeapTuple newtup, bool old_key_copied = false; Page page, newpage; + bool wait = (options & TABLE_UPDATE_WAIT) != 0; BlockNumber block; MultiXactStatus mxact_status; Buffer buffer, @@ -4576,7 +4579,7 @@ simple_heap_update(Relation relation, const ItemPointerData *otid, HeapTuple tup result = heap_update(relation, otid, tup, GetCurrentCommandId(true), InvalidSnapshot, - true /* wait for commit */ , + TABLE_UPDATE_WAIT, &tmfd, &lockmode, update_indexes); switch (result) { diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 253a735b6c1..3703d2d18a6 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -310,22 +310,22 @@ heapam_tuple_complete_speculative(Relation relation, TupleTableSlot *slot, static TM_Result heapam_tuple_delete(Relation relation, ItemPointer tid, CommandId cid, - Snapshot snapshot, Snapshot crosscheck, bool wait, - TM_FailureData *tmfd, bool changingPart) + Snapshot snapshot, Snapshot crosscheck, int options, + TM_FailureData *tmfd) { /* * Currently Deleting of index tuples are handled at vacuum, in case if * the storage itself is cleaning the dead tuples by itself, it is the * time to call the index tuple deletion also. */ - return heap_delete(relation, tid, cid, crosscheck, wait, tmfd, changingPart); + return heap_delete(relation, tid, cid, crosscheck, options, tmfd); } static TM_Result heapam_tuple_update(Relation relation, ItemPointer otid, TupleTableSlot *slot, CommandId cid, Snapshot snapshot, Snapshot crosscheck, - bool wait, TM_FailureData *tmfd, + int options, TM_FailureData *tmfd, LockTupleMode *lockmode, TU_UpdateIndexes *update_indexes) { bool shouldFree = true; @@ -336,7 +336,7 @@ heapam_tuple_update(Relation relation, ItemPointer otid, TupleTableSlot *slot, slot->tts_tableOid = RelationGetRelid(relation); tuple->t_tableOid = slot->tts_tableOid; - result = heap_update(relation, otid, tuple, cid, crosscheck, wait, + result = heap_update(relation, otid, tuple, cid, crosscheck, options, tmfd, lockmode, update_indexes); ItemPointerCopy(&tuple->t_self, &slot->tts_tid); diff --git a/src/backend/access/table/tableam.c b/src/backend/access/table/tableam.c index dfda1af412e..4c759f29b3e 100644 --- a/src/backend/access/table/tableam.c +++ b/src/backend/access/table/tableam.c @@ -319,8 +319,7 @@ simple_table_tuple_delete(Relation rel, ItemPointer tid, Snapshot snapshot) result = table_tuple_delete(rel, tid, GetCurrentCommandId(true), snapshot, InvalidSnapshot, - true /* wait for commit */ , - &tmfd, false /* changingPart */ ); + TABLE_DELETE_WAIT, &tmfd); switch (result) { diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 4cd5e262e0f..680c29f35d5 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -1522,14 +1522,17 @@ ExecDeleteAct(ModifyTableContext *context, ResultRelInfo *resultRelInfo, ItemPointer tupleid, bool changingPart) { EState *estate = context->estate; + int options = TABLE_DELETE_WAIT; + + if (changingPart) + options |= TABLE_DELETE_CHANGING_PART; return table_tuple_delete(resultRelInfo->ri_RelationDesc, tupleid, estate->es_output_cid, estate->es_snapshot, estate->es_crosscheck_snapshot, - true /* wait for commit */ , - &context->tmfd, - changingPart); + options, + &context->tmfd); } /* @@ -2333,7 +2336,7 @@ lreplace: estate->es_output_cid, estate->es_snapshot, estate->es_crosscheck_snapshot, - true /* wait for commit */ , + TABLE_UPDATE_WAIT, &context->tmfd, &updateCxt->lockmode, &updateCxt->updateIndexes); diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h index 2fdc50b865b..f74ba9817a7 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -385,13 +385,13 @@ extern void heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples, CommandId cid, int options, BulkInsertState bistate); extern TM_Result heap_delete(Relation relation, const ItemPointerData *tid, - CommandId cid, Snapshot crosscheck, bool wait, - TM_FailureData *tmfd, bool changingPart); + CommandId cid, Snapshot crosscheck, int options, + TM_FailureData *tmfd); extern void heap_finish_speculative(Relation relation, const ItemPointerData *tid); extern void heap_abort_speculative(Relation relation, const ItemPointerData *tid); extern TM_Result heap_update(Relation relation, const ItemPointerData *otid, HeapTuple newtup, - CommandId cid, Snapshot crosscheck, bool wait, + CommandId cid, Snapshot crosscheck, int options, TM_FailureData *tmfd, LockTupleMode *lockmode, TU_UpdateIndexes *update_indexes); extern TM_Result heap_lock_tuple(Relation relation, HeapTuple tuple, diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index 06084752245..15f6d6a3436 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -260,6 +260,13 @@ typedef struct TM_IndexDeleteOp #define TABLE_INSERT_FROZEN 0x0004 #define TABLE_INSERT_NO_LOGICAL 0x0008 +/* "options" flag bits for table_tuple_update */ +#define TABLE_UPDATE_WAIT 0x0001 + +/* "options" flag bits for table_tuple_delete */ +#define TABLE_DELETE_WAIT 0x0001 +#define TABLE_DELETE_CHANGING_PART 0x0002 + /* flag bits for table_tuple_lock */ /* Follow tuples whose update is in progress if lock modes don't conflict */ #define TUPLE_LOCK_FLAG_LOCK_UPDATE_IN_PROGRESS (1 << 0) @@ -535,9 +542,8 @@ typedef struct TableAmRoutine CommandId cid, Snapshot snapshot, Snapshot crosscheck, - bool wait, - TM_FailureData *tmfd, - bool changingPart); + int options, + TM_FailureData *tmfd); /* see table_tuple_update() for reference about parameters */ TM_Result (*tuple_update) (Relation rel, @@ -546,7 +552,7 @@ typedef struct TableAmRoutine CommandId cid, Snapshot snapshot, Snapshot crosscheck, - bool wait, + int options, TM_FailureData *tmfd, LockTupleMode *lockmode, TU_UpdateIndexes *update_indexes); @@ -1459,9 +1465,14 @@ table_multi_insert(Relation rel, TupleTableSlot **slots, int nslots, * cid - delete command ID (used for visibility test, and stored into * cmax if successful) * crosscheck - if not InvalidSnapshot, also check tuple against this - * wait - true if should wait for any conflicting update to commit/abort - * changingPart - true iff the tuple is being moved to another partition - * table due to an update of the partition key. Otherwise, false. + * options - These allow the caller to specify options that may change + * the behavior of the AM. The AM will ignore options that it does not + * support. + * TABLE_DELETE_WAIT -- set if should wait for any conflicting + * update/delete to commit/abort + * TABLE_DELETE_CHANGING_PART -- set iff the tuple is being moved to + * another partition table due to an update of the partition key. + * Otherwise, false. * * Output parameters: * tmfd - filled in failure cases (see below) @@ -1476,12 +1487,12 @@ table_multi_insert(Relation rel, TupleTableSlot **slots, int nslots, */ static inline TM_Result table_tuple_delete(Relation rel, ItemPointer tid, CommandId cid, - Snapshot snapshot, Snapshot crosscheck, bool wait, - TM_FailureData *tmfd, bool changingPart) + Snapshot snapshot, Snapshot crosscheck, int options, + TM_FailureData *tmfd) { return rel->rd_tableam->tuple_delete(rel, tid, cid, snapshot, crosscheck, - wait, tmfd, changingPart); + options, tmfd); } /* @@ -1496,7 +1507,10 @@ table_tuple_delete(Relation rel, ItemPointer tid, CommandId cid, * cid - update command ID (used for visibility test, and stored into * cmax/cmin if successful) * crosscheck - if not InvalidSnapshot, also check old tuple against this - * wait - true if should wait for any conflicting update to commit/abort + * options - These allow the caller to specify options that may change the + * behavior of the AM. The AM will ignore options that it does not support. + * TABLE_UPDATE_WAIT -- set if should wait for any conflicting update to + * commit/abort * * Output parameters: * slot - newly constructed tuple data to store @@ -1522,12 +1536,12 @@ table_tuple_delete(Relation rel, ItemPointer tid, CommandId cid, static inline TM_Result table_tuple_update(Relation rel, ItemPointer otid, TupleTableSlot *slot, CommandId cid, Snapshot snapshot, Snapshot crosscheck, - bool wait, TM_FailureData *tmfd, LockTupleMode *lockmode, + int options, TM_FailureData *tmfd, LockTupleMode *lockmode, TU_UpdateIndexes *update_indexes) { return rel->rd_tableam->tuple_update(rel, otid, slot, cid, snapshot, crosscheck, - wait, tmfd, + options, tmfd, lockmode, update_indexes); } -- 2.47.3
