On Thu, Mar 27, 2025 at 4:42 PM Melanie Plageman <melanieplage...@gmail.com> wrote: > > On Thu, Mar 27, 2025 at 2:46 PM Matheus Alcantara > <matheusssil...@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
