Re: fix bgworkers in EXEC_BACKEND

On Thu, Dec 27, 2012 at 6:15 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> 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.

Isn't that version going to make the source field in pg_settings for
max_connections show override whenever you are using background
workers? Thus breaking things like the fields to tell you which config
file and line it's on?

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

That seems neater.

> 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'd like to hear opinions on just staying with the IMO ugly minimal fix,
> or pursue instead making MaxBackends a macro plus some sort of cache to
> avoid repeated computation.

If my understanding per above is correct, then here's a +1 for the
non-ugly non-minimal fix.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/