Re: table AM option passing

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?= <alvherre(at)kurilemu(dot)de>
> 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/202603171606.kf6pmhscqbqz@alvherre.pgsql
> ---
> 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