Thread: Re: define pg_structiszero(addr, s, r)

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)

This assumption is kind of the problem, isn't it?  Because, you can't 
assume that.  And the existing code is arguably kind of wrong.  But 
moreover, this macro also assumes that the "addr" argument has no random 
padding bits.

In the existing code, you can maybe make a local analysis that the code 
is working correctly, although I'm not actually sure.  But if you are 
repackaging this as a general macro under a general-sounding name, then 
the requirements should be more stringent.

Hi,

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)
>

Thanks for the feedback.
 
> This assumption is kind of the problem, isn't it?  Because, you can't assume
> that.  And the existing code is arguably kind of wrong.  But moreover, this
> macro also assumes that the "addr" argument has no random padding bits.
> 
> In the existing code, you can maybe make a local analysis that the code is
> working correctly, although I'm not actually sure.

I think it is but will give it a closer look.

>  But if you are
> repackaging this as a general macro under a general-sounding name, then the
> requirements should be more stringent.

Agree. That said in v2 ([1]), it has been renamed to pgstat_entry_all_zeros().

I think that I will:

1/ take a closer look regarding the existing assumption
2/ if 1/ outcome is fine, then add more detailed comments around
pgstat_entry_all_zeros() to make sure it's not used outside of the existing
context 

Sounds good to you?

[1]: https://www.postgresql.org/message-id/ZuqHLCdZXtEsbyb/%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

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