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