Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 932b016

Browse files
committed
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 0668719). Due to an oversight in commit 3f1ce97, 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 <noah@leadboat.com> Discussion: https://postgr.es/m/20220807003627.GA4168930%40rfd.leadboat.com
1 parent ff720a5 commit 932b016

File tree

3 files changed

+31
-6
lines changed

3 files changed

+31
-6
lines changed

src/backend/access/transam/xlogreader.c

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -987,6 +987,13 @@ ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr, int reqLen)
987987
targetPageOff == state->segoff && reqLen <= state->readLen)
988988
return state->readLen;
989989

990+
/*
991+
* Invalidate contents of internal buffer before read attempt. Just set
992+
* the length to 0, rather than a full XLogReaderInvalReadState(), so we
993+
* don't forget the segment we last successfully read.
994+
*/
995+
state->readLen = 0;
996+
990997
/*
991998
* Data is not in our buffer.
992999
*
@@ -1067,11 +1074,8 @@ ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr, int reqLen)
10671074
return readLen;
10681075

10691076
err:
1070-
if (state->errormsg_buf[0] != '\0')
1071-
{
1072-
state->errormsg_deferred = true;
1073-
XLogReaderInvalReadState(state);
1074-
}
1077+
XLogReaderInvalReadState(state);
1078+
10751079
return XLREAD_FAIL;
10761080
}
10771081

@@ -1323,6 +1327,16 @@ XLogReaderValidatePageHeader(XLogReaderState *state, XLogRecPtr recptr,
13231327
return true;
13241328
}
13251329

1330+
/*
1331+
* Forget about an error produced by XLogReaderValidatePageHeader().
1332+
*/
1333+
void
1334+
XLogReaderResetError(XLogReaderState *state)
1335+
{
1336+
state->errormsg_buf[0] = '\0';
1337+
state->errormsg_deferred = false;
1338+
}
1339+
13261340
/*
13271341
* Find the first record with an lsn >= RecPtr.
13281342
*

src/backend/access/transam/xlogrecovery.c

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3341,13 +3341,21 @@ XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
33413341
(errmsg_internal("%s", xlogreader->errormsg_buf)));
33423342

33433343
/* reset any error XLogReaderValidatePageHeader() might have set */
3344-
xlogreader->errormsg_buf[0] = '\0';
3344+
XLogReaderResetError(xlogreader);
33453345
goto next_record_is_invalid;
33463346
}
33473347

33483348
return readLen;
33493349

33503350
next_record_is_invalid:
3351+
3352+
/*
3353+
* If we're reading ahead, give up fast. Retries and error reporting will
3354+
* be handled by a later read when recovery catches up to this point.
3355+
*/
3356+
if (xlogreader->nonblocking)
3357+
return XLREAD_WOULDBLOCK;
3358+
33513359
lastSourceFailed = true;
33523360

33533361
if (readFile >= 0)

src/include/access/xlogreader.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -373,6 +373,9 @@ extern DecodedXLogRecord *XLogReadAhead(XLogReaderState *state,
373373
extern bool XLogReaderValidatePageHeader(XLogReaderState *state,
374374
XLogRecPtr recptr, char *phdr);
375375

376+
/* Forget error produced by XLogReaderValidatePageHeader(). */
377+
extern void XLogReaderResetError(XLogReaderState *state);
378+
376379
/*
377380
* Error information from WALRead that both backend and frontend caller can
378381
* process. Currently only errors from pread can be reported.

0 commit comments

Comments
 (0)