Subject: Re: table AM option passing

From: Zsolt Parragi <[email-masked]>
Date: Wed, 1 Apr 2026 00:20:00 +0100

static inline TM_Result
 table_tuple_delete(Relation rel, ItemPointer tid, CommandId cid,
-    Snapshot snapshot, Snapshot crosscheck, bool wait,
-    TM_FailureData *tmfd, bool changingPart)
+    uint32 options, Snapshot snapshot, Snapshot crosscheck,
+    bool wait, TM_FailureData *tmfd)

The doc comment still referneces changingPart
Similarly table_tuple_update doesn't document the new options parameter.

@@ -339,7 +341,8 @@ heapam_tuple_update(Relation relation, ItemPointer
otid, TupleTableSlot *slot,
  slot->tts_tableOid = RelationGetRelid(relation);
  tuple->t_tableOid = slot->tts_tableOid;

- result = heap_update(relation, otid, tuple, cid, crosscheck, wait,
+ result = heap_update(relation, otid, tuple, cid, options,
+ crosscheck, wait,
  tmfd, lockmode, update_indexes);
  ItemPointerCopy(&tuple->t_self, &slot->tts_tid);

options is marked pg_attribute_unused above, that seems misleading.
Should the annotation be part of the heap_update signature instead?

---

From: Chao Li <[email-masked]>
Date: Wed, 1 Apr 2026 14:33:28 +0800

> On Apr 1, 2026, at 00:10, Álvaro Herrera <[email-masked]> wrote:
> 
> Hi,
> 
> On 2026-Mar-30, Álvaro Herrera wrote:
> 
>> I just pushed the part of 0001 that modifies the API of table_insert and
>> its sibling functions.  So here 0001 adds the options bitmask to
>> table_update and table_delete, which is important for REPACK; and 0002
>> is identical as before and makes the interface (IMO) a bit more
>> future-proof.
> 
> FWIW I just posted 0001 as part of the repack series here [1]; 0002 is
> inessential, so I didn't; but here's both of them.  I have drafted
> commit messages also.
> 
> [1] https://postgr.es/m/[email-masked]
> 
> -- 
> Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
> "Pensar que el espectro que vemos es ilusorio no lo despoja de espanto,
> sólo le suma el nuevo terror de la locura" (Perelandra, C.S. Lewis)
> <v4-0001-Give-options-parameter-to-table_delete-table_upda.patch><v4-0002-Change-heapam.c-to-obey-tableam.h-option-bits-dir.patch>

Looks like v4-0001/0002 are pure refactoring. A few small comments:

1 - 0001
For table_tuple_delete(), options is added and changingPart is removed, but the header comment should be updated to reflect the change.

2 - 0002
For table_tuple_update(), options is added, the header comment should be updated as well.

3 - 0002
Now TABLE_INSERT_SKIP_FSM, TABLE_INSERT_FROZEN, TABLE_INSERT_NO_LOGICAL belong to the same options word as HEAP_INSERT_SPECULATIVE, but they are still defined as:
```
#define TABLE_INSERT_SKIP_FSM 0x0002
#define TABLE_INSERT_FROZEN 0x0004
#define TABLE_INSERT_NO_LOGICAL 0x0008
```

Could it make sense to change them to the left-shift form?

4 - 0002 
In heap_multi_insert(), heap_prepare_insert(), and heap_insert(), options is defined as uint32, but in RelationGetBufferForTuple() and raw_heap_insert() it is still defined as int. Would it make sense to take this opportunity to change all “options" to uint32 for consistency? Otherwise, if this is left for later, a trivial follow-up patch just to change int to uint32 may be harder to get processed.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

---

From: Álvaro Herrera <[email-masked]>
Date: Wed, 1 Apr 2026 16:27:18 +0200

On 2026-Apr-01, Chao Li wrote:

> 1 - 0001
> For table_tuple_delete(), options is added and changingPart is
> removed, but the header comment should be updated to reflect the
> change.

True, fixed.

> For table_tuple_update(), options is added, the header comment should

Done.

> Could it make sense to change them to the left-shift form?

Yeah, I don't know.  I don't like this style, but some people like it,
and I don't want to get into an argument about this kind of thing.

> 4 - 0002 
> In heap_multi_insert(), heap_prepare_insert(), and heap_insert(),
> options is defined as uint32, but in RelationGetBufferForTuple() and
> raw_heap_insert() it is still defined as int. Would it make sense to
> take this opportunity to change all “options" to uint32 for
> consistency? Otherwise, if this is left for later, a trivial follow-up
> patch just to change int to uint32 may be harder to get processed.

Hmm, this is actually a problem in 1bd6f22f43ac.  I added a preliminary
patch that should fix this.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"No renuncies a nada. No te aferres a nada."

---

From: Antonin Houska <[email-masked]>
Date: Wed, 01 Apr 2026 17:16:58 +0200

> patch that should fix this.

I've reviewed this patch set too. The only question that occurs to me is
whether INSERT_SKIP_FSM and INSERT_FROZEN shouldn't actually be prefixed with
HEAP_, as these are rather low-level and thus might be considered
AM-specific.

(I like the idea to use the high bits to avoid collisions.)

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

---

From: Álvaro Herrera <[email-masked]>
Date: Wed, 1 Apr 2026 20:41:01 +0200

> HEAP_, as these are rather low-level and thus might be considered

Thanks!  I pushed 0001 and 0002 from this patchset, with minimal
cosmetic corrections.

I realized that patch 0003 is doing two different things, and they
should each be their own patch which can be rejected if we don't like
them; so I split it in two.  One moves the heapam.h-private bit to the
32th bit.  The other removes the duplicity of definitions, so that
heapam.h doesn't have to feel it needs to redefine the tableam.h
interface.

At this point these patches could be considered WIP in the sense that I
wouldn't commit exactly as is (minor corrections might be appropriate,
for example to the nearby comments), but I would like to know opinions
in case we decide to just throw them away.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Las navajas y los monos deben estar siempre distantes"   (Germán Poo)

---

From: Antonin Houska <[email-masked]>
Date: Thu, 02 Apr 2026 07:39:43 +0200

> 32th bit.

I'm sorry I haven't recalled yesterday, but this technique resembles the
DSM keys in parallel.c:

  /*
   * Magic numbers for per-context parallel state sharing.  Higher-level code
   * should use smaller values, leaving these very large ones for use by this
   * module.
   */
  #define PARALLEL_KEY_FIXED		UINT64CONST(0xFFFFFFFFFFFF0001)
  ...

What I found inspiring here is that the "core" uses the high bits while users
of the API use the lower ones. Perhaps it'd be appropriate in v6-0001 to
reserve the high bits for the TABLE_ options and leave the lower ones for the
HEAP_ options. If someone implements a new AM (possibly as an extension), it
should be more comfortable for him.