Re: [HACKERS] fix bgworkers in EXEC_BACKEND

[Alvaro Herrera]

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

[Alvaro Herrera]

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(param-initial_signal_pipe,
***
*** 6061,6066  restore_backend_variables(BackendParameters *param, Port *port)
--- 6071,6078 
  	IsBinaryUpgrade = param-IsBinaryUpgrade;
  	max_safe_fds = param-max_safe_fds;
  
+ 	MaxBackends = param-MaxBackends;
+ 
  #ifdef WIN32
  	

Re: [HACKERS] fix bgworkers in EXEC_BACKEND

[Heikki Linnakangas]

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

[HACKERS] fix bgworkers in EXEC_BACKEND

[Alvaro Herrera]

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
- 	 * introducing a new entry point in guc.c to do it in a cleaner fashion.
- 	 */
- 	

Re: [HACKERS] fix bgworkers in EXEC_BACKEND

[Magnus Hagander]

On Thu, Dec 27, 2012 at 6:15 PM, Alvaro Herrera
alvhe...@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

Re: [HACKERS] fix bgworkers in EXEC_BACKEND

[Heikki Linnakangas]

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 (GetNumShmemAttachedBgworkers()  0)

Re: [HACKERS] fix bgworkers in EXEC_BACKEND

[Stephen Frost]

* 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

[Simon Riggs]

On 27 December 2012 18:06, Heikki Linnakangas hlinnakan...@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

Re: [HACKERS] fix bgworkers in EXEC_BACKEND

[Stephen Frost]

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

[Simon Riggs]

On 27 December 2012 18:36, Stephen Frost sfr...@snowman.net 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

[Alvaro Herrera]

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

[Tom Lane]

Stephen Frost sfr...@snowman.net 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

[Alvaro Herrera]

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

[Andres Freund]

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

[Tom Lane]

Andres Freund and...@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

Re: [HACKERS] fix bgworkers in EXEC_BACKEND

[Andres Freund]

On 2012-12-27 18:44:57 -0500, Tom Lane wrote:
 Andres Freund and...@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

Re: [HACKERS] fix bgworkers in EXEC_BACKEND

[anara...@anarazel.de]

Tom Lane t...@sss.pgh.pa.us schrieb:

Andres Freund and...@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

Re: [HACKERS] fix bgworkers in EXEC_BACKEND

[Tom Lane]

Andres Freund and...@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.gita=commitdiffh=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