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.
