From d0d934490020f9311662da8cdba4ccc2070e420d Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Sat, 3 Sep 2022 12:58:16 +1200 Subject: 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 --- src/backend/access/transam/xlogreader.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) (limited to 'src/backend/access/transam/xlogreader.c') diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c index cf5db23cb86..02d4414bac7 100644 --- 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. * -- cgit v1.2.3