Re: Table AM Interface Enhancements

Hi Andres,

On Wed, Apr 10, 2024 at 7:52 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> 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.

Thank you for your feedback. I've reverted all of above.

------
Regards,
Alexander Korotkov