diff options
| author | Andres Freund <andres@anarazel.de> | 2022-05-02 18:25:00 -0700 |
|---|---|---|
| committer | Andres Freund <andres@anarazel.de> | 2022-05-02 18:25:59 -0700 |
| commit | 9ab3b2bdbb5dc4ff857685eae5645d7c35839055 (patch) | |
| tree | c3b57a23979b13e14614c795e5b715321d820f20 | |
| parent | 5ab8e8014801dd6bc05809e7ba994c013e9ee86b (diff) | |
| download | postgresql-9ab3b2bdbb5dc4ff857685eae5645d7c35839055.tar.gz | |
Fix possibility of self-deadlock in ResolveRecoveryConflictWithBufferPin().
The tests added in 9f8a050f68d failed nearly reliably on FreeBSD in CI, and
occasionally on the buildfarm. That turns out to be caused not by a bug in the
test, but by a longstanding bug in recovery conflict handling.
The standby timeout handler, used by ResolveRecoveryConflictWithBufferPin(),
executed SendRecoveryConflictWithBufferPin() inside a signal handler. A bad
idea, because the deadlock timeout handler (or a spurious latch set) could
have interrupted ProcWaitForSignal(). If unlucky that could cause a
self-deadlock on ProcArrayLock, if the deadlock check is in
SendRecoveryConflictWithBufferPin()->CancelDBBackends().
To fix, set a flag in StandbyTimeoutHandler(), and check the flag in
ResolveRecoveryConflictWithBufferPin().
Subsequently the recovery conflict tests will be backpatched.
Discussion: https://postgr.es/m/20220413002626.udl7lll7f3o7nre7@alap3.anarazel.de
Backpatch: 10-
| -rw-r--r-- | src/backend/storage/ipc/standby.c | 20 |
1 files changed, 10 insertions, 10 deletions
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c index a3ceec88a1..687ce03767 100644 --- a/src/backend/storage/ipc/standby.c +++ b/src/backend/storage/ipc/standby.c @@ -45,6 +45,7 @@ static HTAB *RecoveryLockLists; /* Flags set by timeout handlers */ static volatile sig_atomic_t got_standby_deadlock_timeout = false; +static volatile sig_atomic_t got_standby_delay_timeout = false; static volatile sig_atomic_t got_standby_lock_timeout = false; static void ResolveRecoveryConflictWithVirtualXIDs(VirtualTransactionId *waitlist, @@ -792,7 +793,8 @@ ResolveRecoveryConflictWithBufferPin(void) } /* - * Wait to be signaled by UnpinBuffer(). + * 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 @@ -801,7 +803,9 @@ ResolveRecoveryConflictWithBufferPin(void) */ ProcWaitForSignal(PG_WAIT_BUFFER_PIN); - if (got_standby_deadlock_timeout) + if (got_standby_delay_timeout) + SendRecoveryConflictWithBufferPin(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN); + else if (got_standby_deadlock_timeout) { /* * Send out a request for hot-standby backends to check themselves for @@ -827,6 +831,7 @@ ResolveRecoveryConflictWithBufferPin(void) * individually, but that'd be slower. */ disable_all_timeouts(false); + got_standby_delay_timeout = false; got_standby_deadlock_timeout = false; } @@ -886,8 +891,8 @@ CheckRecoveryConflictDeadlock(void) */ /* - * StandbyDeadLockHandler() will be called if STANDBY_DEADLOCK_TIMEOUT - * occurs before STANDBY_TIMEOUT. + * StandbyDeadLockHandler() will be called if STANDBY_DEADLOCK_TIMEOUT is + * exceeded. */ void StandbyDeadLockHandler(void) @@ -897,16 +902,11 @@ StandbyDeadLockHandler(void) /* * StandbyTimeoutHandler() will be called if STANDBY_TIMEOUT is exceeded. - * Send out a request to release conflicting buffer pins unconditionally, - * so we can press ahead with applying changes in recovery. */ void StandbyTimeoutHandler(void) { - /* forget any pending STANDBY_DEADLOCK_TIMEOUT request */ - disable_timeout(STANDBY_DEADLOCK_TIMEOUT, false); - - SendRecoveryConflictWithBufferPin(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN); + got_standby_delay_timeout = true; } /* |