Hi, Attached is a fix for the issue.
I looked around and didn't find extensions using PageIsVerified[Extended]() in codesearch.debian.org, so I got rid of the compat macro and renamed PageIsVerifiedExtended back to PageIsVerified(). Normally I'd commit tests as part of a fix like this, but since I've already written test infrastructure for checksum failures and their stats as part of aio, and those tests don't work without more of aio applied, I don't think it makes sense to write them for just this test. It's not like anybody has ever bothered to test checksum failures before... Greetings, Andres Freund
>From 24d9e2454662dfda73f2b519368ff7a96ef15f65 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@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
