MIME-Version: 1.0
References: 
 <CAGXsc+ZhGjwm+F42Xmt8Qn1qP_h7woipiV0WsY-e-P7W3ZG2OA@mail.gmail.com>
 <20260605.080932.2120655380457259936.ishii@postgresql.org>
In-Reply-To: <20260605.080932.2120655380457259936.ishii@postgresql.org>
From: Emond Papegaaij <emond.papegaaij@gmail.com>
Date: Fri, 5 Jun 2026 13:49:32 +0200
Message-ID: 
 <CAGXsc+akuig0oA7dJX5BNFVRn+5miTALRZMnPrrt3kY7ypB+Ew@mail.gmail.com>
Subject: Re: Race condition in pcp_node_info can cause it to hang
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%2Bakuig0oA7dJX5BNFVRn%2B5miTALRZMnPrrt3kY7ypB%2BEw%40mail.gmail.com>
Precedence: bulk

Hi,

Thanks for the quick followup!

Best regards,
Emond

Op vr 5 jun 2026 om 01:09 schreef Tatsuo Ishii <ishii@postgresql.org>:
>
> Hi Emond,
>
> > Hi,
> >
> > We've hit another very rare flake in our tests, which can cause
> > pcp_node_info to hang indefinitely. I've analyzed the problem with
> > Claude Code, and it came to the conclusion and (quite small) fix
> > below. Attached is a patch against 4.7.
> >
> > The problem:
> > In inform_node_info() (src/pcp_con/pcp_worker.c), the PCP reply packet
> > reads bi->replication_state and bi->replication_sync_state directly
> > from shared memory twice: once via strlen() to compute the packet
> > length, and once via pcp_write() to write the payload.
> >
> > The streaming-replication check worker rewrites those same
> > shared-memory strings without a lock (it clears them to "" then
> > repopulates them every check cycle and on state transitions,
> > src/streaming_replication/pool_worker_child.c). If the string's length
> > changes between the two reads, the declared wsize no longer matches
> > the bytes actually written, so the PCP byte stream desynchronises. The
> > client then blocks forever in pcp_read() waiting for bytes the server
> > never sends.
> >
> > The fix:
> > Snapshot the two strings into local buffers once, right after bi =3D
> > pool_get_node_info(i),
> > and use the locals for both the length and the payload =E2=80=95 so a s=
ingle
> > packet is always
> > internally consistent. This matches how every other field in the
> > packet is already
> > handled.
>
> Thank you for the report and fix. Yes, I agree there's a race
> condition between sr checker process and pcp_node_info. I think
> introducing a lock to protect bi->replication_state and
> bi->replication_sync_state is overkill. The suggested fix seems to be
> a right direction.  Will push after current release freeze is over
> (supposed to be finished by the end of today).
>
> Regards,
> --
> Tatsuo Ishii
> SRA OSS K.K.
> English: http://www.sraoss.co.jp/index_en/
> Japanese:http://www.sraoss.co.jp