On Thu, Mar 27, 2025 at 4:42 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> On Thu, Mar 27, 2025 at 2:46 PM Matheus Alcantara
> <matheusssilv97@gmail.com> wrote:
> >
> > Just my 0.2 cents. I also like the first approach even though I prefer
> > the v4 version, but anyway, thanks very much for reviewing and
> > committing!
>
> Thanks for the patch!
>
> FWIW, I strongly disliked about v4 that two separate parts of the
> callback were responsible for advancing current_blocknum, one to
> advance it past blocks we chose to skip and the other to advance it to
> the next block.
>
> for (; p->current_blocknum < p->last_exclusive; p->current_blocknum++)
> and
> if (p->current_blocknum < p->last_exclusive)
> return p->current_blocknum++;
>
> I found that alone to be undesirable, but once you add in
> SKIP_PAGES_NONE, I think it was very hard to understand.
>
> Besides that, when we implemented streaming read sequential scan, we
> ended up making dedicated callbacks for the parallel and non-parallel
> cases (see heap_scan_stream_read_next_parallel and
> heap_scan_stream_read_next_serial) because it performed better than a
> single combined callback with a branch. I didn't validate that amcheck
> got the same performance benefit from the dedicated callbacks, but I
> don't see why it would be any different.
Yeah, it totally makes sense to me now, thanks very much for the details!
--
Matheus Alcantara
