On 25.11.25 03:03, Michael Paquier wrote:
> One part of the proposed patch that felt independent to me was the
> renaming and publishing of the two write/read routines for the stats
> files, so I have extracted that in your first patch to reduce the
> blast, and applied that as it can also be useful on its own.
I came across this now. I'm concerned that these write/read routines
for stats files are a bit weirdly defined.
They were originally internal to pgstat.c where they were specifically
tailored to the control flow in that file. For example,
pgstat_write_chunk() contains an internal comment "We check for errors
with ferror() when done writing the stats.", which is fine as a
description of what happens in that file, but it's pretty poor as an API
documentation. On the other hand, pgstat_read_chunk() does do some
error checking, but completely undocumented and weirdly inconsistent
with the write counterpart. Again, this might be fine for an internal
helper but not for an external API.
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 have not found any documentation about this new feature for pluggable
statistics, so it's not clear whether these functions are meant to be
part of the official API for that, or they just happened to be available
for implementing that test module.
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.
