Horton

Re: Table AM Interface Enhancements

Melanie Plageman <melanieplageman@gmail.com>

View thread View on postgr.es
From: Melanie Plageman <melanieplageman@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Alexander Korotkov <aekorotkov@gmail.com>, 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>, Thomas Munro <thomas.munro@gmail.com>, Nazir Bilal Yavuz <byavuz81@gmail.com>
Date: 2024-04-10T20:24:40Z
Lists: pgsql-hackers 
On Wed, Apr 10, 2024 at 4:03 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2024-04-10 15:19:47 +0300, Alexander Korotkov wrote:
> > On Mon, Apr 8, 2024 at 9:54 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > > On Mon, Apr 8, 2024 at 12:33 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > > > Yes, it was my mistake. I got rushing trying to fit this to FF, even doing significant changes just before commit.
> > > > I'll revert this later today.
> >
> > The patch to revert is attached.  Given that revert touches the work
> > done in 041b96802e, I think it needs some feedback before push.
>
> Hm. It's a bit annoying to revert it, you're right. I think on its own the
> revert looks reasonable from what I've seen so far, will continue looking for
> a bit.
>
> I think we'll need to do some cleanup of 041b96802e separately afterwards -
> possibly in 17, possibly in 18.  Particularly post-27bc1772fc8
> acquire_sample_rows() was tied hard to heapam, so it made sense for 041b96802e
> to create the stream in acquire_sample_rows() and have
> block_sampling_read_stream_next() be in analyze.c. But eventually that should
> be in access/heap/. Compared to 16, the state post the revert does tie
> analyze.c a bit closer to the internals of the AM than before, but I'm not
> sure the increase matters.

Yes in an earlier version of 041b96802e, I gave the review feedback
that the read stream should be pushed down into heap-specific code,
but then after 27bc1772fc8, Bilal took the approach of putting the
read stream code in acquire_sample_rows() since that was no longer
table AM-agnostic.

This thread has been moving pretty fast, so could someone point out
which version of the patch has the modifications to
acquire_sample_rows() that would be relevant for Bilal (and others
involved in analyze streaming read) to review? Is it
v1-0001-revert-Generalize-relation-analyze-in-table-AM-in.patch?

- Melanie

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