Fixing code that ignores failure of XLogRecGetBlockTag - Mailing list pgsql-hackers

Currently, XLogRecGetBlockTag has 41 callers, of which only four
bother to check the function's result.  The remainder take it on
faith that they got valid data back, and many of them will
misbehave in seriously nasty ways if they didn't.  (This point
was drawn to my attention by a Coverity complaint.)

I think we should make this a little less fragile.  Since we
already have XLogRecGetBlockTagExtended, I propose that callers
that need to handle the case of no-such-block must use that,
while XLogRecGetBlockTag throws an error.  The attached patch
fixes that up, and also cleans up some random inconsistency
about use of XLogRecHasBlockRef().

            regards, tom lane

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,