fix bgworkers in EXEC_BACKEND

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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

* Heikki Linnakangas (hlinnakangas@vmware.com) wrote:

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.

I tend to prefer Heikki's solution wrt supporting what we do currently.
I do wonder if, perhaps, the reason the assign_XXX() functions were put
in place and used from GUC was a hope that some day we'd actually
support online changing of max_connections (and friends). I realize
that's pretty pie-in-the-sky, but it sure would be nice to reduce the
number of parameters that require a full restart.

All that said, putting those functions back and changing guc.c would
certainly be pretty trivially done, should some new patch come along
that would allow online changing of max_connections.

So, +1 on Heikki's approach.

Thanks,

Stephen

On 27 December 2012 18:06, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:

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.

I admire your forward thinking on that; yes, that could cause
problems. But even then, we would be admitting that nobody now gets a
valid value of MaxBackends, which sounds like it might be a problem in
itself.

Perhaps we should try to solve that a different way? Can we ask for
reservations of bgworkers ahead of running their _init functions,
using an additional API call?

That way we'd know the final value and everybody would have the
correct value at init time.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Simon,

* Simon Riggs (simon@2ndQuadrant.com) wrote:

I admire your forward thinking on that; yes, that could cause
problems. But even then, we would be admitting that nobody now gets a
valid value of MaxBackends, which sounds like it might be a problem in
itself.

I agree that the current implementation could lead to problems/confusion
for contrib module authors, if they're doing something with MaxBackends.

Perhaps we should try to solve that a different way? Can we ask for
reservations of bgworkers ahead of running their _init functions,
using an additional API call?

Before we go there, I'm honestly curious to hear what the use case is
for a contrib module to need that information, particularly at _init
time..? Also, would we need every contrib module to add in that call?
What if they don't create any additional bgworkers? What if they do but
don't provide the API call? I wonder if max_connections, which is well
defined and not subject to other modules or other things happening,
might be a better value to encourage authors to use, if they're looking
for some heuristic for how many possible backends there could be?

That way we'd know the final value and everybody would have the
correct value at init time.

This sounds like an opportunity for people to add another backend worker
and then forget to update what they tell the postmaster and everyone
else about how many backend workers they're going to create.. Or maybe
having a larger value be returned than they actually use.

Thanks,

Stephen

On 27 December 2012 18:36, Stephen Frost <sfrost@snowman.net> wrote:

Simon,

* Simon Riggs (simon@2ndQuadrant.com) wrote:

I admire your forward thinking on that; yes, that could cause
problems. But even then, we would be admitting that nobody now gets a
valid value of MaxBackends, which sounds like it might be a problem in
itself.

I agree that the current implementation could lead to problems/confusion
for contrib module authors, if they're doing something with MaxBackends.

I can't see any problems myself and am happy with Heikki's proposal to
accept that restriction, since other workarounds are possible.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Stephen Frost wrote:

* Heikki Linnakangas (hlinnakangas@vmware.com) wrote:

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.

I tend to prefer Heikki's solution wrt supporting what we do currently.
I do wonder if, perhaps, the reason the assign_XXX() functions were put
in place and used from GUC was a hope that some day we'd actually
support online changing of max_connections (and friends).

No, that's not the reason. The reason is that the "check" hooks did not
exist at all, so both the check and the assignment were done in the
assign hook. Now we're getting rid of the assignment hooks because
they're useless, but the check hooks must, of course, remain. We put
the hooks in because it was the simplest thing we could think of to set
MaxBackends when either max_connections or autovacuum_max_workers were
tweaked. My guess is if we had thought of propagating MaxBackends via
BackendParameters back then, we'd probably gone that route as well.
But certainly we had no intention to make max_connections online changeable.

I too like Heikki's proposed patch much better than mine. Thanks.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Stephen Frost <sfrost@snowman.net> writes:

Simon,
* Simon Riggs (simon@2ndQuadrant.com) wrote:

I admire your forward thinking on that; yes, that could cause
problems. But even then, we would be admitting that nobody now gets a
valid value of MaxBackends, which sounds like it might be a problem in
itself.

I agree that the current implementation could lead to problems/confusion
for contrib module authors, if they're doing something with MaxBackends.

This is more or less a necessary consequence of the fact that _init
functions are now allowed to add background workers. If there is any
code today that expects MaxBackends to be correct at
preload_shared_libraries time, it's already been broken irretrievably
by the bgworkers patch; and we'd be well advised to make that breakage
obvious not subtle.

So I'm +1 for Heikki's proposal as well.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Heikki Linnakangas wrote:

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.

Here's a small patch that applies on top of yours. Do you intend to
commit this? If not, let me know and I'll do it.

Thanks.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2012-12-27 20:06:07 +0200, Heikki Linnakangas wrote:

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():

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.

I am still worried about the following scenario in the EXEC_BACKEND case:

1) postmaster starts
2) reads config
3) executes _PG_init for shared_preload_libraries
4) library 'abc' gets config value 'abc.num_workers = 2' and registers as many workers
5) some time goes by, workers, backends start
6) abc.num_workers gets changed to 3, SIGHUP
7) some worker dies and gets restarted
8) _PG_init in the new worker does one more RegisterBackgroundWorker than before
9) ???

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund <andres@2ndquadrant.com> writes:

I am still worried about the following scenario in the EXEC_BACKEND case:

1) postmaster starts
2) reads config
3) executes _PG_init for shared_preload_libraries
4) library 'abc' gets config value 'abc.num_workers = 2' and registers as many workers
5) some time goes by, workers, backends start
6) abc.num_workers gets changed to 3, SIGHUP

This is broken whether it's EXEC_BACKEND or not: you don't get to change
anything that determines the number of workers post-startup.
num_workers should have been declared PGC_POSTMASTER.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2012-12-27 18:44:57 -0500, Tom Lane wrote:

Andres Freund <andres@2ndquadrant.com> writes:

I am still worried about the following scenario in the EXEC_BACKEND case:

1) postmaster starts
2) reads config
3) executes _PG_init for shared_preload_libraries
4) library 'abc' gets config value 'abc.num_workers = 2' and registers as many workers
5) some time goes by, workers, backends start
6) abc.num_workers gets changed to 3, SIGHUP

This is broken whether it's EXEC_BACKEND or not: you don't get to change
anything that determines the number of workers post-startup.
num_workers should have been declared PGC_POSTMASTER.

Well, the problem is, a shared library can't do that
afaics. abc.num_workers would be using custom_variable_classes (well,
whatever its called now, that it doesn't need to be configured).

There is no predefined 'num_workers' variable or anything like it in the
patch, but I guess some of the module authors will come of up with
configuration variables like that.

I personally want e.g. 'bdr.databases = 'a, b, c' which obviously has
the same problem...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane <tgl@sss.pgh.pa.us> schrieb:

Andres Freund <andres@2ndquadrant.com> writes:

I am still worried about the following scenario in the EXEC_BACKEND

case:

1) postmaster starts
2) reads config
3) executes _PG_init for shared_preload_libraries
4) library 'abc' gets config value 'abc.num_workers = 2' and

registers as many workers

5) some time goes by, workers, backends start
6) abc.num_workers gets changed to 3, SIGHUP

This is broken whether it's EXEC_BACKEND or not: you don't get to
change
anything that determines the number of workers post-startup.
num_workers should have been declared PGC_POSTMASTER.

BTW, I think it happens not to be broken in the non EXEC_BACKEND case because the registration point for bgworkers is _PG_init which will only be called once without EXEC_BACKEND, so config changes to SIGHUP'able variables won't change a thing dangerous as the bgworker stuff is all set up.

Andres

--- 
Please excuse the brevity and formatting - I am writing this on my mobile phone.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund <andres@2ndquadrant.com> writes:

On 2012-12-27 18:44:57 -0500, Tom Lane wrote:

This is broken whether it's EXEC_BACKEND or not: you don't get to change
anything that determines the number of workers post-startup.
num_workers should have been declared PGC_POSTMASTER.

Well, the problem is, a shared library can't do that
afaics. abc.num_workers would be using custom_variable_classes (well,
whatever its called now, that it doesn't need to be configured).

We fixed that a few years ago, no?

http://git.postgresql.org/gitweb/?p=postgresql.git&amp;a=commitdiff&amp;h=4605d1c98

If that's broken, we certainly need to fix it again. This issue exists
for any parameter that feeds into shared memory sizing, which is exactly
why we changed it back then.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 27.12.2012 22:46, Alvaro Herrera wrote:

Heikki Linnakangas wrote:

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.

Here's a small patch that applies on top of yours. Do you intend to
commit this? If not, let me know and I'll do it.

Wasn't planning to, feel free to commit.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Heikki Linnakangas wrote:

On 27.12.2012 22:46, Alvaro Herrera wrote:

Heikki Linnakangas wrote:

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.

Actually this patch still needed one more change, because we weren't
rechecking that we're not beyond the MAX_BACKENDS value after bgworker
registration.

This is hard to hit, because with the current compile constants you need
over eight million backends in total to hit that limit (and 360 GB of
shared memory), but it seems dangerous to leave that without any
protection.

I kinda hate this a bit because I had to move MAX_BACKENDS from a
private value in guc.c to postmaster.h. But the wording of the error
message is what took the most time:

+               ereport(LOG,                                                                                                                                            
+                               (errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED),                                                                                         
+                                errmsg("too many background workers"),                                                                                                 
+                                errdetail("Up to %d background workers can be registered with the current settings.",                                                  
+                                                  MAX_BACKENDS - (MaxConnections + autovacuum_max_workers + 1))));                                                     

Completely different ideas for this error message are welcome. My first
try enumerated all the variables involved.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

I committed this with minor tweaks to avoid having to scan the
registered workers list on each registration. Opinions on this error
report are still welcome:

+               ereport(LOG,
+                               (errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED),
+                                errmsg("too many background workers"),
+                                errdetail("Up to %d background workers can be registered with the current settings.",
+                                                  MAX_BACKENDS - (MaxConnections + autovacuum_max_workers + 1))));

Thanks to everyone for their input --- and happy 2013 hacking to all.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers