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

On Mon, Dec 08, 2025 at 09:57:15PM -0600, Sami Imseih wrote:
> The way v5 is dealing with a deserialize failure is that when
> it goes to error, the pgstat_reset_after_failure() will reset the
> stats for all kinds, since pgstat_drop_all_entries() is called
> during that call. So there is nothing for an extension to have
> to do on its own. The extension will then clean-up resources
> at the end when all the kinds are iterated over and
> kind_info->end_extra_stats(STATS_READ) is called for each
> kind.
>
> Let me know if I'm still missing something?

It seems to me that you are missing nothing here, and that Chao has
missed the fact that the end of pgstat_read_statsfile() does a "goto
done", meaning that we would take a round of
end_extra_stats(STATS_READ) to do all the cleanup after resetting all
the stats. That's what I would expect.

+static inline bool pgstat_check_extra_callbacks(PgStat_Kind kind);
[...]
@@ -645,6 +656,13 @@ pgstat_initialize(void)
+ /* Check a kind's extra-data callback setup */
+ for (PgStat_Kind kind = PGSTAT_KIND_BUILTIN_MIN; kind <= PGSTAT_KIND_BUILTIN_MAX; kind++)
+ if (!pgstat_check_extra_callbacks(kind))
+ ereport(ERROR,
+ errmsg("incomplete extra serialization callbacks for stats kind %d",
+ kind));

Why does this part need to run each time a backend initializes its
access to pgstats? Shouldn't this happen only once when a stats kind
is registered? pgstat_register_kind() should be the only code path
that does such sanity checks.

By the way, checking that to_serialized_extra_stats and
kind_info->from_serialized_extra_stats need to be both defined is
fine as these are coupled together, but I am not following the reason
why end_extra_stats would need to be included in the set? For
example, a stats kind could decide to add some data to the main
pgstats file without creating extra files, hence they may not need to
define end_extra_stats.
--
Michael