Ensure recovery pause feature doesn't pause unless users can connect.

If we're not in hot standby mode, then there's no way for users to connect
to reset the recoveryPause flag, so we shouldn't pause.  The code was aware
of this but the test to see if pausing was safe was seriously inadequate:
it wasn't paying attention to reachedConsistency, and besides what it was
testing was that we could legally enter hot standby, not that we have
done so.  Get rid of that in favor of checking LocalHotStandbyActive,
which because of the coding in CheckRecoveryConsistency is tantamount to
checking that we have told the postmaster to enter hot standby.

Also, move the recoveryPausesHere() call that reacts to asynchronous
recoveryPause requests so that it's not in the middle of application of a
WAL record.  I put it next to the recoveryStopsHere() call --- in future
those are going to need to interact significantly, so this seems like a
good waystation.

Also, don't bother trying to read another WAL record if we've already
decided not to continue recovery.  This was no big deal when the code was
written originally, but now that reading a record might entail actions like
fetching an archive file, it seems a bit silly to do it like that.

Per report from Jeff Janes and subsequent discussion.  The pause feature
needs quite a lot more work, but this gets rid of some indisputable bugs,
and seems safe enough to back-patch.

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 187b60999e00a1254e6c766ce170c3a39c917db1..7adbc3e82ec0595d48aa6d6ce5b272eb727092f1 100644 (file)
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5916,13 +5916,19 @@ recoveryStopsHere(XLogRecord *record, bool *includeThis)
 }
 
 /*
- * Recheck shared recoveryPause by polling.
+ * Wait until shared recoveryPause flag is cleared.
  *
- * XXX Can also be done with shared latch.
+ * XXX Could also be done with shared latch, avoiding the pg_usleep loop.
+ * Probably not worth the trouble though.  This state shouldn't be one that
+ * anyone cares about server power consumption in.
  */
 static void
 recoveryPausesHere(void)
 {
+   /* Don't pause unless users can connect! */
+   if (!LocalHotStandbyActive)
+       return;
+
    ereport(LOG,
            (errmsg("recovery has paused"),
             errhint("Execute pg_xlog_replay_resume() to continue.")));
@@ -6650,7 +6656,6 @@ StartupXLOG(void)
        {
            bool        recoveryContinue = true;
            bool        recoveryApply = true;
-           bool        recoveryPause = false;
            ErrorContextCallback errcontext;
            TimestampTz xtime;
 
@@ -6692,22 +6697,36 @@ StartupXLOG(void)
                /* Allow read-only connections if we're consistent now */
                CheckRecoveryConsistency();
 
+               /*
+                * Pause WAL replay, if requested by a hot-standby session via
+                * SetRecoveryPause().
+                *
+                * Note that we intentionally don't take the info_lck spinlock
+                * here.  We might therefore read a slightly stale value of
+                * the recoveryPause flag, but it can't be very stale (no
+                * worse than the last spinlock we did acquire).  Since a
+                * pause request is a pretty asynchronous thing anyway,
+                * possibly responding to it one WAL record later than we
+                * otherwise would is a minor issue, so it doesn't seem worth
+                * adding another spinlock cycle to prevent that.
+                */
+               if (xlogctl->recoveryPause)
+                   recoveryPausesHere();
+
                /*
                 * Have we reached our recovery target?
                 */
                if (recoveryStopsHere(record, &recoveryApply))
                {
-                   /*
-                    * Pause only if users can connect to send a resume
-                    * message
-                    */
-                   if (recoveryPauseAtTarget && standbyState == STANDBY_SNAPSHOT_READY)
+                   if (recoveryPauseAtTarget)
                    {
                        SetRecoveryPause(true);
                        recoveryPausesHere();
                    }
                    reachedStopPoint = true;    /* see below */
                    recoveryContinue = false;
+
+                   /* Exit loop if we reached non-inclusive recovery target */
                    if (!recoveryApply)
                        break;
                }
@@ -6740,15 +6759,8 @@ StartupXLOG(void)
                 */
                SpinLockAcquire(&xlogctl->info_lck);
                xlogctl->replayEndRecPtr = EndRecPtr;
-               recoveryPause = xlogctl->recoveryPause;
                SpinLockRelease(&xlogctl->info_lck);
 
-               /*
-                * Pause only if users can connect to send a resume message
-                */
-               if (recoveryPause && standbyState == STANDBY_SNAPSHOT_READY)
-                   recoveryPausesHere();
-
                /*
                 * If we are attempting to enter Hot Standby mode, process
                 * XIDs we see
@@ -6792,10 +6804,16 @@ StartupXLOG(void)
                xlogctl->recoveryLastRecPtr = EndRecPtr;
                SpinLockRelease(&xlogctl->info_lck);
 
+               /* Remember this record as the last-applied one */
                LastRec = ReadRecPtr;
 
+               /* Exit loop if we reached inclusive recovery target */
+               if (!recoveryContinue)
+                   break;
+
+               /* Else, try to fetch the next WAL record */
                record = ReadRecord(NULL, LOG, false);
-           } while (record != NULL && recoveryContinue);
+           } while (record != NULL);
 
            /*
             * end of main redo apply loop