Re: Table AM Interface Enhancements

On Thu, Apr 11, 2024 at 6:04 PM Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
>
> On Fri, Apr 12, 2024 at 12:04 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2024-04-11 20:46:02 +0300, Alexander Korotkov wrote:
> > > I hope this work is targeting pg18.
> >
> > I think anything of the scope discussed by Melanie would be very clearly
> > targeting 18. For 17, I don't know yet whether we should revert the the
> > ANALYZE streaming read user (041b96802ef), just do a bit of comment polishing,
> > or some other small change.
> >
> > One oddity is that before 041b96802ef, the opportunities for making the
> > interface cleaner were less apparent, because c6fc50cb4028 increased the
> > coupling between analyze.c and the way the table storage works.
>
> Thank you for pointing this out about c6fc50cb4028, I've missed this.
>
> > > Otherwise, do I get this right that this post feature-freeze works on
> > > designing a new API? Yes, 27bc1772fc masked the problem. But it was
> > > committed on Mar 30.
> >
> > Note that there were versions of the patch that were targeting the
> > pre-27bc1772fc interface.
>
> Sure, I've checked this before writing. It looks quite similar to the
> result of applying my revert patch [1] to the head.
>
> Let me describe my view over the current situation.
>
> 1) If we just apply my revert patch and leave c6fc50cb4028 and
> 041b96802ef in the tree, then we get our table AM API narrowed. As
> you expressed the current API requires block numbers to be 1:1 with
> the actual physical on-disk location [2]. Not a secret I think the
> current API is quite restrictive. And we're getting the ANALYZE
> interface narrower than it was since 737a292b5de. Frankly speaking, I
> don't think this is acceptable.
>
> 2) Pushing down the read stream and prefetch to heap am is related to
> difficulties [3], [4]. That's quite a significant piece of work to be
> done post FF.

I had operated under the assumption that we needed to push the
streaming read code into heap AM because that is what we did for
sequential scan, but now that I think about it, I don't see why we
would have to. Bilal's patch pre-27bc1772fc did not do this. But I
think the code in acquire_sample_rows() isn't more tied to heap AM
after 041b96802ef than it was before it. Are you of the opinion that
the code with 041b96802ef ties acquire_sample_rows() more closely to
heap format?

- Melanie