MIME-Version: 1.0
References: 
 <CAGXsc+Y7OKp93zSqt1WUCh7ABC7kyZqrxJPFA8AC1woB30S8EQ@mail.gmail.com>
 <20260702.163922.30147080599128209.ishii@postgresql.org>
In-Reply-To: <20260702.163922.30147080599128209.ishii@postgresql.org>
From: Emond Papegaaij <emond.papegaaij@gmail.com>
Date: Thu, 2 Jul 2026 11:21:13 +0200
Message-ID: 
 <CAGXsc+aizSg6Z22=pV8NsFb-FukzT2XP1YriOkOJjOBLVdN-vg@mail.gmail.com>
Subject: Re: [PATCH] Segfault in pool_do_auth() when failover races with a new
 connection
To: Tatsuo Ishii <ishii@postgresql.org>
Cc: pgpool-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAGXsc%2BaizSg6Z22%3DpV8NsFb-FukzT2XP1YriOkOJjOBLVdN-vg%40mail.gmail.com>
Precedence: bulk

Hi,

The new patch looks fine to me. There's however one thing Claude
pointed out. According to it, my_main_node_id,
private_backend_status[], and my_backend_status[] are per-process
globals (copied on fork), not shared memory. So, making any of those
volatile should not make any difference.

Best regards,
Emond

Op do 2 jul 2026 om 09:39 schreef Tatsuo Ishii <ishii@postgresql.org>:
>
> Hi Emond,
>
> > Hi,
> >
> > Last night one of CI builds detected a segmentation fault in pgpool
> > (4.7.2). I've ran an analysis on the coredump with Claude and came to
> > the conclusion that this can happen when trying to open a connection
> > in the middle of a failover.
> >
> > Symptom / backtrace (resolved from the core dump):
> > pool_do_auth (src/auth/pool_auth.c:349)   <-- SIGSEGV, fault addr 0x0
> > connect_backend (src/protocol/child.c:1099)  (PG_TRY block)
> > get_backend_connection (src/protocol/child.c:2109)
> > do_child (src/protocol/child.c:233)
> >
> > The crashing statement is:
> > protoMajor =3D MAIN_CONNECTION(cp)->sp->major;   /* pool_auth.c:349 */
> > i.e. cp->slots[MAIN_NODE_ID] is NULL. From the core, the pool had
> > slots[0] =3D=3D NULL while slots[1]/slots[2] were valid, and MAIN_NODE_=
ID =3D=3D 0.
> >
> > pool_initialize_private_backend_status() snapshots
> > private_backend_status[] and my_main_node_id non=E2=80=91atomically w.r=
.t. the
> > parent's failover, so a child can capture my_main_node_id =3D=3D 0 whil=
e
> > node 0 is already CON_DOWN. new_connection() then never creates
> > slots[0], and pool_do_auth() dereferences the NULL main slot. (Same
> > class as the NULL=E2=80=91MAIN_CONNECTION case noted in
> > pool_virtual_main_db_node_id().)
>
> From pool_initialize_private_backend_status():
>
>         for (i =3D 0; i < MAX_NUM_BACKENDS; i++)
>         {
>                 private_backend_status[i] =3D BACKEND_INFO(i).backend_sta=
tus;
>                 /* my_backend_status is referred to by VALID_BACKEND macr=
o. */
>                 my_backend_status[i] =3D &private_backend_status[i];
>         }
>
>         my_main_node_id =3D REAL_MAIN_NODE_ID;
>
> BACKEND_INFO(i).backend_status are updated in the pgpool main process
> at line 1793 of failover(pgpool_main.c) by calling
> handle_failover_request(), but REAL_MAIN_NODE_ID (actually
> Req_info->main_node_id, shared memory) is updated at 1846 of
> failover() by calling save_node_info(). So updating REAL_MAIN_NODE_ID
> tends to slightly delay after updating
> BACKEND_INFO(i).backend_status. This may explain why my_main_node is
> set the stale data (1).
>
> Another possibility is my_main_node_id being lacking volatile
> qualification. This also could set stale data to my_main_node_id (2).
>
> Of course (1) and (2) are not mutually exclusive. So your patch for
> (1) and fix for (2) need to be both applied.
>
> By the way in your patch there is loops like this. This can be
> replaced by existing get_next_main_node().
>
> +               for (i =3D 0; i < NUM_BACKENDS; i++)
> +               {
> +                       if (private_backend_status[i] =3D=3D CON_UP ||
> +                               private_backend_status[i] =3D=3D CON_CONN=
ECT_WAIT)
> +                       {
> +                               my_main_node_id =3D i;
> +                               break;
> +                       }
> +               }
>
> All of the above in mind, I propose attached patch.
>
> What do you think?
> --
> Tatsuo Ishii
> SRA OSS K.K.
> English: http://www.sraoss.co.jp/index_en/
> Japanese:http://www.sraoss.co.jp