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

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index a03122df8d60c816533e10b3fb56e9aa7234a012..ba11bcd99e9cbc2531bf2770e1230dd6714dbef5 100644 (file)
--- 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 fba124b940480c1230021a0bffd1d7953ed3310b..f9186ca233ae4523f4439d662919ea510f1edb00 100644 (file)
--- 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 dff1e7685e9b7ea07df022e7e7c2f8859e69807c..c0dfea40c70f3b209760f0ca0d95518b93c65179 100644 (file)
--- 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 b3e37003ac5ea4a77f6dd84292d9e2aa8270f2c4..cf5db23cb86bf8934fadf0617435969f8793771a 100644 (file)
--- 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 4ee29182ac864e9aa260e5591bd048d4ada3021a..26be94b3f19eab95a27851ebb810607a46ffd81b 100644 (file)
--- 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 b5d34c61e6669d42f2b177f8a049122dce9b056a..425702641a6767af125f633a5676e057f124dd62 100644 (file)
--- 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 dfa836d1561292bf22b59c76923195bc1aafc863..87b9f75f2c02d71479327fc32ca60c804a7cb701 100644 (file)
--- 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 7b8b98cc96796fddc495c7df9d6cc46ab0fe9265..4f265ef54605820dbf5019f157350b8784c077e3 100644 (file)
--- 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 727e9fe971838ff95b9ed781f87c51bf382ecec5..e73ea4a8408e179d5ecf4aff71e9612f96c7c7e5 100644 (file)
--- 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,