Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 27 Mar 2025 07:13:08 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "Maksim.Melnikov" <m.melnikov@postgrespro.ru>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: sync_standbys_defined read/write race on startup
Message-ID: <Z-R79EJVYfRy-EpW@paquier.xyz>
References: <163fcbec-900b-4b07-beaa-d2ead8634bec@postgrespro.ru>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="HRkZBi6QxhAtYwD3"
Content-Disposition: inline
In-Reply-To: <163fcbec-900b-4b07-beaa-d2ead8634bec@postgrespro.ru>
Archived-At: 
 <https://www.postgresql.org/message-id/Z-R79EJVYfRy-EpW%40paquier.xyz>
Precedence: bulk


--HRkZBi6QxhAtYwD3
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 26, 2025 at 05:17:17PM +0300, Maksim.Melnikov wrote:
> Test check visibility of prepared transactions in standby after a restart
> while primary is down.
> Failed assert check that changes, commited on transaction on primary node,
> were synchronously replicated to standby node.
> In test flow we have primary server stop/start events.
> Assert failure is hard to reproduce, but I found how to do it synthetical=
ly,
> can do it by injecting in code synthetical sleep call
>=20
> --- a/src/backend/replication/syncrep.c
> +++ b/src/backend/replication/syncrep.c
> @@ -924,6 +924,7 @@ SyncRepUpdateSyncStandbysDefined(void)
>=20
> =A0=A0=A0=A0=A0=A0=A0 if (sync_standbys_defined !=3D WalSndCtl->sync_stan=
dbys_defined)
> =A0=A0=A0=A0=A0=A0=A0 {
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 sleep(2);
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 LWLockAcquire(SyncRepLock, =
LW_EXCLUSIVE);
>=20
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 /*

Nice investigation to find out that this specific path faces a race
here.  Tests on Windows are usually slower and good at catching these.
It may be possible that the buildfarm has reported that, actually.

> So my question is
> can we accept such behavior as bug of postgres codebase, or, on other way,
> it is bug of unstable test?
> P.S.
> For me it is looking strange, that postgres start processing workload bef=
ore
> full config initializing.
> Attached possible patch for codebase.

Yeah, I agree that it is a bug, and your approach to be more careful
in SyncRepWaitForLSN() so as we wait until the configuration is loaded
and set from the checkpointer sounds like the sensible thing to do,
because the backends have no idea yet what they should do as the
configuration is not loaded.

Tracking if the configuration has been properly loaded in
WalSndCtlData makes sense here, but I am confused by the patch: you
have defined SYNCSTANDBYSDEFINED but sync_standbys_defined never sets
it.  It may be simpler to use a separate boolean flag for this
purpose.  WalSndCtlData is a private structure holding the state of
the WAL senders internally, so ABI does not stress me much here
(flexible array at the end of the structure, as well..).

Also mentioned by Kirill downthread: let's add a regression test with
an injection point that does a wait.  This race condition is worth
having a test for.  You could define the point of where you have added
your hardcoded sleep(), for example.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmfke/MACgkQnvQgOdby
QH3Gtg/+LTyEDFY495dBjafzpp3uNuQfnTCs4iFiqhtfEYW0URaqHirQ7FV1q6Dp
l+wGXoDuAm/YpYZQhBqJ4fOdFctLO0IxAk9xctCbMbmifmHD7DlJxclY2zneGOO/
wxBZ1hT1Wf6P1ZoGuRN2sdu59DDdNYalHqimVHWlzLh1mBCOsTxffTjvVrmGs6It
W6kOjSBH6nDxA7BquSsT56iFyoO1j+lhZBZ5YnNtKUpjNjKstYlnvFIQpzD/PsPJ
IK+lVszL0/qojW5KUEfYTAc5G+v1/lRRJY4NCyGs3Nqlh2HPLsfJTTOVvA/drdYP
EJB60zSbpoaiN9Lv/vBEP+tK477LUcDYpTPFJnc1kUSd/C6L9/vIt7GeNv0QKmXm
NpgQFOkg3V95xjJd4llBUqW/ieqNt51tV4iOfiuOnh5aMnX0wYkzo1Bi4fUAKTzg
1FeH28xaG4m/n9c6Xjky30e+X+pvdJVPh35d2Y9Hk4KpprxQQaIVtfglXXuJKw7S
Jt57yqYkpaX5B7Q2pp774x81p07UUPknJBdiP94mXJlTMuHRKVH4DgVZJh1Sz8p6
ohhVSabnnHQHTtH9+YQh6Tb7tkDsn50WDd6pkDwEwtrNmhmXNAejClHw/DGGnKg9
1cW8VtwESl9O5+JzsLOn9dosKHOVHg2hR78fikzN0d2kfWmh3dk=
=x9sA
-----END PGP SIGNATURE-----

--HRkZBi6QxhAtYwD3--