Make CREATE/DROP/RENAME DATABASE wait a little bit to see if other backends

will exit before failing because of conflicting DB usage.  Per discussion,
this seems a good idea to help mask the fact that backend exit takes nonzero
time.  Remove a couple of thereby-obsoleted sleeps in contrib and PL
regression test sequences.

contrib/Makefile

-# $PostgreSQL: pgsql/contrib/Makefile,v 1.76 2007/05/17 19:11:24 momjian Exp $
+# $PostgreSQL: pgsql/contrib/Makefile,v 1.77 2007/06/01 19:38:07 tgl Exp $
 
 subdir = contrib
 top_builddir = ..
@@ -57,12 +57,9 @@ all install installdirs uninstall distprep clean distclean maintainer-clean:
 		$(MAKE) -C $$dir $@ || exit; \
 	done
 
-# We'd like check operations to run all the subtests before failing;
-# also insert a sleep to ensure the previous test backend exited before
-# we try to drop the regression database.
+# We'd like check operations to run all the subtests before failing.
 check installcheck:
 	@CHECKERR=0; for dir in $(WANTED_DIRS); do \
-		sleep 1; \
 		$(MAKE) -C $$dir $@ || CHECKERR=$$?; \
 	done; \
 	exit $$CHECKERR

src/backend/commands/dbcommands.c

@@ -13,7 +13,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/commands/dbcommands.c,v 1.194 2007/04/12 15:04:35 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/dbcommands.c,v 1.195 2007/06/01 19:38:07 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -244,17 +244,6 @@ createdb(const CreatedbStmt *stmt)
 							dbtemplate)));
 	}
 
-	/*
-	 * The source DB can't have any active backends, except this one
-	 * (exception is to allow CREATE DB while connected to template1).
-	 * Otherwise we might copy inconsistent data.
-	 */
-	if (DatabaseCancelAutovacuumActivity(src_dboid, true))
-		ereport(ERROR,
-				(errcode(ERRCODE_OBJECT_IN_USE),
-				 errmsg("source database \"%s\" is being accessed by other users",
-						dbtemplate)));
-
 	/* If encoding is defaulted, use source's encoding */
 	if (encoding < 0)
 		encoding = src_encoding;
@@ -333,6 +322,21 @@ createdb(const CreatedbStmt *stmt)
 				(errcode(ERRCODE_DUPLICATE_DATABASE),
 				 errmsg("database \"%s\" already exists", dbname)));
 
+	/*
+	 * The source DB can't have any active backends, except this one
+	 * (exception is to allow CREATE DB while connected to template1).
+	 * Otherwise we might copy inconsistent data.
+	 *
+	 * This should be last among the basic error checks, because it involves
+	 * potential waiting; we may as well throw an error first if we're gonna
+	 * throw one.
+	 */
+	if (CheckOtherDBBackends(src_dboid))
+		ereport(ERROR,
+				(errcode(ERRCODE_OBJECT_IN_USE),
+				 errmsg("source database \"%s\" is being accessed by other users",
+						dbtemplate)));
+
 	/*
 	 * Select an OID for the new database, checking that it doesn't have
 	 * a filename conflict with anything already existing in the tablespace
@@ -542,13 +546,6 @@ dropdb(const char *dbname, bool missing_ok)
 	Relation	pgdbrel;
 	HeapTuple	tup;
 
-	AssertArg(dbname);
-
-	if (strcmp(dbname, get_database_name(MyDatabaseId)) == 0)
-		ereport(ERROR,
-				(errcode(ERRCODE_OBJECT_IN_USE),
-				 errmsg("cannot drop the currently open database")));
-
 	/*
 	 * Look up the target database's OID, and get exclusive lock on it. We
 	 * need this to ensure that no new backend starts up in the target
@@ -595,11 +592,19 @@ dropdb(const char *dbname, bool missing_ok)
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 				 errmsg("cannot drop a template database")));
 
+	/* Obviously can't drop my own database */
+	if (db_id == MyDatabaseId)
+		ereport(ERROR,
+				(errcode(ERRCODE_OBJECT_IN_USE),
+				 errmsg("cannot drop the currently open database")));
+
 	/*
-	 * Check for active backends in the target database.  (Because we hold the
+	 * Check for other backends in the target database.  (Because we hold the
 	 * database lock, no new ones can start after this.)
+	 *
+	 * As in CREATE DATABASE, check this after other error conditions.
 	 */
-	if (DatabaseCancelAutovacuumActivity(db_id, false))
+	if (CheckOtherDBBackends(db_id))
 		ereport(ERROR,
 				(errcode(ERRCODE_OBJECT_IN_USE),
 				 errmsg("database \"%s\" is being accessed by other users",
@@ -699,6 +704,26 @@ RenameDatabase(const char *oldname, const char *newname)
 				(errcode(ERRCODE_UNDEFINED_DATABASE),
 				 errmsg("database \"%s\" does not exist", oldname)));
 
+	/* must be owner */
+	if (!pg_database_ownercheck(db_id, GetUserId()))
+		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
+					   oldname);
+
+	/* must have createdb rights */
+	if (!have_createdb_privilege())
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				 errmsg("permission denied to rename database")));
+
+	/*
+	 * Make sure the new name doesn't exist.  See notes for same error in
+	 * CREATE DATABASE.
+	 */
+	if (OidIsValid(get_database_oid(newname)))
+		ereport(ERROR,
+				(errcode(ERRCODE_DUPLICATE_DATABASE),
+				 errmsg("database \"%s\" already exists", newname)));
+
 	/*
 	 * XXX Client applications probably store the current database somewhere,
 	 * so renaming it could cause confusion.  On the other hand, there may not
@@ -713,30 +738,15 @@ RenameDatabase(const char *oldname, const char *newname)
 	/*
 	 * Make sure the database does not have active sessions.  This is the same
 	 * concern as above, but applied to other sessions.
+	 *
+	 * As in CREATE DATABASE, check this after other error conditions.
 	 */
-	if (DatabaseCancelAutovacuumActivity(db_id, false))
+	if (CheckOtherDBBackends(db_id))
 		ereport(ERROR,
 				(errcode(ERRCODE_OBJECT_IN_USE),
 				 errmsg("database \"%s\" is being accessed by other users",
 						oldname)));
 
-	/* make sure the new name doesn't exist */
-	if (OidIsValid(get_database_oid(newname)))
-		ereport(ERROR,
-				(errcode(ERRCODE_DUPLICATE_DATABASE),
-				 errmsg("database \"%s\" already exists", newname)));
-
-	/* must be owner */
-	if (!pg_database_ownercheck(db_id, GetUserId()))
-		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
-					   oldname);
-
-	/* must have createdb rights */
-	if (!have_createdb_privilege())
-		ereport(ERROR,
-				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-				 errmsg("permission denied to rename database")));
-
 	/* rename */
 	newtup = SearchSysCacheCopy(DATABASEOID,
 								ObjectIdGetDatum(db_id),

src/backend/storage/ipc/procarray.c

@@ -23,7 +23,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/storage/ipc/procarray.c,v 1.24 2007/04/03 16:34:36 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/storage/ipc/procarray.c,v 1.25 2007/06/01 19:38:07 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -683,62 +683,6 @@ GetSnapshotData(Snapshot snapshot, bool serializable)
 	return snapshot;
 }
 
-/*
- * DatabaseCancelAutovacuumActivity -- are there any backends running in the
- * given DB, apart from autovacuum?  If an autovacuum process is running on the
- * database, kill it and restart the counting.
- *
- * If 'ignoreMyself' is TRUE, ignore this particular backend while checking
- * for backends in the target database.
- *
- * This function is used to interlock DROP DATABASE against there being
- * any active backends in the target DB --- dropping the DB while active
- * backends remain would be a Bad Thing.  Note that we cannot detect here
- * the possibility of a newly-started backend that is trying to connect
- * to the doomed database, so additional interlocking is needed during
- * backend startup.
- */
-bool
-DatabaseCancelAutovacuumActivity(Oid databaseId, bool ignoreMyself)
-{
-	ProcArrayStruct *arrayP = procArray;
-	int			index;
-	int			num;
-
-restart:
-	num = 0;
-
-	CHECK_FOR_INTERRUPTS();
-
-	LWLockAcquire(ProcArrayLock, LW_SHARED);
-
-	for (index = 0; index < arrayP->numProcs; index++)
-	{
-		PGPROC	   *proc = arrayP->procs[index];
-
-		if (proc->databaseId == databaseId)
-		{
-			if (ignoreMyself && proc == MyProc)
-				continue;
-
-			num++;
-
-			if (proc->isAutovacuum)
-			{
-				/* an autovacuum -- kill it and restart */
-				LWLockRelease(ProcArrayLock);
-				kill(proc->pid, SIGINT);
-				pg_usleep(100 * 1000);		/* 100ms */
-				goto restart;
-			}
-		}
-	}
-
-	LWLockRelease(ProcArrayLock);
-
-	return (num != 0);
-}
-
 /*
  * GetTransactionsInCommit -- Get the XIDs of transactions that are committing
  *
@@ -1005,6 +949,91 @@ CountUserBackends(Oid roleid)
 	return count;
 }
 
+/*
+ * CheckOtherDBBackends -- check for other backends running in the given DB
+ *
+ * If there are other backends in the DB, we will wait a maximum of 5 seconds
+ * for them to exit.  Autovacuum backends are encouraged to exit early by
+ * sending them SIGINT, but normal user backends are just waited for.
+ *
+ * The current backend is always ignored; it is caller's responsibility to
+ * check whether the current backend uses the given DB, if it's important.
+ *
+ * Returns TRUE if there are (still) other backends in the DB, FALSE if not.
+ *
+ * This function is used to interlock DROP DATABASE and related commands
+ * against there being any active backends in the target DB --- dropping the
+ * DB while active backends remain would be a Bad Thing.  Note that we cannot
+ * detect here the possibility of a newly-started backend that is trying to
+ * connect to the doomed database, so additional interlocking is needed during
+ * backend startup.  The caller should normally hold an exclusive lock on the
+ * target DB before calling this, which is one reason we mustn't wait
+ * indefinitely.
+ */
+bool
+CheckOtherDBBackends(Oid databaseId)
+{
+	ProcArrayStruct *arrayP = procArray;
+	int			tries;
+
+	/* 50 tries with 100ms sleep between tries makes 5 sec total wait */
+	for (tries = 0; tries < 50; tries++)
+	{
+		bool		found = false;
+		int			index;
+
+		CHECK_FOR_INTERRUPTS();
+
+		LWLockAcquire(ProcArrayLock, LW_SHARED);
+
+		for (index = 0; index < arrayP->numProcs; index++)
+		{
+			PGPROC	   *proc = arrayP->procs[index];
+
+			if (proc->databaseId != databaseId)
+				continue;
+			if (proc == MyProc)
+				continue;
+
+			found = true;
+
+			if (proc->isAutovacuum)
+			{
+				/* an autovacuum --- send it SIGINT before sleeping */
+				int		autopid = proc->pid;
+
+				/*
+				 * It's a bit awkward to release ProcArrayLock within the loop,
+				 * but we'd probably better do so before issuing kill().  We
+				 * have no idea what might block kill() inside the kernel...
+				 */
+				LWLockRelease(ProcArrayLock);
+
+				(void) kill(autopid, SIGINT);		/* ignore any error */
+
+				break;
+			}
+			else
+			{
+				LWLockRelease(ProcArrayLock);
+				break;
+			}
+		}
+
+		/* if found is set, we released the lock within the loop body */
+		if (!found)
+		{
+			LWLockRelease(ProcArrayLock);
+			return false;				/* no conflicting backends, so done */
+		}
+
+		/* else sleep and try again */
+		pg_usleep(100 * 1000L);			/* 100ms */
+	}
+
+	return true;						/* timed out, still conflicts */
+}
+
 
 #define XidCacheRemove(i) \
 	do { \

src/include/storage/procarray.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2007, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/storage/procarray.h,v 1.13 2007/04/03 16:34:36 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/storage/procarray.h,v 1.14 2007/06/01 19:38:07 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -32,11 +32,11 @@ extern bool HaveTransactionsInCommit(TransactionId *xids, int nxids);
 extern PGPROC *BackendPidGetProc(int pid);
 extern int	BackendXidGetPid(TransactionId xid);
 extern bool IsBackendPid(int pid);
-extern bool DatabaseCancelAutovacuumActivity(Oid databaseId, bool ignoreMyself);
 
 extern int	CountActiveBackends(void);
 extern int	CountDBBackends(Oid databaseid);
 extern int	CountUserBackends(Oid roleid);
+extern bool CheckOtherDBBackends(Oid databaseId);
 
 extern void XidCacheRemoveRunningXids(TransactionId xid,
 						  int nxids, TransactionId *xids);

src/pl/Makefile

@@ -4,7 +4,7 @@
 #
 # Copyright (c) 1994, Regents of the University of California
 #
-# $PostgreSQL: pgsql/src/pl/Makefile,v 1.25 2007/02/09 15:55:59 petere Exp $
+# $PostgreSQL: pgsql/src/pl/Makefile,v 1.26 2007/06/01 19:38:07 tgl Exp $
 #
 #-------------------------------------------------------------------------
 
@@ -32,12 +32,9 @@ all install installdirs uninstall distprep:
 clean distclean maintainer-clean:
 	@for dir in $(DIRS); do $(MAKE) -C $$dir $@; done
 
-# We'd like check operations to run all the subtests before failing;
-# also insert a sleep to ensure the previous test backend exited before
-# we try to drop the regression database.
+# We'd like check operations to run all the subtests before failing.
 check installcheck:
 	@CHECKERR=0; for dir in $(DIRS); do \
-		sleep 1; \
 		$(MAKE) -C $$dir $@ || CHECKERR=$$?; \
 	done; \
 	exit $$CHECKERR