On 27.12.2012 19:15, Alvaro Herrera wrote:
I committed background workers three weeks ago, claiming it worked on
EXEC_BACKEND, and shortly thereafter I discovered that it didn't. I
noticed that the problem is the kludge to cause postmaster and children
to recompute MaxBackends after shared_preload_libraries is processed; so
the minimal fix is to duplicate this bit, from PostmasterMain() into
SubPostmasterMain():
@@ -4443,6 +4443,17 @@ SubPostmasterMain(int argc, char *argv[])
*/
process_shared_preload_libraries();
+ /*
+ * If loadable modules have added background workers, MaxBackends needs to
+ * be updated. Do so now by forcing a no-op update of max_connections.
+ * XXX This is a pretty ugly way to do it, but it doesn't seem worth
+ * introducing a new entry point in guc.c to do it in a cleaner fashion.
+ */
+ if (GetNumShmemAttachedBgworkers()> 0)
+ SetConfigOption("max_connections",
+ GetConfigOption("max_connections", false, false),
+ PGC_POSTMASTER, PGC_S_OVERRIDE);
I considered this pretty ugly when I first wrote it, and as the comment
says I tried to add something to guc.c to make it cleaner, but it was
even uglier.
Might be cleaner to directly assign the correct value to MaxBackends
above, ie. "MaxBackends = MaxConnections + newval + 1 +
GetNumShmemAttachedBgworkers()". With a comment to remind that it needs
to be kept in sync with the other places where that calculation is done,
in guc.c. Or put that calculation in a new function and call it above
and in guc.c.
Thinking about this some more, it might be cleaner to move the
responsibility of setting MaxBackends out of guc.c, into postmaster.c.
The guc machinery would set max_connections and autovacuum_max_workers
as usual, but not try to set MaxBackends. After reading the config file
in postmaster.c, calculate MaxBackends.
This would have the advantage that MaxBackends would be kept set at
zero, until we know the final value. That way it's obvious that you
cannot trust the value of MaxBackends in a contrib module
preload-function, for example, which would reduce the chance of
programmer mistakes.
So I now came up with a completely different idea: how about making
MaxBackends a macro, i.e.
+#define MaxBackends (MaxConnections + autovacuum_max_workers + 1 + \
+ GetNumShmemAttachedBgworkers())
so that instead of having guc.c recompute it, each caller that needs to
value obtains it up to date all the time? This additionally means that
assign_maxconnections and assign_autovacuum_max_workers go away (only
the check routines remain). Patch attached.
The one problem I see as serious with this approach is that it'd be
moderately expensive (i.e. not just fetch a value from memory) to
compute the value because it requires a walk of the registered workers
list. For most callers this wouldn't be a problem because it's just
during shmem sizing/creation; but there are places such as multixact.c
and async.c that use it routinely, so it's likely that we need to cache
the value somehow. It seems relatively straightforward though.
I don't like that. The result of GetNumShmemAttachedBgWorkers() doesn't
change after postmaster startup, so it seems silly to call it
repeatedly. And from a readability point of view, it makes you think
that it might change, because it's recalculated every time.
If I'm reading the code correctly, GetNumShmemAttachedBgWorkers() works
by walking through a backend-local list. What happens if a background
worker fails to register itself when preloaded in one backend? That
backend would calculate a different value of MaxBackends, with
interesting consequences. That would be a clear case of "don't do that",
but nevertheless, I think it would be better if we didn't rely on that.
I'd suggest adding MaxBackends to the list of variables passed from
postmaster to backends via BackendParameters.
All in all, I propose the attached. Not tested on Windows.
- Heikki
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 8f39aec..9dc8bd3 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -499,6 +499,7 @@ typedef struct
bool redirection_done;
bool IsBinaryUpgrade;
int max_safe_fds;
+ int MaxBackends;
#ifdef WIN32
HANDLE PostmasterHandle;
HANDLE initial_signal_pipe;
@@ -897,15 +898,11 @@ PostmasterMain(int argc, char *argv[])
process_shared_preload_libraries();
/*
- * If loadable modules have added background workers, MaxBackends needs to
- * be updated. Do so now by forcing a no-op update of max_connections.
- * XXX This is a pretty ugly way to do it, but it doesn't seem worth
- * introducing a new entry point in guc.c to do it in a cleaner fashion.
+ * Now that loadable modules have had their chance to register background
+ * workers, calculate MaxBackends.
*/
- if (GetNumShmemAttachedBgworkers() > 0)
- SetConfigOption("max_connections",
- GetConfigOption("max_connections", false, false),
- PGC_POSTMASTER, PGC_S_OVERRIDE);
+ MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
+ GetNumShmemAttachedBgworkers();
/*
* Establish input sockets.
@@ -5836,6 +5833,8 @@ save_backend_variables(BackendParameters *param, Port *port,
param->IsBinaryUpgrade = IsBinaryUpgrade;
param->max_safe_fds = max_safe_fds;
+ param->MaxBackends = MaxBackends;
+
#ifdef WIN32
param->PostmasterHandle = PostmasterHandle;
if (!write_duplicated_handle(¶m->initial_signal_pipe,
@@ -6061,6 +6060,8 @@ restore_backend_variables(BackendParameters *param, Port *port)
IsBinaryUpgrade = param->IsBinaryUpgrade;
max_safe_fds = param->max_safe_fds;
+ MaxBackends = param->MaxBackends;
+
#ifdef WIN32
PostmasterHandle = param->PostmasterHandle;
pgwin32_initial_signal_pipe = param->initial_signal_pipe;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 2cf34ce..eca6a60 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -199,9 +199,7 @@ static const char *show_tcp_keepalives_idle(void);
static const char *show_tcp_keepalives_interval(void);
static const char *show_tcp_keepalives_count(void);
static bool check_maxconnections(int *newval, void **extra, GucSource source);
-static void assign_maxconnections(int newval, void *extra);
static bool check_autovacuum_max_workers(int *newval, void **extra, GucSource source);
-static void assign_autovacuum_max_workers(int newval, void *extra);
static bool check_effective_io_concurrency(int *newval, void **extra, GucSource source);
static void assign_effective_io_concurrency(int newval, void *extra);
static void assign_pgstat_temp_directory(const char *newval, void *extra);
@@ -1615,7 +1613,7 @@ static struct config_int ConfigureNamesInt[] =
},
&MaxConnections,
100, 1, MAX_BACKENDS,
- check_maxconnections, assign_maxconnections, NULL
+ check_maxconnections, NULL, NULL
},
{
@@ -2290,7 +2288,7 @@ static struct config_int ConfigureNamesInt[] =
},
&autovacuum_max_workers,
3, 1, MAX_BACKENDS,
- check_autovacuum_max_workers, assign_autovacuum_max_workers, NULL
+ check_autovacuum_max_workers, NULL, NULL
},
{
@@ -8636,13 +8634,6 @@ check_maxconnections(int *newval, void **extra, GucSource source)
return true;
}
-static void
-assign_maxconnections(int newval, void *extra)
-{
- MaxBackends = newval + autovacuum_max_workers + 1 +
- GetNumShmemAttachedBgworkers();
-}
-
static bool
check_autovacuum_max_workers(int *newval, void **extra, GucSource source)
{
@@ -8652,12 +8643,6 @@ check_autovacuum_max_workers(int *newval, void **extra, GucSource source)
return true;
}
-static void
-assign_autovacuum_max_workers(int newval, void *extra)
-{
- MaxBackends = MaxConnections + newval + 1 + GetNumShmemAttachedBgworkers();
-}
-
static bool
check_effective_io_concurrency(int *newval, void **extra, GucSource source)
{
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
