From bd037dc928dd126e5623117b2fe7633ec3fa7c40 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 11 Apr 2022 17:43:46 -0400 Subject: [PATCH] Make XLogRecGetBlockTag() throw error if there's no such block. All but a few existing callers assume without checking that this function succeeds. While it probably will, that's a poor excuse for not checking. Let's make it return void and instead throw an error if it doesn't find the block reference. Callers that actually need to handle the no-such-block case must now use the underlying function XLogRecGetBlockTagExtended. In addition to being a bit less error-prone, this should also serve to suppress some Coverity complaints about XLogRecGetBlockRefInfo. While at it, clean up some inconsistency about use of the XLogRecHasBlockRef macro: make XLogRecGetBlockTagExtended use that instead of open-coding the same condition, and avoid calling XLogRecHasBlockRef twice in relevant code paths. (That is, calling XLogRecHasBlockRef followed by XLogRecGetBlockTag is now deprecated: use XLogRecGetBlockTagExtended instead.) Patch HEAD only; this doesn't seem to have enough value to consider a back-branch API break. Discussion: https://postgr.es/m/425039.1649701221@sss.pgh.pa.us --- src/backend/access/heap/heapam.c | 2 +- src/backend/access/nbtree/nbtxlog.c | 2 +- src/backend/access/rmgrdesc/xlogdesc.c | 11 +++++------ src/backend/access/transam/xlogreader.c | 24 +++++++++++++++++------ src/backend/access/transam/xlogrecovery.c | 7 ++++--- src/backend/access/transam/xlogutils.c | 3 ++- src/bin/pg_rewind/parsexlog.c | 3 ++- src/bin/pg_waldump/pg_waldump.c | 5 ++--- src/include/access/xlogreader.h | 2 +- 9 files changed, 36 insertions(+), 23 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index a03122df8d..ba11bcd99e 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -9363,7 +9363,7 @@ heap_xlog_update(XLogReaderState *record, bool hot_update) oldtup.t_len = 0; XLogRecGetBlockTag(record, 0, &rnode, NULL, &newblk); - if (XLogRecGetBlockTag(record, 1, NULL, NULL, &oldblk)) + if (XLogRecGetBlockTagExtended(record, 1, NULL, NULL, &oldblk, NULL)) { /* HOT updates are never done across pages */ Assert(!hot_update); diff --git a/src/backend/access/nbtree/nbtxlog.c b/src/backend/access/nbtree/nbtxlog.c index fba124b940..f9186ca233 100644 --- a/src/backend/access/nbtree/nbtxlog.c +++ b/src/backend/access/nbtree/nbtxlog.c @@ -267,7 +267,7 @@ btree_xlog_split(bool newitemonleft, XLogReaderState *record) XLogRecGetBlockTag(record, 0, NULL, NULL, &origpagenumber); XLogRecGetBlockTag(record, 1, NULL, NULL, &rightpagenumber); - if (!XLogRecGetBlockTag(record, 2, NULL, NULL, &spagenumber)) + if (!XLogRecGetBlockTagExtended(record, 2, NULL, NULL, &spagenumber, NULL)) spagenumber = P_NONE; /* diff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c index dff1e7685e..c0dfea40c7 100644 --- a/src/backend/access/rmgrdesc/xlogdesc.c +++ b/src/backend/access/rmgrdesc/xlogdesc.c @@ -219,15 +219,14 @@ XLogRecGetBlockRefInfo(XLogReaderState *record, bool pretty, for (block_id = 0; block_id <= XLogRecMaxBlockId(record); block_id++) { - RelFileNode rnode = {InvalidOid, InvalidOid, InvalidOid}; - ForkNumber forknum = InvalidForkNumber; - BlockNumber blk = InvalidBlockNumber; + RelFileNode rnode; + ForkNumber forknum; + BlockNumber blk; - if (!XLogRecHasBlockRef(record, block_id)) + if (!XLogRecGetBlockTagExtended(record, block_id, + &rnode, &forknum, &blk, NULL)) continue; - XLogRecGetBlockTag(record, block_id, &rnode, &forknum, &blk); - if (detailed_format) { /* Get block references in detailed format. */ diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c index b3e37003ac..cf5db23cb8 100644 --- a/src/backend/access/transam/xlogreader.c +++ b/src/backend/access/transam/xlogreader.c @@ -37,6 +37,8 @@ #include "miscadmin.h" #include "pgstat.h" #include "utils/memutils.h" +#else +#include "common/logging.h" #endif static void report_invalid_record(XLogReaderState *state, const char *fmt,...) @@ -1918,14 +1920,25 @@ err: /* * Returns information about the block that a block reference refers to. - * See XLogRecGetBlockTagExtended(). + * + * This is like XLogRecGetBlockTagExtended, except that the block reference + * must exist and there's no access to prefetch_buffer. */ -bool +void XLogRecGetBlockTag(XLogReaderState *record, uint8 block_id, RelFileNode *rnode, ForkNumber *forknum, BlockNumber *blknum) { - return XLogRecGetBlockTagExtended(record, block_id, rnode, forknum, blknum, - NULL); + if (!XLogRecGetBlockTagExtended(record, block_id, rnode, forknum, blknum, + NULL)) + { +#ifndef FRONTEND + elog(ERROR, "failed to locate backup block with ID %d in WAL record", + block_id); +#else + pg_fatal("failed to locate backup block with ID %d in WAL record", + block_id); +#endif + } } /* @@ -1944,8 +1957,7 @@ XLogRecGetBlockTagExtended(XLogReaderState *record, uint8 block_id, { DecodedBkpBlock *bkpb; - if (block_id > record->record->max_block_id || - !record->record->blocks[block_id].in_use) + if (!XLogRecHasBlockRef(record, block_id)) return false; bkpb = &record->record->blocks[block_id]; diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index 4ee29182ac..26be94b3f1 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -2172,10 +2172,10 @@ xlog_block_info(StringInfo buf, XLogReaderState *record) ForkNumber forknum; BlockNumber blk; - if (!XLogRecHasBlockRef(record, block_id)) + if (!XLogRecGetBlockTagExtended(record, block_id, + &rnode, &forknum, &blk, NULL)) continue; - XLogRecGetBlockTag(record, block_id, &rnode, &forknum, &blk); if (forknum != MAIN_FORKNUM) appendStringInfo(buf, "; blkref #%d: rel %u/%u/%u, fork %u, blk %u", block_id, @@ -2303,7 +2303,8 @@ verifyBackupPageConsistency(XLogReaderState *record) Buffer buf; Page page; - if (!XLogRecGetBlockTag(record, block_id, &rnode, &forknum, &blkno)) + if (!XLogRecGetBlockTagExtended(record, block_id, + &rnode, &forknum, &blkno, NULL)) { /* * WAL record doesn't contain a block reference with the given id. diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c index b5d34c61e6..425702641a 100644 --- a/src/backend/access/transam/xlogutils.c +++ b/src/backend/access/transam/xlogutils.c @@ -369,7 +369,8 @@ XLogReadBufferForRedoExtended(XLogReaderState *record, &prefetch_buffer)) { /* Caller specified a bogus block_id */ - elog(PANIC, "failed to locate backup block with ID %d", block_id); + elog(PANIC, "failed to locate backup block with ID %d in WAL record", + block_id); } /* diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c index dfa836d156..87b9f75f2c 100644 --- a/src/bin/pg_rewind/parsexlog.c +++ b/src/bin/pg_rewind/parsexlog.c @@ -450,7 +450,8 @@ extractPageInfo(XLogReaderState *record) ForkNumber forknum; BlockNumber blkno; - if (!XLogRecGetBlockTag(record, block_id, &rnode, &forknum, &blkno)) + if (!XLogRecGetBlockTagExtended(record, block_id, + &rnode, &forknum, &blkno, NULL)) continue; /* We only care about the main fork; others are copied in toto */ diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c index 7b8b98cc96..4f265ef546 100644 --- a/src/bin/pg_waldump/pg_waldump.c +++ b/src/bin/pg_waldump/pg_waldump.c @@ -405,11 +405,10 @@ XLogRecordMatchesRelationBlock(XLogReaderState *record, ForkNumber forknum; BlockNumber blk; - if (!XLogRecHasBlockRef(record, block_id)) + if (!XLogRecGetBlockTagExtended(record, block_id, + &rnode, &forknum, &blk, NULL)) continue; - XLogRecGetBlockTag(record, block_id, &rnode, &forknum, &blk); - if ((matchFork == InvalidForkNumber || matchFork == forknum) && (RelFileNodeEquals(matchRnode, emptyRelFileNode) || RelFileNodeEquals(matchRnode, rnode)) && diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h index 727e9fe971..e73ea4a840 100644 --- a/src/include/access/xlogreader.h +++ b/src/include/access/xlogreader.h @@ -429,7 +429,7 @@ extern FullTransactionId XLogRecGetFullXid(XLogReaderState *record); extern bool RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page); extern char *XLogRecGetBlockData(XLogReaderState *record, uint8 block_id, Size *len); -extern bool XLogRecGetBlockTag(XLogReaderState *record, uint8 block_id, +extern void XLogRecGetBlockTag(XLogReaderState *record, uint8 block_id, RelFileNode *rnode, ForkNumber *forknum, BlockNumber *blknum); extern bool XLogRecGetBlockTagExtended(XLogReaderState *record, uint8 block_id,