Subject: Re: Clear padding in PgStat_HashKey keys

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 4 Nov 2024 08:52:04 +0000

Hi,

On Mon, Nov 04, 2024 at 04:25:00PM +0900, Michael Paquier wrote:
> On Sun, Nov 03, 2024 at 04:25:41AM +0000, Bertrand Drouvot wrote:
> > We are using sizeof(PgStat_HashKey) in pgstat_cmp_hash_key() and we compute the
> > hash hash key in pgstat_hash_hash_key() using the PgStat_HashKey struct size as
> > input: this lead to unexpected results if the keys contain random data in the 
> > padding bytes.
> 
> So you've seen that your patch was behaving weirdly once you have
> added padding because the hash key size has been extended, leading to
> relfilenode entries not being fetched when they should, right?

Yeah, but not only the relfilenode ones. All kinds were affected as random data
was in the padding bytes for all of them.

Regards,

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

---

From: Michael Paquier <[email-masked]>
Date: Mon, 4 Nov 2024 18:49:04 +0900

> was in the padding bytes for all of them.

A quick test where I add some padding junk in PgStat_HashKey proves
that you are right.  I'm wondering if we should backpatch that,
actually, down to where it has been introduced.  We are unlikely going
to change this structure, but if we do for the sake of a bug fix,
which is always a possibility as ABI does not matter much for this
internal structure, that's potentially trouble waiting ahead.
Thoughts?
--
Michael

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 4 Nov 2024 10:07:37 +0000

Hi,

On Mon, Nov 04, 2024 at 06:49:04PM +0900, Michael Paquier wrote:
> that you are right.

Thanks for the testing!

> I'm wondering if we should backpatch that,
> to change this structure,

Yeah.

> but if we do for the sake of a bug fix,

That's right.

> Thoughts?

hm, yeah I think that it could fall into the "low-risk fixes" category [0] and
that we can opt for backpatch.

[0]: https://www.postgresql.org/support/versioning/

Regards,

---

From: Michael Paquier <[email-masked]>
Date: Tue, 5 Nov 2024 09:43:11 +0900

>> internal structure, that's potentially trouble waiting ahead.

After sleeping on it, it would be annoying to come back to this issue
for a separate backpatchable change, and what's proposed is simple.
So applied down to 15.
--
Michael