| From: | Álvaro Herrera <alvherre(at)kurilemu(dot)de> |
|---|---|
| To: | Pg Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | table AM option passing |
| Date: | 2026-03-17 16:50:41 |
| Message-ID: | 202603171606.kf6pmhscqbqz@alvherre.pgsql |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hello,
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.
Now I don't want consistency just for the sake of it. The issue is that
the REPACK CONCURRENTLY patch wants to add one more option to those two
routines. (In reality, the repack patch as posted doesn't do that
because it deals with heapam.c routines directly instead of going
through table-AM, so there was no need to touch tableam.h; but of course
that's not a good way to implement it, because then you can't repack
tables that aren't heapam-based. So that patch would get more invasive
as we add those bool options everywhere.)
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.
(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.)
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.
Anyway, this is the reason I only defined these flags in tableam.h and
nothing appears in heapam.h about it. They're just something heapam.c
is forced to know about.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Al principio era UNIX, y UNIX habló y dijo: "Hello world\n".
No dijo "Hello New Jersey\n", ni "Hello USA\n".
| Attachment | Content-Type | Size |
|---|---|---|
| v1-0001-table_tuple_update-_delete-use-an-options-bitmask.patch | text/x-diff | 11.7 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Masahiko Sawada | 2026-03-17 16:50:48 | Re: POC: Parallel processing of indexes in autovacuum |
| Previous Message | Nathan Bossart | 2026-03-17 16:45:41 | Re: pg_restore --format= option(without any value) should report an error as pg_dump is reporting an error |