From ee3f8d3d3aec0d7c961d6b398d31504bb272a450 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Fri, 6 Aug 2021 18:54:39 -0700 Subject: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use of pgstat by AV. Previously pgstat_initialize() was called in InitPostgres() and AuxiliaryProcessMain(). As it turns out there was at least one case where we reported stats before pgstat_initialize() was called, see AutoVacWorkerMain()'s intentionally early call to pgstat_report_autovac(). This turns out to not be a problem with the current pgstat implementation as pgstat_initialize() only registers a shutdown callback. But in the shared memory based stats implementation we are working towards pgstat_initialize() has to do more work. After b406478b87e BaseInit() is a central place where initialization shared by normal backends and auxiliary backends can be put. Obviously BaseInit() is called before InitPostgres() registers ShutdownPostgres. Previously ShutdownPostgres was the first before_shmem_exit callback, now that's commonly pgstats. That should be fine. Previously pgstat_initialize() was not called in bootstrap mode, but there does not appear to be a need for that. It's now done unconditionally. To detect future issues like this, assertions are added to a few places verifying that the pgstat subsystem is initialized and not yet shut down. Author: Andres Freund Discussion: https://postgr.es/m/20210405092914.mmxqe7j56lsjfsej@alap3.anarazel.de Discussion: https://postgr.es/m/20210802164124.ufo5buo4apl6yuvs@alap3.anarazel.de --- src/backend/utils/init/postinit.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) (limited to 'src/backend/utils/init/postinit.c') diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 420b246fb13..e37b86494e1 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -517,6 +517,14 @@ BaseInit(void) */ DebugFileOpen(); + /* + * Initialize statistics reporting. This needs to happen early to ensure + * that pgstat's shutdown callback runs after the shutdown callbacks of + * all subsystems that can produce stats (like e.g. transaction commits + * can). + */ + pgstat_initialize(); + /* Do local initialization of file, storage and buffer managers */ InitFileAccess(); InitSync(); @@ -646,10 +654,6 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username, /* Initialize portal manager */ EnablePortalManager(); - /* Initialize stats collection --- must happen before first xact */ - if (!bootstrap) - pgstat_initialize(); - /* Initialize status reporting */ if (!bootstrap) pgstat_beinit(); @@ -662,11 +666,12 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username, /* * Set up process-exit callback to do pre-shutdown cleanup. This is the - * first before_shmem_exit callback we register; thus, this will be the - * last thing we do before low-level modules like the buffer manager begin - * to close down. We need to have this in place before we begin our first - * transaction --- if we fail during the initialization transaction, as is - * entirely possible, we need the AbortTransaction call to clean up. + * one of the first before_shmem_exit callbacks we register; thus, this + * will be one the last things we do before low-level modules like the + * buffer manager begin to close down. We need to have this in place + * before we begin our first transaction --- if we fail during the + * initialization transaction, as is entirely possible, we need the + * AbortTransaction call to clean up. */ before_shmem_exit(ShutdownPostgres, 0); -- cgit v1.2.3