Hi,
On Thu, Oct 31, 2024 at 09:59:35AM +0900, Michael Paquier wrote:
> On Thu, Oct 31, 2024 at 07:55:45AM +1100, Peter Smith wrote:
> > +/*
> > + * Test if a memory region starting at p and of size len is full of zeroes.
> > + */
> > +static inline bool
> > +pg_mem_is_all_zeros(const void *ptr, size_t len)
> >
> > The function comment should say 'ptr' instead of 'p', right?
>
> Yes.
Thank you both for looking at it. Agree, done in the new version attached.
> +static inline bool
> +pg_mem_is_all_zeros(const void *ptr, size_t len)
>
> While we're talking about wordsmithing things, I would not choose
> "mem" for this routine, just stick to "memory". There is not much in
> the code that does memory-specific things like what you are proposing
> here. Still, this would be more consistent with the macros for memory
> barriers at least. Hence, "pg_memory_is_all_zeros()" makes more
> sense?
That makes sense to me, done that way in the attached.
> The location of memutils.h is sensible.
Thanks for sharing your thoughts on it.
> + if (pg_mem_is_all_zeros(pagebytes , (BLCKSZ / sizeof(size_t))))
>
> Extra space not required after pagebytes.
Fat finger here, thanks!
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From 88f5ebbbee3cdc7b773a1d0f5a7cf6f9d909b784 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@gmail.com>
Date: Tue, 10 Sep 2024 01:22:02 +0000
Subject: [PATCH v6] New pg_memory_is_all_zeros(ptr, len) inline function
This new function allows to test if a memory region starting at ptr and of
size len is full of zeroes.
---
src/backend/storage/page/bufpage.c | 13 +------------
src/backend/utils/activity/pgstat_bgwriter.c | 5 +++--
src/backend/utils/activity/pgstat_checkpointer.c | 7 +++----
src/backend/utils/activity/pgstat_relation.c | 7 ++-----
src/include/utils/memutils.h | 16 ++++++++++++++++
5 files changed, 25 insertions(+), 23 deletions(-)
19.6% src/backend/storage/page/
58.8% src/backend/utils/activity/
21.4% src/include/utils/
diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index be6f1f62d2..5ee1e58cd4 100644
--- a/src/backend/storage/page/bufpage.c
+++ b/src/backend/storage/page/bufpage.c
@@ -89,10 +89,8 @@ PageIsVerifiedExtended(Page page, BlockNumber blkno, int flags)
{
PageHeader p = (PageHeader) page;
size_t *pagebytes;
- int i;
bool checksum_failure = false;
bool header_sane = false;
- bool all_zeroes = false;
uint16 checksum = 0;
/*
@@ -126,18 +124,9 @@ PageIsVerifiedExtended(Page page, BlockNumber blkno, int flags)
}
/* Check all-zeroes case */
- all_zeroes = true;
pagebytes = (size_t *) page;
- for (i = 0; i < (BLCKSZ / sizeof(size_t)); i++)
- {
- if (pagebytes[i] != 0)
- {
- all_zeroes = false;
- break;
- }
- }
- if (all_zeroes)
+ if (pg_memory_is_all_zeros(pagebytes, (BLCKSZ / sizeof(size_t))))
return true;
/*
diff --git a/src/backend/utils/activity/pgstat_bgwriter.c b/src/backend/utils/activity/pgstat_bgwriter.c
index 364a7a2024..85d53d82f2 100644
--- a/src/backend/utils/activity/pgstat_bgwriter.c
+++ b/src/backend/utils/activity/pgstat_bgwriter.c
@@ -17,6 +17,7 @@
#include "postgres.h"
+#include "utils/memutils.h"
#include "utils/pgstat_internal.h"
@@ -30,7 +31,6 @@ void
pgstat_report_bgwriter(void)
{
PgStatShared_BgWriter *stats_shmem = &pgStatLocal.shmem->bgwriter;
- static const PgStat_BgWriterStats all_zeroes;
Assert(!pgStatLocal.shmem->is_shutdown);
pgstat_assert_is_up();
@@ -39,7 +39,8 @@ 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)
+ if (pg_memory_is_all_zeros(&PendingBgWriterStats,
+ sizeof(struct PgStat_BgWriterStats)))
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..258337cfb8 100644
--- a/src/backend/utils/activity/pgstat_checkpointer.c
+++ b/src/backend/utils/activity/pgstat_checkpointer.c
@@ -17,6 +17,7 @@
#include "postgres.h"
+#include "utils/memutils.h"
#include "utils/pgstat_internal.h"
@@ -29,8 +30,6 @@ PgStat_CheckpointerStats PendingCheckpointerStats = {0};
void
pgstat_report_checkpointer(void)
{
- /* We assume this initializes to zeroes */
- static const PgStat_CheckpointerStats all_zeroes;
PgStatShared_Checkpointer *stats_shmem = &pgStatLocal.shmem->checkpointer;
Assert(!pgStatLocal.shmem->is_shutdown);
@@ -40,8 +39,8 @@ 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)
+ if (pg_memory_is_all_zeros(&PendingCheckpointerStats,
+ sizeof(struct PgStat_CheckpointerStats)))
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..337954d15e 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -801,7 +801,6 @@ pgstat_twophase_postabort(TransactionId xid, uint16 info,
bool
pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
{
- static const PgStat_TableCounts all_zeroes;
Oid dboid;
PgStat_TableStatus *lstats; /* pending stats entry */
PgStatShared_Relation *shtabstats;
@@ -816,11 +815,9 @@ 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)
- {
+ if (pg_memory_is_all_zeros(&lstats->counts,
+ sizeof(struct PgStat_TableCounts)))
return true;
- }
if (!pgstat_lock_entry(entry_ref, nowait))
return false;
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index cd9596ff21..0094492d9c 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -189,4 +189,20 @@ extern MemoryContext BumpContextCreate(MemoryContext parent,
#define SLAB_DEFAULT_BLOCK_SIZE (8 * 1024)
#define SLAB_LARGE_BLOCK_SIZE (8 * 1024 * 1024)
+/*
+ * Test if a memory region starting at ptr and of size len is full of zeroes.
+ */
+static inline bool
+pg_memory_is_all_zeros(const void *ptr, size_t len)
+{
+ const char *p = (const char *) ptr;
+
+ for (size_t i = 0; i < len; i++)
+ {
+ if (p[i] != 0)
+ return false;
+ }
+ return true;
+}
+
#endif /* MEMUTILS_H */
--
2.34.1
