Subject: Re: make MaxBackends available in _PG_init

From: Michael Paquier <[email-masked]>
Date: Wed, 9 Feb 2022 14:17:59 +0900

On Tue, Feb 08, 2022 at 10:57:37PM -0500, Robert Haas wrote:
> Well I didn't do anything myself except review and commit Nathan's
> patch, so I suppose you mean he could have done that, but fair enough.
> I don't mind if you want to change it around.

Okay, I'd rather apply the same rule everywhere for consistency, then,
like in the attached.  That's minimal, still.
--
Michael
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index d44001a49f..455d895a44 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -1633,6 +1633,7 @@ SignalBackends(void)
 	int32	   *pids;
 	BackendId  *ids;
 	int			count;
+	int			max_backends = GetMaxBackends();
 
 	/*
 	 * Identify backends that we need to signal.  We don't want to send
@@ -1642,8 +1643,8 @@ SignalBackends(void)
 	 * XXX in principle these pallocs could fail, which would be bad. Maybe
 	 * preallocate the arrays?  They're not that large, though.
 	 */
-	pids = (int32 *) palloc(GetMaxBackends() * sizeof(int32));
-	ids = (BackendId *) palloc(GetMaxBackends() * sizeof(BackendId));
+	pids = (int32 *) palloc(max_backends * sizeof(int32));
+	ids = (BackendId *) palloc(max_backends * sizeof(BackendId));
 	count = 0;
 
 	LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE);
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 1528d788d0..ee2e15c17e 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -2924,6 +2924,7 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
 	LWLock	   *partitionLock;
 	int			count = 0;
 	int			fast_count = 0;
+	int			max_backends = GetMaxBackends();
 
 	if (lockmethodid <= 0 || lockmethodid >= lengthof(LockMethods))
 		elog(ERROR, "unrecognized lock method: %d", lockmethodid);
@@ -2942,12 +2943,12 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
 			vxids = (VirtualTransactionId *)
 				MemoryContextAlloc(TopMemoryContext,
 								   sizeof(VirtualTransactionId) *
-								   (GetMaxBackends() + max_prepared_xacts + 1));
+								   (max_backends + max_prepared_xacts + 1));
 	}
 	else
 		vxids = (VirtualTransactionId *)
 			palloc0(sizeof(VirtualTransactionId) *
-					(GetMaxBackends() + max_prepared_xacts + 1));
+					(max_backends + max_prepared_xacts + 1));
 
 	/* Compute hash code and partition lock, and look up conflicting modes. */
 	hashcode = LockTagHashCode(locktag);
@@ -3104,7 +3105,7 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
 
 	LWLockRelease(partitionLock);
 
-	if (count > GetMaxBackends() + max_prepared_xacts)	/* should never happen */
+	if (count > max_backends + max_prepared_xacts)	/* should never happen */
 		elog(PANIC, "too many conflicting locks found");
 
 	vxids[count].backendId = InvalidBackendId;
diff --git a/src/backend/utils/adt/lockfuncs.c b/src/backend/utils/adt/lockfuncs.c
index 4e517b28e1..944cd6df03 100644
--- a/src/backend/utils/adt/lockfuncs.c
+++ b/src/backend/utils/adt/lockfuncs.c
@@ -559,13 +559,14 @@ pg_safe_snapshot_blocking_pids(PG_FUNCTION_ARGS)
 	int		   *blockers;
 	int			num_blockers;
 	Datum	   *blocker_datums;
+	int			max_backends = GetMaxBackends();
 
 	/* A buffer big enough for any possible blocker list without truncation */
-	blockers = (int *) palloc(GetMaxBackends() * sizeof(int));
+	blockers = (int *) palloc(max_backends * sizeof(int));
 
 	/* Collect a snapshot of processes waited for by GetSafeSnapshot */
 	num_blockers =
-		GetSafeSnapshotBlockingPids(blocked_pid, blockers, GetMaxBackends());
+		GetSafeSnapshotBlockingPids(blocked_pid, blockers, max_backends);
 
 	/* Convert int array to Datum array */
 	if (num_blockers > 0)

---

From: Robert Haas <[email-masked]>
Date: Wed, 9 Feb 2022 12:31:16 -0500

> like in the attached.  That's minimal, still.

That's fine with me. In the interest of full disclosure, I did kind of
notice this when reviewing the patch, though perhaps not every
instance, and just decided that it didn't seem important enough to
worry about. I'm totally OK with you thinking otherwise, though,
especially since you also volunteered to do the work thus generated.
:-)

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

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 9 Feb 2022 09:53:38 -0800

> especially since you also volunteered to do the work thus generated.

This is fine with me as well.  I only left these out because the extra
variable felt unnecessary to me for these functions.

While you are at it, would you mind fixing the misspelling of
OldestVisibleMXactId in multixact.c (as per the attached)?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index f00c272521..6a70d49738 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -285,7 +285,7 @@ typedef struct MultiXactStateData
  * Pointers to the state data in shared memory
  *
  * The index of the last element of the OldestMemberMXactId and
- * OldestVisibleMXacId arrays can be obtained with GetMaxOldestSlot().
+ * OldestVisibleMXactId arrays can be obtained with GetMaxOldestSlot().
  */
 static MultiXactStateData *MultiXactState;
 static MultiXactId *OldestMemberMXactId;

---

From: Michael Paquier <[email-masked]>
Date: Thu, 10 Feb 2022 10:47:39 +0900

>> especially since you also volunteered to do the work thus generated.

I guessed so :p

> variable felt unnecessary to me for these functions.

Okay, done, then.

> OldestVisibleMXactId in multixact.c (as per the attached)?

Indeed.  Fixed as well.
--
Michael

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 10 Feb 2022 11:26:12 -0800

>> OldestVisibleMXactId in multixact.c (as per the attached)?

Thanks!

---

From: Julien Rouhaud <[email-masked]>
Date: Wed, 23 Mar 2022 12:52:29 +0800

Hi,

Sorry for showing up this late, but I'm a bit confused with the new situation.

Unless I'm missing something, the new situation is that the system is supposed
to prevent access to MaxBackends during s_p_l_pg_init, for reasons I totally
agree with, but without doing anything for extensions that actually need to
access it at that time.  So what are extensions supposed to do now if they do
need the information during their _PG_init() / RequestAddinShmemSpace()?

Note that I have two of such extensions.  They actually only need it to give
access to the queryid in the background workers since it's otherwise not
available, but there's at least pg_wait_sampling extension that needs the value
to request shmem for other needs.

One funny note is that my extensions aren't using MaxBackends but instead
compute it based on MaxConnections, autovacuum_max_workers and so on.  So those
two extensions aren't currently broken, or more accurately not more than they
previously were.  Is there any reasoning to not protect all the variables that
contribute to MaxBackends?

---

From: Robert Haas <[email-masked]>
Date: Wed, 23 Mar 2022 08:32:39 -0400

> need the information during their _PG_init() / RequestAddinShmemSpace()?

Well, the conclusion upthread was that extensions might change the
values of those GUCs from _PG_init(). If that's a real thing, then
what you're asking for here is impossible, because the final value is
indeterminate until all such extensions have finished twiddling those
the GUCs. On the other hand, it's definitely intended that extensions
should RequestAddinShmemSpace() from _PG_init(), and wanting to size
that memory based on MaxBackends is totally reasonable. Do we need to
add another function, alongside _PG_init(), that gets called after
MaxBackends is determined and before it's too late to
RequestAddinShmemSpace()?

---

From: Julien Rouhaud <[email-masked]>
Date: Wed, 23 Mar 2022 21:03:18 +0800

> RequestAddinShmemSpace()?

Yes, I don't see how we can support such extensions without an additional hook,
probably called right after InitializeMaxBackends() since at least
InitializeShmemGUCs() will need to know about those extra memory needs.

I'm not sure how to prevent third party code from messing with the gucs in it,
but a clear indication in the hook comment should probably be enough.  It's not
like it's hard for third-party code author to break something anyway.

And should we do something about MaxConnections, autovacuum_max_workers,
max_worker_processes and max_wal_senders too?  It's unlikely that I'm the only
one who's computing my own MaxBackends to workaround the previous 0 value that
was available during _PG_init, and I'm not sure that everyone will notice such
a new hook.

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 24 Mar 2022 13:20:50 -0700

> InitializeShmemGUCs() will need to know about those extra memory needs.

Another possibility could be to add a hook that is called _before_
_PG_init() where libraries are permitted to adjust GUCs.  After the library
is loaded, we first call this _PG_change_GUCs() function, then we
initialize MaxBackends, and then we finally call _PG_init().  This way,
extensions would have access to MaxBackends within _PG_init(), and if an
extension really needed to alter GUCs, іt could define this new function.

> like it's hard for third-party code author to break something anyway.

ERROR-ing in SetConfigOption() might be another way to dissuade folks from
messing with GUCs.  This is obviously not a perfect solution.

---

From: Robert Haas <[email-masked]>
Date: Thu, 24 Mar 2022 16:27:36 -0400

> extension really needed to alter GUCs, іt could define this new function.

Yeah, I think this might be better.

---

From: Julien Rouhaud <[email-masked]>
Date: Fri, 25 Mar 2022 10:39:51 +0800

> Yeah, I think this might be better.

Well, if it's before _PG_init() then it won't be a new hook but a new symbol
that has to be handled with dlsym.

But it seems to be that the bigger problem is that this approach won't fix
anything unless we can prevent third-party code from messing with GUCs after
that point as we could otherwise have discrepancies between GetMaxBackends()
and the underlying GUCs until every single extension that wants to change GUC
is modified uses this new symbol.  And I also don't see how we could force that
unless we have a better GUC API that doesn't entirely relies on strings,
otherwise a simple thing like "allow 5 extra bgworkers" is going to be really
painful.

A new hook after _PG_init() sure leaves the burden to the few extension that
needs to allocate shmem based on MaxBackends, but there probably isn't that
much (I have a few dozens locally cloned and found only 1 apart from mine, and
I would be happy to take care of those), and it also means that they have a
chance to do something correct even if other extensions messing with GUCs
aren't fixed.

Arguably, you could certainly try to change GUCs in that new hook, but it
wouldn't be any different from doing so in shmem_startup_hook so I don't think
it's really a problem.

---

From: Julien Rouhaud <[email-masked]>
Date: Fri, 25 Mar 2022 11:11:46 +0800

> wouldn't be any different from doing so in shmem_startup_hook so I don't think

As an example, here's a POC for a new shmem_request_hook hook after _PG_init().
With it I could easily fix pg_wait_sampling shmem allocation (and checked that
it's indeed requesting the correct size).

---

From: Michael Paquier <[email-masked]>
Date: Fri, 25 Mar 2022 14:08:09 +0900

> it's indeed requesting the correct size).

Are you sure that the end of a release cycle is the good moment to
begin designing new hooks?  Anything added is something we are going
to need supporting moving forward.  My brain is telling me that we
ought to revisit the business with GetMaxBackends() properly instead,
and perhaps revert that.

This solution makes me uneasy from the start (already stated
upthread), because it is not a solution to the original problem, just
a safeguard that handles one small-ish portion of the whole parameter
calculation cycle.
--
Michael

---

From: Julien Rouhaud <[email-masked]>
Date: Fri, 25 Mar 2022 14:35:42 +0800

> ought to revisit the business with GetMaxBackends() properly instead,

I agree, and as I mentioned in my original email I don't think that the
committed patch is actually adding something on which we can really build on.
So I'm also in favor of reverting, as it seems like be a better option in the
long run to have a clean and broader solution.

---

From: Andres Freund <[email-masked]>
Date: Fri, 25 Mar 2022 15:23:17 -0700

Hi,

> long run to have a clean and broader solution.

I don't really understand. The issue that started this thread was bugs in
extensions due to accessing MaxBackends before it is initialized - which the
patch prevents. The stuff that you're complaining about / designing here
doesn't seem related to that. I like the idea of the hooks etc, but I fail to
see why we "ought to revisit the business with GetMaxBackends()"?

Greetings,

Andres Freund

---

From: Julien Rouhaud <[email-masked]>
Date: Sat, 26 Mar 2022 15:22:03 +0800

Hi,

On Fri, Mar 25, 2022 at 03:23:17PM -0700, Andres Freund wrote:
> patch prevents.

Well, the patch prevents accessing a 0-valued MaxBackends but doesn't do
anything to solve the original complaint.  It's not like extensions won't need
to access that information during _PG_init anymore.

> The stuff that you're complaining about / designing here

Because this GetMaxBackends() doesn't solve the problem nor brings any
infrastructure that could be reused to solve it.

I think that the root issue could be rephrased with:

Can we initialize MaxBackends earlier so that _PG_init() can see it because
maintaining MaxConnections + autovacuum_max_workers + 1 + max_worker_processes
+ max_wal_senders is troublesome.

And indeed, any third party code that previously needed to access what
MaxBackends is supposed to store should already be using that formula, and
the new GetMaxBackends() doesn't do anything about it.  So all extensions will
be as broken as before, except the few that were using MaxBackends without
realizing it's 0.  And if those exist (there's actually one) they're not that
broken, probably because MaxBackend is only used to request additional shmem,
with wanted value small enough so that it's compensated by the extra 100kB
shmem postgres allocates.

Since all those underlying GUCs shouldn't be accessible, we need some more
general infrastructure that would work for those too on top of a way to access
MaxBackends when extensions needs it.

Note that the only use case I'm aware of is for RequestAddinShmemSpace, so a
hook after _PG_init like the example shmem_request_hook would be enough for the
latter, but maybe there are more use cases for which it wouldn't.

---

From: Andres Freund <[email-masked]>
Date: Sat, 26 Mar 2022 10:23:16 -0700

Hi,

> to access that information during _PG_init anymore.

It resolves the pretty common bug that an extension breaks once it's used via
s_p_l instead of loaded on-demand because MaxBackends isn't initialized in the
s_p_l case.

> the new GetMaxBackends() doesn't do anything about it.

It couldn't rely on MaxBackends before. It can't rely on GetMaxBackends()
now. You can see why I think that what you want is unrelated to the
introduction of GetMaxBackends().

If we introduce a separate hook that allows to influence things like
max_connections or whatnot we'd *even more* need a way to verify whether it's
legal to access MaxBackends in that moment.

> So all extensions will be as broken as before, except the few that were
> using MaxBackends without realizing it's 0. And if those exist (there's
> actually one) they're not that broken, probably because MaxBackend is only
> used to request additional shmem, with wanted value small enough so that
> it's compensated by the extra 100kB shmem postgres allocates.

I don't think it's rare at all.

Greetings,

Andres Freund

---

From: Julien Rouhaud <[email-masked]>
Date: Sun, 27 Mar 2022 12:15:25 +0800

> s_p_l instead of loaded on-demand because MaxBackends isn't initialized in the

I can hear the argument.  However I don't know any extension that relies on
MaxBackends and doesn't need to be in s_p_l, and unless I'm missing something
no one provided such an example, only people needing the value for
RequestAddinShmemSpace().

> introduction of GetMaxBackends().

Sure, but code also couldn't really rely on MaxConnections or any other similar
GUCs and yet nothing is done for that, and the chosen approach doesn't help for
that.

The only difference is that those GUCs are less broken, as in the value is
likely to be valid more often, and closer to the final value when it's not.
But still broken.

I think GetMaxBackends() is more likely to force authors to rely on computing
the value using the underlying GUCs, since there's nothing else that can be
done.  And if that's an acceptable answer, why aren't we computing MaxBackends
before and after processing s_p_l?

> legal to access MaxBackends in that moment.

Again, I'm not opposed to validating that MaxBackends is valid when third-party
code is accessing it, quite the opposite.  I'm opposed to do so *only* for
MaxBackends, and without providing a way for third-party code to access that
value when they need it, which is for all the cases I know (after more digging
that's now 5 extensions) for RequestAddinShmemSpace().

---

From: Robert Haas <[email-masked]>
Date: Tue, 29 Mar 2022 12:22:21 -0400

> introduction of GetMaxBackends().

It's not, though, because the original proposal was to change things
around so that the value of MaxBackends would have been reliable in
_PG_init(). If we'd done that, then extensions that are using it in
_PG_init() would have gone from being buggy to being not-buggy. But
since you advocated against that change, we instead committed
something that caused them to go from being buggy to failing outright.
That's pretty painful for people with such extensions. And IMHO, it's
*much* more legitimate to want to size a data structure based on the
value of MaxBackends than it is for extensions to override GUC values.
If we can make the latter use case work in a sane way, OK, although I
have my doubts about how sane it really is, but it can't be at the
expense of telling extensions that have been (incorrectly) using
MaxBackends in _PG_init() that we're throwing them under the bus.

IMHO, the proper thing to do if certain GUC values are required for an
extension to work is to put that information in the documentation and
error out at an appropriate point if the user does not follow the
directions. Then this issue does not arise. But there's no reasonable
workaround for being unable to size data structures based on
MaxBackends.

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 30 Mar 2022 09:30:51 -0700

> workaround for being unable to size data structures based on

FWIW I would be on board with reverting all the GetMaxBackends() stuff if
we made the value available in _PG_init() and stopped supporting GUC
overrides by extensions (e.g., ERROR-ing in SetConfigOption() when
process_shared_preload_libraries_in_progress is true).

---

From: Julien Rouhaud <[email-masked]>
Date: Sat, 9 Apr 2022 21:24:42 +0800

> process_shared_preload_libraries_in_progress is true).

Yeah I would prefer this approach too, although it couldn't prevent extension
from directly modifying the underlying variables so I don't know how effective
it would be.

On the bright side, I see that citus is using SetConfigOption() to increase
max_prepared_transactions [1].  That's the only extension mentioned in that
thread that does modify some GUC, and this GUC was already marked as
PGDLLIMPORT since 2017 so it probably wasn't done that way for Windows
compatibility reason.

In the meantime, should we add an open item?

[1] https://github.com/citusdata/citus/blob/master/src/backend/distributed/transaction/transaction_management.c#L656-L657

---

From: Michael Paquier <[email-masked]>
Date: Mon, 11 Apr 2022 08:34:09 +0900

> In the meantime, should we add an open item?

Yes, I think that we need to discuss more the matter here, so added one.
--
Michael

---

From: Julien Rouhaud <[email-masked]>
Date: Mon, 11 Apr 2022 23:36:31 +0800

Hi,

On Mon, Apr 11, 2022 at 10:47:17AM -0400, Robert Haas wrote:
> On Sat, Apr 9, 2022 at 9:24 AM Julien Rouhaud <[email-masked]> wrote:
>
> I don't quite understand this, but I think if we want to support this
> kind of thing it needs a separate hook.

My point was that even if we say we don't support this, we have no way to
actually fully enforce it, as the variables are exported.  So it's good that
the only know case is using the GUC API, since we can enforce this one.

---

From: Julien Rouhaud <[email-masked]>
Date: Tue, 12 Apr 2022 13:08:35 +0800

Hi,

On Mon, Apr 11, 2022 at 02:14:35PM -0700, Nathan Bossart wrote:
> On Mon, Apr 11, 2022 at 01:44:42PM -0700, Nathan Bossart wrote:
> > On Mon, Apr 11, 2022 at 04:36:36PM -0400, Robert Haas wrote:
> >> If we throw an error while defining_custom_guc is true, how will it
> >> ever again become false?
> > 
> > Ah, I knew I was forgetting something this morning.
> > 
> > It won't, but the only place it is presently needed is when loading
> > shared_preload_libraries, so I believe startup will fail anyway.  However,
> > I can see defining_custom_guc being used elsewhere, so that is probably not
> > good enough.  Another approach could be to add a static
> > set_config_option_internal() function with a boolean argument to indicate
> > whether it is being used while defining a custom GUC.  I'll adjust 0003
> > with that approach unless a better idea prevails.
> 
> Here's a new patch set.  I've only changed 0003 as described above.  My
> apologies for the unnecessary round trip.

It looks sensible to me, although I think I would apply 0003 before 0002.

+   if (process_shared_preload_libraries_in_progress &&
+       !allow_when_loading_preload_libs)
+       ereport(ERROR,
+               (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
+                errmsg("cannot change parameters while loading "
+                       "\"shared_preload_libraries\"")));
+

I think the error message should mention at least which GUC is being modified.

---

From: Michael Paquier <[email-masked]>
Date: Wed, 13 Apr 2022 16:54:29 +0900

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