MIME-Version: 1.0
References: 
 <CAO+_mTQgQyTYwDh=U8iTnsDmOGyWsZJjUV31SmEYwmw6_xY6Bw@mail.gmail.com>
 <DJ4TOYZGE7M8.1U3BNIRMQVXQX@gmail.com>
 <f4c7fe76-6b98-4d92-a090-2ba8184b8997@vondra.me>
 <DJ4VE1MCA80G.1AG5OOFGIWCI0@gmail.com>
 <f6fa92fe-8408-465d-b775-1e0f10cd499a@vondra.me>
In-Reply-To: <f6fa92fe-8408-465d-b775-1e0f10cd499a@vondra.me>
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Mon, 15 Jun 2026 10:35:43 -0400
Message-ID: 
 <CAAKRu_aekYazEexpT0KtjFaJOFD=Z=4Of8vwLrf+fsuJBSaBcg@mail.gmail.com>
Subject: Re: Autoprewarm workers terminated due to a segmentation fault
To: Tomas Vondra <tomas@vondra.me>
Cc: Matheus Alcantara <matheusssilv97@gmail.com>,
 Glauber Batista <glauberrbatista@gmail.com>,
	pgsql-bugs@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAAKRu_aekYazEexpT0KtjFaJOFD%3DZ%3D4Of8vwLrf%2BfsuJBSaBcg%40mail.gmail.com>
Precedence: bulk

On Thu, Jun 11, 2026 at 10:40=E2=80=AFAM Tomas Vondra <tomas@vondra.me> wro=
te:
>
> After looking a bit closer, I think this bug was introduced by
>
>     commit 6acab8bdbcda735ef47b1bb0ba2284d6c465cd88
>     Author: Melanie Plageman <melanieplageman@gmail.com>
>     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 =3D 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 =3D p.pos;
if (i >=3D apw_state->prewarm_stop_idx)
    break;
blk =3D 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 =3D block_info[i];

            /* Stop when we reach a different relation. */
            if (blk.tablespace !=3D tablespace ||
                blk.filenumber !=3D 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