Date: Tue, 12 Apr 2022 15:26:16 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Robert Haas <robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>,
	Julien Rouhaud <rjuju123@gmail.com>,
	Michael Paquier <michael@paquier.xyz>,
	"Bossart, Nathan" <bossartn@amazon.com>,
	Fujii Masao <masao.fujii@oss.nttdata.com>,
	"wangsh.fnst@fujitsu.com" <wangsh.fnst@fujitsu.com>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Greg Sabino Mullane <htamfids@gmail.com>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Subject: Re: make MaxBackends available in _PG_init
Message-ID: <20220412222616.GA2113503@nathanxps13>
References: <20220411211435.GA2057981@nathanxps13>
 <20220412174427.sngj2uexzsttazs2@alap3.anarazel.de>
 <20220412180306.GA2063516@nathanxps13>
 <CA+TgmoY4og1VADa78VySwsTjtTQbtXbnRPSq9x=e6ogS_wWfaw@mail.gmail.com>
 <908945.1649791317@sss.pgh.pa.us>
 <CA+TgmoZM7=VoAey4rf+sZEKFpGDWhqEqk_MHoBPXkYbAONGcyQ@mail.gmail.com>
 <20220412195908.GA2064996@nathanxps13>
 <918045.1649797122@sss.pgh.pa.us>
 <20220412210745.GB2065815@nathanxps13>
 <995617.1649799996@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="IJpNTDwzlM2Ie8A6"
Content-Disposition: inline
In-Reply-To: <995617.1649799996@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/20220412222616.GA2113503%40nathanxps13>
Precedence: bulk


--IJpNTDwzlM2Ie8A6
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Apr 12, 2022 at 05:46:36PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@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

--IJpNTDwzlM2Ie8A6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0001-Fix-comments-about-bgworker-registration-before-M.patch"

From 4c5ebca537ffdfbf61079a82b18ce7bc97222c69 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@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


--IJpNTDwzlM2Ie8A6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0002-Add-a-new-shmem_request_hook-hook.patch"

From 862a5b9a8a27995157a8b71b8f3ce09bf6ed17c5 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@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


--IJpNTDwzlM2Ie8A6--