Horton

Re: Table AM Interface Enhancements

Alexander Korotkov <aekorotkov@gmail.com>

View thread View on postgr.es
From: Alexander Korotkov <aekorotkov@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Robert Haas <robertmhaas@gmail.com>, Pavel Borisov <pashkin.elfe@gmail.com>, Japin Li <japinli@hotmail.com>, Mark Dilger <mark.dilger@enterprisedb.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Michael Paquier <michael@paquier.xyz>, Heikki Linnakangas <hlinnaka@iki.fi>
Date: 2024-04-11T13:26:15Z
Lists: pgsql-hackers 
Hi Andres,

On Wed, Apr 10, 2024 at 7:52 PM Andres Freund <andres@anarazel.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

Commits

  1. Remove extra comment at TableAmRoutine.scan_analyze_next_block

  2. revert: Generalize relation analyze in table AM interface

  3. Revert: Allow table AM to store complex data structures in rd_amcache

  4. Revert: Allow table AM tuple_insert() method to return the different slot

  5. Revert: Allow locking updated tuples in tuple_update() and tuple_delete()

  6. Revert: Let table AM insertion methods control index insertion

  7. Revert: Custom reloptions for table AM

  8. Provide a way block-level table AMs could re-use acquire_sample_rows()

  9. Custom reloptions for table AM

  10. Use streaming I/O in ANALYZE.

  11. Revert "Custom reloptions for table AM"

  12. Let table AM insertion methods control index insertion

  13. Generalize relation analyze in table AM interface

  14. Improve error message for tts_(virtual|minimal)_is_current_xact_tuple

  15. Add comments on some MinimalTupleSlots methods usage

  16. Add TupleTableSlotOps.is_current_xact_tuple() method

  17. Allow table AM tuple_insert() method to return the different slot

  18. Allow table AM to store complex data structures in rd_amcache