Horton

Re: Table AM Interface Enhancements

Pavel Borisov <pashkin.elfe@gmail.com>

View thread View on postgr.es
From: Pavel Borisov <pashkin.elfe@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Alexander Korotkov <aekorotkov@gmail.com>, Melanie Plageman <melanieplageman@gmail.com>, Andres Freund <andres@anarazel.de>, 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-15T19:14:01Z
Lists: pgsql-hackers 
On Mon, 15 Apr 2024 at 22:09, Robert Haas <robertmhaas@gmail.com> wrote:

> On Mon, Apr 15, 2024 at 12:37 PM Pavel Borisov <pashkin.elfe@gmail.com>
> wrote:
> > In my understanding, the downside of 041b96802ef is bringing
> read_stream* things from being heap-only-related up to the level of
> acquire_sample_rows() that is not supposed to be tied to heap. And changing
> *_analyze_next_block() function signature to use ReadStream explicitly in
> the signature.
>
> I don't think that really clarifies anything. The ReadStream is
> basically just acting as a wrapper for a stream of block numbers, and
> the API took a BlockNumber before. So why does it make any difference?
>
> If I understand correctly, Alexander thinks that, before 041b96802ef,
> the block number didn't necessarily have to be the physical block
> number on disk, but could instead be any 32-bit quantity that the
> table AM wanted to pack into the block number. But I don't think
> that's true, because acquire_sample_rows() was already passing those
> block numbers to PrefetchBuffer(), which already requires physical
> block numbers.
>

Hi, Robert!

Why it makes a difference looks a little bit unclear to me, I can't comment
on this. I noticed that before 041b96802ef we had a block number and block
sampler state that tied acquire_sample_rows() to the actual block
structure. After we have the whole struct ReadStream which doesn't comprise
just a wrapper for the same variables, but the state that ties
acquire_sample_rows() to the streaming read algorithm (and heap). Yes, we
don't have other access methods other than heap implemented for analyze
routine, so the patch works anyway, but from the view on
acquire_sample_rows() as a general method that is intended to have
different implementations in the future it doesn't look good.

It's my impression on 041b96802ef, please forgive me if I haven't
understood something.

Regards,
Pavel Borisov
Supabase

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