Clear padding in PgStat_HashKey keys
Hi hackers,
While working on a rebase for [0]/messages/by-id/ZlGYokUIlERemvpB@ip-10-97-1-34.eu-west-3.compute.internal, I noticed some weird behavior on the stats.
The issue is that [0]/messages/by-id/ZlGYokUIlERemvpB@ip-10-97-1-34.eu-west-3.compute.internal, in conjonction with b14e9ce7d5, does introduce padding in
the PgStat_HashKey:
(gdb) ptype /o struct PgStat_HashKey
/* offset | size */ type = struct PgStat_HashKey {
/* 0 | 4 */ uint32 kind;
/* 4 | 4 */ Oid dboid;
/* 8 | 8 */ uint64 objid;
/* 16 | 4 */ RelFileNumber relfile;
/* XXX 4-byte padding */
/* total size (bytes): 24 */
}
But, the keys are initialized that way:
"
PgStat_HashKey key = {.kind = kind,.dboid = dboid,.objid = objid,.relfile = refile};
"
which could lead to random data in the padding bytes.
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.
Even if currently there is no issues, as without [0]/messages/by-id/ZlGYokUIlERemvpB@ip-10-97-1-34.eu-west-3.compute.internal there is no padding:
(gdb) ptype /o struct PgStat_HashKey
/* offset | size */ type = struct PgStat_HashKey {
/* 0 | 4 */ uint32 kind;
/* 4 | 4 */ Oid dboid;
/* 8 | 8 */ uint64 objid;
/* total size (bytes): 16 */
}
I think that we should ensure to $SUBJECT (to prevent unexpected results should
padding be introduced in the future).
For example we currently ensure the same for LOCALLOCKTAG localtag in LockHeldByMe()
while there is no padding:
gdb) ptype /o struct LOCALLOCKTAG
/* offset | size */ type = struct LOCALLOCKTAG {
/* 0 | 16 */ LOCKTAG lock;
/* 16 | 4 */ LOCKMODE mode;
/* total size (bytes): 20 */
}
So, please find attached a patch to $SUBJECT.
[0]: /messages/by-id/ZlGYokUIlERemvpB@ip-10-97-1-34.eu-west-3.compute.internal
Looking forward to your feedback,
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Clear-padding-in-PgStat_HashKey-keys.patchtext/x-diff; charset=us-asciiDownload
From 0a590aad0ff014dff60e5ee73bef2c5d7a659804 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Sat, 2 Nov 2024 14:21:18 +0000
Subject: [PATCH v1] Clear padding in PgStat_HashKey keys
PgStat_HashKey keys are currently initialized in a way that could result in random
data in the padding bytes (if there was padding in PgStat_HashKey which is not
the case currently).
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. So, we have to ensure that no random data can be stored in the padding
bytes (if any) of a PgStat_HashKey key.
---
src/backend/utils/activity/pgstat.c | 3 +++
src/backend/utils/activity/pgstat_shmem.c | 18 ++++++++++++++++--
2 files changed, 19 insertions(+), 2 deletions(-)
100.0% src/backend/utils/activity/
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index be48432cc3..ea8c5691e8 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -938,6 +938,9 @@ pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, uint64 objid)
pgstat_prep_snapshot();
+ /* clear padding */
+ memset(&key, 0, sizeof(struct PgStat_HashKey));
+
key.kind = kind;
key.dboid = dboid;
key.objid = objid;
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index a09c6fee05..c1b7ff76b1 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -432,11 +432,18 @@ PgStat_EntryRef *
pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create,
bool *created_entry)
{
- PgStat_HashKey key = {.kind = kind,.dboid = dboid,.objid = objid};
+ PgStat_HashKey key;
PgStatShared_HashEntry *shhashent;
PgStatShared_Common *shheader = NULL;
PgStat_EntryRef *entry_ref;
+ /* clear padding */
+ memset(&key, 0, sizeof(struct PgStat_HashKey));
+
+ key.kind = kind;
+ key.dboid = dboid;
+ key.objid = objid;
+
/*
* passing in created_entry only makes sense if we possibly could create
* entry.
@@ -908,10 +915,17 @@ pgstat_drop_database_and_contents(Oid dboid)
bool
pgstat_drop_entry(PgStat_Kind kind, Oid dboid, uint64 objid)
{
- PgStat_HashKey key = {.kind = kind,.dboid = dboid,.objid = objid};
+ PgStat_HashKey key;
PgStatShared_HashEntry *shent;
bool freed = true;
+ /* clear padding */
+ memset(&key, 0, sizeof(struct PgStat_HashKey));
+
+ key.kind = kind;
+ key.dboid = dboid;
+ key.objid = objid;
+
/* delete local reference */
if (pgStatEntryRefHash)
{
--
2.34.1
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?
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?
--
Michael
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]/messages/by-id/8c1e502a337e9557278f31abf877c321@anastigmatix.net. 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]: /messages/by-id/8c1e502a337e9557278f31abf877c321@anastigmatix.net
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
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
On Mon, Nov 04, 2024 at 08:52:04AM +0000, Bertrand Drouvot wrote:
Yeah, but not only the relfilenode ones. All kinds were affected as random data
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
Hi,
On Mon, Nov 04, 2024 at 06:49:04PM +0900, Michael Paquier wrote:
On Mon, Nov 04, 2024 at 08:52:04AM +0000, Bertrand Drouvot wrote:
Yeah, but not only the relfilenode ones. All kinds were affected as random data
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.
Thanks for the testing!
I'm wondering if we should backpatch that,
actually, down to where it has been introduced. We are unlikely going
to change this structure,
Yeah.
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.
That's right.
Thoughts?
hm, yeah I think that it could fall into the "low-risk fixes" category [0]https://www.postgresql.org/support/versioning/ and
that we can opt for backpatch.
[0]: https://www.postgresql.org/support/versioning/
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Mon, Nov 04, 2024 at 10:07:37AM +0000, Bertrand Drouvot wrote:
On Mon, Nov 04, 2024 at 06:49:04PM +0900, Michael Paquier wrote:
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.That's right.
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