Subject: Re: table AM option passing

From: Nathan Bossart <[email-masked]>
Date: Tue, 17 Mar 2026 12:31:51 -0500

On Tue, Mar 17, 2026 at 05:50:41PM +0100, Álvaro Herrera wrote:
> 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.

Seems entirely reasonable to me.  I read through the patch and nothing
stood out.

> (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.)

Why bits64 and not bits32?  I must be missing something.

> 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.

Probably a good idea.

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 17 Mar 2026 15:04:08 -0500

On Tue, Mar 17, 2026 at 08:47:22PM +0100, Álvaro Herrera wrote:
> Does anybody oppose changing table_tuple_insert() to use bits32 instead
> of integer for the 'options' argument?

No objections here.

-- 
nathan

---

From: Zsolt Parragi <[email-masked]>
Date: Wed, 18 Mar 2026 20:20:20 +0000

Hello!

I think there's a change missing in simple_table_tuple_update that
works by accident, as true == 1 == TABLE_UPDATE_WAIT.

Maybe the values could use a different starting value instead of 1 to
surface possible issues?

+ * 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.

"Otherwise, false" seems like a leftover from the previous comment version?

tableam.h also have two leftover "wait == false" comments.

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 18 Mar 2026 16:17:40 -0500

On Tue, Mar 17, 2026 at 05:09:49PM -0400, Andres Freund wrote:
> Personally I object to the existence of the bits* types, to me they're just
> noise over using the corresponding unsigned integer types. One more thing that
> one has to just know what it means without there being any actual improved
> type checking or such. It's not like using bits* would make it any easier to
> make the underlying type a struct or such (which is different to
> e.g. TransactionId, we could probably replace that with a struct without crazy
> amounts of trouble).

Yeah, I don't see why you'd prefer bits32 over uint32.  If anything, uint32
is probably less confusing because most hackers will have used it before.
AFAICT the bits* types are a relic of the 80s, and there used to be other
types like bool8 and word32, all of which were just uint* behind the
scenes.  Those were removed in 2004 by commit ca7a1f0c86.  I assume bits*
was left behind because it was still in use.

> I think we should just rip the bits* types out and replace them with the
> underlying types.

+1.  If there seems to be consensus, I'm happy to write the patch.

-- 
nathan