Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 25 Sep 2023 12:29:54 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Thomas Munro <thomas.munro@gmail.com>,
	Alexander Lakhin <exclusion@gmail.com>,
	Sergei Kornilov <sk@zsrv.org>, Noah Misch <noah@leadboat.com>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	pgsql-bugs@lists.postgresql.org,
	Fujii Masao <masao.fujii@oss.nttdata.com>, pgbf@twiska.com
Subject: Re: BUG #17928: Standby fails to decode WAL on termination of primary
Message-ID: <ZRD-smlSMRiEEzEZ@paquier.xyz>
References: 
 <CA+hUKGK1+3rj3c7L9HPCc8HajTVmkTx1CEkU5=rqgV3b4OvWeQ@mail.gmail.com>
 <ZQ4r1vHcryBsSi_V@paquier.xyz>
 <CA+hUKGKs=yj55ejAwuUJQuURU=uKjYgRqEQcCi1GrP2+=jUgNQ@mail.gmail.com>
 <ZQ5tIoarrcwAlbVZ@paquier.xyz>
 <CA+hUKGJf3Hhb2MB88-rW2di2H9XT0xr6-hd6ZjGEwdJs3A=b+Q@mail.gmail.com>
 <ZQ9zf1QO8CP4TZRO@paquier.xyz>
 <CA+hUKGKo5O_f8aBDoEMkxHmuWdFfmPPo6wL4q85b_0PDuHwZEA@mail.gmail.com>
 <ZRDNNf8Etlvuo48a@paquier.xyz>
 <CA+hUKG+cXwfAk0dEbD5CZ76p1uADGywuvb6_N4Uhziek54FZHg@mail.gmail.com>
 <64446.1695611228@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="//WpZs9mVBWm3pyp"
Content-Disposition: inline
In-Reply-To: <64446.1695611228@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/ZRD-smlSMRiEEzEZ%40paquier.xyz>
Precedence: bulk


--//WpZs9mVBWm3pyp
Content-Type: multipart/mixed; boundary="1NcO58BoXxNivBB1"
Content-Disposition: inline


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

On Sun, Sep 24, 2023 at 11:07:08PM -0400, Tom Lane wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
>> On Mon, Sep 25, 2023 at 12:58=E2=80=AFPM Michael Paquier <michael@paquie=
r.xyz> wrote:
>>> A bunch of architectures that are not Intel are failing.  Here is a
>>> summary based on the buildfarm reports:
>>> mamba, macppc with gcc 10.4.0
>=20
> That one is mine, so let me know if there's something particular
> you'd like me to check.

That would help, thanks!  Could it be possible to check if the
attached patch applied on HEAD influences the fate of the recovery
test 009_twophase.pl?  That's one thing I am really doubting about,
and we've not done much since REL_16_STABLE has been forked..

(I'm still trying to get an environment where this can be reproduced,
but qemu takes time.)
--
Michael

--1NcO58BoXxNivBB1
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-Revert-Optimize-WAL-insertion-lock-acquisition-and-r.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 3adaa49be439ba61f58f1ee8c1d2041765c247e7 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Mon, 25 Sep 2023 12:24:50 +0900
Subject: [PATCH] Revert "Optimize WAL insertion lock acquisition and release
 with some atomics"

This reverts commit 71e4cc6b8ec6a08f81973bd387fe575134cd0bdf.
---
 src/include/storage/lwlock.h      |  6 ++--
 src/backend/access/transam/xlog.c |  4 +--
 src/backend/storage/lmgr/lwlock.c | 46 +++++++++++++++++--------------
 3 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h
index d77410bdea..34169e5889 100644
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -129,14 +129,14 @@ extern bool LWLockAcquire(LWLock *lock, LWLockMode mo=
de);
 extern bool LWLockConditionalAcquire(LWLock *lock, LWLockMode mode);
 extern bool LWLockAcquireOrWait(LWLock *lock, LWLockMode mode);
 extern void LWLockRelease(LWLock *lock);
-extern void LWLockReleaseClearVar(LWLock *lock, pg_atomic_uint64 *valptr, =
uint64 val);
+extern void LWLockReleaseClearVar(LWLock *lock, uint64 *valptr, uint64 val=
);
 extern void LWLockReleaseAll(void);
 extern bool LWLockHeldByMe(LWLock *lock);
 extern bool LWLockAnyHeldByMe(LWLock *lock, int nlocks, size_t stride);
 extern bool LWLockHeldByMeInMode(LWLock *lock, LWLockMode mode);
=20
-extern bool LWLockWaitForVar(LWLock *lock, pg_atomic_uint64 *valptr, uint6=
4 oldval, uint64 *newval);
-extern void LWLockUpdateVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64=
 val);
+extern bool LWLockWaitForVar(LWLock *lock, uint64 *valptr, uint64 oldval, =
uint64 *newval);
+extern void LWLockUpdateVar(LWLock *lock, uint64 *valptr, uint64 val);
=20
 extern Size LWLockShmemSize(void);
 extern void CreateLWLocks(void);
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam=
/xlog.c
index fcbde10529..6227230ae1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -376,7 +376,7 @@ typedef struct XLogwrtResult
 typedef struct
 {
 	LWLock		lock;
-	pg_atomic_uint64 insertingAt;
+	XLogRecPtr	insertingAt;
 	XLogRecPtr	lastImportantAt;
 } WALInsertLock;
=20
@@ -4634,7 +4634,7 @@ XLOGShmemInit(void)
 	for (i =3D 0; i < NUM_XLOGINSERT_LOCKS; i++)
 	{
 		LWLockInitialize(&WALInsertLocks[i].l.lock, LWTRANCHE_WAL_INSERT);
-		pg_atomic_init_u64(&WALInsertLocks[i].l.insertingAt, InvalidXLogRecPtr);
+		WALInsertLocks[i].l.insertingAt =3D InvalidXLogRecPtr;
 		WALInsertLocks[i].l.lastImportantAt =3D InvalidXLogRecPtr;
 	}
=20
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/l=
wlock.c
index 315a78cda9..752a5e7359 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -1547,8 +1547,9 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
  * *result is set to true if the lock was free, and false otherwise.
  */
 static bool
-LWLockConflictsWithVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 oldv=
al,
-					   uint64 *newval, bool *result)
+LWLockConflictsWithVar(LWLock *lock,
+					   uint64 *valptr, uint64 oldval, uint64 *newval,
+					   bool *result)
 {
 	bool		mustwait;
 	uint64		value;
@@ -1572,10 +1573,13 @@ LWLockConflictsWithVar(LWLock *lock, pg_atomic_uint=
64 *valptr, uint64 oldval,
 	*result =3D false;
=20
 	/*
-	 * Reading this value atomically is safe even on platforms where uint64
-	 * cannot be read without observing a torn value.
+	 * Read value using the lwlock's wait list lock, as we can't generally
+	 * rely on atomic 64 bit reads/stores.  TODO: On platforms with a way to
+	 * do atomic 64 bit reads/writes the spinlock should be optimized away.
 	 */
-	value =3D pg_atomic_read_u64(valptr);
+	LWLockWaitListLock(lock);
+	value =3D *valptr;
+	LWLockWaitListUnlock(lock);
=20
 	if (value !=3D oldval)
 	{
@@ -1608,8 +1612,7 @@ LWLockConflictsWithVar(LWLock *lock, pg_atomic_uint64=
 *valptr, uint64 oldval,
  * an implied barrier via spinlock or LWLock to avoid memory ordering issu=
es.
  */
 bool
-LWLockWaitForVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 oldval,
-				 uint64 *newval)
+LWLockWaitForVar(LWLock *lock, uint64 *valptr, uint64 oldval, uint64 *newv=
al)
 {
 	PGPROC	   *proc =3D MyProc;
 	int			extraWaits =3D 0;
@@ -1737,32 +1740,29 @@ LWLockWaitForVar(LWLock *lock, pg_atomic_uint64 *va=
lptr, uint64 oldval,
  * LWLockUpdateVar - Update a variable and wake up waiters atomically
  *
  * Sets *valptr to 'val', and wakes up all processes waiting for us with
- * LWLockWaitForVar().  It first sets the value atomically and then wakes =
up
- * waiting processes so that any process calling LWLockWaitForVar() on the=
 same
- * lock is guaranteed to see the new value, and act accordingly.
+ * LWLockWaitForVar().  Setting the value and waking up the processes happ=
en
+ * atomically so that any process calling LWLockWaitForVar() on the same l=
ock
+ * is guaranteed to see the new value, and act accordingly.
  *
  * The caller must be holding the lock in exclusive mode.
  */
 void
-LWLockUpdateVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 val)
+LWLockUpdateVar(LWLock *lock, uint64 *valptr, uint64 val)
 {
 	proclist_head wakeup;
 	proclist_mutable_iter iter;
=20
 	PRINT_LWDEBUG("LWLockUpdateVar", lock, LW_EXCLUSIVE);
=20
-	/*
-	 * Note that pg_atomic_exchange_u64 is a full barrier, so we're guaranteed
-	 * that the variable is updated before waking up waiters.
-	 */
-	pg_atomic_exchange_u64(valptr, val);
-
 	proclist_init(&wakeup);
=20
 	LWLockWaitListLock(lock);
=20
 	Assert(pg_atomic_read_u32(&lock->state) & LW_VAL_EXCLUSIVE);
=20
+	/* Update the lock's value */
+	*valptr =3D val;
+
 	/*
 	 * See if there are any LW_WAIT_UNTIL_FREE waiters that need to be woken
 	 * up. They are always in the front of the queue.
@@ -1878,13 +1878,17 @@ LWLockRelease(LWLock *lock)
  * LWLockReleaseClearVar - release a previously acquired lock, reset varia=
ble
  */
 void
-LWLockReleaseClearVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 val)
+LWLockReleaseClearVar(LWLock *lock, uint64 *valptr, uint64 val)
 {
+	LWLockWaitListLock(lock);
+
 	/*
-	 * Note that pg_atomic_exchange_u64 is a full barrier, so we're guaranteed
-	 * that the variable is updated before releasing the lock.
+	 * Set the variable's value before releasing the lock, that prevents race
+	 * a race condition wherein a new locker acquires the lock, but hasn't yet
+	 * set the variables value.
 	 */
-	pg_atomic_exchange_u64(valptr, val);
+	*valptr =3D val;
+	LWLockWaitListUnlock(lock);
=20
 	LWLockRelease(lock);
 }
--=20
2.40.1


--1NcO58BoXxNivBB1--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmUQ/rIACgkQnvQgOdby
QH0bJQ/7BQxDRi/cq4cUwWyyblgIDeGGMzNz9yHYI8SI11jH6r/qSo9FoynRxfRR
AeNC9kk69djEwRbZxVMMS7uerWVFdSfnqDo7mYN00DGYAJaVWViFB0ck4Qkwzl9z
tvFg/qnB9ajT5Kwu+14K1WHJGfiYWIX4bsawF6awi2NSBFBcjz1MoRsizUWADAcv
LYJDvUk25K/G/BapRCgQHMqDouxnJ2FQK5Ue8bwW4j/d8nW7GTy5A5DkeHCdI5d5
3TLNgbqcZD8IbLtIex0F1suhZOoTN56ZCL2n3bA3hPw3WVgoYh/4MvN+NYe+SvWA
mff7zj+zpFdx8PbmJsUtvLchtSACdlMw5ED8dbSIX9Z52AXn3RNCHMHNYUh/q9co
lAC/EPLgAMITf9BjpC5cgJD/9YLWfMQQD/O2DNJD7BKpcqJdvupPt+skTEnHm6MV
LYsUjmKZW8zUWPHTYVVq7zbI0YUy4M0vu+k0TFQ5/cSBJrxwB1wOGTwggrzvBEuN
nk8hfQp+qYDRe6hBx9vwGVNvD0XVxATPcHDmZcm1KD8Vr/vx2GAl8YBW+pU1C8eu
aI+l2kJXukLSByxAYsoJ+PKpYMudjrVJQCdvKleiR9dl+6wIGyHZ38DAijhbocaw
tF8OCJErWiic0j6nmFyAsoarmvcMFpQZSxepbkqOXGP/Uut8zTg=
=nhsX
-----END PGP SIGNATURE-----

--//WpZs9mVBWm3pyp--