Re: Using read stream in autoprewarm

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;

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.

- Melanie