Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 8 May 2024 13:24:23 +0900
From: Michael Paquier <michael@paquier.xyz>
To: m.litsarev@postgrespro.ru
Cc: Tristan Partin <tristan@neon.tech>, pgsql-hackers@lists.postgresql.org
Subject: Re: SQL function which allows to distinguish a server being in point
 in time recovery mode and an ordinary replica
Message-ID: <Zjr-d9jSVz0Khw88@paquier.xyz>
References: <06c02ef916d85e77778e5cdfdf67a3df@postgrespro.ru>
 <D0L038YNGLYO.D2Q6GHBR7U40@neon.tech>
 <Zh26u-ifdXMPTrdq@paquier.xyz>
 <fac0db6695a7fefe33621a18f0484c49@postgrespro.ru>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="h+LEj+iq9ZRvyG3c"
Content-Disposition: inline
In-Reply-To: <fac0db6695a7fefe33621a18f0484c49@postgrespro.ru>
Archived-At: <https://www.postgresql.org/message-id/Zjr-d9jSVz0Khw88%40paquier.xyz>
Precedence: bulk


--h+LEj+iq9ZRvyG3c
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, May 06, 2024 at 06:55:46PM +0300, m.litsarev@postgrespro.ru wrote:
> as a first step I have introduced the `SharedRecoveryDataFlags` bitmask
> instead of three boolean SharedHotStandbyActive, SharedPromoteIsTriggered
> and SharedStandbyModeRequested flags (the last one from my previous patch)
> and made minimal updates in corresponding code based on that change.

Thanks for the patch.

 /*
- * Local copy of SharedHotStandbyActive variable. False actually means "not
+ * Local copy of XLR_HOT_STANDBY_ACTIVE flag. False actually means "not
  * known, need to check the shared state".
  */
 static bool LocalHotStandbyActive =3D false;
=20
 /*
- * Local copy of SharedPromoteIsTriggered variable. False actually means "=
not
+ * Local copy of XLR_PROMOTE_IS_TRIGGERED flag. False actually means "not
  * known, need to check the shared state".
  */
 static bool LocalPromoteIsTriggered =3D false;

It's a bit strange to have a bitwise set of flags in shmem while we
keep these local copies as booleans.  Perhaps it would be cleaner to
merge both local variables into a single bits32 store?

+   uint32      SharedRecoveryDataFlags;

I'd switch to bits32 for flags here.

+bool
+StandbyModeIsRequested(void)
+{
+	/*
+	 * Spinlock is not needed here because XLR_STANDBY_MODE_REQUESTED flag
+	 * can only be read after startup process is done.
+	 */
+	return (XLogRecoveryCtl->SharedRecoveryDataFlags & XLR_STANDBY_MODE_REQUE=
STED) !=3D 0;
+}

How about introducing a single wrapper function that returns the whole
value SharedRecoveryDataFlags, with the flags published in a header?
Sure, XLR_HOT_STANDBY_ACTIVE is not really exciting because being able
to query a standby implies it, but XLR_PROMOTE_IS_TRIGGERED could be
interesting?  Then this could be used with a function that returns a
text[] array with all the states retrieved?

The refactoring pieces and the function pieces should be split, for
clarity.
--
Michael

--h+LEj+iq9ZRvyG3c
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmY6/ncACgkQnvQgOdby
QH0Q9g//Sm66iYPHbsLEqGNa/0dfu7All9nt2VOwJjkfk3bDSrItMR30mMFcjWhO
5tpvFQu0FKSB7EnMR/2Di8ESlmPcqz+De2A7RpcZmTJL+Kiv5400Ws4+1hn/QJfe
PEUwVHrJ/4Eu1zWKZV3hDJr+bprq2dMoAoZLyeGTIZ+W1ipqVEIh7UTyuau7/rrl
42DoNENWpjgnxL4Na6sWHlJ9Vmt7Vc3XX++oj1Ua5vLIYcnGK4WXY6HCm9Z4kKwF
ghVWLj7yqsORJn0Fih0CC8uV7osDwYunND6CokkxJ+OL7nL2ewoZHKqZdXW59hmV
GtKmR+d36dZrnsuYNZ20FALH67GHO56AZY9MlqkDZ/7O2If3kBpa0+gZ8qTj5UPK
NXNrjgUHbgHHl0NHx001VWo0NGWM5GABflzbm0F4fzFuaOyATSQp6Cm2OfsXKlOj
87+zVghYrqlcZQZb2G+O1fhyOvdc5ile/VmmSnFbluNNFuQl8xWUcvobhjvw2Gr3
Eky1yykETASFyCEuBeKOIpUMuoEFxHZSf0xxBznCiT7ODq62BYD43a9PKGdft+Lw
G4D757M0jvMS58lQvj1EFy83TUbMgKYkwW/oC84mnGusBFqdQ/K0P7g0Bc+M3Gmw
4H6NpGt52rgo4cbU139eK8kpaU6lIudbQnnfTGATf36oMf5V508=
=j+WC
-----END PGP SIGNATURE-----

--h+LEj+iq9ZRvyG3c--