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-

diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 2850867323b600f9c36ab45c449065572af9f8ba..8c5e8432e7372687b785bded10ce1e132f5019b1 100644 (file)
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -46,6 +46,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,
@@ -793,7 +794,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
@@ -802,7 +804,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
@@ -828,6 +832,7 @@ ResolveRecoveryConflictWithBufferPin(void)
     * individually, but that'd be slower.
     */
    disable_all_timeouts(false);
+   got_standby_delay_timeout = false;
    got_standby_deadlock_timeout = false;
 }
 
@@ -887,8 +892,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)
@@ -898,16 +903,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;
 }
 
 /*