Hello,
Thanks Zsolt for the notes -- I have fixed these.
On 2026-Mar-17, Andres Freund wrote:
> 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.
True -- I agree that 'wait' is not "the same kind" of option, makes
sense that it would be separate from options that affect how the tuple
is stored. tuple_delete's changingPart however looks like it should be
part of a bitmask of options.
The repack patch wants to add further options to both tuple_update and
tuple_delete, namely NO_LOGICAL which pretty much mirrors what
TABLE_INSERT_NO_LOGICAL does.
So the attached 0001 adds a 'bits32 options' argument to both
table_update and table_delete, and makes TABLE_DELETE_CHANGING_PARTITION
the first option for table_delete to replace the 'bool changingPart'.
There are no options for table_update() at present, but it seems
inconsistent to not have an argument to it, so I added one that for now
remains unused. The first such option is going to be added by repack.
> > While at it, I noticed that the table_insert() and heap_insert() uses
> > one set of value definitions for each half of the interface; [...]
> 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.
Yeah, it makes sense -- I mean, it's not wrong. But I think it's a bit
awkward. In a way, it's as if heapam.c continues to live in a world
where tableam.h could one day be removed, so it (heapam.h) needs to
redefine its interface in an agnostic way. But I think that's somewhat
delusional, and there's no value in keeping this separation. My
proposal is simply that heapam.c can be made to work under the tableam.h
names of those flag bits, and no abstraction is being broken. 0002 does
this, as well as move HEAP_INSERT_SPECULATIVE be the highest bit rather
than immediately follow the TABLE_INSERT_* ones.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Essentially, you're proposing Kevlar shoes as a solution for the problem
that you want to walk around carrying a loaded gun aimed at your foot.
(Tom Lane)
