From c80a9bb1787d9efc4eb90fb8c0ce0e4bb105e230 Mon Sep 17 00:00:00 2001 From: alterego655 <824662526@qq.com> Date: Tue, 1 Sep 2026 20:31:08 +0800 Subject: [PATCH v9 1/2] Fix premature wakeups in hot-standby buffer-pin conflict waits ProcWaitForSignal() waits on the generic process latch and can therefore return for reasons unrelated to either a buffer unpin or a recovery-conflict timeout.  ResolveRecoveryConflictWithBufferPin() nevertheless treated every return as the end of the current wait, disabled all timeouts, and returned to LockBufferForCleanup(), which then armed a new relative deadlock timeout. This can prevent the deadlock timeout from ever expiring.  The timeout subsystem may leave a kernel alarm armed after disabling the logical timeout for which it was set, such as the startup-progress timeout.  If that alarm fires before the current buffer-pin deadlock deadline, handle_sig_alarm() wakes the startup process even though no logical timeout has expired, and reprograms the kernel alarm for the current deadlock deadline.  The resulting latch wake causes the wait code to discard that deadline and arm a new relative deadline farther in the future.  The already-programmed kernel alarm now precedes the replacement deadline and can wake the startup process prematurely again.  Repeating this sequence can postpone the deadlock timeout unboundedly while the pin conflict persists. Consequently, the startup process may never send the RECOVERY_CONFLICT_BUFFERPIN_DEADLOCK request.  With no finite max_standby_*_delay to provide another breaker, recovery can remain stalled unboundedly. After a wakeup, recheck whether only the startup process's own pin remains.  If other pins remain and neither recovery-conflict deadline has been reached, ensure that the process remains registered as the pin-count waiter and continue waiting without tearing down or restarting the active timeouts.  This makes unrelated latch wakeups harmless while preserving the original timeout deadlines. --- src/backend/storage/buffer/bufmgr.c | 104 ++++++++++++++++++++++++++++ src/backend/storage/ipc/standby.c | 84 +++++++++++++++------- src/include/storage/bufmgr.h | 1 + 3 files changed, 163 insertions(+), 26 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 17f142e4c5b..c96ceb1513a 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,62 @@ BufferGetLSNAtomic(Buffer buffer) #endif } +/* + * PinCountWaiterCheckReadyForCleanup + * Recheck whether the current cleanup-lock wait has completed and, if + * necessary, rearm the shared pin-count notification. + * + * The caller must own the backend-local cleanup wait for this buffer, as + * indicated by PinCountWaitBuf. BM_PIN_COUNT_WAITER may still be set for + * this process, or it may already have been cleared by WakePinCountWaiter() + * before signaling us. + * + * Return true if our own pin is the only remaining shared pin. Otherwise, + * ensure that this process is registered as the shared pin-count waiter and + * return false. + */ +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.51.0