Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 30 Oct 2025 08:36:43 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Xuneng Zhou <xunengzhou@gmail.com>
Cc: Noah Misch <noah@leadboat.com>, ryanzxg@gmail.com,
	pgsql-bugs@lists.postgresql.org, Euler Taveira <euler@eulerto.com>
Subject: Re: BUG #19093: Behavioral change in walreceiver termination between
 PostgreSQL 14.17 and 14.18
Message-ID: <aQKlC1v2_MXGV6_9@paquier.xyz>
References: <19093-c4fff49a608f82a0@postgresql.org>
 <CABPTF7XKZV2d4eWhH0ybfv5J0hHyzPxPDmebzQXsSt-8XvO1sA@mail.gmail.com>
 <20251027041241.ab.nmisch@google.com>
 <aQGfoKGgmAbPATp5@paquier.xyz>
 <CABPTF7VsyWXAQyc-qc8i1OkwsP5hWE6WoGVCH1zbiUhX0=rFCw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="+KHcMK6jdnK3hobd"
Content-Disposition: inline
In-Reply-To: 
 <CABPTF7VsyWXAQyc-qc8i1OkwsP5hWE6WoGVCH1zbiUhX0=rFCw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/aQKlC1v2_MXGV6_9%40paquier.xyz>
Precedence: bulk


--+KHcMK6jdnK3hobd
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Oct 29, 2025 at 02:36:10PM +0800, Xuneng Zhou wrote:
> Should we wrap this assertion within #ifdef USE_ASSERT_CHECKING?
>=20
> #ifdef USE_ASSERT_CHECKING
> {
> WalRcvData *walrcv =3D WalRcv;
> WalRcvState state;
> SpinLockAcquire(&walrcv->mutex);
> state =3D walrcv->walRcvState;
> SpinLockRelease(&walrcv->mutex);
> Assert(state !=3D WALRCV_STOPPING);
> }
> #endif

Yep.  Or provide a wrapper routine that's able to retrieve the state
of a WAL receiver, then add it in xlogrecovery.c within an Assert().
It may be better to only do that on HEAD, the back-branches stress me
a bit when we enforce such new assumptions..  Perhaps Noah has a
different opinion.

> However, I think Michael=E2=80=99s approach is better =E2=80=94 it=E2=80=
=99s simpler, more
> straightforward, and captures the buggy behavior well. I plan to
> incorporate it into the next version of the patch.

Thanks!

One weakness of this approach is that this test would not be able to
work if the error message is reworded for some reason.  On stable
branches, I doubt that this error message will ever change, but it
could be updated to something else on HEAD.

Hence, I would suggest one extra improvement for HEAD: let's also add
a check for a valid case where a WAL receiver is stopped.  We have one
in 040_standby_failover_slots_sync.pl, where primary_conninfo is
updated in standby1 and its configuration reloaded, so I'd be tempted
to add a valid check in the server logs of $standby1.  One difference
is that we should grab the offset of the logs before the reload, then
make sure that the log entry gets created with a wait_for_log(), as
there may be a delay between the reload and the moment the log entry
is generated.
--
Michael

--+KHcMK6jdnK3hobd
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmkCpQsACgkQnvQgOdby
QH18Nw/8CmMM6Lp+TdJ25JY+uk0y7sl/3xh/GDnjB+pZl2I9FTzK1H8X1b/7a0Ei
ilvkHuaUforXNrHS1EKq3rzLoWyh/udqiP5ods4pX6ZljC1rviZ5jXe+FyGQaixN
+1cmrOihT8prDGjtTlxg/yjMGaXzCs/suRRWm4pJPi3VLWvb+EUP6E9IZ424oFz3
vp3Bf9rocgfdSZeZxXBivc19gziGnOdyDSTgHmFL8XOwF+FF5Utf8CT2IqSHR4Ka
7tP3zb++WiAebyZoa+GfWUv/9XnLY4hH4cdvN3sWOjmEC+OMf7zOxGfOba1D6qs+
tHbrtbLjA2k6XFTTiHOxJC5XkpL/bID/V57dguBPkOYG76oqVdS+tDWTajc6Now5
6ALFLxV+j6uondgjskSX0CP8UOODQ2VeKIo0pfWJectFItESUXORe7TgjNIvgKjw
w8viK5SebfPp2MrSDFYogw6b9KtHstL4i2nkmPCdD5IYN7EHvz1QCK8K9BQd2WVw
IP0MDqnaM9QuTjwNmbMXDjTQqOGbDQYaOr2N85PDsWNpdZrBBU5hlsScHYhoadXL
012slLmjeH4uQDylQyew1G2RygGYBlGCuvA2/DnFGvHUSx9EayI0gEGY4LCyOUgA
PNSLeeyrkeBzULbc08r9yrPd/t97YGtaALxfzfvyND03Iw+OBuI=
=lJjs
-----END PGP SIGNATURE-----

--+KHcMK6jdnK3hobd--