Re: Using read stream in autoprewarm

Hi,

Thank you for looking into this!

On Mon, 31 Mar 2025 at 17:42, Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> On Sun, Mar 30, 2025 at 10:01 AM Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com> wrote:
> >
> > > Some review feedback on your v4: I don't think we need the
> > > rs_have_free_buffer per_buffer_data. We can just check
> > > have_free_buffers() in both the callback and main loop in
> > > autoprewarm_database_main(). I also think you want a comment about why
> > > first_block is needed. And I think you need to guard the
> > > read_stream_end() after the loop -- what if we never made a read
> > > stream because we errored out for all the block's relations or
> > > something?
> >
> > All of these are addressed. One extra thing I noticed is we were not
> > checking if blocknum < number_of_block_in_relation at the first_block
> > case in the stream callback, this is fixed now.
>
> I'm wondering why you need to check if have_free_buffer() in the else
> branch after getting the buffer from the read stream API. Can't you
> just put it back in the for loop condition? Seems like it would have
> the same effect.
>
> - for (; pos < apw_state->prewarm_stop_idx; pos++)
> + for (; pos < apw_state->prewarm_stop_idx && have_free_buffer(); pos++)
> {
> BlockInfoRecord *blk = &block_info[pos];
> Buffer buf;
> @@ -640,9 +640,6 @@ autoprewarm_database_main(Datum main_arg)
> apw_state->prewarmed_blocks++;
> ReleaseBuffer(buf);
> }
> - /* There are no free buffers left in shared buffers, break the loop. */
> - else if (!have_free_buffer())
> - break;
>

You are right, done. Attached as v6.

> Looking at the code some more, I feel stuck on my original point about
> incrementing the position in two places.
> AFAICT, you are going to end up going through the array twice with this design.
> Because you don't set the pos variable in autoprewarm_database_main()
> from the p->pos variable which the read stream callback is
> incrementing, if the read stream callback increments p->pos it a few
> positions yielding those blocks to the read stream machinery to read,
> you are then going to iterate over those positions in the array again
> in the autoprewarm_database_main() loop.
>
> I think you can get around this by setting pos from p->pos in
> autoprewarm_database_main() after read_stream_next_buffer(). Or by
> using p->pos in the loop in autoprewarm_database_main() (which is
> basically what Andres suggested). I'm not sure, though, if this has
> any problems. Like not closing a relation in the right place.

I worked on an alternative approach, I refactored code a bit. It does
not traverse the list two times and I think the code is more suitable
to use read streams now. I simply get how many blocks are processed by
read streams and move the list forward by this number, so the actual
loop skips these blocks. This approach is attached with 'alternative'
prefix.

--
Regards,
Nazir Bilal Yavuz
Microsoft