pg_stat_database.checksum_failures vs shared relations

From 24d9e2454662dfda73f2b519368ff7a96ef15f65 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Fri, 28 Mar 2025 12:29:40 -0400
Subject: [PATCH] Fix mis-attribution of checksum failure stats to the wrong
 database

Checksum failure stats could be attributed to the wrong database in two cases:

- when a read of a shared relation encountered a checksum error , it would be
  attributed to the current database, instead of the "database" representing
  shared relations

- when using CREATE DATABASE ... STRATEGY WAL_LOG checksum errors in the
  source database would be attributed to the current database

The checksum stats reporting via PageIsVerifiedExtended(PIV_REPORT_STAT) does
not have access to the information about what database a page belongs to.

This fixes the issue by removing PIV_REPORT_STAT and delegating the
responsibility to report stats to the caller, which now can learn about the
number of stats via a new optional argument.

As this changes the signature of PageIsVerifiedExtended() and all callers
should adapt to the new signature, use the occasion to rename the function to
PageIsVerified() and remove the compatibility macro.

We could instead have fixed this by adding information about the database to
the args of PageIsVerified(), but there are soon-to-be-applied patches that
need to separate the stats reporting from the PageIsVerified() call
anyway. Those patches also include testing for the failure paths, something we
inexplicably have not had.

As there is no caller of pgstat_report_checksum_failure() left, remove it.

It'd be possible, but awkward to fix this in the back branches. We considered
doing the work not quite worth it, as mis-attributed stats should still elicit
concern. The emitted error messages do allow to attribute the errors
correctly.

Discussion: https://postgr.es/m/5tyic6epvdlmd6eddgelv47syg2b5cpwffjam54axp25xyq2ga@ptwkinxqo3az
Discussion: https://postgr.es/m/mglpvvbhighzuwudjxzu4br65qqcxsnyvio3nl4fbog3qknwhg@e4gt7npsohuz
---
 src/include/pgstat.h                         |  1 -
 src/include/storage/bufpage.h                | 20 ++++++++------------
 src/backend/catalog/storage.c                | 16 ++++++++++++++--
 src/backend/storage/buffer/bufmgr.c          | 16 +++++++++++++---
 src/backend/storage/page/bufpage.c           | 20 +++++++++++++-------
 src/backend/utils/activity/pgstat_database.c |  9 ---------
 6 files changed, 48 insertions(+), 34 deletions(-)

diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 5bfe19e66be..9f3d13bf1ce 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -612,7 +612,6 @@ extern void pgstat_report_autovac(Oid dboid);
 extern void pgstat_report_recovery_conflict(int reason);
 extern void pgstat_report_deadlock(void);
 extern void pgstat_report_checksum_failures_in_db(Oid dboid, int failurecount);
-extern void pgstat_report_checksum_failure(void);
 extern void pgstat_report_connect(Oid dboid);
 extern void pgstat_update_parallel_workers_stats(PgStat_Counter workers_to_launch,
 												 PgStat_Counter workers_launched);
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index 6646b6f6371..b943db707db 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -465,31 +465,27 @@ do { \
 #define PAI_OVERWRITE			(1 << 0)
 #define PAI_IS_HEAP				(1 << 1)
 
-/* flags for PageIsVerifiedExtended() */
+/* flags for PageIsVerified() */
 #define PIV_LOG_WARNING			(1 << 0)
-#define PIV_REPORT_STAT			(1 << 1)
 
 #define PageAddItem(page, item, size, offsetNumber, overwrite, is_heap) \
 	PageAddItemExtended(page, item, size, offsetNumber, \
 						((overwrite) ? PAI_OVERWRITE : 0) | \
 						((is_heap) ? PAI_IS_HEAP : 0))
 
-#define PageIsVerified(page, blkno) \
-	PageIsVerifiedExtended(page, blkno, \
-						   PIV_LOG_WARNING | PIV_REPORT_STAT)
-
 /*
- * Check that BLCKSZ is a multiple of sizeof(size_t).  In
- * PageIsVerifiedExtended(), it is much faster to check if a page is
- * full of zeroes using the native word size.  Note that this assertion
- * is kept within a header to make sure that StaticAssertDecl() works
- * across various combinations of platforms and compilers.
+ * Check that BLCKSZ is a multiple of sizeof(size_t).  In PageIsVerified(), it
+ * is much faster to check if a page is full of zeroes using the native word
+ * size.  Note that this assertion is kept within a header to make sure that
+ * StaticAssertDecl() works across various combinations of platforms and
+ * compilers.
  */
 StaticAssertDecl(BLCKSZ == ((BLCKSZ / sizeof(size_t)) * sizeof(size_t)),
 				 "BLCKSZ has to be a multiple of sizeof(size_t)");
 
 extern void PageInit(Page page, Size pageSize, Size specialSize);
-extern bool PageIsVerifiedExtended(PageData *page, BlockNumber blkno, int flags);
+extern bool PageIsVerified(PageData *page, BlockNumber blkno, int flags,
+						   bool *checksum_failure_p);
 extern OffsetNumber PageAddItemExtended(Page page, Item item, Size size,
 										OffsetNumber offsetNumber, int flags);
 extern Page PageGetTempPage(const PageData *page);
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index 624ed41bbf3..cacf16c1cdb 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -27,6 +27,7 @@
 #include "catalog/storage.h"
 #include "catalog/storage_xlog.h"
 #include "miscadmin.h"
+#include "pgstat.h"
 #include "storage/bulk_write.h"
 #include "storage/freespace.h"
 #include "storage/proc.h"
@@ -507,6 +508,8 @@ RelationCopyStorage(SMgrRelation src, SMgrRelation dst,
 	for (blkno = 0; blkno < nblocks; blkno++)
 	{
 		BulkWriteBuffer buf;
+		bool		checksum_failure;
+		bool		verified;
 
 		/* If we got a cancel signal during the copy of the data, quit */
 		CHECK_FOR_INTERRUPTS();
@@ -514,8 +517,17 @@ RelationCopyStorage(SMgrRelation src, SMgrRelation dst,
 		buf = smgr_bulk_get_buf(bulkstate);
 		smgrread(src, forkNum, blkno, (Page) buf);
 
-		if (!PageIsVerifiedExtended((Page) buf, blkno,
-									PIV_LOG_WARNING | PIV_REPORT_STAT))
+		verified = PageIsVerified((Page) buf, blkno, PIV_LOG_WARNING,
+								  &checksum_failure);
+
+		if (checksum_failure)
+		{
+			RelFileLocatorBackend rloc = src->smgr_rlocator;
+
+			pgstat_report_checksum_failures_in_db(rloc.locator.dbOid, 1);
+		}
+
+		if (!verified)
 		{
 			/*
 			 * For paranoia's sake, capture the file path before invoking the
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 323382dcfa8..5cac8cd7389 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -770,7 +770,7 @@ ReadBuffer(Relation reln, BlockNumber blockNum)
  * In RBM_NORMAL mode, the page is read from disk, and the page header is
  * validated.  An error is thrown if the page header is not valid.  (But
  * note that an all-zero page is considered "valid"; see
- * PageIsVerifiedExtended().)
+ * PageIsVerified().)
  *
  * RBM_ZERO_ON_ERROR is like the normal mode, but if the page header is not
  * valid, the page is zeroed instead of throwing an error. This is intended
@@ -1569,6 +1569,8 @@ WaitReadBuffers(ReadBuffersOperation *operation)
 		{
 			BufferDesc *bufHdr;
 			Block		bufBlock;
+			bool		verified;
+			bool		checksum_failure;
 
 			if (persistence == RELPERSISTENCE_TEMP)
 			{
@@ -1582,8 +1584,16 @@ WaitReadBuffers(ReadBuffersOperation *operation)
 			}
 
 			/* check for garbage data */
-			if (!PageIsVerifiedExtended((Page) bufBlock, io_first_block + j,
-										PIV_LOG_WARNING | PIV_REPORT_STAT))
+			verified = PageIsVerified((Page) bufBlock, io_first_block + j,
+									  PIV_LOG_WARNING, &checksum_failure);
+			if (checksum_failure)
+			{
+				RelFileLocatorBackend rloc = operation->smgr->smgr_rlocator;
+
+				pgstat_report_checksum_failures_in_db(rloc.locator.dbOid, 1);
+			}
+
+			if (!verified)
 			{
 				if ((operation->flags & READ_BUFFERS_ZERO_ON_ERROR) || zero_damaged_pages)
 				{
diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index ecc81aacfc3..5d1b039fcbb 100644
--- a/src/backend/storage/page/bufpage.c
+++ b/src/backend/storage/page/bufpage.c
@@ -61,7 +61,7 @@ PageInit(Page page, Size pageSize, Size specialSize)
 
 
 /*
- * PageIsVerifiedExtended
+ * PageIsVerified
  *		Check that the page header and checksum (if any) appear valid.
  *
  * This is called when a page has just been read in from disk.  The idea is
@@ -81,11 +81,13 @@ PageInit(Page page, Size pageSize, Size specialSize)
  * If flag PIV_LOG_WARNING is set, a WARNING is logged in the event of
  * a checksum failure.
  *
- * If flag PIV_REPORT_STAT is set, a checksum failure is reported directly
- * to pgstat.
+ * To allow the caller to report statistics about checksum failures,
+ * *checksum_failure_p can be passed in. Note that there may be checksum
+ * failures even if this function returns true, due to
+ * ignore_checksum_failure.
  */
 bool
-PageIsVerifiedExtended(PageData *page, BlockNumber blkno, int flags)
+PageIsVerified(PageData *page, BlockNumber blkno, int flags, bool *checksum_failure_p)
 {
 	const PageHeaderData *p = (const PageHeaderData *) page;
 	size_t	   *pagebytes;
@@ -93,6 +95,9 @@ PageIsVerifiedExtended(PageData *page, BlockNumber blkno, int flags)
 	bool		header_sane = false;
 	uint16		checksum = 0;
 
+	if (checksum_failure_p)
+		*checksum_failure_p = false;
+
 	/*
 	 * Don't verify page data unless the page passes basic non-zero test
 	 */
@@ -103,7 +108,11 @@ PageIsVerifiedExtended(PageData *page, BlockNumber blkno, int flags)
 			checksum = pg_checksum_page(page, blkno);
 
 			if (checksum != p->pd_checksum)
+			{
 				checksum_failure = true;
+				if (checksum_failure_p)
+					*checksum_failure_p = true;
+			}
 		}
 
 		/*
@@ -141,9 +150,6 @@ PageIsVerifiedExtended(PageData *page, BlockNumber blkno, int flags)
 					 errmsg("page verification failed, calculated checksum %u but expected %u",
 							checksum, p->pd_checksum)));
 
-		if ((flags & PIV_REPORT_STAT) != 0)
-			pgstat_report_checksum_failure();
-
 		if (header_sane && ignore_checksum_failure)
 			return true;
 	}
diff --git a/src/backend/utils/activity/pgstat_database.c b/src/backend/utils/activity/pgstat_database.c
index 05a8ccfdb75..c0c02efd9d3 100644
--- a/src/backend/utils/activity/pgstat_database.c
+++ b/src/backend/utils/activity/pgstat_database.c
@@ -159,15 +159,6 @@ pgstat_report_checksum_failures_in_db(Oid dboid, int failurecount)
 	pgstat_unlock_entry(entry_ref);
 }
 
-/*
- * Report one checksum failure in the current database.
- */
-void
-pgstat_report_checksum_failure(void)
-{
-	pgstat_report_checksum_failures_in_db(MyDatabaseId, 1);
-}
-
 /*
  * Report creation of temporary file.
  */
-- 
2.48.1.76.g4e746b1a31.dirty