Re: fix bgworkers in EXEC_BACKEND

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