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