Horton

Re: Table AM Interface Enhancements

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Pavel Borisov <pashkin.elfe@gmail.com>, Japin Li <japinli@hotmail.com>, Mark Dilger <mark.dilger@enterprisedb.com>, pgsql-hackers@lists.postgresql.org
Date: 2024-04-07T23:49:31Z
Lists: pgsql-hackers 
Hi,

On 2024-04-08 02:25:17 +0300, Alexander Korotkov wrote:
> On Mon, Apr 8, 2024 at 12:40 AM Andres Freund <andres@anarazel.de> wrote:
> > On 2024-03-30 23:33:04 +0200, Alexander Korotkov wrote:
> > > I've pushed 0001, 0002 and 0006.
> >
> > I briefly looked at 27bc1772fc81 and I don't think the state post this commit
> > makes sense. Before this commit another block based AM could implement analyze
> > without much code duplication. Now a large portion of analyze.c has to be
> > copied, because they can't stop acquire_sample_rows() from calling
> > heapam_scan_analyze_next_block().
> >
> > I'm quite certain this will break a few out-of-core AMs in a way that can't
> > easily be fixed.
>
> I was under the impression there are not so many out-of-core table
> AMs, which have non-dummy analysis implementations.

I know of at least 4 that have some production usage.


> And even if there are some, duplicating acquire_sample_rows() isn't a big
> deal.

I don't agree. The code has evolved a bunch over time, duplicating it into
various AMs is a bad idea.


> But given your feedback, I'd like to propose to keep both options
> open.  Turn back the block-level API for analyze, but let table-AM
> implement its own analyze function.  Then existing out-of-core AMs
> wouldn't need to do anything (or probably just set the new API method
> to NULL).

I think this patch simply hasn't been reviewed even close to careful enough
and should be reverted. It's IMO to late for a redesign.  Sorry for not
looking earlier, I was mostly out sick for the last few months.

I think a dedicated tableam callback for sample acquisition probably makes
sense, but if we want that, we need to provide an easy way for AMs that are
sufficiently block-like to reuse the code, not have two different ways to
implement analyze.

ISTM that ->relation_analyze is quite misleading as a name. For one, it it
just sets some callbacks, no? But more importantly, it sounds like it'd
actually allow to wrap the whole analyze process, rather than just the
acquisition of samples.

Greetings,

Andres Freund

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