]> granicus.if.org Git - postgresql/commitdiff
Make CREATE/DROP/RENAME DATABASE wait a little bit to see if other backends
authorTom Lane <tgl@sss.pgh.pa.us>
Fri, 1 Jun 2007 19:38:07 +0000 (19:38 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
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