Code review for superuser_reserved_connections patch. Don't try to do

database access outside a transaction; revert bogus performance improvement
in SIBackendInit(); improve comments; add documentation (this part courtesy
Neil Conway).

4 files changed, 51 insertions, 47 deletions

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 187f0191c94..8f34a3fd2dc 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -37,7 +37,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/postmaster/postmaster.c,v 1.298 2002/11/18 00:40:46 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/postmaster/postmaster.c,v 1.299 2002/11/21 06:36:08 tgl Exp $
  *
  * NOTES
  *
@@ -154,12 +154,11 @@ int			MaxBackends = DEF_MAXBACKENDS;
 /*
  * ReservedBackends is the number of backends reserved for superuser use.
  * This number is taken out of the pool size given by MaxBackends so
- * number of backend slots available to none super users is
- * (MaxBackends - ReservedBackends). Note, existing super user
- * connections are not taken into account once this lower limit has
- * been reached, i.e. superuser connections made before the lower limit
- * is reached always count towards that limit and are not taken from
- * ReservedBackends.
+ * number of backend slots available to non-superusers is
+ * (MaxBackends - ReservedBackends).  Note what this really means is
+ * "if there are <= ReservedBackends connections available, only superusers
+ * can make new connections" --- pre-existing superuser connections don't
+ * count against the limit.
  */
 int			ReservedBackends = 2;
 
@@ -568,7 +567,15 @@ PostmasterMain(int argc, char *argv[])
 	}
 
 	/*
-	 * Check for invalid combinations of switches
+	 * Now we can set the data directory, and then read postgresql.conf.
+	 */
+	checkDataDir(potential_DataDir);	/* issues error messages */
+	SetDataDir(potential_DataDir);
+
+	ProcessConfigFile(PGC_POSTMASTER);
+
+	/*
+	 * Check for invalid combinations of GUC settings.
 	 */
 	if (NBuffers < 2 * MaxBackends || NBuffers < 16)
 	{
@@ -581,16 +588,11 @@ PostmasterMain(int argc, char *argv[])
 		ExitPostmaster(1);
 	}
 
-	checkDataDir(potential_DataDir);	/* issues error messages */
-	SetDataDir(potential_DataDir);
-
-	ProcessConfigFile(PGC_POSTMASTER);
-
-	/*
-	 * Force an exit if ReservedBackends is not less than MaxBackends.
-	 */
 	if (ReservedBackends >= MaxBackends)
-		elog(FATAL, "superuser_reserved_connections must be less than max_connections.");
+	{
+		postmaster_error("superuser_reserved_connections must be less than max_connections.");
+		ExitPostmaster(1);
+	}
 
 	/*
 	 * Now that we are done processing the postmaster arguments, reset
diff --git a/src/backend/storage/ipc/sinval.c b/src/backend/storage/ipc/sinval.c
index 87f7a292452..239c3bc8d30 100644
--- a/src/backend/storage/ipc/sinval.c
+++ b/src/backend/storage/ipc/sinval.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/storage/ipc/sinval.c,v 1.52 2002/09/04 20:31:25 momjian Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/storage/ipc/sinval.c,v 1.53 2002/11/21 06:36:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -542,12 +542,11 @@ BackendIdGetProc(BackendId procId)
 /*
  * CountEmptyBackendSlots - count empty slots in backend process table
  *
- * Doesn't count since the procState array could be large and we've already
- * allowed for that by running a freeBackends counter in the SI segment.
- * Unlike CountActiveBackends() we do not need to interrogate the
- * backends to determine the free slot count.
- * Goes for a lock despite being a trival look up in case other backends
- * are busy starting or exiting since there is scope for confusion.
+ * We don't actually need to count, since sinvaladt.c maintains a
+ * freeBackends counter in the SI segment.
+ *
+ * Acquiring the lock here is almost certainly overkill, but just in
+ * case fetching an int is not atomic on your machine ...
  */
 int
 CountEmptyBackendSlots(void)
diff --git a/src/backend/storage/ipc/sinvaladt.c b/src/backend/storage/ipc/sinvaladt.c
index b4ab1689f94..6850fa8cf52 100644
--- a/src/backend/storage/ipc/sinvaladt.c
+++ b/src/backend/storage/ipc/sinvaladt.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/storage/ipc/sinvaladt.c,v 1.48 2002/08/29 21:02:12 momjian Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/storage/ipc/sinvaladt.c,v 1.49 2002/11/21 06:36:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -92,13 +92,6 @@ SIBackendInit(SISeg *segP)
 	int			index;
 	ProcState  *stateP = NULL;
 
-	if (segP->freeBackends == 0)
-	{
-		/* out of procState slots */
-		MyBackendId = InvalidBackendId;
-		return 0;
-	}
-
 	/* Look for a free entry in the procState array */
 	for (index = 0; index < segP->lastBackend; index++)
 	{
@@ -111,9 +104,18 @@ SIBackendInit(SISeg *segP)
 
 	if (stateP == NULL)
 	{
-		stateP = &segP->procState[segP->lastBackend];
-		Assert(stateP->nextMsgNum < 0);
-		segP->lastBackend++;
+		if (segP->lastBackend < segP->maxBackends)
+		{
+			stateP = &segP->procState[segP->lastBackend];
+			Assert(stateP->nextMsgNum < 0);
+			segP->lastBackend++;
+		}
+		else
+		{
+			/* out of procState slots */
+			MyBackendId = InvalidBackendId;
+			return 0;
+		}
 	}
 
 	MyBackendId = (stateP - &segP->procState[0]) + 1;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 9ac71c6a812..3ad2c50a843 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/utils/init/postinit.c,v 1.117 2002/10/03 19:19:09 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/utils/init/postinit.c,v 1.118 2002/11/21 06:36:08 tgl Exp $
  *
  *
  *-------------------------------------------------------------------------
@@ -378,6 +378,18 @@ InitPostgres(const char *dbname, const char *username)
 	RelationCacheInitializePhase3();
 
 	/*
+	 * Check a normal user hasn't connected to a superuser reserved slot.
+	 * We can't do this till after we've read the user information, and
+	 * we must do it inside a transaction since checking superuserness
+	 * may require database access.  The superuser check is probably the
+	 * most expensive part; don't do it until necessary.
+	 */
+	if (ReservedBackends > 0 &&
+		CountEmptyBackendSlots() < ReservedBackends &&
+		!superuser())
+		elog(FATAL, "Non-superuser connection limit exceeded");
+
+	/*
 	 * Initialize various default states that can't be set up until we've
 	 * selected the active user and done ReverifyMyDatabase.
 	 */
@@ -397,17 +409,6 @@ InitPostgres(const char *dbname, const char *username)
 	/* close the transaction we started above */
 	if (!bootstrap)
 		CommitTransactionCommand(true);
-
-	/*
-	 * Check a normal user hasn't connected to a superuser reserved slot.
-	 * Do this here since we need the user information and that only
-	 * happens after we've started bringing the shared memory online. So
-	 * we wait until we've registered exit handlers and potentially shut
-	 * an open transaction down for an as safety conscious rejection as
-	 * possible.
-	 */
-	if (CountEmptyBackendSlots() < ReservedBackends && !superuser())
-		elog(ERROR, "Non-superuser connection limit exceeded");
 }
 
 /*