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
*** 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
  	PostmasterHandle = param->PostmasterHandle;
  	pgwin32_initial_signal_pipe = param->initial_signal_pipe;
*** a/src/backend/utils/init/globals.c
--- b/src/backend/utils/init/globals.c
***************
*** 103,115 **** 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):
   */
  int			NBuffers = 1000;
- int			MaxBackends = 100;
  int			MaxConnections = 90;
  
  int			VacuumCostPageHit = 1;		/* GUC parameters for vacuum */
  int			VacuumCostPageMiss = 10;
--- 103,116 ----
  int			maintenance_work_mem = 16384;
  
  /*
!  * 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			MaxConnections = 90;
+ int			MaxBackends = 0;
  
  int			VacuumCostPageHit = 1;		/* GUC parameters for vacuum */
  int			VacuumCostPageMiss = 10;
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 103,119 ****
  #define MAX_KILOBYTES	(INT_MAX / 1024)
  #endif
  
- /*
-  * Note: MAX_BACKENDS is limited to 2^23-1 because inval.c stores the
-  * backend ID as a 3-byte signed integer.  Even if that limitation were
-  * removed, we still could not exceed INT_MAX/4 because some places compute
-  * 4*MaxBackends without any overflow check.  This is rechecked in
-  * check_maxconnections, since MaxBackends is computed as MaxConnections
-  * plus the number of bgworkers plus autovacuum_max_workers plus one (for the
-  * autovacuum launcher).
-  */
- #define MAX_BACKENDS	0x7fffff
- 
  #define KB_PER_MB (1024)
  #define KB_PER_GB (1024*1024)
  
--- 103,108 ----
***************
*** 199,207 **** static const char *show_tcp_keepalives_idle(void);
  static const char *show_tcp_keepalives_interval(void);
  static const char *show_tcp_keepalives_count(void);
  static bool check_maxconnections(int *newval, void **extra, GucSource source);
- static void assign_maxconnections(int newval, void *extra);
  static bool check_autovacuum_max_workers(int *newval, void **extra, GucSource source);
- static void assign_autovacuum_max_workers(int newval, void *extra);
  static bool check_effective_io_concurrency(int *newval, void **extra, GucSource source);
  static void assign_effective_io_concurrency(int newval, void *extra);
  static void assign_pgstat_temp_directory(const char *newval, void *extra);
--- 188,194 ----
***************
*** 1615,1621 **** static struct config_int ConfigureNamesInt[] =
  		},
  		&MaxConnections,
  		100, 1, MAX_BACKENDS,
! 		check_maxconnections, assign_maxconnections, NULL
  	},
  
  	{
--- 1602,1608 ----
  		},
  		&MaxConnections,
  		100, 1, MAX_BACKENDS,
! 		check_maxconnections, NULL, NULL
  	},
  
  	{
***************
*** 2290,2296 **** static struct config_int ConfigureNamesInt[] =
  		},
  		&autovacuum_max_workers,
  		3, 1, MAX_BACKENDS,
! 		check_autovacuum_max_workers, assign_autovacuum_max_workers, NULL
  	},
  
  	{
--- 2277,2283 ----
  		},
  		&autovacuum_max_workers,
  		3, 1, MAX_BACKENDS,
! 		check_autovacuum_max_workers, NULL, NULL
  	},
  
  	{
***************
*** 8636,8648 **** check_maxconnections(int *newval, void **extra, GucSource source)
  	return true;
  }
  
- static void
- assign_maxconnections(int newval, void *extra)
- {
- 	MaxBackends = newval + autovacuum_max_workers + 1 +
- 		GetNumShmemAttachedBgworkers();
- }
- 
  static bool
  check_autovacuum_max_workers(int *newval, void **extra, GucSource source)
  {
--- 8623,8628 ----
***************
*** 8652,8663 **** check_autovacuum_max_workers(int *newval, void **extra, GucSource source)
  	return true;
  }
  
- static void
- assign_autovacuum_max_workers(int newval, void *extra)
- {
- 	MaxBackends = MaxConnections + newval + 1 + GetNumShmemAttachedBgworkers();
- }
- 
  static bool
  check_effective_io_concurrency(int *newval, void **extra, GucSource source)
  {
--- 8632,8637 ----
*** a/src/include/postmaster/postmaster.h
--- b/src/include/postmaster/postmaster.h
***************
*** 61,64 **** extern Size ShmemBackendArraySize(void);
--- 61,73 ----
  extern void ShmemBackendArrayAllocation(void);
  #endif
  
+ /*
+  * Note: MAX_BACKENDS is limited to 2^23-1 because inval.c stores the
+  * backend ID as a 3-byte signed integer.  Even if that limitation were
+  * removed, we still could not exceed INT_MAX/4 because some places compute
+  * 4*MaxBackends without any overflow check.  This is rechecked in the relevant
+  * GUC check hooks and in RegisterBackgroundWorker().
+  */
+ #define MAX_BACKENDS	0x7fffff
+ 
  #endif   /* _POSTMASTER_H */
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to