Fix cache invalidation bug in recovery_prefetch.

XLogPageRead() can retry internally after a pread() system call has
succeeded, in the case of short reads, and page validation failures
while in standby mode (see commit 0668719801).  Due to an oversight in
commit 3f1ce973, these cases could leave stale data in the internal
cache of xlogreader.c without marking it invalid.  The main defense
against stale cached data on failure to read a page was in the error
handling path of the calling function ReadPageInternal(), but that
wasn't quite enough for errors handled internally by XLogPageRead()'s
retry loop if we then exited with XLREAD_WOULDBLOCK.

1.  ReadPageInternal() now marks the cache invalid before calling the
    page_read callback, by setting state->readLen to 0.  It'll be set to
    a non-zero value only after a successful read.  It'll stay valid as
    long as the caller requests data in the cached range.

2.  XLogPageRead() no long performs internal retries while reading
    ahead.  While such retries should work, the general philosophy is
    that we should give up prefetching if anything unusual happens so we
    can handle it when recovery catches up, to reduce the complexity of
    the system.  Let's do that here too.

3.  While here, a new function XLogReaderResetError() improves the
    separation between xlogrecovery.c and xlogreader.c, where the former
    previously clobbered the latter's internal error buffer directly.
    The new function makes this more explicit, and also clears a related
    flag, without which a standby would needlessly retry in the outer
    function.

Thanks to Noah Misch for tracking down the conditions required for a
rare build farm failure in src/bin/pg_ctl/t/003_promote.pl, and
providing a reproducer.

Back-patch to 15.

Reported-by: Noah Misch 
Discussion: https://postgr.es/m/20220807003627.GA4168930%40rfd.leadboat.com

diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index cf5db23cb86bf8934fadf0617435969f8793771a..02d4414bac76aa8be2b6ccc4238f8eb38aea73c4 100644 (file)
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -986,6 +986,13 @@ ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr, int reqLen)
        targetPageOff == state->segoff && reqLen <= state->readLen)
        return state->readLen;
 
+   /*
+    * Invalidate contents of internal buffer before read attempt.  Just set
+    * the length to 0, rather than a full XLogReaderInvalReadState(), so we
+    * don't forget the segment we last successfully read.
+    */
+   state->readLen = 0;
+
    /*
     * Data is not in our buffer.
     *
@@ -1066,11 +1073,8 @@ ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr, int reqLen)
    return readLen;
 
 err:
-   if (state->errormsg_buf[0] != '\0')
-   {
-       state->errormsg_deferred = true;
-       XLogReaderInvalReadState(state);
-   }
+   XLogReaderInvalReadState(state);
+
    return XLREAD_FAIL;
 }
 
@@ -1322,6 +1326,16 @@ XLogReaderValidatePageHeader(XLogReaderState *state, XLogRecPtr recptr,
    return true;
 }
 
+/*
+ * Forget about an error produced by XLogReaderValidatePageHeader().
+ */
+void
+XLogReaderResetError(XLogReaderState *state)
+{
+   state->errormsg_buf[0] = '\0';
+   state->errormsg_deferred = false;
+}
+
 /*
  * Find the first record with an lsn >= RecPtr.
  *

diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index f6f894b36d19daaca2272935cfcdddba8020cad3..f4aeda771c028b0666c9c47e27665c7ee422e689 100644 (file)
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -3339,13 +3339,21 @@ retry:
                    (errmsg_internal("%s", xlogreader->errormsg_buf)));
 
        /* reset any error XLogReaderValidatePageHeader() might have set */
-       xlogreader->errormsg_buf[0] = '\0';
+       XLogReaderResetError(xlogreader);
        goto next_record_is_invalid;
    }
 
    return readLen;
 
 next_record_is_invalid:
+
+   /*
+    * If we're reading ahead, give up fast.  Retries and error reporting will
+    * be handled by a later read when recovery catches up to this point.
+    */
+   if (xlogreader->nonblocking)
+       return XLREAD_WOULDBLOCK;
+
    lastSourceFailed = true;
 
    if (readFile >= 0)

diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index e73ea4a8408e179d5ecf4aff71e9612f96c7c7e5..6b851693b168453e21da3f44d2eb23828a0c8ff1 100644 (file)
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -373,6 +373,9 @@ extern DecodedXLogRecord *XLogReadAhead(XLogReaderState *state,
 extern bool XLogReaderValidatePageHeader(XLogReaderState *state,
                                         XLogRecPtr recptr, char *phdr);
 
+/* Forget error produced by XLogReaderValidatePageHeader(). */
+extern void XLogReaderResetError(XLogReaderState *state);
+
 /*
  * Error information from WALRead that both backend and frontend caller can
  * process.  Currently only errors from pg_pread can be reported.