Revert "Get rid of the dedicated latch for signaling the startup process".

Revert ac22929a26, as well as the followup fix 113d3591b8. Because it broke
the assumption that the startup process waiting for the recovery conflict
on buffer pin should be waken up only by buffer unpin or the timeout enabled
in ResolveRecoveryConflictWithBufferPin(). It caused, for example,
SIGHUP signal handler or walreceiver process to wake that startup process
up unnecessarily frequently.

Additionally, add the comments about why that dedicated latch that
the reverted  tried to get rid of should not be removed.

Thanks to Kyotaro Horiguchi for the discussion.

Author: Fujii Masao
Discussion: https://postgr.es/m/d8c0c608-021b-3c73-fffd-3240829ee986@oss.nttdata.com

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 8dd225c2e140ac13273be1337935faf749e0322b..b1e5d2dbffd3bdd737f2e74105db0dde4704b25b 100644 (file)
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -681,8 +681,18 @@ typedef struct XLogCtlData
     * recoveryWakeupLatch is used to wake up the startup process to continue
     * WAL replay, if it is waiting for WAL to arrive or failover trigger file
     * to appear.
+    *
+    * Note that the startup process also uses another latch, its procLatch,
+    * to wait for recovery conflict. If we get rid of recoveryWakeupLatch for
+    * signaling the startup process in favor of using its procLatch, which
+    * comports better with possible generic signal handlers using that latch.
+    * But we should not do that because the startup process doesn't assume
+    * that it's waken up by walreceiver process or SIGHUP signal handler
+    * while it's waiting for recovery conflict. The separate latches,
+    * recoveryWakeupLatch and procLatch, should be used for inter-process
+    * communication for WAL replay and recovery conflict, respectively.
     */
-   Latch       *recoveryWakeupLatch;
+   Latch       recoveryWakeupLatch;
 
    /*
     * During recovery, we keep a copy of the latest checkpoint record here.
@@ -5186,6 +5196,7 @@ XLOGShmemInit(void)
    SpinLockInit(&XLogCtl->Insert.insertpos_lck);
    SpinLockInit(&XLogCtl->info_lck);
    SpinLockInit(&XLogCtl->ulsn_lck);
+   InitSharedLatch(&XLogCtl->recoveryWakeupLatch);
 }
 
 /*
@@ -6121,7 +6132,7 @@ recoveryApplyDelay(XLogReaderState *record)
 
    while (true)
    {
-       ResetLatch(MyLatch);
+       ResetLatch(&XLogCtl->recoveryWakeupLatch);
 
        /* might change the trigger file's location */
        HandleStartupProcInterrupts();
@@ -6140,7 +6151,7 @@ recoveryApplyDelay(XLogReaderState *record)
 
        elog(DEBUG2, "recovery apply delay %ld milliseconds", msecs);
 
-       (void) WaitLatch(MyLatch,
+       (void) WaitLatch(&XLogCtl->recoveryWakeupLatch,
                         WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
                         msecs,
                         WAIT_EVENT_RECOVERY_APPLY_DELAY);
@@ -6469,11 +6480,11 @@ StartupXLOG(void)
    }
 
    /*
-    * Advertise our latch that other processes can use to wake us up
-    * if we're going to sleep during recovery.
+    * Take ownership of the wakeup latch if we're going to sleep during
+    * recovery.
     */
    if (ArchiveRecoveryRequested)
-       XLogCtl->recoveryWakeupLatch = &MyProc->procLatch;
+       OwnLatch(&XLogCtl->recoveryWakeupLatch);
 
    /* Set up XLOG reader facility */
    MemSet(&private, 0, sizeof(XLogPageReadPrivate));
@@ -7484,11 +7495,11 @@ StartupXLOG(void)
        ResetUnloggedRelations(UNLOGGED_RELATION_INIT);
 
    /*
-    * We don't need the latch anymore. It's not strictly necessary to reset
-    * it to NULL, but let's do it for the sake of tidiness.
+    * We don't need the latch anymore. It's not strictly necessary to disown
+    * it, but let's do it for the sake of tidiness.
     */
    if (ArchiveRecoveryRequested)
-       XLogCtl->recoveryWakeupLatch = NULL;
+       DisownLatch(&XLogCtl->recoveryWakeupLatch);
 
    /*
     * We are now done reading the xlog from stream. Turn off 
@@ -12300,12 +12311,12 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
                        wait_time = wal_retrieve_retry_interval -
                            TimestampDifferenceMilliseconds(last_fail_time, now);
 
-                       (void) WaitLatch(MyLatch,
+                       (void) WaitLatch(&XLogCtl->recoveryWakeupLatch,
                                         WL_LATCH_SET | WL_TIMEOUT |
                                         WL_EXIT_ON_PM_DEATH,
                                         wait_time,
                                         WAIT_EVENT_RECOVERY_RETRIEVE_RETRY_INTERVAL);
-                       ResetLatch(MyLatch);
+                       ResetLatch(&XLogCtl->recoveryWakeupLatch);
                        now = GetCurrentTimestamp();
 
                        /* Handle interrupt signals of startup process */
@@ -12559,11 +12570,11 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
                     * to react to a trigger file promptly and to check if the
                     * WAL receiver is still active.
                     */
-                   (void) WaitLatch(MyLatch,
+                   (void) WaitLatch(&XLogCtl->recoveryWakeupLatch,
                                     WL_LATCH_SET | WL_TIMEOUT |
                                     WL_EXIT_ON_PM_DEATH,
                                     5000L, WAIT_EVENT_RECOVERY_WAL_STREAM);
-                   ResetLatch(MyLatch);
+                   ResetLatch(&XLogCtl->recoveryWakeupLatch);
                    break;
                }
 
@@ -12735,8 +12746,7 @@ CheckPromoteSignal(void)
 void
 WakeupRecovery(void)
 {
-   if (XLogCtl->recoveryWakeupLatch)
-       SetLatch(XLogCtl->recoveryWakeupLatch);
+   SetLatch(&XLogCtl->recoveryWakeupLatch);
 }
 
 /*

diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index eab9c8c4ed332fa100f646985b12615d851ec925..64af7b8707cc6fcf89c346b088b621f3f851f111 100644 (file)
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -37,6 +37,7 @@
 /*
  * Flags set by interrupt handlers for later service in the redo loop.
  */
+static volatile sig_atomic_t got_SIGHUP = false;
 static volatile sig_atomic_t shutdown_requested = false;
 static volatile sig_atomic_t promote_signaled = false;
 
@@ -48,6 +49,7 @@ static volatile sig_atomic_t in_restore_command = false;
 
 /* Signal handlers */
 static void StartupProcTriggerHandler(SIGNAL_ARGS);
+static void StartupProcSigHupHandler(SIGNAL_ARGS);
 
 
 /* --------------------------------
@@ -62,7 +64,19 @@ StartupProcTriggerHandler(SIGNAL_ARGS)
    int         save_errno = errno;
 
    promote_signaled = true;
-   SetLatch(MyLatch);
+   WakeupRecovery();
+
+   errno = save_errno;
+}
+
+/* SIGHUP: set flag to re-read config file at next convenient time */
+static void
+StartupProcSigHupHandler(SIGNAL_ARGS)
+{
+   int         save_errno = errno;
+
+   got_SIGHUP = true;
+   WakeupRecovery();
 
    errno = save_errno;
 }
@@ -77,7 +91,7 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
        proc_exit(1);
    else
        shutdown_requested = true;
-   SetLatch(MyLatch);
+   WakeupRecovery();
 
    errno = save_errno;
 }
@@ -123,9 +137,9 @@ HandleStartupProcInterrupts(void)
    /*
     * Process any requests or signals received recently.
     */
-   if (ConfigReloadPending)
+   if (got_SIGHUP)
    {
-       ConfigReloadPending = false;
+       got_SIGHUP = false;
        StartupRereadConfig();
    }
 
@@ -158,7 +172,7 @@ StartupProcessMain(void)
    /*
     * Properly accept or ignore signals the postmaster might send us.
     */
-   pqsignal(SIGHUP, SignalHandlerForConfigReload); /* reload config file */
+   pqsignal(SIGHUP, StartupProcSigHupHandler); /* reload config file */
    pqsignal(SIGINT, SIG_IGN);  /* ignore query cancel */
    pqsignal(SIGTERM, StartupProcShutdownHandler);  /* request shutdown */
    /* SIGQUIT handler was already set up by InitPostmasterChild */

diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 4ea3cf1f5cb573aef3a8b1bb420fce2343024be9..92d9027776c6596587a990ca7e9426ff07db2b56 100644 (file)
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -519,7 +519,14 @@ ResolveRecoveryConflictWithBufferPin(void)
        enable_timeouts(timeouts, 2);
    }
 
-   /* Wait to be signaled by UnpinBuffer() */
+   /*
+    * Wait to be signaled by UnpinBuffer().
+    *
+    * 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(PG_WAIT_BUFFER_PIN);
 
    /*