Thread: Re: Clear padding in PgStat_HashKey keys

On Mon, 4 Nov 2024 at 08:25, Michael Paquier <michael@paquier.xyz> wrote:
> Perhaps it would be simpler to use a {0} like anywhere else for
> PgStat_HashKey in pgstat_fetch_entry() and pgstat_drop_entry(), then
> initialize the individual fields?

{0} doesn't clear padding, it only sets all the fields to 0.

So in many places we use memset or MemSet to clear the padding already:

rg 'memset.*key' -S | wc -l
31

And even using memset in this manner isn't a standards compliant way
of handling this problem[0]. But it seems to have worked well enough
in practice.

Whether it's worth changing this now or only when we actually
introduce padding is another question though. But the code itself and
reasoning is sensible imo.

[0]:
https://www.postgresql.org/message-id/flat/8c1e502a337e9557278f31abf877c321%40anastigmatix.net#4113241930633758cdbc22f49365806f

Hi,

On Mon, Nov 04, 2024 at 09:27:43AM +0100, Jelte Fennema-Nio wrote:
> On Mon, 4 Nov 2024 at 08:25, Michael Paquier <michael@paquier.xyz> wrote:
> > Perhaps it would be simpler to use a {0} like anywhere else for
> > PgStat_HashKey in pgstat_fetch_entry() and pgstat_drop_entry(), then
> > initialize the individual fields?
> 
> {0} doesn't clear padding, it only sets all the fields to 0.

Thank you both to look at it!

> 
> So in many places we use memset or MemSet to clear the padding already:
> 
> rg 'memset.*key' -S | wc -l
> 31

Yeah, and 9fd45870c1 did not touch some of them (like the one I mentioned up-thread
in LOCALLOCKTAG localtag in LockHeldByMe()).

> And even using memset in this manner isn't a standards compliant way
> of handling this problem[0]. But it seems to have worked well enough
> in practice.
> 
> Whether it's worth changing this now or only when we actually
> introduce padding is another question though.

I think it's worth to do it now:

1/ for example, it's done in LOCALLOCKTAG localtag in LockHeldByMe() while
LOCALLOCKTAG does not contain padding (see up-thread).

2/ the one that will add padding could miss this thread and spend some time
to figure out why his patch does break existing stats.

3/ when dealing with structs that are used as hash key I think we should not wait
for padding to be introduced.

Regards,

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