From e9593403d2624867e89c5906a8a03bd8e9635cd8 Mon Sep 17 00:00:00 2001 From: alterego655 <824662526@qq.com> Date: Tue, 1 Sep 2026 20:31:08 +0800 Subject: [PATCH v10 1/3] Handle unrelated wakeups during recovery buffer pin and lock waits ProcWaitForSignal() can wake the startup process even when the buffer pin or lock it is waiting for has not been released and no timeout has expired. Both recovery conflict handlers would then cancel their timers and return. If the conflict remained, the next call would start a new deadlock timeout with a later deadline. An alarm left over from an earlier wait can make this happen repeatedly. Canceling a logical timeout does not necessarily cancel the kernel alarm. If that alarm fires before the current deadlock deadline, the signal handler wakes startup and schedules another alarm for that deadline. Startup then restarts its deadlock timeout, moving the deadline past the pending alarm again. This can continue indefinitely, so the deadlock timeout never fires. Startup may therefore never ask the conflicting backends to check for deadlocks. If the applicable max_standby_streaming_delay or max_standby_archive_delay is -1, there is no delay limit to force cancellation, and recovery can remain stuck. Keep the existing timers running when a wakeup does not end the wait. For buffer pins, check whether other processes still hold pins and, if necessary, register for another notification when they release them. For relation locks, check whether startup is still waiting for the lock. If the conflict remains and neither timeout has expired, wait again without restarting the timers. When the standby delay limit has already passed and no timers were set, continue returning after a wakeup so the caller can retry. --- src/backend/storage/buffer/bufmgr.c | 102 ++++++++++++++++ src/backend/storage/ipc/standby.c | 178 +++++++++++++++++----------- src/include/storage/bufmgr.h | 1 + 3 files changed, 213 insertions(+), 68 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index f81c7732e68..6733c3c2dcd 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -3471,6 +3471,52 @@ WakePinCountWaiter(BufferDesc *buf) UnlockBufHdr(buf); } +/* + * Register the current process to be notified when only its own pin remains, + * then unlock the buffer header. Return true if the process needs to wait, + * or false if the pin count has already dropped to one. + * + * The caller must hold the buffer header lock, pass the current buffer state + * returned by LockBufHdr(), and ensure that no other process 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. * @@ -6679,6 +6725,62 @@ CheckBufferIsPinnedOnce(Buffer buffer) } } +/* + * PinCountWaiterCheckReadyForCleanup + * Check whether only our own pin remains and, if necessary, register + * for another 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 backends 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; +} + /* * LockBufferForCleanup - lock a buffer in preparation for deleting items * diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c index 7f011e04990..89319db0311 100644 --- a/src/backend/storage/ipc/standby.c +++ b/src/backend/storage/ipc/standby.c @@ -627,6 +627,7 @@ ResolveRecoveryConflictWithLock(LOCKTAG locktag, bool logging_conflict) { TimestampTz ltime; TimestampTz now; + bool timeouts_armed = false; Assert(InHotStandby); @@ -697,61 +698,80 @@ ResolveRecoveryConflictWithLock(LOCKTAG locktag, bool logging_conflict) cnt++; enable_timeouts(timeouts, cnt); + timeouts_armed = true; } - /* Wait to be signaled by the release of the Relation Lock */ - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); - /* - * Exit if ltime is reached. Then all the backends holding conflicting - * locks will be canceled in the next ResolveRecoveryConflictWithLock() - * call. + * ProcWaitForSignal() can wake up even when the lock wait has not ended + * and neither timeout has expired. Keep waiting with the same timeouts + * in that case. Returning to the caller would restart the deadlock + * timeout with a later deadline, which could keep it from ever expiring. + * + * If the standby delay limit was already reached above, no timeouts were + * set. Return after any wakeup so the caller can try resolving the + * conflict again. */ - if (got_standby_lock_timeout) - goto cleanup; - - if (got_standby_deadlock_timeout) + for (;;) { - VirtualTransactionId *backends; + ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); - backends = GetLockConflicts(&locktag, AccessExclusiveLock, NULL); + if (!timeouts_armed) + break; - /* Quick exit if there's no work to be done */ - if (!VirtualTransactionIdIsValid(*backends)) - goto cleanup; + if (*((volatile ProcWaitStatus *) &MyProc->waitStatus) != PROC_WAIT_STATUS_WAITING) + break; /* - * Send signals to all the backends holding the conflicting locks, to - * ask them to check themselves for deadlocks. + * Exit if ltime is reached. Then all the backends holding conflicting + * locks will be canceled in the next + * ResolveRecoveryConflictWithLock() call. */ - while (VirtualTransactionIdIsValid(*backends)) + if (got_standby_lock_timeout) + break; + + if (got_standby_deadlock_timeout) { - (void) SignalRecoveryConflictWithVirtualXID(*backends, - RECOVERY_CONFLICT_STARTUP_DEADLOCK); - backends++; - } + VirtualTransactionId *backends; - /* - * Exit if the recovery conflict has not been logged yet even though - * logging is enabled, so that the caller can log that. Then - * RecoveryConflictWithLock() is called again and we will wait again - * for the lock to be released. - */ - if (logging_conflict) - goto cleanup; + backends = GetLockConflicts(&locktag, AccessExclusiveLock, NULL); - /* - * Wait again here to be signaled by the release of the Relation Lock, - * to prevent the subsequent RecoveryConflictWithLock() from causing - * deadlock_timeout and sending a request for deadlocks check again. - * Otherwise the request continues to be sent every deadlock_timeout - * until the relation locks are released or ltime is reached. - */ - got_standby_deadlock_timeout = false; - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); - } + /* Quick exit if there's no work to be done */ + if (!VirtualTransactionIdIsValid(*backends)) + break; + + /* + * Send signals to all the backends holding the conflicting locks, + * to ask them to check themselves for deadlocks. + */ + while (VirtualTransactionIdIsValid(*backends)) + { + (void) SignalRecoveryConflictWithVirtualXID(*backends, + RECOVERY_CONFLICT_STARTUP_DEADLOCK); + backends++; + } -cleanup: + /* + * Exit if the recovery conflict has not been logged yet even + * though logging is enabled, so that the caller can log that. + * Then RecoveryConflictWithLock() is called again and we will + * wait again for the lock to be released. + */ + if (logging_conflict) + break; + + /* + * Wait again here to be signaled by the release of the Relation + * Lock, to prevent the subsequent RecoveryConflictWithLock() from + * causing deadlock_timeout and sending a request for deadlocks + * check again. Otherwise the request continues to be sent every + * deadlock_timeout until the relation locks are released or ltime + * is reached. + */ + got_standby_deadlock_timeout = false; + ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); + break; + } + } /* * Clear any timeout requests established above. We assume here that the @@ -790,17 +810,25 @@ 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 SetStartupBufferPinWaitBuf(). */ void ResolveRecoveryConflictWithBufferPin(void) { TimestampTz ltime; + Buffer buffer; + bool timeouts_armed = false; Assert(InHotStandby); + buffer = GetStartupBufferPinWaitBuf(); + Assert(BufferIsValid(buffer)); + ltime = GetStandbyLimitTime(); - if (GetCurrentTimestamp() >= ltime && ltime != 0) + if (ltime != 0 && GetCurrentTimestamp() >= ltime) { /* * We're already behind, so clear a path as quickly as possible. @@ -831,37 +859,51 @@ ResolveRecoveryConflictWithBufferPin(void) cnt++; enable_timeouts(timeouts, cnt); + timeouts_armed = true; } /* - * 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() can wake up for unrelated reasons. If timeouts + * were set, keep waiting with the same deadlines until a timeout expires + * or only our own pin remains. The helper also makes sure we are + * registered for another unpin notification before waiting again. If the + * delay limit had already passed, return after any wakeup so the caller + * can retry. */ - 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. - * - * 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); + ProcWaitForSignal(WAIT_EVENT_BUFFER_CLEANUP); + + if (!timeouts_armed) + break; + + if (PinCountWaiterCheckReadyForCleanup(buffer)) + break; + + /* Request cancellation if the delay timeout has expired. */ + 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..a3d297fb2ee 100644 --- a/src/include/storage/bufmgr.h +++ b/src/include/storage/bufmgr.h @@ -341,6 +341,7 @@ LockBuffer(Buffer buffer, BufferLockMode mode) extern bool ConditionalLockBuffer(Buffer buffer); extern void LockBufferForCleanup(Buffer buffer); +extern bool PinCountWaiterCheckReadyForCleanup(Buffer buffer); extern bool ConditionalLockBufferForCleanup(Buffer buffer); extern bool IsBufferCleanupOK(Buffer buffer); extern bool HoldingBufferPinThatDelaysRecovery(void); -- 2.51.0