On Wed, Apr 10, 2024 at 4:03 PM Andres Freund <and...@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 <robertmh...@gmail.com> wrote: > > > On Mon, Apr 8, 2024 at 12:33 PM Alexander Korotkov <aekorot...@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
