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

From: Sami Imseih <samimseih(at)gmail(dot)com>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Peter Eisentraut <peter(at)eisentraut(dot)org>, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: [Proposal] Adding callback support for custom statistics kinds
Date: 2025-12-17 17:01:01
Message-ID: CAA5RZ0t-WDUhjkpdnfxH_OZWJAKtozQSV+nv3+H4ft4+FxB1Yw@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
v1-0001-Fix-const-correctness-in-pgstat-serialization-cal.patch application/octet-stream 3.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2025-12-17 17:06:31 Re: Updating IPC::Run in CI?
Previous Message Andres Freund 2025-12-17 16:50:32 Re: pg_dump: Remove trivial usage of PQExpBuffer