On Tue, Apr 19, 2022 at 05:49:13PM +0800, Julien Rouhaud wrote: > On Mon, Apr 18, 2022 at 08:17:04PM -0400, Tom Lane wrote: >> Nathan Bossart <nathandboss...@gmail.com> writes: >> > I'm looking for a clean way to ERROR if someone attempts to call >> > RequestAddinShmemSpace() or RequestNamedLWLockTranche() outside of the >> > hook. Currently, we are using static variables in ipci.c and lwlock.c to >> > silently ignore invalid requests. I could add a new 'extern bool' called >> > 'process_shmem_requests_in_progress', but extensions could easily hack >> > around that to allow requests in _PG_init(). Maybe I am overthinking all >> > this and that is good enough. >> >> If they do that and it breaks something, that's their fault not ours. >> (It's not like there's not $BIGNUM ways for a C-language module to >> break the backend, anyway.) > > Agreed. Similarly the process_shared_preload_libraries_in_progress flag could > be modified by extension, and that wouldn't be any better. > >> BTW, I'd make such errors FATAL, as it's unlikely that we can recover >> cleanly from an error during initialization of a loadable module. >> The module's likely to be only partially initialized/hooked in. > > While at it, should we make process_shmem_requests_in_progress true when the > new hook is called? The hook should only be called when that's the case, and > extension authors may feel like asserting it.
Okay, I did it this way in v5. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
>From f7c137ffe6fd8ba24f74398333fdad8832647f09 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nathandboss...@gmail.com> Date: Tue, 12 Apr 2022 14:57:00 -0700 Subject: [PATCH v5 1/2] Fix comments about bgworker registration before MaxBackends initialization Since 6bc8ef0b, InitializeMaxBackends() has used max_worker_processes instead of adapting MaxBackends to the number of background workers registered by modules loaded in shared_preload_libraries (at this time, bgworkers were only static, but gained dynamic capabilities as a matter of supporting parallel queries meaning that a control cap was necessary). Some comments referred to the past registration logic, making them confusing and incorrect, so fix these. Some of the out-of-core modules that could be loaded in this path sometimes like to manipulate dynamically some of the resource-related GUCs for their own needs, this commit adds a note about that. Author: Nathan Bossart Discussion: https://postgr.es/m/20220127181815.GA551692@nathanxps13 --- src/backend/postmaster/postmaster.c | 10 ++++------ src/backend/utils/init/postinit.c | 5 ++--- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 964a56dec4..ce4007bb2c 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -1005,10 +1005,8 @@ PostmasterMain(int argc, char *argv[]) LocalProcessControlFile(false); /* - * Register the apply launcher. Since it registers a background worker, - * it needs to be called before InitializeMaxBackends(), and it's probably - * a good idea to call it before any modules had chance to take the - * background worker slots. + * Register the apply launcher. It's probably a good idea to call this + * before any modules had a chance to take the background worker slots. */ ApplyLauncherRegister(); @@ -1029,8 +1027,8 @@ PostmasterMain(int argc, char *argv[]) #endif /* - * Now that loadable modules have had their chance to register background - * workers, calculate MaxBackends. + * Now that loadable modules have had their chance to alter any GUCs, + * calculate MaxBackends. */ InitializeMaxBackends(); diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 9139fe895c..a28612b375 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -538,9 +538,8 @@ pg_split_opts(char **argv, int *argcp, const char *optstr) /* * Initialize MaxBackends value from config options. * - * This must be called after modules have had the chance to register background - * workers in shared_preload_libraries, and before shared memory size is - * determined. + * This must be called after modules have had the chance to alter GUCs in + * shared_preload_libraries and before shared memory size is determined. * * Note that in EXEC_BACKEND environment, the value is passed down from * postmaster to subprocesses via BackendParameters in SubPostmasterMain; only -- 2.25.1
>From 77cc9c5181d4f6dd477542d4875c166236aec2ed Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nathandboss...@gmail.com> Date: Mon, 18 Apr 2022 15:25:37 -0700 Subject: [PATCH v5 2/2] Add a new shmem_request_hook hook. Currently, preloaded libraries are expected to request additional shared memory and LWLocks in _PG_init(). However, it is not unusal for such requests to depend on MaxBackends, which won't be initialized at that time. Such requests could also depend on GUCs that other modules might change. This introduces a new hook where modules can safely use MaxBackends and GUCs to request additional shared memory and LWLocks. Furthermore, this change restricts requests for shared memory and LWLocks to this hook. Previously, libraries could make requests until the size of the main shared memory segment was calculated. Unlike before, we no longer silently ignore requests received at invalid times. Instead, we FATAL if someone tries to request additional shared memory or LWLocks outside of the hook. Authors: Julien Rouhaud, Nathan Bossart Discussion: https://postgr.es/m/20220412210112.GA2065815%40nathanxps13 --- contrib/pg_prewarm/autoprewarm.c | 27 ++++++++++++++++++- .../pg_stat_statements/pg_stat_statements.c | 27 +++++++++++++------ src/backend/postmaster/postmaster.c | 5 ++++ src/backend/storage/ipc/ipci.c | 20 +++++--------- src/backend/storage/lmgr/lwlock.c | 18 ++++--------- src/backend/utils/init/miscinit.c | 15 +++++++++++ src/include/miscadmin.h | 5 ++++ src/tools/pgindent/typedefs.list | 1 + 8 files changed, 82 insertions(+), 36 deletions(-) diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c index 45e012a63a..14345d060a 100644 --- a/contrib/pg_prewarm/autoprewarm.c +++ b/contrib/pg_prewarm/autoprewarm.c @@ -83,6 +83,7 @@ typedef struct AutoPrewarmSharedState } AutoPrewarmSharedState; void _PG_init(void); +void _PG_fini(void); void autoprewarm_main(Datum main_arg); void autoprewarm_database_main(Datum main_arg); @@ -96,6 +97,8 @@ static void apw_start_database_worker(void); static bool apw_init_shmem(void); static void apw_detach_shmem(int code, Datum arg); static int apw_compare_blockinfo(const void *p, const void *q); +static void autoprewarm_shmem_request(void); +static shmem_request_hook_type prev_shmem_request_hook = NULL; /* Pointer to shared-memory state. */ static AutoPrewarmSharedState *apw_state = NULL; @@ -139,13 +142,35 @@ _PG_init(void) MarkGUCPrefixReserved("pg_prewarm"); - RequestAddinShmemSpace(MAXALIGN(sizeof(AutoPrewarmSharedState))); + prev_shmem_request_hook = shmem_request_hook; + shmem_request_hook = autoprewarm_shmem_request; /* Register autoprewarm worker, if enabled. */ if (autoprewarm) apw_start_leader_worker(); } +/* + * Module unload callback. + */ +void +_PG_fini(void) +{ + shmem_request_hook = prev_shmem_request_hook; +} + +/* + * Requests any additional shared memory required for autoprewarm. + */ +static void +autoprewarm_shmem_request(void) +{ + if (prev_shmem_request_hook) + prev_shmem_request_hook(); + + RequestAddinShmemSpace(MAXALIGN(sizeof(AutoPrewarmSharedState))); +} + /* * Main entry point for the leader autoprewarm process. Per-database workers * have a separate entry point. diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index df2ce63790..87b75d779e 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -252,6 +252,7 @@ static int exec_nested_level = 0; static int plan_nested_level = 0; /* Saved hook values in case of unload */ +static shmem_request_hook_type prev_shmem_request_hook = NULL; static shmem_startup_hook_type prev_shmem_startup_hook = NULL; static post_parse_analyze_hook_type prev_post_parse_analyze_hook = NULL; static planner_hook_type prev_planner_hook = NULL; @@ -317,6 +318,7 @@ PG_FUNCTION_INFO_V1(pg_stat_statements_1_10); PG_FUNCTION_INFO_V1(pg_stat_statements); PG_FUNCTION_INFO_V1(pg_stat_statements_info); +static void pgss_shmem_request(void); static void pgss_shmem_startup(void); static void pgss_shmem_shutdown(int code, Datum arg); static void pgss_post_parse_analyze(ParseState *pstate, Query *query, @@ -452,17 +454,11 @@ _PG_init(void) MarkGUCPrefixReserved("pg_stat_statements"); - /* - * Request additional shared resources. (These are no-ops if we're not in - * the postmaster process.) We'll allocate or attach to the shared - * resources in pgss_shmem_startup(). - */ - RequestAddinShmemSpace(pgss_memsize()); - RequestNamedLWLockTranche("pg_stat_statements", 1); - /* * Install hooks. */ + prev_shmem_request_hook = shmem_request_hook; + shmem_request_hook = pgss_shmem_request; prev_shmem_startup_hook = shmem_startup_hook; shmem_startup_hook = pgss_shmem_startup; prev_post_parse_analyze_hook = post_parse_analyze_hook; @@ -488,6 +484,7 @@ void _PG_fini(void) { /* Uninstall hooks. */ + shmem_request_hook = prev_shmem_request_hook; shmem_startup_hook = prev_shmem_startup_hook; post_parse_analyze_hook = prev_post_parse_analyze_hook; planner_hook = prev_planner_hook; @@ -498,6 +495,20 @@ _PG_fini(void) ProcessUtility_hook = prev_ProcessUtility; } +/* + * shmem_request hook: request additional shared resources. We'll allocate or + * attach to the shared resources in pgss_shmem_startup(). + */ +static void +pgss_shmem_request(void) +{ + if (prev_shmem_request_hook) + prev_shmem_request_hook(); + + RequestAddinShmemSpace(pgss_memsize()); + RequestNamedLWLockTranche("pg_stat_statements", 1); +} + /* * shmem_startup hook: allocate or attach to shared memory, * then load any pre-existing statistics from file. diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index ce4007bb2c..99d5b2fc1f 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -1032,6 +1032,11 @@ PostmasterMain(int argc, char *argv[]) */ InitializeMaxBackends(); + /* + * Give preloaded libraries a chance to request additional shared memory. + */ + process_shmem_requests(); + /* * Now that loadable modules have had their chance to request additional * shared memory, determine the value of any runtime-computed GUCs that diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c index 75e456360b..26372d95b3 100644 --- a/src/backend/storage/ipc/ipci.c +++ b/src/backend/storage/ipc/ipci.c @@ -55,25 +55,21 @@ int shared_memory_type = DEFAULT_SHARED_MEMORY_TYPE; shmem_startup_hook_type shmem_startup_hook = NULL; static Size total_addin_request = 0; -static bool addin_request_allowed = true; - /* * RequestAddinShmemSpace * Request that extra shmem space be allocated for use by * a loadable module. * - * This is only useful if called from the _PG_init hook of a library that - * is loaded into the postmaster via shared_preload_libraries. Once - * shared memory has been allocated, calls will be ignored. (We could - * raise an error, but it seems better to make it a no-op, so that - * libraries containing such calls can be reloaded if needed.) + * This may only be called via the shmem_request_hook of a library that is + * loaded into the postmaster via shared_preload_libraries. Calls from + * elsewhere will fail. */ void RequestAddinShmemSpace(Size size) { - if (IsUnderPostmaster || !addin_request_allowed) - return; /* too late */ + if (!process_shmem_requests_in_progress) + elog(FATAL, "cannot request additional shared memory outside shmem_request_hook"); total_addin_request = add_size(total_addin_request, size); } @@ -83,9 +79,6 @@ RequestAddinShmemSpace(Size size) * * If num_semaphores is not NULL, it will be set to the number of semaphores * required. - * - * Note that this function freezes the additional shared memory request size - * from loadable modules. */ Size CalculateShmemSize(int *num_semaphores) @@ -152,8 +145,7 @@ CalculateShmemSize(int *num_semaphores) size = add_size(size, ShmemBackendArraySize()); #endif - /* freeze the addin request size and include it */ - addin_request_allowed = false; + /* include additional requested shmem from preload libraries */ size = add_size(size, total_addin_request); /* might as well round it off to a multiple of a typical page size */ diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c index fef462b110..8aef909037 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -243,8 +243,6 @@ int NamedLWLockTrancheRequests = 0; /* points to data in shared memory: */ NamedLWLockTranche *NamedLWLockTrancheArray = NULL; -static bool lock_named_request_allowed = true; - static void InitializeLWLocks(void); static inline void LWLockReportWaitStart(LWLock *lock); static inline void LWLockReportWaitEnd(void); @@ -458,9 +456,6 @@ LWLockShmemSize(void) for (i = 0; i < NamedLWLockTrancheRequests; i++) size = add_size(size, strlen(NamedLWLockTrancheRequestArray[i].tranche_name) + 1); - /* Disallow adding any more named tranches. */ - lock_named_request_allowed = false; - return size; } @@ -691,12 +686,9 @@ LWLockRegisterTranche(int tranche_id, const char *tranche_name) * Request that extra LWLocks be allocated during postmaster * startup. * - * This is only useful for extensions if called from the _PG_init hook - * of a library that is loaded into the postmaster via - * shared_preload_libraries. Once shared memory has been allocated, calls - * will be ignored. (We could raise an error, but it seems better to make - * it a no-op, so that libraries containing such calls can be reloaded if - * needed.) + * This may only be called via the shmem_request_hook of a library that is + * loaded into the postmaster via shared_preload_libraries. Calls from + * elsewhere will fail. * * The tranche name will be user-visible as a wait event name, so try to * use a name that fits the style for those. @@ -706,8 +698,8 @@ RequestNamedLWLockTranche(const char *tranche_name, int num_lwlocks) { NamedLWLockTrancheRequest *request; - if (IsUnderPostmaster || !lock_named_request_allowed) - return; /* too late */ + if (!process_shmem_requests_in_progress) + elog(FATAL, "cannot request additional LWLocks outside shmem_request_hook"); if (NamedLWLockTrancheRequestArray == NULL) { diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index 30f0f19dd5..546a8e573e 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -1612,6 +1612,9 @@ char *local_preload_libraries_string = NULL; bool process_shared_preload_libraries_in_progress = false; bool process_shared_preload_libraries_done = false; +shmem_request_hook_type shmem_request_hook = NULL; +bool process_shmem_requests_in_progress = false; + /* * load the shared libraries listed in 'libraries' * @@ -1695,6 +1698,18 @@ process_session_preload_libraries(void) true); } +/* + * process any shared memory requests from preloaded libraries + */ +void +process_shmem_requests(void) +{ + process_shmem_requests_in_progress = true; + if (shmem_request_hook) + shmem_request_hook(); + process_shmem_requests_in_progress = false; +} + void pg_bindtextdomain(const char *domain) { diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 53fd168d93..0af130fbc5 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -465,6 +465,7 @@ extern void BaseInit(void); extern PGDLLIMPORT bool IgnoreSystemIndexes; extern PGDLLIMPORT bool process_shared_preload_libraries_in_progress; extern PGDLLIMPORT bool process_shared_preload_libraries_done; +extern PGDLLIMPORT bool process_shmem_requests_in_progress; extern PGDLLIMPORT char *session_preload_libraries_string; extern PGDLLIMPORT char *shared_preload_libraries_string; extern PGDLLIMPORT char *local_preload_libraries_string; @@ -478,9 +479,13 @@ extern bool RecheckDataDirLockFile(void); extern void ValidatePgVersion(const char *path); extern void process_shared_preload_libraries(void); extern void process_session_preload_libraries(void); +extern void process_shmem_requests(void); extern void pg_bindtextdomain(const char *domain); extern bool has_rolreplication(Oid roleid); +typedef void (*shmem_request_hook_type) (void); +extern PGDLLIMPORT shmem_request_hook_type shmem_request_hook; + /* in executor/nodeHash.c */ extern size_t get_hash_memory_limit(void); diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 87ee7bf866..71a97654e0 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3549,6 +3549,7 @@ shm_mq_result shm_toc shm_toc_entry shm_toc_estimator +shmem_request_hook_type shmem_startup_hook_type sig_atomic_t sigjmp_buf -- 2.25.1
