Thread

Re: [Proposal] Adding callback support for custom statistics kinds

Sami Imseih <samimseih@gmail.com> — 2025-12-17T17:01:01Z

> 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)