Hi, On Thu, Jul 24, 2025 at 09:34:45AM +0900, Michael Paquier wrote: > On Wed, Jul 23, 2025 at 12:00:55PM -0400, Andres Freund wrote: > > On 2025-07-23 09:54:12 +0900, Michael Paquier wrote: > >> Noted. I was wondering originally if the threshold for the builtin > >> and custom kinds should be lowered, as well. Perhaps that's just too > >> optimistic, so we can first lower things. Say PGSTAT_KIND_CUSTOM_MIN > >> to 32 and PGSTAT_KIND_MAX to 48 or so to begin with? I don't see a > >> strict policy here, but these would make the whole cheaper to begin > >> with, with enough margin for any new stats kinds. > > > > Yes, 256 seems pointlessly large. > > I still cannot put my finger on what a good number should be, but I've > settled to something that I think should be good enough for an initial > release: > PGSTAT_KIND_CUSTOM_MIN 128 -> 24 > PGSTAT_KIND_MAX 256 -> 32 > > These numbers mean that we have enough room for 7 more builtins kinds,
11 for builtins kinds? (from 13 to 23) > same for custom kinds. 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). > Even if this is an issue, this could always be > moved up at some point even across major releases, even if that would > mean for extension developers to switch to a different ID. Yeah... * 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. > >> Then, about the loop used here, I'd be OK to keep the past shortcuts > >> for the builtin fixed-sized stats kinds with the requirement that new > >> builtin stats kinds need to hack into pgstat_report_stat() themselves > >> on efficiency grounds, combined with a second loop for custom kinds, > >> taken only if pgstat_register_kind() has been used by tracking if > >> that's the case in a flag. Do you have a specific preference here? > > > > I think the downstream approach of having a flag that says if there are > > *any* > > pending stats is better. > > Works for me. I am finishing with the attached, meaning that > workloads that trigger no stats can take the fast-exit path with a > single boolean check. What do you think? That would work but I'm a bit worried about: - it's easy to miss and error prone to update pgstat_report_fixed (for new code path, new builtin fixed-sized stats kinds) - it's easy to miss for extensions that they have to update this global variable. Indeed, the cb have_static_pending_cb has been removed (there is the new comment in PgStat_KindInfo though, but I've the feeling it's easier to miss as compared to the cb) 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? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
