public inbox for [email protected]
help / color / mirror / Atom feedRe: make MaxBackends available in _PG_init
29+ messages / 6 participants
[nested] [flat]
* Re: make MaxBackends available in _PG_init
@ 2022-04-12 18:03 Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Nathan Bossart @ 2022-04-12 18:03 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; Tom Lane <[email protected]>; [email protected] <[email protected]>
On Tue, Apr 12, 2022 at 10:44:27AM -0700, Andres Freund wrote:
> On 2022-04-11 14:14:35 -0700, Nathan Bossart wrote:
>> Here's a new patch set. I've only changed 0003 as described above. My
>> apologies for the unnecessary round trip.
>
> ISTM we shouldn't apply 0002, 0003 to master before we've branches 15 off..
That seems reasonable to me.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
@ 2022-04-12 19:12 ` Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:49 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
0 siblings, 2 replies; 29+ messages in thread
From: Robert Haas @ 2022-04-12 19:12 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; Tom Lane <[email protected]>; [email protected] <[email protected]>
On Tue, Apr 12, 2022 at 2:03 PM Nathan Bossart <[email protected]> wrote:
> On Tue, Apr 12, 2022 at 10:44:27AM -0700, Andres Freund wrote:
> > On 2022-04-11 14:14:35 -0700, Nathan Bossart wrote:
> >> Here's a new patch set. I've only changed 0003 as described above. My
> >> apologies for the unnecessary round trip.
> >
> > ISTM we shouldn't apply 0002, 0003 to master before we've branches 15 off..
>
> That seems reasonable to me.
Gah, I hate putting this off for another year, but I guess I'm also
not convinced that 0003 has the right idea, so maybe it's for the
best. Here's what's bugging me: 0003 decrees that _PG_init() is the
Wrong Place to Adjust GUC Settings. Now, that begs the question: what
is the right place to adjust GUC settings? I argued upthread that
extensions shouldn't be overriding values from postgresql.conf at all,
but on further reflection I think there's at least one legitimate use
case for this sort of thing: some kind of auto-tuning module that, for
example, looks at how much memory you have and sets shared_buffers or
work_mem or something based on the result. It's arguable how well
something like this can actually work, but we probably shouldn't try
to prevent people from doing it. I'm a little less clear why Citus
thinks this is an appropriate thing for it to do, vs. telling the user
to configure a useful value, and I'd be curious to hear an explanation
around that.
But if there's even one use case where adjusting GUCs at this phase is
reasonable, then 0003 isn't really good enough. We need an 0004 that
provides a new hook in a place where such changes can safely be made.
Meanwhile, committed 0001.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
@ 2022-04-12 19:21 ` Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
1 sibling, 1 reply; 29+ messages in thread
From: Tom Lane @ 2022-04-12 19:21 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
Robert Haas <[email protected]> writes:
> Gah, I hate putting this off for another year, but I guess I'm also
> not convinced that 0003 has the right idea, so maybe it's for the
> best. Here's what's bugging me: 0003 decrees that _PG_init() is the
> Wrong Place to Adjust GUC Settings. Now, that begs the question: what
> is the right place to adjust GUC settings? I argued upthread that
> extensions shouldn't be overriding values from postgresql.conf at all,
> but on further reflection I think there's at least one legitimate use
> case for this sort of thing: some kind of auto-tuning module that, for
> example, looks at how much memory you have and sets shared_buffers or
> work_mem or something based on the result.
Yeah. It's a very long way from "changing shared memory sizes here
doesn't work" to "no extension is allowed to change any GUC at load
time". A counterexample is that it's not clear why an extension
shouldn't be allowed to define a GUC using DefineCustomXXXVariable
and then immediately set it to some other value. I think trying to
forbid that across-the-board is going to mostly result in breaking
a lot of cases that are perfectly safe.
regards, tom lane
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
@ 2022-04-12 19:30 ` Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Robert Haas @ 2022-04-12 19:30 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
On Tue, Apr 12, 2022 at 3:22 PM Tom Lane <[email protected]> wrote:
> Yeah. It's a very long way from "changing shared memory sizes here
> doesn't work" to "no extension is allowed to change any GUC at load
> time". A counterexample is that it's not clear why an extension
> shouldn't be allowed to define a GUC using DefineCustomXXXVariable
> and then immediately set it to some other value. I think trying to
> forbid that across-the-board is going to mostly result in breaking
> a lot of cases that are perfectly safe.
Hmm, that's an interesting case which I hadn't considered. It seems
like sort of a lame thing to do, because why wouldn't you just create
the GUC with the right initial value instead of modifying it after the
fact? But maybe there's some use case in which it makes sense to do it
that way. A read-only GUC that advertises some calculated value,
perhaps?
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
@ 2022-04-12 19:59 ` Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Nathan Bossart @ 2022-04-12 19:59 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
On Tue, Apr 12, 2022 at 03:30:23PM -0400, Robert Haas wrote:
> On Tue, Apr 12, 2022 at 3:22 PM Tom Lane <[email protected]> wrote:
>> Yeah. It's a very long way from "changing shared memory sizes here
>> doesn't work" to "no extension is allowed to change any GUC at load
>> time". A counterexample is that it's not clear why an extension
>> shouldn't be allowed to define a GUC using DefineCustomXXXVariable
>> and then immediately set it to some other value. I think trying to
>> forbid that across-the-board is going to mostly result in breaking
>> a lot of cases that are perfectly safe.
>
> Hmm, that's an interesting case which I hadn't considered. It seems
> like sort of a lame thing to do, because why wouldn't you just create
> the GUC with the right initial value instead of modifying it after the
> fact? But maybe there's some use case in which it makes sense to do it
> that way. A read-only GUC that advertises some calculated value,
> perhaps?
I think it'd be reasonable to allow changing custom GUCs in _PG_init().
Those aren't going to impact things like MaxBackends.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
@ 2022-04-12 20:58 ` Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 21:14 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
0 siblings, 2 replies; 29+ messages in thread
From: Tom Lane @ 2022-04-12 20:58 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Robert Haas <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
Nathan Bossart <[email protected]> writes:
> I think it'd be reasonable to allow changing custom GUCs in _PG_init().
> Those aren't going to impact things like MaxBackends.
It's reasonable to allow changing *most* GUCs in _PG_init(); let us
please not break cases that work fine today.
It seems after a bit of reflection that what we ought to do is identify
the subset of PGC_POSTMASTER GUCs that feed into shared memory sizing,
mark those with a new GUC flag, and not allow them to be changed after
shared memory is set up. (An alternative to a new flag bit is to
subdivide the PGC_POSTMASTER context into two values, but that seems
like it'd be far more invasive. A flag bit needn't be user-visible.)
Then, what we'd need to do is arrange for shared-preload modules to
receive their _PG_init call before shared memory creation (when they
can still twiddle such GUCs) and then another callback later.
regards, tom lane
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
@ 2022-04-12 21:07 ` Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
1 sibling, 1 reply; 29+ messages in thread
From: Nathan Bossart @ 2022-04-12 21:07 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
On Tue, Apr 12, 2022 at 04:58:42PM -0400, Tom Lane wrote:
> Nathan Bossart <[email protected]> writes:
>> I think it'd be reasonable to allow changing custom GUCs in _PG_init().
>> Those aren't going to impact things like MaxBackends.
>
> It's reasonable to allow changing *most* GUCs in _PG_init(); let us
> please not break cases that work fine today.
Right, this is a fair point.
> It seems after a bit of reflection that what we ought to do is identify
> the subset of PGC_POSTMASTER GUCs that feed into shared memory sizing,
> mark those with a new GUC flag, and not allow them to be changed after
> shared memory is set up. (An alternative to a new flag bit is to
> subdivide the PGC_POSTMASTER context into two values, but that seems
> like it'd be far more invasive. A flag bit needn't be user-visible.)
> Then, what we'd need to do is arrange for shared-preload modules to
> receive their _PG_init call before shared memory creation (when they
> can still twiddle such GUCs) and then another callback later.
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?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
@ 2022-04-12 21:46 ` Tom Lane <[email protected]>
2022-04-12 22:26 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Tom Lane @ 2022-04-12 21:46 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Robert Haas <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
Nathan Bossart <[email protected]> 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.)
regards, tom lane
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
@ 2022-04-12 22:26 ` Nathan Bossart <[email protected]>
2022-04-13 13:16 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Nathan Bossart @ 2022-04-12 22:26 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
On Tue, Apr 12, 2022 at 05:46:36PM -0400, Tom Lane wrote:
> Nathan Bossart <[email protected]> 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
Attachments:
[text/x-diff] v3-0001-Fix-comments-about-bgworker-registration-before-M.patch (3.0K, ../../20220412222616.GA2113503@nathanxps13/2-v3-0001-Fix-comments-about-bgworker-registration-before-M.patch)
download | inline diff:
From 4c5ebca537ffdfbf61079a82b18ce7bc97222c69 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
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
[text/x-diff] v3-0002-Add-a-new-shmem_request_hook-hook.patch (2.9K, ../../20220412222616.GA2113503@nathanxps13/3-v3-0002-Add-a-new-shmem_request_hook-hook.patch)
download | inline diff:
From 862a5b9a8a27995157a8b71b8f3ce09bf6ed17c5 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <[email protected]>
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
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 22:26 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
@ 2022-04-13 13:16 ` Robert Haas <[email protected]>
2022-04-13 13:27 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Robert Haas @ 2022-04-13 13:16 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
On Tue, Apr 12, 2022 at 6:26 PM Nathan Bossart <[email protected]> wrote:
> 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.
Hmm. I suppose I was thinking that we'd go the other way around: move
the call to InitializeMaxBackends() earlier, as proposed previously,
and add a hook to allow extensions to get control before that point.
The reason that I like that approach is that I suspect that it's more
common for extensions to want to size shared memory data structures
than it is for them to want to change GUCs, and therefore I thought
that approach would fix things for the most amount of people with the
least amount of code change. But it seems like maybe Tom thinks I'm
incorrect about the relative frequency of those things, so I don't
know.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 22:26 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-13 13:16 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
@ 2022-04-13 13:27 ` Tom Lane <[email protected]>
2022-04-13 14:22 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Tom Lane @ 2022-04-13 13:27 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
Robert Haas <[email protected]> writes:
> Hmm. I suppose I was thinking that we'd go the other way around: move
> the call to InitializeMaxBackends() earlier, as proposed previously,
> and add a hook to allow extensions to get control before that point.
> The reason that I like that approach is that I suspect that it's more
> common for extensions to want to size shared memory data structures
> than it is for them to want to change GUCs, and therefore I thought
> that approach would fix things for the most amount of people with the
> least amount of code change. But it seems like maybe Tom thinks I'm
> incorrect about the relative frequency of those things, so I don't
> know.
Maybe I'm missing something, but I figured we'd keep the _PG_init
calls where they are to minimize side-effects, and then add an optional
hook just before/after shared memory size is determined. Cases that
work well now continue to work well, and cases that don't work so
well can be fixed by making use of the hook. In particular you
can still do RequestAddinShmemSpace() in _PG_init as long as the
request size doesn't depend on factors that other extensions might
change. If you're doing something funny then you might need to
postpone RequestAddinShmemSpace till the new hook call.
regards, tom lane
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 22:26 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-13 13:16 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 13:27 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
@ 2022-04-13 14:22 ` Robert Haas <[email protected]>
2022-04-13 14:43 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Robert Haas @ 2022-04-13 14:22 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
On Wed, Apr 13, 2022 at 9:27 AM Tom Lane <[email protected]> wrote:
> Robert Haas <[email protected]> writes:
> > Hmm. I suppose I was thinking that we'd go the other way around: move
> > the call to InitializeMaxBackends() earlier, as proposed previously,
> > and add a hook to allow extensions to get control before that point.
> > The reason that I like that approach is that I suspect that it's more
> > common for extensions to want to size shared memory data structures
> > than it is for them to want to change GUCs, and therefore I thought
> > that approach would fix things for the most amount of people with the
> > least amount of code change. But it seems like maybe Tom thinks I'm
> > incorrect about the relative frequency of those things, so I don't
> > know.
>
> Maybe I'm missing something, but I figured we'd keep the _PG_init
> calls where they are to minimize side-effects, and then add an optional
> hook just before/after shared memory size is determined. Cases that
> work well now continue to work well, and cases that don't work so
> well can be fixed by making use of the hook. In particular you
> can still do RequestAddinShmemSpace() in _PG_init as long as the
> request size doesn't depend on factors that other extensions might
> change. If you're doing something funny then you might need to
> postpone RequestAddinShmemSpace till the new hook call.
I wouldn't say that approach is wrong. I think we're just prioritizing
different things. I think that the pattern of wanting to request
shared memory in _PG_init() is common, and there are lots of
extensions that already do it that way, and if we pursue this plan,
then all of those extensions really ought to be updated to use the new
hook, including pg_prewarm and pg_stat_statements. They may continue
to work if they don't, but they'll be doing something that is not
really best practice any more but will happen to work until you make
your request for shared memory dependent on the wrong thing, or until
you load up another module at the same time that tweaks some GUC upon
which your calculation depends (imagine an autotuner that adjusts
pg_stat_statements.max at startup time). So I think we'll be
inflicting subtle bugs on extension authors that will never really get
fully sorted out.
Now, on the other hand, I think that the pattern of changing GUCs in
_PG_init() is comparatively uncommon. I don't believe that we have any
in-core code that does it, and the only out-of-core code that does to
my knowledge is Citus. So if we subtly change the best practices
around setting GUCs in _PG_init() hooks, I think that's going to
affect a vastly smaller number of extensions. Either way, we're
changing best practices without really creating a hard break, but I'd
rather move the wood for the small number of extensions that are
tweaking GUCs in _PG_init() than the larger number of extensions (or
so I suppose) that are requesting shared memory there.
Now, we could also decide to create a hard compatibility break to
force extension authors to update. I think that could be done in one
of two ways. One possibility is to refuse SetConfigOption() in
_PG_init(); anyone who wants to do that has to use the new set-a-GUC
hook we simultaneously add. The other is to refuse
RequestAddinShmemSpace() and RequestNamedLWLockTranche() in
_PG_init(); anyone who wants to do that has to use the new
request-shmem-resources hook that we simultaneously add. I tend to
think that the latter is a slightly more principled option, because I
doubt that refusing SetConfigOption() is a bullet-proof defense
against, well, I don't know, anything really. You can hackily modify
GUCs by other means, and you can do bad math with things that aren't
GUCs. I am however pretty sure that if we refuse to provide shmem
space unless you request it from a new add-shmem hook, that will be
pretty water-tight, and it doesn't seem like it should be a big issue
for extension authors to move that code to some new hook we invent.
So in the end I see basically four possibilities here:
A. No hard compatibility break, but invent a set-a-GUC hook that runs earlier.
B. No hard compatibility break, but invent a request-shmem hook that runs later.
C. Invent a set-a-GUC hook that runs earlier AND refuse to set GUCs in _PG_init.
D. Invent a request-shmem hook that runs later AND refuse to accept
shmem requests in _PG_init.
My preferred options are A or D for the reasons explained above. It
seems like you prefer B.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 22:26 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-13 13:16 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 13:27 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 14:22 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
@ 2022-04-13 14:43 ` Tom Lane <[email protected]>
2022-04-13 15:25 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Tom Lane @ 2022-04-13 14:43 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
Robert Haas <[email protected]> writes:
> I wouldn't say that approach is wrong. I think we're just prioritizing
> different things. I think that the pattern of wanting to request
> shared memory in _PG_init() is common, and there are lots of
> extensions that already do it that way, and if we pursue this plan,
> then all of those extensions really ought to be updated to use the new
> hook, including pg_prewarm and pg_stat_statements. They may continue
> to work if they don't, but they'll be doing something that is not
> really best practice any more but will happen to work until you make
> your request for shared memory dependent on the wrong thing, or until
> you load up another module at the same time that tweaks some GUC upon
> which your calculation depends (imagine an autotuner that adjusts
> pg_stat_statements.max at startup time). So I think we'll be
> inflicting subtle bugs on extension authors that will never really get
> fully sorted out.
Yeah, there is something to be said for preventing subtle interactions
between extensions.
> So in the end I see basically four possibilities here:
> A. No hard compatibility break, but invent a set-a-GUC hook that runs earlier.
> B. No hard compatibility break, but invent a request-shmem hook that runs later.
> C. Invent a set-a-GUC hook that runs earlier AND refuse to set GUCs in _PG_init.
> D. Invent a request-shmem hook that runs later AND refuse to accept
> shmem requests in _PG_init.
I dislike A for the reason I already stated: _PG_init should be the
first code we run in an extension. Not doing that is just too hacky
for words. While B breaks the least stuff in the short run, I agree
that it leaves extension authors on the hook to avoid unpleasant
interactions, and that the only way they can be sure to do so is
to move their shmem requests to the new hook. So if we're willing
to accept a hard compatibility break to prevent such bugs, then
I too prefer D to C. What I'm not quite convinced about is whether
the problem is big enough to warrant a compatibility break.
regards, tom lane
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 22:26 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-13 13:16 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 13:27 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 14:22 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 14:43 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
@ 2022-04-13 15:25 ` Robert Haas <[email protected]>
2022-04-13 15:39 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Robert Haas @ 2022-04-13 15:25 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
On Wed, Apr 13, 2022 at 10:43 AM Tom Lane <[email protected]> wrote:
> Yeah, there is something to be said for preventing subtle interactions
> between extensions.
>
> > So in the end I see basically four possibilities here:
>
> > A. No hard compatibility break, but invent a set-a-GUC hook that runs earlier.
> > B. No hard compatibility break, but invent a request-shmem hook that runs later.
> > C. Invent a set-a-GUC hook that runs earlier AND refuse to set GUCs in _PG_init.
> > D. Invent a request-shmem hook that runs later AND refuse to accept
> > shmem requests in _PG_init.
>
> I dislike A for the reason I already stated: _PG_init should be the
> first code we run in an extension. Not doing that is just too hacky
> for words.
That seems like a fair position, even if I don't really understand why
it would be as bad as "too hacky for words."
> While B breaks the least stuff in the short run, I agree
> that it leaves extension authors on the hook to avoid unpleasant
> interactions, and that the only way they can be sure to do so is
> to move their shmem requests to the new hook. So if we're willing
> to accept a hard compatibility break to prevent such bugs, then
> I too prefer D to C. What I'm not quite convinced about is whether
> the problem is big enough to warrant a compatibility break.
It's sort of a philosophical question. How do you measure the size of
such a problem? What units do you even use for such a size
measurement? How big does it have to be to justify a compatibility
break? Presumably it depends on the size of the compatibility break,
which is also not subject to any sort of objective measurement.
I guess my feeling about this is that _PG_init() is sort of a grab
bag, and that's not very good. You're supposed to register new GUCs
there, and request shared memory space, and install any hook functions
you want to use, and maybe some other stuff. But why is it appropriate
for all of those things to happen at the same time? I think it pretty
clearly isn't, and that's why you see _PG_init() functions testing
process_shared_preload_libraries_in_progress, and why
RequestAddinShmemSpace() is a no-op if it's not the right time for
such things. To me, all of that is just a sign that the system is
badly designed. Imagine if someone proposed to make XLogInsert() or
SetConfigOption() or LockBuffer() sometimes just return without doing
anything. We would just call those functions in places where those
actions weren't appropriate, and the function would just do nothing
silently without signalling an error. Surely such a proposal would be
shot down as an awful idea, and the only reason the _PG_init() case is
any different is because it's not new. But it doesn't seem to me that
it's any better of an idea here than it would be there.
And under proposal D we'd actually be fixing that, because we'd have a
hook that is the right place to request shared memory and we'd
complain if those functions were called from anywhere else.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 22:26 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-13 13:16 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 13:27 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 14:22 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 14:43 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:25 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
@ 2022-04-13 15:39 ` Tom Lane <[email protected]>
2022-04-13 15:50 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Tom Lane @ 2022-04-13 15:39 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
Robert Haas <[email protected]> writes:
> I guess my feeling about this is that _PG_init() is sort of a grab
> bag, and that's not very good. You're supposed to register new GUCs
> there, and request shared memory space, and install any hook functions
> you want to use, and maybe some other stuff. But why is it appropriate
> for all of those things to happen at the same time? I think it pretty
> clearly isn't, and that's why you see _PG_init() functions testing
> process_shared_preload_libraries_in_progress, and why
> RequestAddinShmemSpace() is a no-op if it's not the right time for
> such things. To me, all of that is just a sign that the system is
> badly designed.
Hmm, that's a good point. If we can replace "RequestAddinShmemSpace
does nothing if called at the wrong time" with "RequestAddinShmemSpace
throws error if called at the wrong time", that seems like a pretty
clear improvement in robustness.
Is there anything else typically done in _PG_init that has to be
conditional on process_shared_preload_libraries_in_progress? I recall
something about reserving LWLocks, which probably should get the same
treatment. If we can get to a point where _PG_init shouldn't need to
care about process_shared_preload_libraries_in_progress, because all
the stuff that would care is moved to this new hook, then that would
be very clear cleanup.
regards, tom lane
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 22:26 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-13 13:16 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 13:27 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 14:22 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 14:43 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:25 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 15:39 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
@ 2022-04-13 15:50 ` Robert Haas <[email protected]>
2022-04-13 16:05 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Robert Haas @ 2022-04-13 15:50 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
On Wed, Apr 13, 2022 at 11:39 AM Tom Lane <[email protected]> wrote:
> Is there anything else typically done in _PG_init that has to be
> conditional on process_shared_preload_libraries_in_progress? I recall
> something about reserving LWLocks, which probably should get the same
> treatment. If we can get to a point where _PG_init shouldn't need to
> care about process_shared_preload_libraries_in_progress, because all
> the stuff that would care is moved to this new hook, then that would
> be very clear cleanup.
What's a little wonky right now is that it's fairly common for
extensions to just return straight-off without doing *anything at all*
if !process_shared_preload_libraries_in_progress. See
pg_stat_statements for example. It seems kind of strange to me to make
registering GUCs dependent on
process_shared_preload_libraries_in_progress; why shouldn't that just
happen always? It's understandable that we don't want to install the
hook functions if we're not being loaded from shared_preload_libaries,
though.
It may be too much to hope that we're going to completely get rid of
process_shared_preload_libraries_in_progress tests. But even given
that, I think having a request-shared-mem hook makes sense and would
make things cleaner than they are today. Maybe in the future we'll end
up with other hooks as well, like a "this is the place to register
GUCs" hook and a perhaps a "this is the place to register you custom
rmgr" hook. I'm not really sure that we need those and I don't want to
make things complicated just for the heck of it, but it's not shocking
that different operations need to happen at different times in the
startup sequence, and I don't think we should be afraid to split up
the monolithic _PG_init() into as many separate things as are required
to make it work right.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 22:26 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-13 13:16 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 13:27 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 14:22 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 14:43 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:25 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 15:39 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:50 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
@ 2022-04-13 16:05 ` Tom Lane <[email protected]>
2022-04-13 18:30 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Tom Lane @ 2022-04-13 16:05 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
Robert Haas <[email protected]> writes:
> What's a little wonky right now is that it's fairly common for
> extensions to just return straight-off without doing *anything at all*
> if !process_shared_preload_libraries_in_progress. See
> pg_stat_statements for example. It seems kind of strange to me to make
> registering GUCs dependent on
> process_shared_preload_libraries_in_progress; why shouldn't that just
> happen always? It's understandable that we don't want to install the
> hook functions if we're not being loaded from shared_preload_libaries,
> though.
Yeah, I was just investigating that. The problem that pg_stat_statements
has is that it wants to create PGC_POSTMASTER GUCs, which is something
that guc.c specifically forbids:
/*
* Only allow custom PGC_POSTMASTER variables to be created during shared
* library preload; any later than that, we can't ensure that the value
* doesn't change after startup. This is a fatal elog if it happens; just
* erroring out isn't safe because we don't know what the calling loadable
* module might already have hooked into.
*/
if (context == PGC_POSTMASTER &&
!process_shared_preload_libraries_in_progress)
elog(FATAL, "cannot create PGC_POSTMASTER variables after startup");
The key reason guc.c does that is that if it allowed the case, then there
would be no guarantee that a "PGC_POSTMASTER" GUC has the same value in
every backend of the cluster, which'd likely break most use-cases for
such a GUC (it'd certainly break pg_stat_statements, which assumes that
the local setting of that GUC reflects the size of its shmem area).
Perhaps we can improve on that situation with some more thought, but
I'm not very clear on how.
> It may be too much to hope that we're going to completely get rid of
> process_shared_preload_libraries_in_progress tests.
Perhaps, but this is definitely an area that could stand to have some
serious design thought put into it. You're quite right that it's
largely a mess today, but I think proposals like "forbid setting GUCs
during _PG_init" would add to the mess rather than clean things up.
regards, tom lane
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 22:26 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-13 13:16 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 13:27 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 14:22 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 14:43 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:25 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 15:39 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:50 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 16:05 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
@ 2022-04-13 18:30 ` Nathan Bossart <[email protected]>
2022-04-14 05:50 ` Re: make MaxBackends available in _PG_init Julien Rouhaud <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Nathan Bossart @ 2022-04-13 18:30 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
On Wed, Apr 13, 2022 at 12:05:08PM -0400, Tom Lane wrote:
> Robert Haas <[email protected]> writes:
>> It may be too much to hope that we're going to completely get rid of
>> process_shared_preload_libraries_in_progress tests.
>
> Perhaps, but this is definitely an area that could stand to have some
> serious design thought put into it. You're quite right that it's
> largely a mess today, but I think proposals like "forbid setting GUCs
> during _PG_init" would add to the mess rather than clean things up.
+1. The simplest design might be to just make a separate preload hook.
_PG_init() would install hooks, and then this preload hook would do
anything that needs to happen when the library is loaded via s_p_l.
However, I think we still want a new shmem request hook where MaxBackends
is initialized.
If we do move forward with the shmem request hook, do we want to disallow
shmem requests anywhere else, or should we just leave it up to extension
authors to do the right thing? Many shmem requests in _PG_init() are
probably okay unless they depend on MaxBackends or another GUC that someone
might change. Given that, I think I currently prefer the latter (option B
from upthread).
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 22:26 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-13 13:16 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 13:27 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 14:22 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 14:43 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:25 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 15:39 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:50 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 16:05 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 18:30 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
@ 2022-04-14 05:50 ` Julien Rouhaud <[email protected]>
2022-04-14 16:22 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Julien Rouhaud @ 2022-04-14 05:50 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
On Wed, Apr 13, 2022 at 11:30:40AM -0700, Nathan Bossart wrote:
> On Wed, Apr 13, 2022 at 12:05:08PM -0400, Tom Lane wrote:
> > Robert Haas <[email protected]> writes:
> >> It may be too much to hope that we're going to completely get rid of
> >> process_shared_preload_libraries_in_progress tests.
> >
> > Perhaps, but this is definitely an area that could stand to have some
> > serious design thought put into it. You're quite right that it's
> > largely a mess today, but I think proposals like "forbid setting GUCs
> > during _PG_init" would add to the mess rather than clean things up.
>
> +1. The simplest design might be to just make a separate preload hook.
> _PG_init() would install hooks, and then this preload hook would do
> anything that needs to happen when the library is loaded via s_p_l.
> However, I think we still want a new shmem request hook where MaxBackends
> is initialized.
Yeah this one seems needed no matter what.
> If we do move forward with the shmem request hook, do we want to disallow
> shmem requests anywhere else, or should we just leave it up to extension
> authors to do the right thing? Many shmem requests in _PG_init() are
> probably okay unless they depend on MaxBackends or another GUC that someone
> might change. Given that, I think I currently prefer the latter (option B
> from upthread).
I'd be in favor of a hard break. There are already multiple extensions that
relies on non final value of GUCs to size their shmem request. And as an
extension author it can be hard to realize that, as those extensions work just
fine until someone wants to try it with some other extension that changes some
GUC. Forcing shmem request in a new hook will make sure that it's *always*
correct, and that only requires very minimal work on the extension side.
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 22:26 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-13 13:16 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 13:27 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 14:22 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 14:43 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:25 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 15:39 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:50 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 16:05 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 18:30 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-14 05:50 ` Re: make MaxBackends available in _PG_init Julien Rouhaud <[email protected]>
@ 2022-04-14 16:22 ` Nathan Bossart <[email protected]>
2022-04-14 16:32 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Nathan Bossart @ 2022-04-14 16:22 UTC (permalink / raw)
To: Julien Rouhaud <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
On Thu, Apr 14, 2022 at 01:50:24PM +0800, Julien Rouhaud wrote:
> On Wed, Apr 13, 2022 at 11:30:40AM -0700, Nathan Bossart wrote:
>> If we do move forward with the shmem request hook, do we want to disallow
>> shmem requests anywhere else, or should we just leave it up to extension
>> authors to do the right thing? Many shmem requests in _PG_init() are
>> probably okay unless they depend on MaxBackends or another GUC that someone
>> might change. Given that, I think I currently prefer the latter (option B
>> from upthread).
>
> I'd be in favor of a hard break. There are already multiple extensions that
> relies on non final value of GUCs to size their shmem request. And as an
> extension author it can be hard to realize that, as those extensions work just
> fine until someone wants to try it with some other extension that changes some
> GUC. Forcing shmem request in a new hook will make sure that it's *always*
> correct, and that only requires very minimal work on the extension side.
Yeah, this is a good point. If we're okay with breaking existing
extensions like this, I will work on a patch.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 22:26 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-13 13:16 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 13:27 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 14:22 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 14:43 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:25 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 15:39 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:50 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 16:05 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 18:30 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-14 05:50 ` Re: make MaxBackends available in _PG_init Julien Rouhaud <[email protected]>
2022-04-14 16:22 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
@ 2022-04-14 16:32 ` Robert Haas <[email protected]>
2022-04-14 16:39 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Robert Haas @ 2022-04-14 16:32 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Julien Rouhaud <[email protected]>; Tom Lane <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
On Thu, Apr 14, 2022 at 12:22 PM Nathan Bossart
<[email protected]> wrote:
> > I'd be in favor of a hard break. There are already multiple extensions that
> > relies on non final value of GUCs to size their shmem request. And as an
> > extension author it can be hard to realize that, as those extensions work just
> > fine until someone wants to try it with some other extension that changes some
> > GUC. Forcing shmem request in a new hook will make sure that it's *always*
> > correct, and that only requires very minimal work on the extension side.
>
> Yeah, this is a good point. If we're okay with breaking existing
> extensions like this, I will work on a patch.
I tend to think it's a good idea. It's not great to inconvenience
extension authors, but I think that with the kind of design we are
talking about here it will be pretty straightforward to see how to
update your extension: move the code that request shmem resources into
the new hook. If you want to be compatible with older PostgreSQL
releases by conditional compilation, conditionally call that function
from _PG_init() when PG_VERSION_NUM < whatever.
Compare that with the current situation, where things seem to mostly
work but sometimes don't, and it's not quite clear what to do about
it.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 22:26 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-13 13:16 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 13:27 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 14:22 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 14:43 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:25 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 15:39 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:50 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 16:05 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 18:30 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-14 05:50 ` Re: make MaxBackends available in _PG_init Julien Rouhaud <[email protected]>
2022-04-14 16:22 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-14 16:32 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
@ 2022-04-14 16:39 ` Tom Lane <[email protected]>
2022-04-18 22:52 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Tom Lane @ 2022-04-14 16:39 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Julien Rouhaud <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
Robert Haas <[email protected]> writes:
> On Thu, Apr 14, 2022 at 12:22 PM Nathan Bossart
> <[email protected]> wrote:
>>> I'd be in favor of a hard break.
>> Yeah, this is a good point. If we're okay with breaking existing
>> extensions like this, I will work on a patch.
> I tend to think it's a good idea.
I've come around to that view as well.
regards, tom lane
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 22:26 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-13 13:16 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 13:27 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 14:22 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 14:43 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:25 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 15:39 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:50 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 16:05 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 18:30 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-14 05:50 ` Re: make MaxBackends available in _PG_init Julien Rouhaud <[email protected]>
2022-04-14 16:22 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-14 16:32 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-14 16:39 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
@ 2022-04-18 22:52 ` Nathan Bossart <[email protected]>
2022-04-18 23:33 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Nathan Bossart @ 2022-04-18 22:52 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Julien Rouhaud <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
On Thu, Apr 14, 2022 at 12:39:46PM -0400, Tom Lane wrote:
> Robert Haas <[email protected]> writes:
>> On Thu, Apr 14, 2022 at 12:22 PM Nathan Bossart
>> <[email protected]> wrote:
>>>> I'd be in favor of a hard break.
>
>>> Yeah, this is a good point. If we're okay with breaking existing
>>> extensions like this, I will work on a patch.
>
>> I tend to think it's a good idea.
>
> I've come around to that view as well.
Here is a new patch set that introduces the aforementioned "hard break."
I noticed that requests for more LWLocks follow a similar pattern as
regular shared memory requests, and I figured that we would want to do
something similar for those, but I wasn't sure exactly how to proceed. I
saw two options: 1) use shmem_request_hook for both regular requests and
LWLock requests or 2) introduce an lwlock_request_hook. My instinct was
that option 1 was preferable, but AFAICT this requires introducing a new
external variable for inspecting whether the request is made at a valid
time. This would be similar to
process_shared_preload_libraries_in_progress, which I believe means a
determined extension author could easily hack around the request
restrictions. I thought option 2 added too much machinery to work around
this problem. For now, I haven't made any changes for LWLock requests.
What are your thoughts?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v4-0001-Fix-comments-about-bgworker-registration-before-M.patch (3.0K, ../../20220418225244.GA2387491@nathanxps13/2-v4-0001-Fix-comments-about-bgworker-registration-before-M.patch)
download | inline diff:
From 55af4473761a8bcfe8c2ff940a5df4625a40653a Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Tue, 12 Apr 2022 14:57:00 -0700
Subject: [PATCH v4 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
[text/x-diff] v4-0002-Add-a-new-shmem_request_hook-hook.patch (10.4K, ../../20220418225244.GA2387491@nathanxps13/3-v4-0002-Add-a-new-shmem_request_hook-hook.patch)
download | inline diff:
From 135c1363d0ff2aaa1886d4ed59016c3b610c375a Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Mon, 18 Apr 2022 15:25:37 -0700
Subject: [PATCH v4 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. 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.
Furthermore, this change restricts shared memory requests by
preloaded libraries to this hook. Previously, libraries could
request additional shared memory until the size of the main shared
memory segment was calculated. Besides decoupling a common library
task from _PG_init(), this ensures that shared memory requests are
only allowed when MaxBackends is initialized and GUCs should not be
changed. Unlike before, we no longer silently ignore requests
received at invalid times. Instead, we ERROR if someone tries to
request additional shared memory 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 | 39 +++++++++++++------
src/include/storage/ipc.h | 2 +
src/include/storage/shmem.h | 1 +
src/tools/pgindent/typedefs.list | 1 +
7 files changed, 81 insertions(+), 21 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..57663ddc6a 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.
+ */
+ ProcessShmemRequests();
+
/*
* 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..117aacba7d 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -52,28 +52,45 @@
/* 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;
-static bool addin_request_allowed = true;
+static bool addin_request_allowed = false;
+/*
+ * ProcessShmemRequests
+ *
+ * Calls to RequestAddinShmemSpace() by preloaded libraries are only allowed in
+ * the shmem_request_hook.
+ */
+void
+ProcessShmemRequests(void)
+{
+ Assert(MaxBackends > 0);
+
+ addin_request_allowed = true;
+
+ if (shmem_request_hook)
+ shmem_request_hook();
+
+ addin_request_allowed = false;
+}
/*
* 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 ERROR.
*/
void
RequestAddinShmemSpace(Size size)
{
if (IsUnderPostmaster || !addin_request_allowed)
- return; /* too late */
+ elog(ERROR, "cannot request additional shared memory outside shmem_request_hook");
total_addin_request = add_size(total_addin_request, size);
}
@@ -83,9 +100,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)
@@ -93,6 +107,8 @@ CalculateShmemSize(int *num_semaphores)
Size size;
int numSemas;
+ Assert(!addin_request_allowed);
+
/* Compute number of semaphores we'll need */
numSemas = ProcGlobalSemas();
numSemas += SpinlockSemas();
@@ -152,8 +168,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/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/include/storage/shmem.h b/src/include/storage/shmem.h
index de9e7c6e73..bdb0acfedf 100644
--- a/src/include/storage/shmem.h
+++ b/src/include/storage/shmem.h
@@ -46,6 +46,7 @@ extern Size add_size(Size s1, Size s2);
extern Size mul_size(Size s1, Size s2);
/* ipci.c */
+extern void ProcessShmemRequests(void);
extern void RequestAddinShmemSpace(Size size);
/* size constants for the shmem index table */
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
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:07 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 22:26 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-13 13:16 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 13:27 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 14:22 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 14:43 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:25 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 15:39 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 15:50 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-13 16:05 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-13 18:30 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-14 05:50 ` Re: make MaxBackends available in _PG_init Julien Rouhaud <[email protected]>
2022-04-14 16:22 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-14 16:32 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-14 16:39 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-18 22:52 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
@ 2022-04-18 23:33 ` Tom Lane <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Tom Lane @ 2022-04-18 23:33 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Robert Haas <[email protected]>; Julien Rouhaud <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
Nathan Bossart <[email protected]> writes:
> I noticed that requests for more LWLocks follow a similar pattern as
> regular shared memory requests, and I figured that we would want to do
> something similar for those, but I wasn't sure exactly how to proceed. I
> saw two options: 1) use shmem_request_hook for both regular requests and
> LWLock requests or 2) introduce an lwlock_request_hook. My instinct was
> that option 1 was preferable,
Yeah, I agree, which says that maybe the hook name needs to be something
else (not that I have a good proposal).
> but AFAICT this requires introducing a new
> external variable for inspecting whether the request is made at a valid
> time.
Uh, why? It'd be the core code's responsibility to place the hook
call at a point where you could do both.
regards, tom lane
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
@ 2022-04-12 21:14 ` Robert Haas <[email protected]>
2022-04-12 21:43 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
1 sibling, 1 reply; 29+ messages in thread
From: Robert Haas @ 2022-04-12 21:14 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
On Tue, Apr 12, 2022 at 4:58 PM Tom Lane <[email protected]> wrote:
> It's reasonable to allow changing *most* GUCs in _PG_init(); let us
> please not break cases that work fine today.
>
> It seems after a bit of reflection that what we ought to do is identify
> the subset of PGC_POSTMASTER GUCs that feed into shared memory sizing,
> mark those with a new GUC flag, and not allow them to be changed after
> shared memory is set up. (An alternative to a new flag bit is to
> subdivide the PGC_POSTMASTER context into two values, but that seems
> like it'd be far more invasive. A flag bit needn't be user-visible.)
> Then, what we'd need to do is arrange for shared-preload modules to
> receive their _PG_init call before shared memory creation (when they
> can still twiddle such GUCs) and then another callback later.
I dunno, I feel like this is over-engineered. An extra hook wouldn't
really affect anyone who doesn't need to use it, and I doubt that
adapting to it would create much pain for anyone who is knowledgeable
enough to be writing these kinds of extensions in the first place.
This design is something everyone who adds a GUC from now until the
end of the project needs to know about for the benefit of a tiny
number of extension authors who weren't really going to have a big
problem anyway.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:14 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
@ 2022-04-12 21:43 ` Tom Lane <[email protected]>
2022-04-13 07:54 ` Re: make MaxBackends available in _PG_init Michael Paquier <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Tom Lane @ 2022-04-12 21:43 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
Robert Haas <[email protected]> writes:
> On Tue, Apr 12, 2022 at 4:58 PM Tom Lane <[email protected]> wrote:
>> It seems after a bit of reflection that what we ought to do is identify
>> the subset of PGC_POSTMASTER GUCs that feed into shared memory sizing,
>> mark those with a new GUC flag, and not allow them to be changed after
>> shared memory is set up.
> I dunno, I feel like this is over-engineered.
It probably is. I'm just offering this as a solution if people want to
insist on a mechanism to prevent unsafe GUC changes. If we drop the
idea of trying to forcibly prevent extensions from Doing It Wrong,
then we don't need this, only the extra hook.
regards, tom lane
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:21 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 19:30 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 19:59 ` Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
2022-04-12 21:14 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
2022-04-12 21:43 ` Re: make MaxBackends available in _PG_init Tom Lane <[email protected]>
@ 2022-04-13 07:54 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Michael Paquier @ 2022-04-13 07:54 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Nathan Bossart <[email protected]>; Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected] <[email protected]>
On Tue, Apr 12, 2022 at 05:43:17PM -0400, Tom Lane wrote:
> It probably is. I'm just offering this as a solution if people want to
> insist on a mechanism to prevent unsafe GUC changes. If we drop the
> idea of trying to forcibly prevent extensions from Doing It Wrong,
> then we don't need this, only the extra hook.
FWIW, I'd be fine with a simple solution like what Julien has been
proposing with the extra hook, rather than a GUC to enforce all that.
That may be nice in the long-run, but the potential benefits may not
be completely clear, either, after a closer read of this thread.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: make MaxBackends available in _PG_init
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]>
@ 2022-04-12 19:49 ` Nathan Bossart <[email protected]>
1 sibling, 0 replies; 29+ messages in thread
From: Nathan Bossart @ 2022-04-12 19:49 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; Bossart, Nathan <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; Tom Lane <[email protected]>; [email protected] <[email protected]>
On Tue, Apr 12, 2022 at 03:12:42PM -0400, Robert Haas wrote:
> But if there's even one use case where adjusting GUCs at this phase is
> reasonable, then 0003 isn't really good enough. We need an 0004 that
> provides a new hook in a place where such changes can safely be made.
I think that is doable. IMO it should be Ń•omething like _PG_change_GUCs()
that is called before _PG_init(). The other option is to add a hook called
after _PG_init() where MaxBackends is available (in which case we likely
want GetMaxBackends() again). Thoughts?
> Meanwhile, committed 0001.
Thanks.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v1] solaris: Use versioning scripts instead of -Bsymbolic
@ 2022-08-23 08:50 Andres Freund <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Andres Freund @ 2022-08-23 08:50 UTC (permalink / raw)
-Bsymbolic causes a lot of "ld: warning: symbol referencing errors"
warnings. It's quite easy to add the symbol versioning script, we just need a
slightly different parameter name.
Discussion: https://postgr.es/m/[email protected]
---
src/Makefile.shlib | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/Makefile.shlib b/src/Makefile.shlib
index 2af6192f0f3..3202b2e67de 100644
--- a/src/Makefile.shlib
+++ b/src/Makefile.shlib
@@ -188,10 +188,15 @@ ifeq ($(PORTNAME), linux)
endif
ifeq ($(PORTNAME), solaris)
- LINK.shared = $(COMPILER) -shared -Wl,-Bsymbolic
+ LINK.shared = $(COMPILER) -shared
ifdef soname
LINK.shared += -Wl,-soname,$(soname)
endif
+ BUILD.exports = ( echo '{ global:'; $(AWK) '/^[^\#]/ {printf "%s;\n",$$1}' $<; echo ' local: *; };' ) >$@
+ exports_file = $(SHLIB_EXPORTS:%.txt=%.list)
+ ifneq (,$(exports_file))
+ LINK.shared += -Wl,-M$(exports_file)
+ endif
endif
ifeq ($(PORTNAME), cygwin)
--
2.37.0.3.g30cc8d0f14
--aym2ahtpoerge4dw--
^ permalink raw reply [nested|flat] 29+ messages in thread
end of thread, other threads:[~2022-08-23 08:50 UTC | newest]
Thread overview: 29+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-04-12 18:03 Re: make MaxBackends available in _PG_init Nathan Bossart <[email protected]>
2022-04-12 19:12 ` Robert Haas <[email protected]>
2022-04-12 19:21 ` Tom Lane <[email protected]>
2022-04-12 19:30 ` Robert Haas <[email protected]>
2022-04-12 19:59 ` Nathan Bossart <[email protected]>
2022-04-12 20:58 ` Tom Lane <[email protected]>
2022-04-12 21:07 ` Nathan Bossart <[email protected]>
2022-04-12 21:46 ` Tom Lane <[email protected]>
2022-04-12 22:26 ` Nathan Bossart <[email protected]>
2022-04-13 13:16 ` Robert Haas <[email protected]>
2022-04-13 13:27 ` Tom Lane <[email protected]>
2022-04-13 14:22 ` Robert Haas <[email protected]>
2022-04-13 14:43 ` Tom Lane <[email protected]>
2022-04-13 15:25 ` Robert Haas <[email protected]>
2022-04-13 15:39 ` Tom Lane <[email protected]>
2022-04-13 15:50 ` Robert Haas <[email protected]>
2022-04-13 16:05 ` Tom Lane <[email protected]>
2022-04-13 18:30 ` Nathan Bossart <[email protected]>
2022-04-14 05:50 ` Julien Rouhaud <[email protected]>
2022-04-14 16:22 ` Nathan Bossart <[email protected]>
2022-04-14 16:32 ` Robert Haas <[email protected]>
2022-04-14 16:39 ` Tom Lane <[email protected]>
2022-04-18 22:52 ` Nathan Bossart <[email protected]>
2022-04-18 23:33 ` Tom Lane <[email protected]>
2022-04-12 21:14 ` Robert Haas <[email protected]>
2022-04-12 21:43 ` Tom Lane <[email protected]>
2022-04-13 07:54 ` Michael Paquier <[email protected]>
2022-04-12 19:49 ` Nathan Bossart <[email protected]>
2022-08-23 08:50 [PATCH v1] solaris: Use versioning scripts instead of -Bsymbolic Andres Freund <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox