From bd0a260928971feec484a22f0b86e5d1936c974f Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 1 Jun 2007 19:38:07 +0000 Subject: [PATCH] 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 | 7 +- src/backend/commands/dbcommands.c | 88 +++++++++-------- src/backend/storage/ipc/procarray.c | 143 +++++++++++++++++----------- src/include/storage/procarray.h | 4 +- src/pl/Makefile | 7 +- 5 files changed, 141 insertions(+), 108 deletions(-) diff --git a/contrib/Makefile b/contrib/Makefile index dfb65d68be..6a0d331911 100644 --- a/contrib/Makefile +++ b/contrib/Makefile @@ -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 diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c index ca04444f2f..e99ab5d200 100644 --- a/src/backend/commands/dbcommands.c +++ b/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), diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 22d031f9eb..287d3b4ee5 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/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 { \ diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h index cd2df66380..dafb83a965 100644 --- a/src/include/storage/procarray.h +++ b/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); diff --git a/src/pl/Makefile b/src/pl/Makefile index 1b4ef86a59..4a0f365472 100644 --- a/src/pl/Makefile +++ b/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