Re: Using read stream in autoprewarm

On Wed, Apr 2, 2025 at 6:26 AM Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com> wrote:
>
> On Wed, 2 Apr 2025 at 01:36, Melanie Plageman <melanieplageman(at)gmail(dot)com> wrote:
> >
> > I know you mentioned off-list that you don't like the handling of
> > global objects in my version, but I prefer doing it this way (even
> > though we have to check for in the loop condition) to having to set
> > the current database once we reach non-shared objects. It feels too
> > fiddly. This way seems less error prone. Looking at this version, what
> > do you think? Could we do it better?
>
> I think there might be a problem with that approach. Let's say that we
> are able to open relation when database oid = 0 and filenumber = 18.
> Then we are trying to find a valid fork now. We couldn't find a valid
> fork immediately, so we continued looping. Then database oid is
> changed from 0 to let's say 1 but filenumber remains the same. We are
> still in the valid fork loop, so relation remains from the database
> oid = 0. Isn't that wrong?

Yep, you are totally right. The code was wrong. We could fix it by
setting current_db to the valid database once we've prewarmed the
global objects, but we need that logic in three places, so that seems
quite undesirable.

In attached v9, I've added a patch to apw_load_buffers() which invokes
autoprewarm_database_main() for the global objects alone but while
connected to the first valid database. It's not the best solution but
I think it is better than having that fiddly logic everywhere about
database 0.

This made me think, I wonder if we could connect to template0 or
template1 to prewarm the global objects. Then we could also prewarm if
only global objects are present (that doesn't seem very important but
it would be a side effect). It might be more clear to connect to
template0/1 instead of the first valid database to prewarm global
objects. I don't know if there is some reason not to do this -- like
maybe bg workers aren't allowed or something?

> 0001:
>
> ReadBufferExtended() can be called in its own minimal loop, otherwise
> we end up doing unnecessary checks for each ReadBufferExtended() call.
> This is not a problem when the 0002 is applied.

Could you provide a snippet of example code? If we call
ReadBufferExtended() in a loop, the block won't be changing, so I
don't see how that will help.

> 0002:
>
> We don't skip blocks whose blocknum is more than nblocks_in_fork. We
> can add that either to a stream callback like you did before or after
> the read_stream_end. I prefer stream callback because of the reason
> below [1].

Yep, I also thought we had to have that logic, but because we sort by
db,rel,fork,blkno, I think blocks with blocknumber >= nblocks_in_fork
will be last and so we just want to move on to the next fork.

> > On another topic, what are the minimal places we need to call
> > have_free_buffers() (in this version)? I haven't even started looking
> > at the last patch you've been sending that is about checking the
> > freelist. I'll have to do that next.
>
> I think its current places are good enough. We may add one after the
> read_stream_end if we want to handle blk->blocknum >= nblocks_in_fork
> after the read stream finishes. If we handle that in the stream
> callback then no need to add have_free_buffers() [1].

As long as we have it in the callback, I don't think that we need
have_free_buffers() after read_stream_end() since it is in the while
loop condition which we will immediately execute after
read_stream_end().

I was also wondering about the other patch in your earlier set that
set stop_idx from get_number_of_free_buffers(). Could you tell me more
about that? What does it do and why is it needed with the read stream
but wasn't needed before?

- Melanie