Re: Table AM Interface Enhancements

Hi,

On 2024-04-08 14:54:46 -0400, Robert Haas wrote:
> Exactly how much is getting reverted here? I see these, all since March 23rd:

IMO:

> dd1f6b0c17 Provide a way block-level table AMs could re-use
> acquire_sample_rows()

Should be reverted.

> 9bd99f4c26 Custom reloptions for table AM

Hm. There are some oddities here:

- It doesn't seem great that relcache.c now needs to know about the default
values for all kinds of reloptions.

- why is there table_reloptions() and tableam_reloptions()?

- Why does extractRelOptions() need a TableAmRoutine parameter, extracted by a
caller, instead of doing that work itself?

> 97ce821e3e Fix the parameters order for
> TableAmRoutine.relation_copy_for_cluster()

Shouldn't be, this is a clear fix.

> b1484a3f19 Let table AM insertion methods control index insertion

I'm not sure. I'm not convinced this is right, nor the opposite. If the
tableam takes control of index insertion, shouldn't nodeModifyTuple know this
earlier, so it doesn't prepare a bunch of index insertion state? Also,
there's pretty much no motivating explanation in the commit.

> 27bc1772fc Generalize relation analyze in table AM interface

Should be reverted.

> 87985cc925 Allow locking updated tuples in tuple_update() and tuple_delete()

Strongly suspect this should be reverted. The last time this was committed it
was far from ready. It's very easy to cause corruption due to subtle bugs in
this area.

> c35a3fb5e0 Allow table AM tuple_insert() method to return the different slot

If the AM returns a different slot, who is responsible for cleaning it up? And
how is creating a new slot for every insert not going to be a measurable
overhead?

> 02eb07ea89 Allow table AM to store complex data structures in rd_amcache

I am doubtful this is right. Is it really sufficient to have a callback for
freeing? What happens when relcache entries are swapped as part of a rebuild?
That works for "flat" caches, but I don't immediately see how it works for
more complicated datastructures. At least from the commit message it's hard
to evaluate how this actually intended to be used.

Greetings,

Andres Freund