Re: define pg_structiszero(addr, s, r)

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