pg_stat_statements: improve loading and saving routines for the dump file
Hi!
Currently in pg_stat_statements save/load routines the whole pgssEntry
entity data are written/read with its last field
slock_t mutex;
which is actually not used then.
This small patch fixes this issue. Hope, it will be useful.
Respectfully,
Mikhail Litsarev,
Postgres Professional: https://postgrespro.com
Attachments:
v1-0001-Exclude-pgssEntry-mutex-for-rw-dump-file.patchtext/x-diff; name=v1-0001-Exclude-pgssEntry-mutex-for-rw-dump-file.patchDownload
From 40392a495a91dfc1ee2c62812949eebe54276625 Mon Sep 17 00:00:00 2001
From: Mikhail Litsarev <m.litsarev@postgrespro.ru>
Date: Mon, 20 Jan 2025 15:40:12 +0300
Subject: [PATCH] pg_stat_statements: improve loading and saving routines for
the dump file.
Exclude reading/writing pgssEntry mutex from/into the dump file.
Update the magic number identifying the stats file format.
---
contrib/pg_stat_statements/pg_stat_statements.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index bebf8134eb0..0511a2b7ca1 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -82,7 +82,7 @@ PG_MODULE_MAGIC;
#define PGSS_TEXT_FILE PG_STAT_TMP_DIR "/pgss_query_texts.stat"
/* Magic number identifying the stats file format */
-static const uint32 PGSS_FILE_HEADER = 0x20220408;
+static const uint32 PGSS_FILE_HEADER = 0x20250120;
/* PostgreSQL major version number, changes in which invalidate all entries */
static const uint32 PGSS_PG_MAJOR_VERSION = PG_VERSION_NUM / 100;
@@ -233,7 +233,8 @@ typedef struct pgssEntry
int encoding; /* query text encoding */
TimestampTz stats_since; /* timestamp of entry allocation */
TimestampTz minmax_stats_since; /* timestamp of last min/max values reset */
- slock_t mutex; /* protects the counters only */
+ /* protects the counters only. Should be the very last field */
+ slock_t mutex;
} pgssEntry;
/*
@@ -625,7 +626,8 @@ pgss_shmem_startup(void)
pgssEntry *entry;
Size query_offset;
- if (fread(&temp, sizeof(pgssEntry), 1, file) != 1)
+ /* Read whole pgssEntry excluding very last mutex field */
+ if (fread(&temp, offsetof(pgssEntry, mutex), 1, file) != 1)
goto read_error;
/* Encoding is the only field we can easily sanity-check */
@@ -782,7 +784,8 @@ pgss_shmem_shutdown(int code, Datum arg)
if (qstr == NULL)
continue; /* Ignore any entries with bogus texts */
- if (fwrite(entry, sizeof(pgssEntry), 1, file) != 1 ||
+ /* Write whole pgssEntry excluding very last mutex field */
+ if (fwrite(entry, offsetof(pgssEntry, mutex), 1, file) != 1 ||
fwrite(qstr, 1, len + 1, file) != len + 1)
{
/* note: we assume hash_seq_term won't change errno */
--
2.34.1
Hello, Mikhail.
1) I'd add to comment a reason, why mutex should be last.
// Mutex should be last field, as this field isn't copied to dump file
+ /* protects the counters only. Should be the very last field, as this field isn't copied to dump file
+ slock_t mutex;
} pgssEntry;2) You didn't take into account the upgrade. Saved in Postgres with this
byte and try to load in version without this byte.
On 1/20/25 16:49, m.litsarev@postgrespro.ru wrote:
Hi!
Currently in pg_stat_statements save/load routines the whole pgssEntry
entity data are written/read with its last field
slock_t mutex;
which is actually not used then.
This small patch fixes this issue. Hope, it will be useful.Respectfully,
Mikhail Litsarev,
Postgres Professional: https://postgrespro.com
--
Best wishes,
Ivan Kush
Tantor Labs LLC
// Mutex should be last field, as this field isn't copied to dump file
Updated.
2) You didn't take into account the upgrade. Saved in Postgres with
this byte and try to load in version without this byte.
The PGSS_DUMP_FILE format is defined by PGSS_FILE_HEADER magic number
(the first four bytes in the dump file). Once they does not coinside
(when the file is being read) the statistics are not loaded from the
dump file and set to zero and the irrelevant dump file is skipped. This
is the actual behaviour implemented in the code.
Respectfully,
Mikhail Litsarev,
Postgres Professional: https://postgrespro.com
Attachments:
v2-0001-Exclude-pgssEntry-mutex-for-rw-dump-file.patchtext/x-diff; name=v2-0001-Exclude-pgssEntry-mutex-for-rw-dump-file.patchDownload
From 21f579992e54d724d0c6d409c7fdf51f1c4619d7 Mon Sep 17 00:00:00 2001
From: Mikhail Litsarev <m.litsarev@postgrespro.ru>
Date: Mon, 20 Jan 2025 15:40:12 +0300
Subject: [PATCH] pg_stat_statements: improve loading and saving routines for
the dump file.
Exclude reading/writing pgssEntry mutex from/into the dump file.
Update the magic number identifying the stats file format.
---
contrib/pg_stat_statements/pg_stat_statements.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index bebf8134eb0..c52d3da1190 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -82,7 +82,7 @@ PG_MODULE_MAGIC;
#define PGSS_TEXT_FILE PG_STAT_TMP_DIR "/pgss_query_texts.stat"
/* Magic number identifying the stats file format */
-static const uint32 PGSS_FILE_HEADER = 0x20220408;
+static const uint32 PGSS_FILE_HEADER = 0x20250120;
/* PostgreSQL major version number, changes in which invalidate all entries */
static const uint32 PGSS_PG_MAJOR_VERSION = PG_VERSION_NUM / 100;
@@ -233,7 +233,11 @@ typedef struct pgssEntry
int encoding; /* query text encoding */
TimestampTz stats_since; /* timestamp of entry allocation */
TimestampTz minmax_stats_since; /* timestamp of last min/max values reset */
- slock_t mutex; /* protects the counters only */
+ /*
+ * protects the counters only. Should be the very last field, as this field
+ * isn't copied to PGSS_DUMP_FILE
+ */
+ slock_t mutex;
} pgssEntry;
/*
@@ -625,7 +629,8 @@ pgss_shmem_startup(void)
pgssEntry *entry;
Size query_offset;
- if (fread(&temp, sizeof(pgssEntry), 1, file) != 1)
+ /* Read whole pgssEntry excluding very last mutex field */
+ if (fread(&temp, offsetof(pgssEntry, mutex), 1, file) != 1)
goto read_error;
/* Encoding is the only field we can easily sanity-check */
@@ -782,7 +787,8 @@ pgss_shmem_shutdown(int code, Datum arg)
if (qstr == NULL)
continue; /* Ignore any entries with bogus texts */
- if (fwrite(entry, sizeof(pgssEntry), 1, file) != 1 ||
+ /* Write whole pgssEntry excluding very last mutex field */
+ if (fwrite(entry, offsetof(pgssEntry, mutex), 1, file) != 1 ||
fwrite(qstr, 1, len + 1, file) != len + 1)
{
/* note: we assume hash_seq_term won't change errno */
--
2.34.1
What does this patch give on aglobal scale? Does it save much memory or
increase performance? How many times?
On 1/21/25 13:51, m.litsarev@postgrespro.ru wrote:
// Mutex should be last field, as this field isn't copied to dump file
Updated.
2) You didn't take into account the upgrade. Saved in Postgres with
this byte and try to load in version without this byte.The PGSS_DUMP_FILE format is defined by PGSS_FILE_HEADER magic number
(the first four bytes in the dump file). Once they does not coinside
(when the file is being read) the statistics are not loaded from the
dump file and set to zero and the irrelevant dump file is skipped.
This is the actual behaviour implemented in the code.Respectfully,
Mikhail Litsarev,
Postgres Professional: https://postgrespro.com
--
Best wishes,
Ivan Kush
Tantor Labs LLC
This will only reduce the size of the
$PGDATA/pg_stat/pg_stat_statements.txt file. Even with
100k entries, the most I have seen pg_stat_statements.max
set to, that will be less than 1 MB of disk saving. The default
config of 5k entries will be much less.
Regards,
Sami
What does this patch give on aglobal scale? Does it save much memory or
increase performance? How many times?
This patch makes the code semantically more correct and we don't lose
anything. It is obviously not about performance or memory optimisation.
This will only reduce the size of the
$PGDATA/pg_stat/pg_stat_statements.txt file. Even with
100k entries, the most I have seen pg_stat_statements.max
set to, that will be less than 1 MB of disk saving. The default
config of 5k entries will be much less.
Perfectly agree. I would just add that statistics are dropped into the
pg_stat_statements.txt file at server stop which is not a very frequent
event.
Respectfully,
Mikhail Litsarev,
Postgres Professional: https://postgrespro.com
m.litsarev@postgrespro.ru writes:
What does this patch give on aglobal scale? Does it save much memory or
increase performance? How many times?
This patch makes the code semantically more correct and we don't lose
anything. It is obviously not about performance or memory optimisation.
Indeed not: on my machine I see sizeof(pgssEntry) = 432. It's full of
int64 fields, so the alignment requirement is 8 bytes, meaning that
the mutex field accounts for 8 bytes even though it's likely just 1
or 4 bytes wide. Still, that means what you suggest is only going
to save 8/432 = 1.8% of the on-disk size of the struct. Given that
we also store the SQL query text for each entry, the net savings
fraction is even smaller.
I don't really agree that this is adding any semantic correctness
either. If we were reading directly into a live hashtable entry,
overwriting the mutex field could indeed be bad. But the fread()
call is reading into a local variable "temp" and we only copy
selected fields out of that. So it's just some bytes in a
transient stack entry.
On the whole therefore, I'm inclined to reject this on several
grounds:
* The savings is not worth the costs of bumping the stats file format
magic number (and thereby invalidating everyone's stored statistics).
* I'd rather keep the flexibility to put the mutex wherever we want in
the struct. Right now that isn't worth much either, but depending on
what fields get added in future, we might have the opportunity to move
the mutex to someplace where it fits into padding space and hence adds
nothing to sizeof(pgssEntry).
* Adding the requirement on where the mutex is makes the code more
fragile, since somebody might ignore the comment and place things
incorrectly. I'd like to think that such an error would be spotted
immediately, but we've committed sillier mistakes. The consequences
would likely be that some fields don't get stored/reloaded correctly,
which might escape notice for awhile.
regards, tom lane
Hi!
Thank you for detailed explanations.
Respectfully,
Mikhail Litsarev,
Postgres Professional: https://postgrespro.com