On Mon, Aug 8, 2022 at 8:56 PM Kyotaro Horiguchi
<horikyota....@gmail.com> wrote:
> At Mon, 08 Aug 2022 17:33:22 +0900 (JST), Kyotaro Horiguchi
> <horikyota....@gmail.com> wrote in
> > If WaitForWALToBecomeAvailable returned by promotion, ReadPageInteral
> > misses the chance to inavlidate reader-state. That state is not an
> > error while in StandbyMode.
>
> Mmm... Maybe I wanted to say: (Still I'm not sure the rewrite works..)
>
> If WaitForWALToBecomeAvailable returned by promotion, ReadPageInteral
> would miss the chance to invalidate reader-state. When XLogPageRead
> is called in blocking mode while in StandbyMode (that is, the
> traditional condition) , the function continues retrying until it
> succeeds, or returns XLRAD_FAIL if promote is triggered. In other
> words, it was not supposed to return non-failure while the header
> validation is failing while in standby mode. But while in nonblocking
> mode, the function can return non-failure with lastSourceFailed =
> true, which seems wrong.
New ideas:
0001: Instead of figuring out when to invalidate the cache, let's
just invalidate it before every read attempt. It is only marked valid
after success (ie state->readLen > 0). No need to worry about error
cases.
0002: While here, I don't like xlogrecovery.c clobbering
xlogreader.c's internal error state, so I think we should have a
function for that with a documented purpose. It was also a little
inconsistent that it didn't clear a flag (but not buggy AFAICS; kinda
wondering if I should just get rid of that flag, but that's for
another day).
0003: Thinking about your comments above made me realise that I don't
really want XLogReadPage() to be internally retrying for obscure
failures while reading ahead. I think I prefer to give up on
prefetching as soon as anything tricky happens, and deal with
complexities once recovery catches up to that point. I am still
thinking about this point.
Here's the patch set I'm testing.
From 9f547088e54fcf279c74fbd03e4afaeffec69ea3 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Mon, 29 Aug 2022 19:56:27 +1200
Subject: [PATCH 1/3] Fix cache invalidation in rare recovery_prefetch case.
Since commit 0668719801 we've had an extra page validation and retry
loop inside XLogPageRead() to handle a rare special case. The new
recovery prefetch code from commit 5dc0418f could lead to a PANIC in
this case, because the WAL page cached internally by xlogreader.c was
not invalidated when it should have been.
Make tracking of the cached page's status more robust by invalidating it
just before we attempt any read. Because it is only marked valid when a
read succeeds, now we don't have to worry about invalidation in error
code paths.
The removed line that was setting errormsg_deferred was redundant
because it's already set by report_invalid_record().
Back-patch to 15.
Reported-by: Noah Misch <n...@leadboat.com>
Reviewed-by:
Discussion: https://postgr.es/m/20220807003627.GA4168930%40rfd.leadboat.com
---
src/backend/access/transam/xlogreader.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index f17e80948d..e883ade607 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -987,6 +987,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 read.
+ */
+ state->readLen = 0;
+
/*
* Data is not in our buffer.
*
@@ -1067,11 +1074,6 @@ ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr, int reqLen)
return readLen;
err:
- if (state->errormsg_buf[0] != '\0')
- {
- state->errormsg_deferred = true;
- XLogReaderInvalReadState(state);
- }
return XLREAD_FAIL;
}
--
2.30.2
From 72c55dfec6729e0f26a9fbf3ff184d3b4308a025 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Mon, 29 Aug 2022 21:08:25 +1200
Subject: [PATCH 2/3] Improve xlogrecovery.c/xlogreader.c boundary.
Create a function XLogReaderResetError() to clobber the error message
inside an XLogReaderState, instead of doing it directly from
xlogrecovery.c. This is older code from commit 0668719801, but commit
5dc0418f probably should have tidied this up because it leaves the
internal state a bit out of sync (not a live bug, but a little
confusing).
---
src/backend/access/transam/xlogreader.c | 10 ++++++++++
src/backend/access/transam/xlogrecovery.c | 2 +-
src/include/access/xlogreader.h | 3 +++
3 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index e883ade607..c100e18333 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -1325,6 +1325,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 a59a0e826b..422322cf5a 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -3335,7 +3335,7 @@ 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;
}
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index 87ff00feb7..97b6f00114 100644
--- 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 pread can be reported.
--
2.30.2
From bb8a6d52ca91d463993c3e01a48fc776297b0aeb Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Mon, 29 Aug 2022 21:14:25 +1200
Subject: [PATCH 3/3] Don't retry inside XLogPageRead() if reading ahead.
Give up immediately if we have a short read or a page header
invalidation failure inside XLogPageRead(). Once recovery catches up to
this point, it can deal with it. This doesn't fix any known live bug,
but it fits with the general philosophy of giving up fast when
prefetching runs into trouble.
---
src/backend/access/transam/xlogrecovery.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 422322cf5a..76ce9704ca 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -3342,6 +3342,14 @@ retry:
return readLen;
next_record_is_invalid:
+ /*
+ * If we're reading ahead, give up fast. Standby 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)
--
2.30.2
