Hi,
On Mon, Oct 28, 2024 at 04:32:51PM +0200, Heikki Linnakangas wrote:
> On 18/09/2024 21:57, Bertrand Drouvot wrote:
> > On Wed, Sep 18, 2024 at 10:03:21AM +0200, Peter Eisentraut wrote:
> > > On 18.09.24 06:16, Bertrand Drouvot wrote:
> > > > +#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)
>
> Not new with this patch
Thanks for looking at it!
> but do we guarantee padding bytes to be zeros?
I can see padding in one of the 3 structs of interest: PgStat_BgWriterStats and
PgStat_CheckpointerStats have no padding but PgStat_TableCounts has:
(gdb) ptype /o struct PgStat_TableCounts
/* offset | size */ type = struct PgStat_TableCounts {
/* 0 | 8 */ PgStat_Counter numscans;
/* 8 | 8 */ PgStat_Counter tuples_returned;
/* 16 | 8 */ PgStat_Counter tuples_fetched;
/* 24 | 8 */ PgStat_Counter tuples_inserted;
/* 32 | 8 */ PgStat_Counter tuples_updated;
/* 40 | 8 */ PgStat_Counter tuples_deleted;
/* 48 | 8 */ PgStat_Counter tuples_hot_updated;
/* 56 | 8 */ PgStat_Counter tuples_newpage_updated;
/* 64 | 1 */ _Bool truncdropped;
/* XXX 7-byte hole */
/* 72 | 8 */ PgStat_Counter delta_live_tuples;
/* 80 | 8 */ PgStat_Counter delta_dead_tuples;
/* 88 | 8 */ PgStat_Counter changed_tuples;
/* 96 | 8 */ PgStat_Counter blocks_fetched;
/* 104 | 8 */ PgStat_Counter blocks_hit;
/* total size (bytes): 112 */
}
According to my testing, I can see that "static const PgStat_TableCounts
all_zeroes"
all_zeroes is fully made of zeros (padding included). OTOH I would not bet on
the
fact that's guaranteed to be the case 100% of the time.
But even, if that is not the case I don't think that it is a big issue: the
check is in pgstat_relation_flush_cb() to decide if we want to avoid
unnecessarily
modifying the stats entry. If padding would contain non zeros then we would
"just" unnecessarily modify the stats entry (adding "zeros" to the shared
stats).
> How about this instead:
>
> static inline bool
> pg_is_all_zeros(const char *p, size_t len)
> {
> for (size_t i = 0; i < len; i++)
> {
> if (p[i] != 0)
> return false;
> }
> return true;
> }
>
Yeah, that sounds good to me. It's more generic than the initial proposal that
was taking care of a struct memory area. Also, the same "logic" is already
in PageIsVerifiedExtended().
V3 attached is using the above proposal and also makes the change in
PageIsVerifiedExtended(). Then, the new inline function has been placed in
utils/memutils.h (not sure that's the best place though).
> Is there's a de facto standard name for that function? I was surprised that
> I couldn't find one with a quick google search.
Same here. I was just able to find "memiszero" in [0]. So the naming proposal
in v3 is pg_mem_is_all_zeros().
> That seems like the kind of
> small utility function that every C program needs.
Yeah.
> How performance sensitive is this?
I don't think that's very sensitive. I think it's "cheap" as compared to what
lead
to those code paths (stats increments).
> If it's not, then the above seems like
> the most straightforward way to do this, which is good. If it is performance
> sensitive, it's still good, because the compiler can optimize that well:
> https://godbolt.org/z/x9hPWjheq.
Yeah, I also think that's fine. Peter Smith did some testing in [1] comparing
memcmp and simple loop checking (thanks Peter for the testing!):
"
Iterate 1000000 times...
check zeros using loop -- elapsed=0.041196s
check zeros using memcmp -- elapsed=0.016407s
"
So, in this test, the loop is 0.024789s longer means 0.024789s/1000000=24
Nanosecond
slower per comparison (If my math is correct). I don't see this as a red flag
and
still go with the loop proposal as this is the one already used in
PageIsVerifiedExtended().
[0]: https://in3.readthedocs.io/en/develop/api-c.html
[1]:
https://www.postgresql.org/message-id/CAHut%2BPvHmWiPyqiDRnD7FYsc4QskXpKEZAH3Z8Ahd_GSnRXWrw%40mail.gmail.com
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From aa1b45af21fd363bb4c7b202f7c3aa654048b208 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@gmail.com>
Date: Tue, 10 Sep 2024 01:22:02 +0000
Subject: [PATCH v3] New pg_mem_is_all_zeros(p, len) inline function
This new function allows to test if a memory region (starting at p) 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 | 14 ++++++++++++++
5 files changed, 23 insertions(+), 23 deletions(-)
20.6% src/backend/storage/page/
61.1% src/backend/utils/activity/
18.1% src/include/utils/
diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index be6f1f62d2..306090c778 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_mem_is_all_zeros((const char *) 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..d9c21cf665 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_mem_is_all_zeros((const char *) &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..6f3e2a8a45 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_mem_is_all_zeros((const char *) &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..3bbec950b1 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_mem_is_all_zeros((const char *) &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..2f331514f3 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -189,4 +189,18 @@ 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 p and of size len is full of zeroes.
+ */
+static inline bool
+pg_mem_is_all_zeros(const char *p, size_t len)
+{
+ for (size_t i = 0; i < len; i++)
+ {
+ if (p[i] != 0)
+ return false;
+ }
+ return true;
+}
+
#endif /* MEMUTILS_H */
--
2.34.1
