add validations for required callbacks during pgstat_register_kind()

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

Thanks for taking a look!

Feel free to add me in CC for such changes; this would drop in my
inbox.

Noted.

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

Done in v2-0001.

v2-0002 also makes the errmsg consistent and includes kind names
(except for when a name is not provided)
and kind IDs across all the messages.

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.

hmm, I think it's better to have a clear assertion log ( when built
with asserts ),
than a segfault. Also, it's self-documenting. It is also consistent with
snapshot_cb. I think we should keep it, but I separated that into a v2-0003,
Perhaps you will change your mind.

--
Sami

On Fri, Jul 03, 2026 at 11:04:56AM -0500, Sami Imseih wrote:

v2-0002 also makes the errmsg consistent and includes kind names
(except for when a name is not provided)
and kind IDs across all the messages.

Thanks. Merged 0001 and 0002 together, tweaked two small things
(s_p_l replaced by a %s, and an underscore removed), and applied the
result on HEAD. Consistency is nice.

hmm, I think it's better to have a clear assertion log ( when built
with asserts ),
than a segfault. Also, it's self-documenting. It is also consistent with
snapshot_cb. I think we should keep it, but I separated that into a v2-0003,
Perhaps you will change your mind.

The locations of snapshot_cb are a bit smarter than this proposal:
- The assertion at the beginning of snapshot_fixed() is
unconditionally taken and makes sure that the callback is set.
- The assertion in pgstat_build_snapshot() make sure that no callback
is set if we have a non-fixed kind.
--
Michael