On Thu, Apr 3, 2025 at 11:17 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> I had a quick look at this. Looks good overall, some small remarks:
Thanks for taking a look!
> v12-0001-Autoprewarm-global-objects-separately.patch
>
> > Instead, modify apw_load_buffers() to prewarm the shared objects in one
> > invocation of autoprewarm_database_main() while connected to the first
> > valid database.
>
> So it effectively treats "global objects" as one extra database,
> launching a separate worker process to handle global objects. It took me
> a while to understand that. From the commit message, I understood that
> it still does that within the first worker process invocation, but no. A
> comment somewhere would be good.
Yea, I could have been more explicit about that.
Actually, I was chatting about this with Andres off-list and he was
like, why do you need to check the database at all? Won't
prewarm_stop_idx already have that built in? And I think he's right.
In attached v13, I've added a separate patch (0002) which turns this
check into an assert. And I removed the check from all of the other
loops in the later patches.
> v12-0003-Use-streaming-read-I-O-in-autoprewarm.patch
>
> I wonder if the have_free_buffer() calls work correctly with read
> streams? Or will you "overshoot", prewarming a few more pages after the
> buffer cache is already full? I guess that depends on when exactly the
> read stream code allocates the buffer.
It does have some overshoot -- but a max of io_combine_limit blocks
will be evicted. The read stream code builds up an IO of up to
io_combine_limit blocks before calling StartReadBuffer(). So you could
be in a situation where you weren't quite out of buffers on the
freelist while you are building up the IO and then when you go to pin
those buffers, there aren't enough on the freelist. But I think that's
okay.
> While reviewing this, I noticed a pre-existing bug: The code ignores
> 'tablespace' when deciding if it's reached the end of the current
> relation. I believe it's possible to have two different relations with
> the same relnumber, in different tablespaces.
Good catch. I've included a fix for this in the attached set (0001)
- Melanie
