public inbox for [email protected]  

help / color / mirror / Atom feed

From: Melanie Plageman <[email protected]>
To: Tomas Vondra <[email protected]>
Cc: Matheus Alcantara <[email protected]>
Cc: Glauber Batista <[email protected]>
Cc: [email protected]
Subject: Re: Autoprewarm workers terminated due to a segmentation fault
Date: Mon, 15 Jun 2026 10:35:43 -0400
Message-ID: <CAAKRu_aekYazEexpT0KtjFaJOFD=Z=4Of8vwLrf+fsuJBSaBcg@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAO+_mTQgQyTYwDh=U8iTnsDmOGyWsZJjUV31SmEYwmw6_xY6Bw@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

On Thu, Jun 11, 2026 at 10:40 AM Tomas Vondra <[email protected]> wrote:
>
> After looking a bit closer, I think this bug was introduced by
>
>     commit 6acab8bdbcda735ef47b1bb0ba2284d6c465cd88
>     Author: Melanie Plageman <[email protected]>
>     Date:   Fri Apr 4 15:25:27 2025 -0400
>
>     Refactor autoprewarm_database_main() in preparation for read stream
>
> which happens to advance to the next block in a couple places
>
>     blk = block_info[++i];
>
> before we know it the incremented "i" is a valid element. The following
> commit (d9c7911e1a5f adding the read stream) ends up doing the same
> thing, except the index is incremented in a callback.
>
> Melanie, do you agree with the proposed fix?

Thanks all for investigating this! I agree that the proposed fix is
correct (both v1 and v2). I am a bit torn between the two versions.
v1:

i = p.pos;
if (i >= apw_state->prewarm_stop_idx)
    break;
blk = block_info[i];

preserves what I had intended in that it sets blk as close to where i
was advanced as possible. I had wanted this behavior because we have
the "fast-forwarding" that happens in a few places in that loop, and I
was afraid it would get quite confusing. The other thing I wanted was
the invariant that blk is always the i'th block when exiting the loop.
This was in case future code restructured the outer loop and added
other break conditions and then used blk.

However, I do hate duplicating the loop bounds checking in two places.
And future code that restructures the control flow of this function is
going to have to be extremely careful anyway. The combination of
multiple nested loops, "fast-forwarding" behavior, and skipping
forward X number of blocks with the read stream made the requirements
of this function's control flow quite complex.

v2:

        while (i < apw_state->prewarm_stop_idx)
        {
            ForkNumber    forknum;
            BlockNumber nblocks;
            struct AutoPrewarmReadStreamData p;
            ReadStream *stream;
            Buffer        buf;

            blk = block_info[i];

            /* Stop when we reach a different relation. */
            if (blk.tablespace != tablespace ||
                blk.filenumber != filenumber)
                break;

has the advantage that we set blk directly before using it instead of
at the end of the previous iteration, which people may find less
confusing.

So, overall, I am fine with either approach. Whatever other hackers
find more clear is the right approach. In my attempt to make it clear,
I obviously made it incorrect -- which is far worse :)

- Melanie

view thread (10+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Autoprewarm workers terminated due to a segmentation fault
  In-Reply-To: <CAAKRu_aekYazEexpT0KtjFaJOFD=Z=4Of8vwLrf+fsuJBSaBcg@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox