From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: Alexander Lakhin <exclusion@gmail.com>
Cc: Sergei Kornilov <sk@zsrv.org>
Cc: Noah Misch <noah@leadboat.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-bugs@lists.postgresql.org, Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: pgbf@twiska.com
Subject: Re: BUG #17928: Standby fails to decode WAL on termination of primary
Date: Mon, 25 Sep 2023 12:29:54 +0900
Message-ID: <ZRD-smlSMRiEEzEZ@paquier.xyz> (raw)
In-Reply-To: <64446.1695611228@sss.pgh.pa.us>
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>
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 PM Michael Paquier <michael@paquier.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
>
> 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
Attachments:
[text/x-diff] 0001-Revert-Optimize-WAL-insertion-lock-acquisition-and-r.patch (6.3K, ../ZRD-smlSMRiEEzEZ@paquier.xyz/2-0001-Revert-Optimize-WAL-insertion-lock-acquisition-and-r.patch)
download | inline diff:
From 3adaa49be439ba61f58f1ee8c1d2041765c247e7 Mon Sep 17 00:00:00 2001
From: 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.hindex 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 mode);
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);
-extern bool LWLockWaitForVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 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);
extern Size LWLockShmemSize(void);
extern void CreateLWLocks(void);
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.cindex 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;
@@ -4634,7 +4634,7 @@ XLOGShmemInit(void)
for (i = 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 = InvalidXLogRecPtr;
WALInsertLocks[i].l.lastImportantAt = InvalidXLogRecPtr;
}
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.cindex 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 oldval,- 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_uint64 *valptr, uint64 oldval,
*result = false;
/*
- * 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 = pg_atomic_read_u64(valptr);+ LWLockWaitListLock(lock);+ value = *valptr;+ LWLockWaitListUnlock(lock);
if (value != 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 issues.
*/
bool
-LWLockWaitForVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 oldval,- uint64 *newval)+LWLockWaitForVar(LWLock *lock, uint64 *valptr, uint64 oldval, uint64 *newval)
{
PGPROC *proc = MyProc;
int extraWaits = 0;
@@ -1737,32 +1740,29 @@ LWLockWaitForVar(LWLock *lock, pg_atomic_uint64 *valptr, 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 happen+ * atomically so that any process calling LWLockWaitForVar() on the same lock+ * 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;
PRINT_LWDEBUG("LWLockUpdateVar", lock, LW_EXCLUSIVE);
- /*- * 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);
LWLockWaitListLock(lock);
Assert(pg_atomic_read_u32(&lock->state) & LW_VAL_EXCLUSIVE);
+ /* Update the lock's value */+ *valptr = 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 variable
*/
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 = val;+ LWLockWaitListUnlock(lock);
LWLockRelease(lock);
}
--
2.40.1
[application/pgp-signature] signature.asc (832B, ../ZRD-smlSMRiEEzEZ@paquier.xyz/3-signature.asc)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: michael@paquier.xyz, tgl@sss.pgh.pa.us, thomas.munro@gmail.com, exclusion@gmail.com, sk@zsrv.org, noah@leadboat.com, horikyota.ntt@gmail.com, masao.fujii@oss.nttdata.com, pgbf@twiska.com
Subject: Re: BUG #17928: Standby fails to decode WAL on termination of primary
In-Reply-To: <ZRD-smlSMRiEEzEZ@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox