On Tue, Apr 12, 2022 at 05:46:36PM -0400, Tom Lane wrote: > Nathan Bossart <nathandboss...@gmail.com> writes: >> If we allow changing GUCs in _PG_init() and provide another hook where >> MaxBackends will be initialized, do we need to introduce another GUC flag, >> or can we get away with just blocking all GUC changes when the new hook is >> called? I'm slightly hesitant to add a GUC flag that will need to manually >> maintained. Wouldn't it be easily forgotten? > > On the whole I think Robert's got the right idea: we do not really > need an enforcement mechanism at all. People who are writing > extensions that do this sort of thing will learn how to do it right. > (It's not like there's not a thousand other things they have to get > right.)
Okay. So maybe we only need the attached patches. 0001 is just 5ecd018 un-reverted, and 0002 is Julien's patch from a few weeks ago with some minor edits. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
>From 4c5ebca537ffdfbf61079a82b18ce7bc97222c69 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nathandboss...@gmail.com> Date: Tue, 12 Apr 2022 14:57:00 -0700 Subject: [PATCH v3 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 3dcaf8a4a5..d57fefa9a8 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 862a5b9a8a27995157a8b71b8f3ce09bf6ed17c5 Mon Sep 17 00:00:00 2001 From: Julien Rouhaud <julien.rouh...@free.fr> Date: Fri, 25 Mar 2022 11:05:24 +0800 Subject: [PATCH v3 2/2] Add a new shmem_request_hook hook. Currently, preloaded libraries are expected to request additional shared memory in _PG_init(). However, it is not unusal for such requests to depend on MaxBackends, which won't be initialized at that time. This introduces a new hook where modules can use MaxBackends to request additional shared memory. Author: Julien Rouhaud Discussion: https://postgr.es/m/20220412210112.GA2065815%40nathanxps13 --- src/backend/storage/ipc/ipci.c | 15 +++++++++++++++ src/include/storage/ipc.h | 2 ++ src/tools/pgindent/typedefs.list | 1 + 3 files changed, 18 insertions(+) diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c index 75e456360b..fdd72175d5 100644 --- a/src/backend/storage/ipc/ipci.c +++ b/src/backend/storage/ipc/ipci.c @@ -52,6 +52,7 @@ /* GUCs */ int shared_memory_type = DEFAULT_SHARED_MEMORY_TYPE; +shmem_request_hook_type shmem_request_hook = NULL; shmem_startup_hook_type shmem_startup_hook = NULL; static Size total_addin_request = 0; @@ -152,6 +153,20 @@ CalculateShmemSize(int *num_semaphores) size = add_size(size, ShmemBackendArraySize()); #endif + /* + * Final chance for modules to request additional shared memory. + * + * Ordinarily, modules call RequestAddinShmemSpace() in _PG_init(), but it + * is not unusal for such requests to depend on MaxBackends, which won't be + * initialized at that time. This hook provides an opportunity for modules + * to use MaxBackends when requesting shared memory. + */ + if (addin_request_allowed && shmem_request_hook) + { + Assert(MaxBackends > 0); + shmem_request_hook(); + } + /* freeze the addin request size and include it */ addin_request_allowed = false; size = add_size(size, total_addin_request); diff --git a/src/include/storage/ipc.h b/src/include/storage/ipc.h index fade4dbe63..5f2c6683db 100644 --- a/src/include/storage/ipc.h +++ b/src/include/storage/ipc.h @@ -19,6 +19,7 @@ #define IPC_H typedef void (*pg_on_exit_callback) (int code, Datum arg); +typedef void (*shmem_request_hook_type) (void); typedef void (*shmem_startup_hook_type) (void); /*---------- @@ -75,6 +76,7 @@ extern void on_exit_reset(void); extern void check_on_shmem_exit_lists_are_empty(void); /* ipci.c */ +extern PGDLLIMPORT shmem_request_hook_type shmem_request_hook; extern PGDLLIMPORT shmem_startup_hook_type shmem_startup_hook; extern Size CalculateShmemSize(int *num_semaphores); 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
