public inbox for [email protected]
help / color / mirror / Atom feedFrom: Bossart, Nathan <[email protected]>
To: Fujii Masao <[email protected]>
To: [email protected] <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: Greg Sabino Mullane <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: make MaxBackends available in _PG_init
Date: Fri, 7 Jan 2022 18:09:23 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <4f20d57b2aeb447b8eb1495319940c5f@G08CNEXMBPEKD06.g08.fujitsu.local>
<CALj2ACUHruScDf2dvnVso03bhSdr7MVvLie5C8ND1dR+hJrFEg@mail.gmail.com>
<[email protected]>
<CA+TgmoYKBMdizs7-NBrhLHJXCv4dAAmBKHvER0PDmV+rBCy7KA@mail.gmail.com>
<[email protected]>
<CA+TgmoaaF46+A_Y-DTxDJpPGcqKxJU=ZPzFisqqouRL_WxYBSQ@mail.gmail.com>
<[email protected]>
<CAKAnmm+o5-ROvnghC-79=1Jr4bqJxmyu98Qax=KSukdL6JZm0g@mail.gmail.com>
<[email protected]>
<CAKAnmmJsHnNc65aLQErabTdcF=B+bMfSnLNUezC7nnsMb8bCDg@mail.gmail.com>
<[email protected]>
<CAKAnmmJ_3HQZ7PvpiJoJKYfffp8GOUXE3BVHsD9EOjDrcrU1FA@mail.gmail.com>
<OSBPR01MB42140A01E3441C576539AF38F2FC9@OSBPR01MB4214.jpnprd01.prod.outlook.com>
<[email protected]>
<[email protected]>
On 1/7/22, 8:54 AM, "Fujii Masao" <[email protected]> wrote:
> The patch handles only MaxBackends. But isn't there other variable having the same issue?
I wouldn't be surprised to learn of other cases, but I've only
encountered this specific issue with MaxBackends. I think MaxBackends
is notable because it is more likely to be used by preloaded libraries
but is intentionally initialized after loading them. As noted in
an earlier message on this thread [0], using MaxBackends in a call to
RequestAddinShmemSpace() in _PG_init() may not reliably cause
problems, too.
> It seems overkill to remove "extern" from MaxBackends and replace MaxBackends with GetMaxBackends() in the existing PostgreSQL codes. I'm not sure how much it's actually worth doing that. Instead, isn't it enough to just add the comment like "Use GetMaxBackends() if you want to treat the lookup for uninitialized MaxBackends as an error" in the definition of MaxBackends?
While that approach would provide a way to safely retrieve the value,
I think it would do little to prevent the issue in practice. If the
size of the patch is a concern, we could also convert MaxBackends into
a macro for calling GetMaxBackends(). This could also be a nice way
to avoid breaking extensions that are using it correctly while
triggering ERRORs for extensions that are using it incorrectly. I've
attached a new version of the patch that does it this way.
Nathan
[0] https://postgr.es/m/8499D41B-628A-4CE0-9139-00D718F9D06B%40amazon.com
Attachments:
[application/octet-stream] v5-0001-Disallow-external-access-to-MaxBackends.patch (6.7K, ../[email protected]/2-v5-0001-Disallow-external-access-to-MaxBackends.patch)
download | inline diff:
From f416355c8a73dd7f0879f4f78666499ceda47b72 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 7 Jan 2022 17:48:40 +0000
Subject: [PATCH v5 1/1] Disallow external access to MaxBackends.
Presently, MaxBackends is externally visible, but it may still be
uninitialized in places where it would be convenient to use (e.g.,
_PG_init()). This change renames MaxBackends to
MaxBackendsInternal, makes it static to postinit.c to disallow
direct access, and introduces a MaxBackends macro for calling
GetMaxBackends().
Separately, adjust the comments about needing to register
background workers before initializing MaxBackends. Since
6bc8ef0b, InitializeMaxBackends() has used max_worker_processes
instead of tallying up the number of registered background workers,
so background worker registration is no longer a prerequisite. The
ordering of this logic is still useful for allowing libraries to
adjust GUCs, so the comments have been updated to mention that use-
case.
---
src/backend/postmaster/postmaster.c | 12 ++++------
src/backend/utils/init/globals.c | 4 ----
src/backend/utils/init/postinit.c | 47 ++++++++++++++++++++++++++++++-------
src/include/miscadmin.h | 5 +++-
4 files changed, 47 insertions(+), 21 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 328ecafa8c..6f64a9b4cb 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1014,10 +1014,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 it before
+ * any modules had chance to take the background worker slots.
*/
ApplyLauncherRegister();
@@ -1038,8 +1036,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();
@@ -6496,7 +6494,7 @@ restore_backend_variables(BackendParameters *param, Port *port)
query_id_enabled = param->query_id_enabled;
max_safe_fds = param->max_safe_fds;
- MaxBackends = param->MaxBackends;
+ SetMaxBackends(param->MaxBackends);
#ifdef WIN32
PostmasterHandle = param->PostmasterHandle;
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 381d9e548d..47884bbe85 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -128,15 +128,11 @@ int max_parallel_maintenance_workers = 2;
/*
* Primary determinants of sizes of shared-memory structures.
- *
- * MaxBackends is computed by PostmasterMain after modules have had a chance to
- * register background workers.
*/
int NBuffers = 1000;
int MaxConnections = 90;
int max_worker_processes = 8;
int max_parallel_workers = 8;
-int MaxBackends = 0;
int VacuumCostPageHit = 1; /* GUC parameters for vacuum */
int VacuumCostPageMiss = 2;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 7292e51f7d..aeeb709a67 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -63,6 +63,9 @@
#include "utils/syscache.h"
#include "utils/timeout.h"
+static bool MaxBackendsInitialized = false;
+static int MaxBackendsInternal = 0;
+
static HeapTuple GetDatabaseTuple(const char *dbname);
static HeapTuple GetDatabaseTupleByOid(Oid dboid);
static void PerformAuthentication(Port *port);
@@ -476,9 +479,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
@@ -488,15 +490,42 @@ pg_split_opts(char **argv, int *argcp, const char *optstr)
void
InitializeMaxBackends(void)
{
- Assert(MaxBackends == 0);
-
/* the extra unit accounts for the autovacuum launcher */
- MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
- max_worker_processes + max_wal_senders;
+ SetMaxBackends(MaxConnections + autovacuum_max_workers + 1 +
+ max_worker_processes + max_wal_senders);
+}
- /* internal error because the values were all checked previously */
- if (MaxBackends > MAX_BACKENDS)
+/*
+ * Retrieve the value for MaxBackends.
+ *
+ * If the value has not yet been initialized, this function will ERROR.
+ */
+int
+GetMaxBackends(void)
+{
+ if (!MaxBackendsInitialized)
+ elog(ERROR, "MaxBackends not yet initialized");
+
+ return MaxBackendsInternal;
+}
+
+/*
+ * Set the value for MaxBackends.
+ *
+ * If the value is already initialized or the value provided is not valid, this
+ * function will ERROR.
+ */
+void
+SetMaxBackends(int max_backends)
+{
+ if (MaxBackendsInitialized)
+ elog(ERROR, "MaxBackends already initialized");
+
+ if (max_backends > MAX_BACKENDS)
elog(ERROR, "too many backends configured");
+
+ MaxBackendsInternal = max_backends;
+ MaxBackendsInitialized = true;
}
/*
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 90a3016065..4860a9540a 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -172,7 +172,6 @@ extern PGDLLIMPORT char *DataDir;
extern PGDLLIMPORT int data_directory_mode;
extern PGDLLIMPORT int NBuffers;
-extern PGDLLIMPORT int MaxBackends;
extern PGDLLIMPORT int MaxConnections;
extern PGDLLIMPORT int max_worker_processes;
extern PGDLLIMPORT int max_parallel_workers;
@@ -457,10 +456,14 @@ extern AuxProcType MyAuxProcType;
/* in utils/init/postinit.c */
extern void pg_split_opts(char **argv, int *argcp, const char *optstr);
extern void InitializeMaxBackends(void);
+extern int GetMaxBackends(void);
+extern void SetMaxBackends(int max_backends);
extern void InitPostgres(const char *in_dbname, Oid dboid, const char *username,
Oid useroid, char *out_dbname, bool override_allow_connections);
extern void BaseInit(void);
+#define MaxBackends (GetMaxBackends())
+
/* in utils/init/miscinit.c */
extern bool IgnoreSystemIndexes;
extern PGDLLIMPORT bool process_shared_preload_libraries_in_progress;
--
2.16.6
view thread (5+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: make MaxBackends available in _PG_init
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox