public inbox for [email protected]
help / color / mirror / Atom feedFrom: Sami Imseih <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Chao Li <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: [Proposal] Adding callback support for custom statistics kinds
Date: Wed, 17 Dec 2025 11:01:01 -0600
Message-ID: <CAA5RZ0t-WDUhjkpdnfxH_OZWJAKtozQSV+nv3+H4ft4+FxB1Yw@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CAA5RZ0vQvmatp8VAk6zroyeKDCw+QVq4XmoRNAFogQo6bJEbNQ@mail.gmail.com>
<[email protected]>
<CAA5RZ0swDCK+7M2KQip0tqUsBK3LUHd3F0ytpsKdDCfygHksKg@mail.gmail.com>
<[email protected]>
<CAA5RZ0uC4j2QGK85ma6bkF6YOWBrYX9J3dV2CEMg3d0CBLpHSA@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAA5RZ0ubfcmkkY7i07CmrkY12H8BsRXDoOutYXjYz0gsq7hm-w@mail.gmail.com>
<[email protected]>
<[email protected]>
> On Wed, Dec 17, 2025 at 08:03:36AM +0100, Peter Eisentraut wrote:
> > So it seems to me that either the callbacks API needs some adjustments, or
> > this particular implementation of the callback function is incorrect.
>
> Hmm, you are right that this is not aligned. This can be improved
> with one change for each callback:
> - It is OK with from_serialized_data() to manipulate the header data,
> because we want to fill a portion of the shmem data with extra data
> read from disk (the module wants to add a reference to a DSA stored in
> the shmem entry, read from the second file). So we should discard the
> const marker from the callback definition.
> - The const usage is OK for to_serialized_data(): it is better to
> encourage a policy where the header data cannot be manipulated. So
> the const needs to be kept in the definition, but I also think that we
> should change the module implementation so as the cast to
> PgStatShared_CustomVarEntry is a const.
>
> These changes result in the attached. Sami, what do you think?
I agree. This was a miss during the review. Thanks for raising this.
The fix looks correct to me in which the from_serialized_data callback
is expected to modify the header, to reconstruct the entry and the
to_serialized_data is never expected to modify the header, since we
are only reading what is currently in stats. I can't think of a reason to
ever have to modify the entry while writing out to disk.
I got the attached patch ready with some additional comments in
the callback definitions to clarify the API contract. We only need
to call out the "header' nuance since it's a const in one callback
and not the other. "key" is self documenting being a const in both
cases.
--
Sami Imseih
Amazon Web Services (AWS)
Attachments:
[application/octet-stream] v1-0001-Fix-const-correctness-in-pgstat-serialization-cal.patch (3.3K, ../CAA5RZ0t-WDUhjkpdnfxH_OZWJAKtozQSV+nv3+H4ft4+FxB1Yw@mail.gmail.com/2-v1-0001-Fix-const-correctness-in-pgstat-serialization-cal.patch)
download | inline diff:
From 6cd3a9ad9a31e9d1f8d57815b556642f0a7f2b7d Mon Sep 17 00:00:00 2001
From: Ubuntu <[email protected]>
Date: Wed, 17 Dec 2025 16:23:13 +0000
Subject: [PATCH v1 1/1] Fix const correctness in pgstat serialization
callbacks
4ba012a8ed9c defined the header in from_serialized_data as const, even
though the callback may modify it when reconstructing entry state.
Also update the to_serialized_data callback in test_custom_stats to
make the header parameter const since it should not be modified.
This eliminates unsafe const casts and clarifies the API contract.
Reported-By: Peter Eisentraut <[email protected]>
Author: Michael Paquier <[email protected]>
Reviewed-by: Sami Imseih <[email protected]>
Discussion: https://postgr.es/m/d87a93b0-19c7-4db6-b9c0-d6827e7b2da1%40eisentraut.org
---
src/include/utils/pgstat_internal.h | 5 +++--
src/test/modules/test_custom_stats/test_custom_var_stats.c | 6 +++---
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index 5c1ce4d3d6a..67f7071fbfd 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -329,13 +329,14 @@ typedef struct PgStat_KindInfo
*
* "statfile" is a pointer to the on-disk stats file, named
* PGSTAT_STAT_PERMANENT_FILENAME. "key" is the hash key of the entry
- * just written or read. "header" is a pointer to the stats data.
+ * just written or read. "header" is a pointer to the stats data; it may
+ * be modified only in from_serialized_data to reconstruct entry state.
*/
void (*to_serialized_data) (const PgStat_HashKey *key,
const PgStatShared_Common *header,
FILE *statfile);
bool (*from_serialized_data) (const PgStat_HashKey *key,
- const PgStatShared_Common *header,
+ PgStatShared_Common *header,
FILE *statfile);
/*
diff --git a/src/test/modules/test_custom_stats/test_custom_var_stats.c b/src/test/modules/test_custom_stats/test_custom_var_stats.c
index c71922dc4a8..294085d6866 100644
--- a/src/test/modules/test_custom_stats/test_custom_var_stats.c
+++ b/src/test/modules/test_custom_stats/test_custom_var_stats.c
@@ -92,7 +92,7 @@ static void test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key,
/* Deserialization callback: read auxiliary entry data */
static bool test_custom_stats_var_from_serialized_data(const PgStat_HashKey *key,
- const PgStatShared_Common *header,
+ PgStatShared_Common *header,
FILE *statfile);
/* Finish callback: end of statistics file operations */
@@ -196,7 +196,7 @@ test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key,
{
char *description;
size_t len;
- PgStatShared_CustomVarEntry *entry = (PgStatShared_CustomVarEntry *) header;
+ const PgStatShared_CustomVarEntry *entry = (const PgStatShared_CustomVarEntry *) header;
bool found;
uint32 magic_number = TEST_CUSTOM_VAR_MAGIC_NUMBER;
@@ -276,7 +276,7 @@ test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key,
*/
static bool
test_custom_stats_var_from_serialized_data(const PgStat_HashKey *key,
- const PgStatShared_Common *header,
+ PgStatShared_Common *header,
FILE *statfile)
{
PgStatShared_CustomVarEntry *entry;
--
2.43.0
view thread (47+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [Proposal] Adding callback support for custom statistics kinds
In-Reply-To: <CAA5RZ0t-WDUhjkpdnfxH_OZWJAKtozQSV+nv3+H4ft4+FxB1Yw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox