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
