On Thu, Jul 02, 2026 at 04:48:25PM -0500, Sami Imseih wrote: > Also, there is an assert for snapshot_cb before it's called in > pgstat_build_snapshot_fixed(), but we don't have the same assert for > init_shmem_cb and reset_all_cb. The attached also adds these asserts. > These asserts are needed for built-in stats that don't go through > pgstat_register_kind()
Feel free to add me in CC for such changes; this would drop in my
inbox.
Hmm, why not. It does not make sense to me to define a custom stats
kinds without all three for fixed-sized kinds, as they enforce
in-memory properties (FWIW, I've wanted to add a few more safeguards,
but never got down to define a good line of enforcement). The
pending_size change with flush_pending_cb makes sense as well for
variable stats. Perhaps not something to adjust in v18, even if I
doubt that somebody relies on this behavior, it would be surprising to
get a failure on server restart.
+ ereport(ERROR,
+ (errmsg("custom cumulative statistics property is
invalid"),
+ errhint("Custom cumulative statistics require
init_shmem_cb, reset_all_cb, and snapshot_cb callbacks for fixed-numbered
objects.")));
Function names in error messages ought to be replaced with %s
placeholders, perhaps? The two messages may be better if matching the
surroundings, with "failed to register custom .. with ID" being the
primary message, and the hint telling what's missing.
if (kind_info->fixed_amount)
+ {
+ Assert(kind_info->reset_all_cb != NULL);
kind_info->reset_all_cb(ts);
+ }
[...]
+ Assert(kind_info->reset_all_cb != NULL);
kind_info->reset_all_cb(ts);
[...]
+ Assert(kind_info->init_shmem_cb != NULL);
kind_info->init_shmem_cb(ptr);
These three don't really bring extra value. If the pointer is not
set, we just crash the line after on a NULL pointer dereference,
leading to the same result at the end.
--
Michael
signature.asc
Description: PGP signature
