Date: Tue, 17 Mar 2026 12:31:51 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: table AM option passing
Message-ID: <abmQByncauZoE21V@nathan>
References: <202603171606.kf6pmhscqbqz@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202603171606.kf6pmhscqbqz@alvherre.pgsql>
Archived-At: <https://www.postgresql.org/message-id/abmQByncauZoE21V%40nathan>
Precedence: bulk

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