Sorry for missed patches. Attached now.
On 8/31/26 19:18, Vitaly Davydov wrote:
Hi JH Shin,
> I think PinCountWaiterCheckReadyForCleanup() has a window between
> checking the refcount and setting BM_PIN_COUNT_WAITER. On the master
> branch, UnpinBufferNoOwner() drops pins without taking the header
> lock, so the last other pin can go away in that window and the
> wakeup is lost until the deadlock or standby-limit timeout fires.
> This looks like the same race that commit 8d85cb889a3 fixed in
> LockBufferForCleanup(), and the same fix should work here. After
> publishing the flag, recheck the refcount, and if only our own pin
> remains, clear the flag and return true.
Agree, thank you.
I made new changes in two commits. In the first commit coming from v7
patch
I modified RegisterPinCountWaiter() function to recheck the refcount as
explained by you. In the second commit I've replaced the duplicate
code in
LockBufferForCleanup to register pincount waiter with the call of
RegisterPinCountWaiter(). The second commit is optional but it fits
well with
the current changes.
The patch was rebased to the actual master branch.
P.S. I replaced my old address [email protected] in commits with
my personal email [email protected] that suits better for me.
With best regards,
Vitaly Davydov
[email protected] (aka [email protected])
[email protected]
From 167415c2bc7f2792e2a2357985c2007f1d2c4268 Mon Sep 17 00:00:00 2001
From: Vitaly Davydov <[email protected]>
Date: Mon, 31 Aug 2026 18:38:15 +0300
Subject: [PATCH v8 1/2] Fix recovery-conflict wait loop for buffer pins on hot
standby
When the startup process waits on a buffer pin during recovery, the
buffer-pin wait code treats any latch wakeup as a relevant event,
resetting all timeouts and retrying. On hot standby, periodic SIGALRM
signals from unrelated timers (such as progress reporting) can arrive
before the deadlock timeout expires, causing the wait loop to reset
indefinitely without ever triggering the deadlock detector. This results
in an infinite loop in LockBufferForCleanup and stalls recovery
indefinitely, particularly when max_standby_streaming_delay = -1.
---
src/backend/storage/buffer/bufmgr.c | 106 ++++++++++++++++++++++++++++
src/backend/storage/ipc/standby.c | 84 +++++++++++++++-------
src/include/storage/bufmgr.h | 1 +
3 files changed, 165 insertions(+), 26 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 17f142e4c5b..686265c3d48 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3469,6 +3469,54 @@ WakePinCountWaiter(BufferDesc *buf)
UnlockBufHdr(buf);
}
+/*
+ * Register the current process as the pincount waiter for a shared buffer
+ * and unlock the buffer header. Return true if the process was registered as
+ * the pincount waiter and must now wait to be signaled, or false if the
+ * shared refcount was concurrently reduced to 1 (only our own pin remains),
+ * in which case no wait is necessary.
+ *
+ * The caller must hold the buffer header lock, pass the current buffer state
+ * returned by LockBufHdr(), and ensure that no other backend is already
+ * registered as the waiter.
+ */
+static bool
+RegisterPinCountWaiter(BufferDesc *bufHdr, uint64 buf_state)
+{
+ Assert((buf_state & BM_PIN_COUNT_WAITER) == 0 ||
+ bufHdr->wait_backend_pgprocno == MyProcNumber);
+
+ bufHdr->wait_backend_pgprocno = MyProcNumber;
+ PinCountWaitBuf = bufHdr;
+
+ /*
+ * Publish BM_PIN_COUNT_WAITER while retaining the buffer header lock.
+ * The shared refcount can be decremented while BM_LOCKED is set, so
+ * use an atomic operation that preserves concurrent refcount changes.
+ */
+ pg_atomic_fetch_or_u64(&bufHdr->state, BM_PIN_COUNT_WAITER);
+
+ /*
+ * Recheck the refcount after publishing the waiter flag, while shared
+ * refcount increments are still prevented by BM_LOCKED. If only our
+ * pin remains, the cleanup-lock condition has already been satisfied,
+ * so remove the waiter state and return without sleeping.
+ */
+ buf_state = pg_atomic_read_u64(&bufHdr->state);
+ if (BUF_STATE_GET_REFCOUNT(buf_state) == 1)
+ {
+ UnlockBufHdrExt(bufHdr, buf_state,
+ 0, BM_PIN_COUNT_WAITER,
+ 0);
+ PinCountWaitBuf = NULL;
+ return false;
+ }
+
+ UnlockBufHdr(bufHdr);
+
+ return true;
+}
+
/*
* UnpinBuffer -- make buffer available for replacement.
*
@@ -4763,6 +4811,64 @@ BufferGetLSNAtomic(Buffer buffer)
#endif
}
+/*
+ * PinCountWaiterCheckReadyForCleanup
+ * Recheck whether pin count waiter process (the startup process)
+ * can retry cleanup lock acquisition.
+ *
+ * This is only for the hot-standby path in LockBufferForCleanup(), via
+ * ResolveRecoveryConflictWithBufferPin(), after ProcWaitForSignal() returns.
+ * The caller must already be registered as the shared buffer's
+ * BM_PIN_COUNT_WAITER.
+ *
+ * Returns true when the caller itself is the only remaining pin holder, so it
+ * can retry taking the cleanup lock. Returns false if other backends still
+ * pin the shared buffer. In that case, this function guarantees that the
+ * current backend remains registered as the pincount waiter to be woken when
+ * the buffer refcount drops to 1.
+ */
+bool
+PinCountWaiterCheckReadyForCleanup(Buffer buffer)
+{
+ BufferDesc *bufHdr;
+ uint64 buf_state;
+ uint32 buf_refcount;
+
+ Assert(BufferIsValid(buffer));
+ Assert(!BufferIsLocal(buffer));
+
+ bufHdr = GetBufferDescriptor(buffer - 1);
+ Assert(PinCountWaitBuf == bufHdr);
+
+ buf_state = LockBufHdr(bufHdr);
+ buf_refcount = BUF_STATE_GET_REFCOUNT(buf_state);
+
+ if (buf_refcount == 1)
+ {
+ UnlockBufHdr(bufHdr);
+ return true;
+ }
+
+ if ((buf_state & BM_PIN_COUNT_WAITER) != 0 &&
+ bufHdr->wait_backend_pgprocno != MyProcNumber)
+ {
+ UnlockBufHdr(bufHdr);
+ elog(ERROR, "multiple processes attempting to wait for pincount 1");
+ }
+
+ /*
+ * If other processes still pin the buffer, register this process again as
+ * the pincount waiter to wait again. The refcount may be concurrently
+ * reduced to 1 despite our holding the buffer header lock, in which case
+ * RegisterPinCountWaiter() returns false and the buffer is ready for
+ * cleanup.
+ */
+ if (!RegisterPinCountWaiter(bufHdr, buf_state))
+ return true;
+
+ return false;
+}
+
/* ---------------------------------------------------------------------
* DropRelationBuffers
*
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 7f011e04990..044284b7ba5 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -790,22 +790,36 @@ cleanup:
* Deadlocks are extremely rare, and relatively expensive to check for,
* so we don't do a deadlock check right away ... only if we have had to wait
* at least deadlock_timeout.
+ *
+ * The current process should be the waiter process and should have
+ * published the waited buffer via SetStartupBufferPinWaitBufId().
*/
void
ResolveRecoveryConflictWithBufferPin(void)
{
TimestampTz ltime;
+ int bufid;
Assert(InHotStandby);
+ bufid = GetStartupBufferPinWaitBufId();
+ Assert(bufid >= 0);
+
ltime = GetStandbyLimitTime();
- if (GetCurrentTimestamp() >= ltime && ltime != 0)
+ if (ltime != 0 && GetCurrentTimestamp() >= ltime)
{
/*
* We're already behind, so clear a path as quickly as possible.
*/
SendRecoveryConflictWithBufferPin(RECOVERY_CONFLICT_BUFFERPIN);
+
+ /*
+ * Set delay timeout flag once the timeout is reached (the current
+ * timestamp is greater than the standby limit time). This variable is
+ * use by timeout handlers with the same purpose.
+ */
+ got_standby_delay_timeout = true;
}
else
{
@@ -833,35 +847,53 @@ ResolveRecoveryConflictWithBufferPin(void)
enable_timeouts(timeouts, cnt);
}
- /*
- * Wait to be signaled by UnpinBuffer() or for the wait to be interrupted
- * by one of the timeouts established above.
- *
- * We assume that only UnpinBuffer() and the timeout requests established
- * above can wake us up here. WakeupRecovery() called by walreceiver or
- * SIGHUP signal handler, etc cannot do that because it uses the different
- * latch from that ProcWaitForSignal() waits on.
- */
- ProcWaitForSignal(WAIT_EVENT_BUFFER_CLEANUP);
-
- if (got_standby_delay_timeout)
- SendRecoveryConflictWithBufferPin(RECOVERY_CONFLICT_BUFFERPIN);
- else if (got_standby_deadlock_timeout)
+ for (;;)
{
/*
- * Send out a request for hot-standby backends to check themselves for
- * deadlocks.
+ * Wait to be signaled by UnpinBuffer() or for the wait to be
+ * interrupted by one of the timeouts established above.
*
- * XXX The subsequent ResolveRecoveryConflictWithBufferPin() will wait
- * to be signaled by UnpinBuffer() again and send a request for
- * deadlocks check if deadlock_timeout happens. This causes the
- * request to continue to be sent every deadlock_timeout until the
- * buffer is unpinned or ltime is reached. This would increase the
- * workload in the startup process and backends. In practice it may
- * not be so harmful because the period that the buffer is kept pinned
- * is basically no so long. But we should fix this?
+ * ProcWaitForSignal() can also wake up for unrelated reasons, so
+ * recheck later whether cleanup can proceed.
*/
- SendRecoveryConflictWithBufferPin(RECOVERY_CONFLICT_BUFFERPIN_DEADLOCK);
+ ProcWaitForSignal(WAIT_EVENT_BUFFER_CLEANUP);
+
+ /*
+ * Once the reference count is 1, the waiter process itself is the
+ * only backend pinning the buffer at the moment. There is a chance to
+ * lock the buffer exclusively.
+ */
+ if (PinCountWaiterCheckReadyForCleanup(bufid + 1))
+ break;
+
+ /*
+ * Send the recovery conflict if the standby delay timeout is activated or
+ * standby limit time was already reached. The second condition handles the
+ * fast path when timeouts are not activated.
+ */
+ if (got_standby_delay_timeout)
+ {
+ SendRecoveryConflictWithBufferPin(RECOVERY_CONFLICT_BUFFERPIN);
+ break;
+ }
+ else if (got_standby_deadlock_timeout)
+ {
+ /*
+ * Send out a request for hot-standby backends to check themselves
+ * for deadlocks.
+ *
+ * XXX The subsequent ResolveRecoveryConflictWithBufferPin() will
+ * wait to be signaled by UnpinBuffer() again and send a request
+ * for deadlocks check if deadlock_timeout happens. This causes
+ * the request to continue to be sent every deadlock_timeout until
+ * the buffer is unpinned or ltime is reached. This would increase
+ * the workload in the startup process and backends. In practice
+ * it may not be so harmful because the period that the buffer is
+ * kept pinned is basically no so long. But we should fix this?
+ */
+ SendRecoveryConflictWithBufferPin(RECOVERY_CONFLICT_BUFFERPIN_DEADLOCK);
+ break;
+ }
}
/*
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 6837b35fc6d..cf8ef54aedc 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -313,6 +313,7 @@ extern bool BufferIsPermanent(Buffer buffer);
extern XLogRecPtr BufferGetLSNAtomic(Buffer buffer);
extern void BufferGetTag(Buffer buffer, RelFileLocator *rlocator,
ForkNumber *forknum, BlockNumber *blknum);
+extern bool PinCountWaiterCheckReadyForCleanup(Buffer buffer);
extern void MarkBufferDirtyHint(Buffer buffer, bool buffer_std);
--
2.43.0
From fe0e4c340101489c062cad43f59d7b3a01407718 Mon Sep 17 00:00:00 2001
From: Vitaly Davydov <[email protected]>
Date: Mon, 31 Aug 2026 18:35:41 +0300
Subject: [PATCH v8 2/2] Use RegisterPinCountWaiter() in LockBufferForCleanup()
Replace the duplicated pincount-waiter registration logic in
LockBufferForCleanup() with a call to RegisterPinCountWaiter(), which
already encapsulates the same protocol for publishing
BM_PIN_COUNT_WAITER, rechecking the refcount, and returning false when
only our own pin remains.
---
src/backend/storage/buffer/bufmgr.c | 28 ++++------------------------
1 file changed, 4 insertions(+), 24 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 686265c3d48..277a1b4d993 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -6844,34 +6844,14 @@ LockBufferForCleanup(Buffer buffer)
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
elog(ERROR, "multiple backends attempting to wait for pincount 1");
}
- bufHdr->wait_backend_pgprocno = MyProcNumber;
- PinCountWaitBuf = bufHdr;
-
- /*
- * Publish BM_PIN_COUNT_WAITER while retaining the buffer header lock.
- * The shared refcount can be decremented while BM_LOCKED is set, so
- * use an atomic operation that preserves concurrent refcount changes.
- */
- pg_atomic_fetch_or_u64(&bufHdr->state, BM_PIN_COUNT_WAITER);
-
/*
- * Recheck the refcount after publishing the waiter flag, while shared
- * refcount increments are still prevented by BM_LOCKED. If only our
- * pin remains, the cleanup-lock condition has already been satisfied,
- * so remove the waiter state and return without sleeping.
+ * Register ourselves as the pincount waiter. If the shared refcount
+ * was concurrently reduced to 1 (only our own pin remains),
+ * RegisterPinCountWaiter() returns false and no wait is necessary.
*/
- buf_state = pg_atomic_read_u64(&bufHdr->state);
-
- if (BUF_STATE_GET_REFCOUNT(buf_state) == 1)
- {
- UnlockBufHdrExt(bufHdr, buf_state,
- 0, BM_PIN_COUNT_WAITER,
- 0);
- PinCountWaitBuf = NULL;
+ if (!RegisterPinCountWaiter(bufHdr, buf_state))
goto cleanup_lock_acquired;
- }
- UnlockBufHdr(bufHdr);
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
/* Wait to be signaled by UnpinBuffer() */
--
2.43.0
