Make CREATE/DROP/RENAME DATABASE wait a little bit to see if other backends
authorTom Lane
Fri, 1 Jun 2007 19:38:07 +0000 (19:38 +0000)
committerTom Lane
Fri, 1 Jun 2007 19:38:07 +0000 (19:38 +0000)
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
src/backend/commands/dbcommands.c
src/backend/storage/ipc/procarray.c
src/include/storage/procarray.h
src/pl/Makefile

index dfb65d68bebbd2990d9ea4382ce2a41a01ee7528..6a0d331911d90cdfa98a0c0ab2e6e153847d36e3 100644 (file)
@@ -1,4 +1,4 @@
-# $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
index ca04444f2f06df890ed641da90bcccb931499684..e99ab5d200fef112424ab0b168b0a9f481794d09 100644 (file)
@@ -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),
index 22d031f9eb5968416cf351703dffbf723b019e59..287d3b4ee56162c1a766477bba2e92d0529fad9d 100644 (file)
@@ -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 { \
index cd2df66380937f4a8cd0ec7b4fb0d3180f7ba68f..dafb83a9658f9503d4de39ac1543592b5ad47f81 100644 (file)
@@ -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);
index 1b4ef86a59fe9c3d44725f5bbae1db1514539bef..4a0f3654721a45d2c1352e02d3bf8ecc5c7e63a6 100644 (file)
@@ -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