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

Hi,

Thanks for raising this!

> On Thu, Jul 09, 2026 at 11:10:18AM +0200, Peter Eisentraut wrote:
> > I suggest that unless we can come up with a well-defined, robust, and
> > well-documented API for these, they should be moved back to be internal to
> > pgstat.c, and test_custom_var_stats.c can define its own helper functions.
> > Maybe in the future, when we have more of these, some good API will emerge.
>
> I am not enough attached to these two APIs to keep them published if
> one feels that it is weird to have them; we are talking about only a
> few lines of code duplicated between core and the test module. I
> still find the separation cleaner as done on HEAD, but well that's
> just one opinion.

After looking at this, I think the criticism of the APIs is fair. Particularly
the asymmetry in error handling. In the current state is if writes within a
to_serialized_data callback fail, the bad data is silently written and cleanup
only occurs on the read side during the next startup. So I think it will be
better for the to_serialize_data callback to also return a bool which is
handled by the pgstat_write_statsfile by giving up and cleaning up
the temporary file, etc.

> I also don't think these functions are well-named. They don't do
> anything about "pgstat", they are just convenience wrappers around
> fread() and fwrite() with some sizeof integration (which is also kind of
> fragile, because there is no type checking about what you are passing).

I can see the point here. The extensions could just call fwrite and
fread directly.
So let's just revert ed823da12891, and the test_custom_stats
extension just implements its own wrappers for fread/fwrite.

> I suggest that unless we can come up with a well-defined, robust, and
> well-documented API for these, they should be moved back to be internal
> to pgstat.c, and test_custom_var_stats.c can define its own helper
> functions. Maybe in the future, when we have more of these, some good
> API will emerge.

The attached 0001 reverts ed823da12891. If a strong reason ever comes up
to turn these into an API, we can do that, but to your point there is no reason
for this as they are simply wrapper to fread/fwrite.

0002 fixes the symmetry of error handling which is a change the callbacks
that we could still do. This is also cleaner because when an extension signals
failure, pgstat_write_statsfile() discards the temporary file immediately rather
than writing bad data that would just be discarded on the next startup anyway.

Note that I intentionally kept the core write path using deferred error
checking with ferror(). Core only writes to a single FILE handle, so keeping
it as one check at the end is sufficient and avoid adding if (!...) goto error
in about 12 different places.

--
Sami Imseih
Amazon Web Services (AWS)