Re: Custom pgstat support performance regression for simple queries

On Thu, Jul 24, 2025 at 07:38:46AM +0000, Bertrand Drouvot wrote:
> On Thu, Jul 24, 2025 at 09:34:45AM +0900, Michael Paquier wrote:
>> These numbers mean that we have enough room for 7 more builtins kinds,
>
> 11 for builtins kinds? (from 13 to 23)
>
> 9 for custom kinds including experimental? (24 -> 32)
>
> I think that gives some room (like we'll almost double the builtins kinds if we
> were to use them all).

Yes. Problems with a lack of fingers and caffeine, perhaps.

> * development and have not reserved their own unique kind ID yet. See:
> * https://wiki.postgresql.org/wiki/CustomCumulativeStats
> */
> -#define PGSTAT_KIND_EXPERIMENTAL 128
> +#define PGSTAT_KIND_EXPERIMENTAL 24
>
> Note that we'll have to update the wiki too.

Yep, that was on my TODO list.

> What about?
>
> - create a global variable but that should be used only by extensions? Say,
> pgstat_report_custom_fixed
>
> - for builtin fixed-sized, just rely on have_iostats, have_slrustats and friends
>
> Then in pgstat_report_stat():
>
> - do the check on have_iostats, have_slrustats and friends and on
> pgstat_report_custom_fixed
>
> - reset pgstat_report_custom_fixed
>
> That way I think it's less error prone for the builtin stats and more clear
> for the extensions (as at least "custom" is also part of the global variable
> name and the check in pgstat_report_stat() would make it clear that we rely on
> the custom global variable).
>
> Thoughts?

FWIW, I see more benefits in the simplicity of a single flag to govern
both builtin and custom kinds, but I can see that this may come down
to personal taste. A single flag is simpler here, and cheap.

What have_static_pending_cb was originally aiming for is the removal
of any knowledge specific to stats kinds in the central pgstats APIs,
which is why the flags specific to IO, SLRU and WAL have been moved
out into their own files.

Letting custom stats manipulate pgstat_report_fixed or invent a new
pgstat_report_custom_fixed would be logically the same thing, but
this comes down to the fact that we still want to decide if a report
should be triggered if any of the fixed-numbered stats want to let
pgstat_report_stat() do one round of pending stats flush.

Having a second flag would keep this abstraction level intact. Now
that leads to overcomplicating the API for a small gain in
debuggability to know which part of the subsystem wants the report to
happen at early stages of pgstat_report_stat(). If we want to know
exactly which stats kind wants the flush to happen, it may be better
to reuse your idea of one single uint32 or uint64 with one bit
allocated for each stats kind to have this information available in
pgstat_report_fixed(). However, we already know that with the
flush_static_cb callback, as dedicated paths can be taken for each
stats kinds. Once again, this would require stats kind to set their
bit to force a round of reports, gaining more information before we
try to call each flush_static_cb.

I am going to apply the patch to lower the bounds in a bit, to begin
with, then adjust the wiki.
--
Michael