Re: fix bgworkers in EXEC_BACKEND - Mailing list pgsql-hackers

On Thu, Dec 27, 2012 at 6:15 PM, Alvaro Herrera
<alvherre@2ndquadrant.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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/