Re: Streaming read-ready sequential scan code

On Fri, Apr 5, 2024 at 7:28 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
>
> On Sat, Apr 6, 2024 at 6:55 AM Melanie Plageman
> <melanieplageman(at)gmail(dot)com> wrote:
> > I haven't looked into or reviewed the memory prefetching part.
> >
> > While reviewing 0002, I realized that I don't quite see how
> > read_stream_get_block() will be used in the fastpath -- which it
> > claims in its comments.
>
> Comments improved.

Ah, makes sense now.

> The occasional CI problem I mentioned turned out to be
> read_stream_reset() remembering a little too much state across
> rescans. Fixed.

Nice investigative work figuring this out.

> Thanks both for the feedback on the ring buffer tweaks. Comments
> updated. Here is the full stack of patches I would like to commit
> very soon, though I may leave the memory prefetching part out for a
> bit longer to see if I can find any downside.

0001 LGTM. I did not review 0002 or 0003.

0004 looks good except for one comment typo:

/*
* Tell call not to pin more than half the buffers in the ring.
* This is a trade-off between look ahead distance and deferring
* writeback and associated WAL traffic.
*/

call -> caller

0006, I noticed the commit message is missing an important comma:

Instead of calling ReadBuffer() for each block heap sequential scans and
TID range scans now use the streaming read API introduced in b5a9b18cd0.

should be

Instead of calling ReadBuffer() for each block, heap sequential scans and
TID range scans now use the streaming read API introduced in b5a9b18cd0.

- Melanie