Date: Wed, 18 Sep 2024 04:16:12 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: pgsql-hackers@lists.postgresql.org
Subject: define pg_structiszero(addr, s, r)
Message-ID: <ZupUDDyf1hHI4ibn@ip-10-97-1-34.eu-west-3.compute.internal>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="cRBB5InXlK7bLv09"
Content-Disposition: inline
Archived-At: <https://www.postgresql.org/message-id/ZupUDDyf1hHI4ibn%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk


--cRBB5InXlK7bLv09
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi hackers,

There is some places where we check that a struct is full of zeroes:

pgstat_report_bgwriter()
pgstat_report_checkpointer()
pgstat_relation_flush_cb()

Indeed that's the way we check if there is pending statistics to flush/report.

The current code is like (taking pgstat_relation_flush_cb() as an example):

"
static const PgStat_TableCounts all_zeroes;
.
.
if (memcmp(&lstats->counts, &all_zeroes,
          sizeof(PgStat_TableCounts)) == 0)
.
.
"

The static declaration is not "really" related to the purpose of the function
it is declared in. It's there "only" to initialize a memory area with zeroes 
and to use it in the memcmp.

I think it would make sense to "hide" all of this in a new macro, so please find
attached a patch proposal doing so (Andres suggested something along those lines
in [1] IIUC).

The macro is created in pgstat_internal.h as it looks like that "only" the 
statistics related code would benefit of it currently (could be moved to other
header file later on if needed).

[1]: https://www.postgresql.org/message-id/20230105002733.ealhzubjaiqis6ua%40awork3.anarazel.de

Looking forward to your feedback,

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

--cRBB5InXlK7bLv09
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v1-0001-define-pg_structiszero-addr-s-r.patch"

From 2e9071b54f7fd28027759de871b662a3eaa8e4a3 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Tue, 10 Sep 2024 01:22:02 +0000
Subject: [PATCH v1] define pg_structiszero(addr, s, r)

This new macro allows to test if a memory region (representing a struct "s")
starting at addr and of size sizeof(s) is full of zeroes.
---
 src/backend/utils/activity/pgstat_bgwriter.c     |  6 ++++--
 src/backend/utils/activity/pgstat_checkpointer.c |  9 +++++----
 src/backend/utils/activity/pgstat_relation.c     |  9 ++++-----
 src/include/utils/pgstat_internal.h              | 11 +++++++++++
 4 files changed, 24 insertions(+), 11 deletions(-)
  69.1% src/backend/utils/activity/
  30.8% src/include/utils/

diff --git a/src/backend/utils/activity/pgstat_bgwriter.c b/src/backend/utils/activity/pgstat_bgwriter.c
index 364a7a2024..61cbc27760 100644
--- a/src/backend/utils/activity/pgstat_bgwriter.c
+++ b/src/backend/utils/activity/pgstat_bgwriter.c
@@ -30,7 +30,7 @@ void
 pgstat_report_bgwriter(void)
 {
 	PgStatShared_BgWriter *stats_shmem = &pgStatLocal.shmem->bgwriter;
-	static const PgStat_BgWriterStats all_zeroes;
+	bool		is_all_zeroes;
 
 	Assert(!pgStatLocal.shmem->is_shutdown);
 	pgstat_assert_is_up();
@@ -39,7 +39,9 @@ pgstat_report_bgwriter(void)
 	 * This function can be called even if nothing at all has happened. In
 	 * this case, avoid unnecessarily modifying the stats entry.
 	 */
-	if (memcmp(&PendingBgWriterStats, &all_zeroes, sizeof(all_zeroes)) == 0)
+	pg_structiszero(&PendingBgWriterStats, PgStat_BgWriterStats, is_all_zeroes);
+
+	if (is_all_zeroes)
 		return;
 
 	pgstat_begin_changecount_write(&stats_shmem->changecount);
diff --git a/src/backend/utils/activity/pgstat_checkpointer.c b/src/backend/utils/activity/pgstat_checkpointer.c
index bbfc9c7e18..3b9b688ae8 100644
--- a/src/backend/utils/activity/pgstat_checkpointer.c
+++ b/src/backend/utils/activity/pgstat_checkpointer.c
@@ -29,8 +29,7 @@ PgStat_CheckpointerStats PendingCheckpointerStats = {0};
 void
 pgstat_report_checkpointer(void)
 {
-	/* We assume this initializes to zeroes */
-	static const PgStat_CheckpointerStats all_zeroes;
+	bool		is_all_zeroes;
 	PgStatShared_Checkpointer *stats_shmem = &pgStatLocal.shmem->checkpointer;
 
 	Assert(!pgStatLocal.shmem->is_shutdown);
@@ -40,8 +39,10 @@ pgstat_report_checkpointer(void)
 	 * This function can be called even if nothing at all has happened. In
 	 * this case, avoid unnecessarily modifying the stats entry.
 	 */
-	if (memcmp(&PendingCheckpointerStats, &all_zeroes,
-			   sizeof(all_zeroes)) == 0)
+	pg_structiszero(&PendingCheckpointerStats, PgStat_CheckpointerStats,
+					is_all_zeroes);
+
+	if (is_all_zeroes)
 		return;
 
 	pgstat_begin_changecount_write(&stats_shmem->changecount);
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index 8a3f7d434c..09e519a6ff 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -801,7 +801,7 @@ pgstat_twophase_postabort(TransactionId xid, uint16 info,
 bool
 pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
 {
-	static const PgStat_TableCounts all_zeroes;
+	bool		is_all_zeroes;
 	Oid			dboid;
 	PgStat_TableStatus *lstats; /* pending stats entry  */
 	PgStatShared_Relation *shtabstats;
@@ -816,11 +816,10 @@ pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
 	 * Ignore entries that didn't accumulate any actual counts, such as
 	 * indexes that were opened by the planner but not used.
 	 */
-	if (memcmp(&lstats->counts, &all_zeroes,
-			   sizeof(PgStat_TableCounts)) == 0)
-	{
+	pg_structiszero(&lstats->counts, PgStat_TableCounts, is_all_zeroes);
+
+	if (is_all_zeroes)
 		return true;
-	}
 
 	if (!pgstat_lock_entry(entry_ref, nowait))
 		return false;
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index bba90e898d..1fd91874d9 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -22,6 +22,17 @@
 #include "utils/dsa.h"
 
 
+/*
+ * Test if a memory region (representing a struct "s") starting
+ * at addr and of size sizeof(s) is full of zeroes.
+ */
+#define pg_structiszero(addr, s, r)									\
+	do {															\
+		/* We assume this initializes to zeroes */					\
+		static const s all_zeroes;									\
+		r = (memcmp(addr, &all_zeroes, sizeof(all_zeroes)) == 0);	\
+	} while (0)
+
 /*
  * Types related to shared memory storage of statistics.
  *
-- 
2.34.1


--cRBB5InXlK7bLv09--