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
This commit is contained in:
Tom Lane 2022-04-11 17:43:46 -04:00
parent 9debd12348
commit bd037dc928
9 changed files with 36 additions and 23 deletions

View File

@ -9363,7 +9363,7 @@ heap_xlog_update(XLogReaderState *record, bool hot_update)
oldtup.t_len = 0; oldtup.t_len = 0;
XLogRecGetBlockTag(record, 0, &rnode, NULL, &newblk); 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 */ /* HOT updates are never done across pages */
Assert(!hot_update); Assert(!hot_update);

View File

@ -267,7 +267,7 @@ btree_xlog_split(bool newitemonleft, XLogReaderState *record)
XLogRecGetBlockTag(record, 0, NULL, NULL, &origpagenumber); XLogRecGetBlockTag(record, 0, NULL, NULL, &origpagenumber);
XLogRecGetBlockTag(record, 1, NULL, NULL, &rightpagenumber); XLogRecGetBlockTag(record, 1, NULL, NULL, &rightpagenumber);
if (!XLogRecGetBlockTag(record, 2, NULL, NULL, &spagenumber)) if (!XLogRecGetBlockTagExtended(record, 2, NULL, NULL, &spagenumber, NULL))
spagenumber = P_NONE; spagenumber = P_NONE;
/* /*

View File

@ -219,15 +219,14 @@ XLogRecGetBlockRefInfo(XLogReaderState *record, bool pretty,
for (block_id = 0; block_id <= XLogRecMaxBlockId(record); block_id++) for (block_id = 0; block_id <= XLogRecMaxBlockId(record); block_id++)
{ {
RelFileNode rnode = {InvalidOid, InvalidOid, InvalidOid}; RelFileNode rnode;
ForkNumber forknum = InvalidForkNumber; ForkNumber forknum;
BlockNumber blk = InvalidBlockNumber; BlockNumber blk;
if (!XLogRecHasBlockRef(record, block_id)) if (!XLogRecGetBlockTagExtended(record, block_id,
&rnode, &forknum, &blk, NULL))
continue; continue;
XLogRecGetBlockTag(record, block_id, &rnode, &forknum, &blk);
if (detailed_format) if (detailed_format)
{ {
/* Get block references in detailed format. */ /* Get block references in detailed format. */

View File

@ -37,6 +37,8 @@
#include "miscadmin.h" #include "miscadmin.h"
#include "pgstat.h" #include "pgstat.h"
#include "utils/memutils.h" #include "utils/memutils.h"
#else
#include "common/logging.h"
#endif #endif
static void report_invalid_record(XLogReaderState *state, const char *fmt,...) 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. * 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, XLogRecGetBlockTag(XLogReaderState *record, uint8 block_id,
RelFileNode *rnode, ForkNumber *forknum, BlockNumber *blknum) RelFileNode *rnode, ForkNumber *forknum, BlockNumber *blknum)
{ {
return XLogRecGetBlockTagExtended(record, block_id, rnode, forknum, blknum, if (!XLogRecGetBlockTagExtended(record, block_id, rnode, forknum, blknum,
NULL); 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; DecodedBkpBlock *bkpb;
if (block_id > record->record->max_block_id || if (!XLogRecHasBlockRef(record, block_id))
!record->record->blocks[block_id].in_use)
return false; return false;
bkpb = &record->record->blocks[block_id]; bkpb = &record->record->blocks[block_id];

View File

@ -2172,10 +2172,10 @@ xlog_block_info(StringInfo buf, XLogReaderState *record)
ForkNumber forknum; ForkNumber forknum;
BlockNumber blk; BlockNumber blk;
if (!XLogRecHasBlockRef(record, block_id)) if (!XLogRecGetBlockTagExtended(record, block_id,
&rnode, &forknum, &blk, NULL))
continue; continue;
XLogRecGetBlockTag(record, block_id, &rnode, &forknum, &blk);
if (forknum != MAIN_FORKNUM) if (forknum != MAIN_FORKNUM)
appendStringInfo(buf, "; blkref #%d: rel %u/%u/%u, fork %u, blk %u", appendStringInfo(buf, "; blkref #%d: rel %u/%u/%u, fork %u, blk %u",
block_id, block_id,
@ -2303,7 +2303,8 @@ verifyBackupPageConsistency(XLogReaderState *record)
Buffer buf; Buffer buf;
Page page; 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. * WAL record doesn't contain a block reference with the given id.

View File

@ -369,7 +369,8 @@ XLogReadBufferForRedoExtended(XLogReaderState *record,
&prefetch_buffer)) &prefetch_buffer))
{ {
/* Caller specified a bogus block_id */ /* 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);
} }
/* /*

View File

@ -450,7 +450,8 @@ extractPageInfo(XLogReaderState *record)
ForkNumber forknum; ForkNumber forknum;
BlockNumber blkno; BlockNumber blkno;
if (!XLogRecGetBlockTag(record, block_id, &rnode, &forknum, &blkno)) if (!XLogRecGetBlockTagExtended(record, block_id,
&rnode, &forknum, &blkno, NULL))
continue; continue;
/* We only care about the main fork; others are copied in toto */ /* We only care about the main fork; others are copied in toto */

View File

@ -405,11 +405,10 @@ XLogRecordMatchesRelationBlock(XLogReaderState *record,
ForkNumber forknum; ForkNumber forknum;
BlockNumber blk; BlockNumber blk;
if (!XLogRecHasBlockRef(record, block_id)) if (!XLogRecGetBlockTagExtended(record, block_id,
&rnode, &forknum, &blk, NULL))
continue; continue;
XLogRecGetBlockTag(record, block_id, &rnode, &forknum, &blk);
if ((matchFork == InvalidForkNumber || matchFork == forknum) && if ((matchFork == InvalidForkNumber || matchFork == forknum) &&
(RelFileNodeEquals(matchRnode, emptyRelFileNode) || (RelFileNodeEquals(matchRnode, emptyRelFileNode) ||
RelFileNodeEquals(matchRnode, rnode)) && RelFileNodeEquals(matchRnode, rnode)) &&

View File

@ -429,7 +429,7 @@ extern FullTransactionId XLogRecGetFullXid(XLogReaderState *record);
extern bool RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page); extern bool RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page);
extern char *XLogRecGetBlockData(XLogReaderState *record, uint8 block_id, Size *len); 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, RelFileNode *rnode, ForkNumber *forknum,
BlockNumber *blknum); BlockNumber *blknum);
extern bool XLogRecGetBlockTagExtended(XLogReaderState *record, uint8 block_id, extern bool XLogRecGetBlockTagExtended(XLogReaderState *record, uint8 block_id,