Hi, On 2026-03-17 17:50:41 +0100, Álvaro Herrera wrote: > 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.
I'm not sure these cases are *entirely* comparable. The boolean argument for table_tuple_delete()/table_tuple_update() is whether to wait, which doesn't influence the on-disk layout, whereas TABLE_INSERT_* do influence the disk layout. > 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 am not sure I understand what you mean by that. Just that the flags better always have the same values? I think the background for the HEAP_* ones to exist is just that there were (probably are) direct callers to heap_insert() and it seemed a bit odd to refer to the generic flags and that there was a need for a heap specific private flag. > 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. I'm ok with both of those. > 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) If we introduce new flag things, we should make them unsigned imo. It's a bad habit that we don't do that everywhere. I've spent a fair bit of time finding bugs due to that in the past (e.g. 2a2e1b470b9). Greetings, Andres Freund
