Subject: make MaxBackends available in _PG_init

From: "Wang, Shenhao" <[email-masked]>
Date: Mon, 21 Sep 2020 03:43:41 +0000

Hi Hackers:

I find the value in function _PG_init, the value of MaxBackends is 0.
In source, I find that the postmaster will first load library, and then calculate the value of MaxBackends.

In the old version, the MaxBackends was calculated by:
	 MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
		GetNumShmemAttachedBgworkers();
Because any extension can register workers which will affect the return value of GetNumShmemAttachedBgworkers.
InitializeMaxBackends must be called after shared_preload_libraries. This is also mentioned in comments.

Now, function GetNumShmemAttachedBgworkers was deleted and replaced by guc max_worker_processes, 
so if we changed the calling order like:
	Step1: calling InitializeMaxBackends.
	Step2: calling process_shared_preload_libraries

In this order extension can get the correct value of MaxBackends in _PG_init.

Any thoughts?

Regards

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 21 Sep 2020 17:22:25 +0530

>         Step2: calling process_shared_preload_libraries

Yes, the GetNumShmemAttachedBgworkers() was removed by commit #
dfbba2c86cc8f09cf3ffca3d305b4ce54a7fb49a. ASAICS, changing the order
of InitializeMaxBackends() and process_shared_preload_libraries() has
no problem, as InitializeMaxBackends() doesn't calculate the
MaxBackends based on bgworker infra code, it does calculate based on
GUCs.

Having said that, I'm not quite sure whether any of the bgworker
registration code, for that matter process_shared_preload_libraries()
code path will somewhere use MaxBackends?

> In this order extension can get the correct value of MaxBackends in _PG_init.

Is there any specific use case that any of the _PG_init will use MaxBackends?

I think the InitializeMaxBackends() function comments still say as shown below.

 * 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.

What happens with your patch in EXEC_BACKEND cases? (On linux
EXEC_BACKEND (include/pg_config_manual.h) can be enabled for testing
purposes).

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

---

From: "Bossart, Nathan" <[email-masked]>
Date: Mon, 2 Aug 2021 18:18:25 +0000

> Is there any specific use case that any of the _PG_init will use MaxBackends?

I just encountered the same thing, so I am bumping this thread.  I was
trying to use MaxBackends in a call to RequestAddinShmemSpace() in a
_PG_init() function for a module, but since MaxBackends is not yet
initialized, you essentially need to open-code InitializeMaxBackends()
instead.

I think the comments about needing to register background workers
before initializing MaxBackends have been incorrect since the addition
of max_worker_processes in v9.4 (6bc8ef0b).  Furthermore, I think the
suggested reordering is a good idea because it is not obvious that
MaxBackends will be uninitialized in _PG_init(), and use-cases like
the RequestAddinShmemSpace() one are not guaranteed to fail when
MaxBackends is used incorrectly (presumably due to the 100 KB buffer
added in CreateSharedMemoryAndSemaphores()).

I've attached a new version of the proposed patch with some slight
adjustments and an attempt at a commit message.

Nathan

---

From: Robert Haas <[email-masked]>
Date: Mon, 2 Aug 2021 16:06:15 -0400

> adjustments and an attempt at a commit message.

I think this is a good idea. Anyone object?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 2 Aug 2021 16:27:12 -0400

On 2021-Aug-02, Robert Haas wrote:

> > of max_worker_processes in v9.4 (6bc8ef0b).

> I think this is a good idea. Anyone object?

No objection here.  AFAICS Nathan is correct.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Los dioses no protegen a los insensatos.  Éstos reciben protección de
otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)

---

From: Andres Freund <[email-masked]>
Date: Mon, 2 Aug 2021 13:37:24 -0700

Hi,

> I think this is a good idea. Anyone object?

I'm not so sure it's a good idea. I've seen several shared_preload_library
using extensions that adjust some GUCs (e.g. max_prepared_transactions)
because they need some more resources internally - that's perhaps not a great
idea, but there's also not an obviously better way.

ISTM this would better be solved with making the hook or config logic for
libraries a bit more elaborate (e.g. having a hook you can register for that's
called once all libraries are loaded).

Greetings,

Andres Freund

---

From: "Bossart, Nathan" <[email-masked]>
Date: Mon, 2 Aug 2021 21:57:18 +0000

> idea, but there's also not an obviously better way.

Interesting.  I hadn't heard of extensions adjusting GUCs in
_PG_init() before.  I think the other way to handle that scenario is
to check the GUCs in _PG_init() and fail startup if necessary.
However, while it's probably good to avoid changing GUCs from what
users specified, failing startup isn't exactly user-friendly, either.
In any case, changing a GUC in _PG_init() seems quite risky.  You're
effectively bypassing all of the usual checks.

> called once all libraries are loaded).

My worry is that this requires even more specialized knowledge to get
things right.  If I need to do anything based on the value of a GUC,
I have to register another hook.  In this new hook, we'd likely want
to somehow prevent modules from further adjusting GUCs.  We'd also
need modules to check the GUCs they care about again in case another
module's _PG_init() adjusted it in an incompatible way.  If you detect
a problem in this new hook, you probably need to fail startup.

Perhaps I am making a mountain out of a molehill and the modules that
are adjusting GUCs in _PG_init() are doing so in a generally safe way,
but IMO it ought to ordinarily be discouraged.

Nathan

---

From: Andres Freund <[email-masked]>
Date: Mon, 2 Aug 2021 15:11:39 -0700

Hi,

> to check the GUCs in _PG_init() and fail startup if necessary.

The problem is that that makes it hard to configure things for the users own
needs. If e.g. the user's workload needs a certain number of prepared
transactions and the extension internally as well (as e.g. citus does), the
extension can't know if the configured number is "aimed" to be for the
extension, or for the user's own needs. If you instead just increase
max_prepared_transactions in _PG_init(), the story is different.

> effectively bypassing all of the usual checks.

It doesn't need to bypass all - you can override them using GUC mechanisms at
that point.

> module's _PG_init() adjusted it in an incompatible way.

I think this is overblown. We already size resources *after*
shared_preload_libraries' _PG_init() runs, because that's the whole point of
shared_preload_libraries. What's proposed in this thread is to *disallow*
increasing resource usage in s_p_l _PG_init(), to make one specific case
simpler - but it'll actually also make things more complicated, because other
resources will still only be sized after all of s_p_l has been processed.

> If you detect a problem in this new hook, you probably need to fail startup.

Same is true for _PG_init().

> but IMO it ought to ordinarily be discouraged.

It's not something I would recommend doing blithely - but I also don't think
we have a better answer for some cases.

Greetings,

Andres Freund

---

From: "Bossart, Nathan" <[email-masked]>
Date: Mon, 2 Aug 2021 22:35:13 +0000

> resources will still only be sized after all of s_p_l has been processed.

True.  Perhaps the comments should reference the possibility that a
library will adjust resource usage to explain why
InitializeMaxBackends() is where it is.

Nathan

---

From: Andres Freund <[email-masked]>
Date: Mon, 2 Aug 2021 15:42:04 -0700

Hi,

> InitializeMaxBackends() is where it is.

I've wondered, independent of this thread, about not making MaxBackends
externally visible, and requiring a function call to access it. It should be
easier to find cases of misuse if we errored out when accessed at the wrong
time. And we could use that opportunity to add flags that determine which
types of backends are included (e.g. not including autovac, or additionally
including aux workers or prepared xacts).

Greetings,

Andres Freund

---

From: "Bossart, Nathan" <[email-masked]>
Date: Mon, 2 Aug 2021 22:58:49 +0000

> including aux workers or prepared xacts).

I'm not opposed to this.  I can work on putting a patch together if no
opposition materializes.

Nathan

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 3 Aug 2021 00:19:42 +0000

Hi,

Bossart, Nathan <[email-masked]> wrote:

> I just encountered the same thing, so I am bumping this thread.
Thank you for bumping this thread.

> > time. 

> I'm not opposed to this.  I can work on putting a patch together if no

I think this is a good idea.

Shenhao Wang
Best regards

---

From: Robert Haas <[email-masked]>
Date: Tue, 3 Aug 2021 11:25:29 -0400

> idea, but there's also not an obviously better way.

Blech.

I'm fine with solving the problem some other way, but I say again, blech.

---

From: "Bossart, Nathan" <[email-masked]>
Date: Tue, 3 Aug 2021 23:13:26 +0000

> I'm not opposed to this.  I can work on putting a patch together if no

Here is a first attempt.

Nathan

---

From: "Bossart, Nathan" <[email-masked]>
Date: Sat, 7 Aug 2021 18:01:41 +0000

>> I'm not opposed to this.  I can work on putting a patch together if no

Here is a rebased version of the patch.

Nathan

---

From: Greg Sabino Mullane <[email-masked]>
Date: Mon, 9 Aug 2021 16:13:32 -0400

> Here is a rebased version of the patch.

Giving this a review.

Patch applies cleanly and `make check` works as of
e12694523e7e4482a052236f12d3d8b58be9a22c

Overall looks very nice and tucks MaxBackends safely away.
I have a few suggestions:

> + size = add_size(size, mul_size(GetMaxBackends(0),
sizeof(BTOneVacInfo)));

The use of GetMaxBackends(0) looks weird - can we add another constant in
there
for the "default" case? Or just have GetMaxBackends() work?

> + GMB_MAX_PREPARED_XACTS = 1 << 0, /* include max_prepared_xacts */
s/include/add in/

>  +typedef enum GMBOption
> +{
> + GMB_MAX_PREPARED_XACTS = 1 << 0, /* include max_prepared_xacts */
> + GMB_NUM_AUXILIARY_PROCS = 1 << 1 /* include NUM_AUXILIARY_PROCS */
> +} GMBOption;

Is a typedef enum really needed? As opposed to something like this style:

#define WL_LATCH_SET         (1 << 0)
#define WL_SOCKET_READABLE   (1 << 1)
#define WL_SOCKET_WRITEABLE  (1 << 2)

> -  (MaxBackends + max_prepared_xacts + 1));
> +  (GetMaxBackends(GMB_MAX_PREPARED_XACTS) + 1));

This is a little confusing - there is no indication to the reader that this
is an additive function. Perhaps a little more intuitive name:

> +  (GetMaxBackends(GMB_PLUS_MAX_PREPARED_XACTS) + 1));

However, the more I went through this patch, the more the GetMaxBackends(0)
nagged at me. The vast majority of the calls are with "0". I'd argue for
just having no arguments at all, which removes a bit of code and actually
makes things like this easier to read:

Original change:
> - uint32  TotalProcs = MaxBackends + NUM_AUXILIARY_PROCS +
max_prepared_xacts;
> + uint32  TotalProcs = GetMaxBackends(GMB_NUM_AUXILIARY_PROCS |
GMB_MAX_PREPARED_XACTS);

Versus:
> - uint32  TotalProcs = MaxBackends + NUM_AUXILIARY_PROCS +
max_prepared_xacts;
> + uint32  TotalProcs = GetMaxBackends() + NUM_AUXILIARY_PROCS +
max_prepared_xacts;

> + * This must be called after modules have had the change to alter GUCs in
> + * shared_preload_libraries, and before shared memory size is determined.
s/change/chance/;

> +void
> +SetMaxBackends(int max_backends)
> +{
> + if (MaxBackendsInitialized)
> +  elog(ERROR, "MaxBackends already initialized");

Is this going to get tripped by a call from restore_backend_variables?

Cheers,
Greg

---

From: "Bossart, Nathan" <[email-masked]>
Date: Tue, 10 Aug 2021 00:22:50 +0000

> Giving this a review.

Thanks for your review.

> However, the more I went through this patch, the more the GetMaxBackends(0) nagged at me. The vast majority of the calls are with "0". I'd argue for just having no arguments at all, which removes a bit of code and actually makes things like this easier to read:

I agree.  The argument is nonzero in less than half of the calls to
GetMaxBackends(), and I'm not sure it adds a whole lot of value in the
first place.  I removed the argument in v3.

>> + * shared_preload_libraries, and before shared memory size is determined.

I fixed this in v3.

> Is this going to get tripped by a call from restore_backend_variables?

I ran 'make check-world' with EXEC_BACKEND with no problems, so I
don't think so.

Nathan

---

From: Greg Sabino Mullane <[email-masked]>
Date: Wed, 11 Aug 2021 09:41:43 -0400

> I ran 'make check-world' with EXEC_BACKEND with no problems, so I

v3 looks good, but I'm still not sure how to test the bit mentioned above.
I'm not familiar with this part of the code (SubPostmasterMain etc.), but
running make check-world with EXEC_BACKEND does not seem to execute that
code, as I added exit(1) to restore_backend_variables() and the tests still
ran fine. Further digging shows that even though the #ifdef EXEC_BACKEND
path is triggered, no --fork argument was being passed. Is there something
else one needs to provide to force that --fork (see line 189 of
src/backend/main/main.c) when testing?

Cheers,
Greg

---

From: Tom Lane <[email-masked]>
Date: Wed, 11 Aug 2021 10:08:01 -0400

Greg Sabino Mullane <[email-masked]> writes:
> ran fine.

You must not have enabled EXEC_BACKEND properly.  It's a compile-time
#define that affects multiple modules, so it's easy to get wrong.
The way I usually turn it on is

	make distclean
	./configure ... options of choice ...
	edit src/include/pg_config.h, add "#define EXEC_BACKEND" line
	make, install, test

In this way the setting is persistent till the next distclean/configure
cycle.

			regards, tom lane

---

From: Greg Sabino Mullane <[email-masked]>
Date: Thu, 12 Aug 2021 11:41:15 -0400

> The way I usually turn it on is

Thank you. I was able to get it all working, and withdraw any objections to
that bit of the patch. :)

Cheers,
Greg

---

From: "[email-masked]" <[email-masked]>
Date: Sun, 15 Aug 2021 08:04:38 +0000

Hi,

I noticed that in v3-0001-Disallow-external-access-to-MaxBackends.patch, there are some modifications like:

> -		for (int i = 0; i <= MaxBackends; i++)
> +		for (int i = 0; i <= GetMaxBackends(); i++)

I don't think calling function GetMaxBackends() in the for loop is a good idea.  
How about use a temp variable to save the return value of function GetMaxBackends() ?

Regards,
Shenhao Wang

---

From: "Bossart, Nathan" <[email-masked]>
Date: Mon, 16 Aug 2021 04:02:16 +0000

> How about use a temp variable to save the return value of function GetMaxBackends() ?

I did this in v4.  There may be a couple of remaining places that call
GetMaxBackends() several times, but the function should be relatively
inexpensive.

Nathan

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 28 Jan 2022 20:33:42 -0800

On Sat, Jan 29, 2022 at 11:19:12AM +0900, Michael Paquier wrote:
> On Thu, Jan 27, 2022 at 10:18:15AM -0800, Nathan Bossart wrote:
>> Alright.  I think the comment adjustments still apply, so I split those out
>> to a new patch.
> 
> Looks fine after a second look, so applied.

Thanks!

> As of the issues of this thread, we really have two things to think
> about:
> 1) How do we want to control the access of some parameters in a
> context or another?  One idea would be more control through GUCs, say
> with a set of context-related flags that prevent the read of some
> variables until they are set.  We could encourage the use of
> GetConfigOption() for that.  For MaxBackends, we could add a read-only
> GUC for this purpose.  That's what Andres hinted at upthread, I
> guess.
> 2) How do we deal with unwanted access of shared parameters?  This one
> is not really controllable, is it?  And we are talking about much more
> than MaxBackends.  This could perhaps be addressed with more
> documentation in the headers for the concerned variables, as a first
> step.

Hm.  Perhaps we should understand the full scope of the problem first.
What else besides MaxBackends and the shared memory GUCs won't be properly
initialized when the shared_preload_libraries' _PG_init() functions are
called?  MaxBackends seems to be the only one that folks have experienced
problems with, which is why I initially zeroed in on it.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

---

From: Michael Paquier <[email-masked]>
Date: Sat, 29 Jan 2022 14:24:24 +0900

> problems with, which is why I initially zeroed in on it.

Yeah, it would be good to know the scope before defining the limits
of what could be done.  Another thing may be the interactions with
session_preload_libraries and local_preload_libraries.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Mon, 31 Jan 2022 16:11:48 +0900

> session_preload_libraries and local_preload_libraries.

Worth noting that I have marked marked this patch as committed in the
CF app, as there is nothing else to do for now.
--
Michael