pgstat: Allow checksum errors to be reported in critical sections

anarazel · anarazel · commit b96d3c389755 · 2025-03-30T16:12:04.000-04:00
For AIO we execute completion callbacks in critical sections (to ensure that AIO can in the future be used for WAL, which in turn requires that we can call completion callbacks in critical sections, to get the resources for WAL io). To report checksum errors a backend now has to call pgstat_prepare_report_checksum_failure(), before entering a critical section, which guarantees the relevant pgstats entry is in shared memory, the relevant DSM segment is mapped into the backend's memory and the address is known via a PgStat_EntryRef. Reviewed-by: Noah Misch <noah@leadboat.com> Discussion: https://postgr.es/m/wkjj4p2rmkevutkwc6tewoovdqznj6c6nvjmvii4oo5wmbh5sr@retq7d6uqs4j
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
@@ -1817,6 +1817,7 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
 							   checksum_failures,
 							   readfilename, checksum_failures)));
 
+		pgstat_prepare_report_checksum_failure(dboid);
 		pgstat_report_checksum_failures_in_db(dboid, checksum_failures);
 	}
 
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
@@ -524,6 +524,7 @@ RelationCopyStorage(SMgrRelation src, SMgrRelation dst,
 		{
 			RelFileLocatorBackend rloc = src->smgr_rlocator;
 
+			pgstat_prepare_report_checksum_failure(rloc.locator.dbOid);
 			pgstat_report_checksum_failures_in_db(rloc.locator.dbOid, 1);
 		}
 
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
@@ -1590,6 +1590,7 @@ WaitReadBuffers(ReadBuffersOperation *operation)
 			{
 				RelFileLocatorBackend rloc = operation->smgr->smgr_rlocator;
 
+				pgstat_prepare_report_checksum_failure(rloc.locator.dbOid);
 				pgstat_report_checksum_failures_in_db(rloc.locator.dbOid, 1);
 			}
 
diff --git a/src/backend/utils/activity/pgstat_database.c b/src/backend/utils/activity/pgstat_database.c
@@ -133,8 +133,34 @@ pgstat_report_deadlock(void)
 	dbent->deadlocks++;
 }
 
+/*
+ * Allow this backend to later report checksum failures for dboid, even if in
+ * a critical section at the time of the report.
+ *
+ * Without this function having been called first, the backend might need to
+ * allocate an EntryRef or might need to map in DSM segments. Neither should
+ * happen in a critical section.
+ */
+void
+pgstat_prepare_report_checksum_failure(Oid dboid)
+{
+	Assert(!CritSectionCount);
+
+	/*
+	 * Just need to ensure this backend has an entry ref for the database.
+	 * That will allows us to report checksum failures without e.g. needing to
+	 * map in DSM segments.
+	 */
+	pgstat_get_entry_ref(PGSTAT_KIND_DATABASE, dboid, InvalidOid,
+						 true, NULL);
+}
+
 /*
  * Report one or more checksum failures.
+ *
+ * To be allowed to report checksum failures in critical sections, we require
+ * pgstat_prepare_report_checksum_failure() to have been called before this
+ * function is called.
  */
 void
 pgstat_report_checksum_failures_in_db(Oid dboid, int failurecount)
@@ -147,10 +173,29 @@ pgstat_report_checksum_failures_in_db(Oid dboid, int failurecount)
 
 	/*
 	 * Update the shared stats directly - checksum failures should never be
-	 * common enough for that to be a problem.
+	 * common enough for that to be a problem. Note that we pass create=false
+	 * here, as we want to be sure to not require memory allocations, so this
+	 * can be called in critical sections.
+	 */
+	entry_ref = pgstat_get_entry_ref(PGSTAT_KIND_DATABASE, dboid, InvalidOid,
+									 false, NULL);
+
+	/*
+	 * Should always have been created by
+	 * pgstat_prepare_report_checksum_failure().
+	 *
+	 * When not using assertions, we don't want to crash should something have
+	 * gone wrong, so just return.
 	 */
-	entry_ref =
-		pgstat_get_entry_ref_locked(PGSTAT_KIND_DATABASE, dboid, InvalidOid, false);
+	Assert(entry_ref);
+	if (!entry_ref)
+	{
+		elog(WARNING, "could not report %d conflicts for DB %u",
+			 failurecount, dboid);
+		return;
+	}
+
+	pgstat_lock_entry(entry_ref, false);
 
 	sharedent = (PgStatShared_Database *) entry_ref->shared_stats;
 	sharedent->stats.checksum_failures += failurecount;
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
@@ -611,6 +611,7 @@ extern void pgstat_drop_database(Oid databaseid);
 extern void pgstat_report_autovac(Oid dboid);
 extern void pgstat_report_recovery_conflict(int reason);
 extern void pgstat_report_deadlock(void);
+extern void pgstat_prepare_report_checksum_failure(Oid dboid);
 extern void pgstat_report_checksum_failures_in_db(Oid dboid, int failurecount);
 extern void pgstat_report_connect(Oid dboid);
 extern void pgstat_update_parallel_workers_stats(PgStat_Counter workers_to_launch,

Original file line number	Diff line number	Diff line change
`@@ -1817,6 +1817,7 @@ sendFile(bbsink sink, const char readfilename, const char *tarfilename,`
`1817`	`1817`	`checksum_failures,`
`1818`	`1818`	`readfilename, checksum_failures)));`
`1819`	`1819`
	`1820`	`+ pgstat_prepare_report_checksum_failure(dboid);`
`1820`	`1821`	`pgstat_report_checksum_failures_in_db(dboid, checksum_failures);`
`1821`	`1822`	`}`
`1822`	`1823`
Original file line number	Diff line number	Diff line change
`@@ -524,6 +524,7 @@ RelationCopyStorage(SMgrRelation src, SMgrRelation dst,`
`524`	`524`	`{`
`525`	`525`	`RelFileLocatorBackend rloc = src->smgr_rlocator;`
`526`	`526`
	`527`	`+ pgstat_prepare_report_checksum_failure(rloc.locator.dbOid);`
`527`	`528`	`pgstat_report_checksum_failures_in_db(rloc.locator.dbOid, 1);`
`528`	`529`	`}`
`529`	`530`
Original file line number	Diff line number	Diff line change
`@@ -1590,6 +1590,7 @@ WaitReadBuffers(ReadBuffersOperation *operation)`
`1590`	`1590`	`{`
`1591`	`1591`	`RelFileLocatorBackend rloc = operation->smgr->smgr_rlocator;`
`1592`	`1592`
	`1593`	`+ pgstat_prepare_report_checksum_failure(rloc.locator.dbOid);`
`1593`	`1594`	`pgstat_report_checksum_failures_in_db(rloc.locator.dbOid, 1);`
`1594`	`1595`	`}`
`1595`	`1596`