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