Hi,
On Wed, Sep 18, 2024 at 03:07:15PM +0900, Michael Paquier wrote:
> On Wed, Sep 18, 2024 at 04:16:12AM +0000, Bertrand Drouvot wrote:
> > 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).
>
> I'm OK to add a helper macro in pgstat_internal.h as this is a pattern
> used only for some stats kinds (the other one I'm aware of is the
> allzero check for pages around bufmgr.c), cleaning up all these static
> declarations to make the memcpy() calls cheaper. That can also be
> useful for anybody doing a custom pgstats kind, fixed or
> variable-numbered.
Thanks for looking at it!
>
> #define pg_structiszero(addr, s, r) \
>
> Locating that at the top of pgstat_internal.h seems a bit out of order
> to me. Perhaps it would be better to move it closer to the inline
> functions?
Makes sense, done that way in v2 attached.
>
> Also, is this the best name to use here? Right, this is something
> that may be quite generic. However, if we limit its scope in the
> stats, perhaps this should be named pgstat_entry_all_zeros() or
> something like that?
Agree, we could still rename it later on if there is a need outside of
the statistics code area. Done in v2.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From bf4a9b39e1d6ff729f9b0d4bfbbca06b306dbe18 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@gmail.com>
Date: Tue, 10 Sep 2024 01:22:02 +0000
Subject: [PATCH v2] define pgstat_entry_all_zeros(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 | 7 +++++--
src/backend/utils/activity/pgstat_checkpointer.c | 9 +++++----
src/backend/utils/activity/pgstat_relation.c | 9 ++++-----
src/include/utils/pgstat_internal.h | 10 ++++++++++
4 files changed, 24 insertions(+), 11 deletions(-)
69.6% src/backend/utils/activity/
30.3% src/include/utils/
diff --git a/src/backend/utils/activity/pgstat_bgwriter.c b/src/backend/utils/activity/pgstat_bgwriter.c
index 364a7a2024..3f327241a2 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,10 @@ 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)
+ pgstat_entry_all_zeros(&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..be11d09482 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)
+ pgstat_entry_all_zeros(&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..a8f035dea4 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)
- {
+ pgstat_entry_all_zeros(&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 61b2e1f96b..a788b65446 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -528,6 +528,16 @@ typedef struct PgStat_LocalState
PgStat_Snapshot snapshot;
} PgStat_LocalState;
+/*
+ * Test if a memory region (representing a struct "s") starting
+ * at addr and of size sizeof(s) is full of zeroes.
+ */
+#define pgstat_entry_all_zeros(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)
/*
* Inline functions defined further below.
--
2.34.1
