Re: [HACKERS] fix bgworkers in EXEC_BACKEND
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 Herrerahttp://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
Re: [HACKERS] fix bgworkers in EXEC_BACKEND
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services *** a/src/backend/postmaster/postmaster.c --- b/src/backend/postmaster/postmaster.c *** *** 499,504 typedef struct --- 499,505 bool redirection_done; bool IsBinaryUpgrade; int max_safe_fds; + int MaxBackends; #ifdef WIN32 HANDLE PostmasterHandle; HANDLE initial_signal_pipe; *** *** 897,911 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. */ ! if (GetNumShmemAttachedBgworkers() > 0) ! SetConfigOption("max_connections", ! GetConfigOption("max_connections", false, false), ! PGC_POSTMASTER, PGC_S_OVERRIDE); /* * Establish input sockets. --- 898,908 process_shared_preload_libraries(); /* ! * Now that loadable modules have had their chance to register background ! * workers, calculate MaxBackends. Add one for the autovacuum launcher. */ ! MaxBackends = MaxConnections + autovacuum_max_workers + 1 + ! GetNumShmemAttachedBgworkers(); /* * Establish input sockets. *** *** 5214,5219 RegisterBackgroundWorker(BackgroundWorker *worker) --- 5211,5227 return; } + if (MaxConnections + autovacuum_max_workers + 1 + GetNumShmemAttachedBgworkers() >= + MAX_BACKENDS) + { + 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; + return; + } + /* * Copy the registration data into the registered workers list. */ *** *** 5836,5841 save_backend_variables(BackendParameters *param, Port *port, --- 5844,5851 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,6066 restore_backend_variables(BackendParameters *param, Port *port) --- 6071,6078 IsBinaryUpgrade = param->IsBinaryUpgrade; max_safe_fds = param->max_safe_fds; + Ma
Re: [HACKERS] fix bgworkers in EXEC_BACKEND
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
Re: [HACKERS] fix bgworkers in EXEC_BACKEND
Andres Freund 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&a=commitdiff&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
Re: [HACKERS] fix bgworkers in EXEC_BACKEND
Tom Lane schrieb: >Andres Freund 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
Re: [HACKERS] fix bgworkers in EXEC_BACKEND
On 2012-12-27 18:44:57 -0500, Tom Lane wrote: > Andres Freund 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
Re: [HACKERS] fix bgworkers in EXEC_BACKEND
Andres Freund 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
Re: [HACKERS] fix bgworkers in EXEC_BACKEND
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
Re: [HACKERS] fix bgworkers in EXEC_BACKEND
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c index 8dd2b4b..0d808d0 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -103,13 +103,14 @@ int work_mem = 1024; int maintenance_work_mem = 16384; /* - * Primary determinants of sizes of shared-memory structures. MaxBackends is - * MaxConnections + autovacuum_max_workers + 1 (it is computed by the GUC - * assign hooks for those variables): + * Primary determinants of sizes of shared-memory structures. + * + * MaxBackends is computed by PostmasterMain after modules have had a chance to + * register background workers. */ int NBuffers = 1000; -int MaxBackends = 100; int MaxConnections = 90; +int MaxBackends = 0; int VacuumCostPageHit = 1; /* GUC parameters for vacuum */ int VacuumCostPageMiss = 10; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix bgworkers in EXEC_BACKEND
Stephen Frost writes: > Simon, > * Simon Riggs (si...@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
Re: [HACKERS] fix bgworkers in EXEC_BACKEND
Stephen Frost wrote: > * Heikki Linnakangas (hlinnakan...@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 Herrerahttp://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
Re: [HACKERS] fix bgworkers in EXEC_BACKEND
On 27 December 2012 18:36, Stephen Frost wrote: > Simon, > > * Simon Riggs (si...@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
Re: [HACKERS] fix bgworkers in EXEC_BACKEND
Simon, * Simon Riggs (si...@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 signature.asc Description: Digital signature
Re: [HACKERS] fix bgworkers in EXEC_BACKEND
On 27 December 2012 18:06, Heikki Linnakangas 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
Re: [HACKERS] fix bgworkers in EXEC_BACKEND
* Heikki Linnakangas (hlinnakan...@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 signature.asc Description: Digital signature
Re: [HACKERS] 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 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 (GetNumShmemAttachedBgworke
Re: [HACKERS] fix bgworkers in EXEC_BACKEND
On Thu, Dec 27, 2012 at 6:15 PM, 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. 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
[HACKERS] fix bgworkers in EXEC_BACKEND
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. 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'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. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services *** a/src/backend/access/nbtree/nbtutils.c --- b/src/backend/access/nbtree/nbtutils.c *** *** 21,26 --- 21,27 #include "access/reloptions.h" #include "access/relscan.h" #include "miscadmin.h" + #include "storage/proc.h" #include "utils/array.h" #include "utils/lsyscache.h" #include "utils/memutils.h" *** a/src/backend/access/transam/multixact.c --- b/src/backend/access/transam/multixact.c *** *** 57,62 --- 57,63 #include "miscadmin.h" #include "pg_trace.h" #include "storage/lmgr.h" + #include "storage/proc.h" #include "storage/procarray.h" #include "utils/builtins.h" #include "utils/memutils.h" *** a/src/backend/commands/async.c --- b/src/backend/commands/async.c *** *** 126,131 --- 126,132 #include "miscadmin.h" #include "storage/ipc.h" #include "storage/lmgr.h" + #include "storage/proc.h" #include "storage/procsignal.h" #include "storage/sinval.h" #include "tcop/tcopprot.h" *** a/src/backend/libpq/pqcomm.c --- b/src/backend/libpq/pqcomm.c *** *** 93,98 --- 93,99 #include "libpq/libpq.h" #include "miscadmin.h" #include "storage/ipc.h" + #include "storage/proc.h" #include "utils/guc.h" #include "utils/memutils.h" *** a/src/backend/postmaster/autovacuum.c --- b/src/backend/postmaster/autovacuum.c *** *** 108,114 * GUC parameters */ bool autovacuum_start_daemon = false; ! int autovacuum_max_workers; int autovacuum_naptime; int autovacuum_vac_thresh; double autovacuum_vac_scale; --- 108,114 * GUC parameters */ bool autovacuum_start_daemon = false; ! /* autovacuum_max_workers is elsewhere */ int autovacuum_naptime; int autovacuum_vac_thresh; double autovacuum_vac_scale; *** a/src/backend/postmaster/pgstat.c --- b/src/backend/postmaster/pgstat.c *** *** 52,57 --- 52,58 #include "storage/ipc.h" #include "storage/latch.h" #include "storage/pg_shmem.h" + #include "storage/proc.h" #include "storage/procsignal.h" #include "utils/ascii.h" #include "utils/guc.h" *** a/src/backend/postmaster/postmaster.c --- b/src/backend/postmaster/postmaster.c *** *** 897,913 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 - * introdu