On Fri, Mar 25, 2022 at 10:39:51AM +0800, Julien Rouhaud wrote:
> On Thu, Mar 24, 2022 at 04:27:36PM -0400, Robert Haas wrote:
> > On Thu, Mar 24, 2022 at 4:20 PM Nathan Bossart <nathandboss...@gmail.com>
> > wrote:
> > > Another possibility could be to add a hook that is called _before_
> > > _PG_init() where libraries are permitted to adjust GUCs. After the
> > > library
> > > is loaded, we first call this _PG_change_GUCs() function, then we
> > > initialize MaxBackends, and then we finally call _PG_init(). This way,
> > > extensions would have access to MaxBackends within _PG_init(), and if an
> > > extension really needed to alter GUCs, іt could define this new function.
> >
> > Yeah, I think this might be better.
>
> Well, if it's before _PG_init() then it won't be a new hook but a new symbol
> that has to be handled with dlsym.
>
> But it seems to be that the bigger problem is that this approach won't fix
> anything unless we can prevent third-party code from messing with GUCs after
> that point as we could otherwise have discrepancies between GetMaxBackends()
> and the underlying GUCs until every single extension that wants to change GUC
> is modified uses this new symbol. And I also don't see how we could force
> that
> unless we have a better GUC API that doesn't entirely relies on strings,
> otherwise a simple thing like "allow 5 extra bgworkers" is going to be really
> painful.
>
> A new hook after _PG_init() sure leaves the burden to the few extension that
> needs to allocate shmem based on MaxBackends, but there probably isn't that
> much (I have a few dozens locally cloned and found only 1 apart from mine, and
> I would be happy to take care of those), and it also means that they have a
> chance to do something correct even if other extensions messing with GUCs
> aren't fixed.
>
> Arguably, you could certainly try to change GUCs in that new hook, but it
> wouldn't be any different from doing so in shmem_startup_hook so I don't think
> it's really a problem.
As an example, here's a POC for a new shmem_request_hook hook after _PG_init().
With it I could easily fix pg_wait_sampling shmem allocation (and checked that
it's indeed requesting the correct size).
>From bd6b14c1b5a5cc193913c43497ee2bcd5f2e4418 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 v1] POC: Add a new shmem_request_hook hook.
---
src/backend/storage/ipc/ipci.c | 12 ++++++++++++
src/include/storage/ipc.h | 2 ++
src/tools/pgindent/typedefs.list | 1 +
3 files changed, 15 insertions(+)
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index cd4ebe2fc5..8b5aa128d0 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -51,6 +51,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;
@@ -149,6 +150,17 @@ CalculateShmemSize(int *num_semaphores)
size = add_size(size, ShmemBackendArraySize());
#endif
+ /*
+ * Final round for addin request size, if not already frozen.
+ * Note to extension authors: this hook is aimed to perform
+ * RequestAddinShmemSpace calls only, if those depends on
GetMaxBackends.
+ */
+ if (addin_request_allowed && shmem_request_hook)
+ {
+ Assert(GetMaxBackends() > 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 49688036a7..c59b2d96d8 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -3537,6 +3537,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.33.1
