Rethink the locking mechanisms used for CREATE/DROP/RENAME DATABASE.

The former approach used ExclusiveLock on pg_database, which being a
cluster-wide lock meant only one of these operations could proceed at
a time; worse, it also blocked all incoming connections in ReverifyMyDatabase.
Now that we have LockSharedObject(), we can use locks of different types
applied to databases considered as objects.  This allows much more
flexible management of the interlocking: two CREATE DATABASEs need not
block each other, and need not block connections except to the template
database being used.  Similarly DROP DATABASE doesn't block unrelated
operations.  The locking used in flatfiles.c is also much narrower in
scope than before.  Per recent proposal.

doc/src/sgml/manage-ag.sgml

-<!-- $PostgreSQL: pgsql/doc/src/sgml/manage-ag.sgml,v 2.45 2006/03/10 19:10:48 momjian Exp $ -->
+<!-- $PostgreSQL: pgsql/doc/src/sgml/manage-ag.sgml,v 2.46 2006/05/04 16:07:28 tgl Exp $ -->
 
 <chapter id="managing-databases">
  <title>Managing Databases</title>
@@ -166,6 +166,7 @@ CREATE DATABASE <replaceable>dbname</> OWNER <replaceable>rolename</>;
 <programlisting>
 createdb -O <replaceable>rolename</> <replaceable>dbname</>
 </programlisting>
+   from the shell.
    You must be a superuser to be allowed to create a database for
    someone else (that is, for a role you are not a member of).
   </para>
@@ -220,19 +221,15 @@ createdb -T template0 <replaceable>dbname</>
 
   <para>
    It is possible to create additional template databases, and indeed
-   one might copy any database in a cluster by specifying its name
+   one may copy any database in a cluster by specifying its name
    as the template for <command>CREATE DATABASE</>.  It is important to
    understand, however, that this is not (yet) intended as
-   a general-purpose <quote><command>COPY DATABASE</command></quote> facility.  In particular, it is
-   essential that the source database be idle (no data-altering transactions
-   in progress)
-   for the duration of the copying operation.  <command>CREATE DATABASE</>
-   will check
-   that no session (other than itself) is connected to
-   the source database at the start of the operation, but this does not
-   guarantee that changes cannot be made while the copy proceeds, which
-   would result in an inconsistent copied database.  Therefore,
-   we recommend that databases used as templates be treated as read-only.
+   a general-purpose <quote><command>COPY DATABASE</command></quote> facility.
+   The principal limitation is that no other sessions can be connected to
+   the source database while it is being copied.  <command>CREATE
+   DATABASE</> will fail if any other connection exists when it starts;
+   otherwise, new connections to the source database are locked out
+   until <command>CREATE DATABASE</> completes.
   </para>
 
   <para>

doc/src/sgml/ref/create_database.sgml

 <!--
-$PostgreSQL: pgsql/doc/src/sgml/ref/create_database.sgml,v 1.44 2005/07/31 17:19:17 tgl Exp $
+$PostgreSQL: pgsql/doc/src/sgml/ref/create_database.sgml,v 1.45 2006/05/04 16:07:29 tgl Exp $
 PostgreSQL documentation
 -->
 
@@ -45,7 +45,7 @@ CREATE DATABASE <replaceable class="PARAMETER">name</replaceable>
 
   <para>
    Normally, the creator becomes the owner of the new database.
-   Superusers can create databases owned by other users using the
+   Superusers can create databases owned by other users, by using the
    <literal>OWNER</> clause. They can even create databases owned by
    users with no special privileges. Non-superusers with <literal>CREATEDB</>
    privilege can only create databases owned by themselves.
@@ -104,7 +104,8 @@ CREATE DATABASE <replaceable class="PARAMETER">name</replaceable>
         Character set encoding to use in the new database.  Specify
         a string constant (e.g., <literal>'SQL_ASCII'</literal>),
         or an integer encoding number, or <literal>DEFAULT</literal>
-        to use the default encoding. The character sets supported by the
+        to use the default encoding (namely, the encoding of the
+        template database). The character sets supported by the
         <productname>PostgreSQL</productname> server are described in
         <xref linkend="multibyte-charset-supported">.
        </para>
@@ -169,7 +170,11 @@ CREATE DATABASE <replaceable class="PARAMETER">name</replaceable>
    Although it is possible to copy a database other than <literal>template1</>
    by specifying its name as the template, this is not (yet) intended as
    a general-purpose <quote><command>COPY DATABASE</command></quote> facility.
-   We recommend that databases used as templates be treated as read-only.
+   The principal limitation is that no other sessions can be connected to
+   the template database while it is being copied.  <command>CREATE
+   DATABASE</> will fail if any other connection exists when it starts;
+   otherwise, new connections to the template database are locked out
+   until <command>CREATE DATABASE</> completes.
    See <xref linkend="manage-ag-templatedbs"> for more information.
   </para>
 
@@ -220,6 +225,16 @@ CREATE DATABASE music ENCODING 'LATIN1';
    implementation-defined.
   </para>
  </refsect1>
+
+ <refsect1>
+  <title>See Also</title>
+
+  <simplelist type="inline">
+   <member><xref linkend="sql-alterdatabase" endterm="sql-alterdatabase-title"></member>
+   <member><xref linkend="sql-dropdatabase" endterm="sql-dropdatabase-title"></member>
+  </simplelist>
+ </refsect1>
+
 </refentry>
 
 <!-- Keep this comment at the end of the file

doc/src/sgml/ref/drop_database.sgml

 <!--
-$PostgreSQL: pgsql/doc/src/sgml/ref/drop_database.sgml,v 1.21 2005/11/22 15:24:17 adunstan Exp $
+$PostgreSQL: pgsql/doc/src/sgml/ref/drop_database.sgml,v 1.22 2006/05/04 16:07:29 tgl Exp $
 PostgreSQL documentation
 -->
 
@@ -86,7 +86,7 @@ DROP DATABASE [ IF EXISTS ] <replaceable class="PARAMETER">name</replaceable>
   <title>Compatibility</title>
 
   <para>
-   The is no <command>DROP DATABASE</command> statement in the SQL standard.
+   There is no <command>DROP DATABASE</command> statement in the SQL standard.
   </para>
  </refsect1>
 

src/backend/catalog/pg_shdepend.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/catalog/pg_shdepend.c,v 1.8 2006/03/05 15:58:23 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/catalog/pg_shdepend.c,v 1.9 2006/05/04 16:07:29 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -37,7 +37,6 @@
 #include "lib/stringinfo.h"
 #include "miscadmin.h"
 #include "utils/fmgroids.h"
-#include "utils/inval.h"
 #include "utils/syscache.h"
 
 
@@ -911,12 +910,6 @@ shdepLockAndCheckObject(Oid classId, Oid objectId)
 	/* AccessShareLock should be OK, since we are not modifying the object */
 	LockSharedObject(classId, objectId, 0, AccessShareLock);
 
-	/*
-	 * We have to recognize sinval updates here, else our local syscache may
-	 * still contain the object even if it was just dropped.
-	 */
-	AcceptInvalidationMessages();
-
 	switch (classId)
 	{
 		case AuthIdRelationId:

src/backend/commands/user.c

@@ -6,7 +6,7 @@
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/backend/commands/user.c,v 1.170 2006/03/05 15:58:25 momjian Exp $
+ * $PostgreSQL: pgsql/src/backend/commands/user.c,v 1.171 2006/05/04 16:07:29 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -274,10 +274,9 @@ CreateRole(CreateRoleStmt *stmt)
 
 	/*
 	 * Check the pg_authid relation to be certain the role doesn't already
-	 * exist.  Note we secure exclusive lock because we need to protect our
-	 * eventual update of the flat auth file.
+	 * exist.
 	 */
-	pg_authid_rel = heap_open(AuthIdRelationId, ExclusiveLock);
+	pg_authid_rel = heap_open(AuthIdRelationId, RowExclusiveLock);
 	pg_authid_dsc = RelationGetDescr(pg_authid_rel);
 
 	tuple = SearchSysCache(AUTHNAME,
@@ -377,8 +376,8 @@ CreateRole(CreateRoleStmt *stmt)
 				GetUserId(), false);
 
 	/*
-	 * Now we can clean up; but keep lock until commit (to avoid possible
-	 * deadlock when commit code tries to acquire lock).
+	 * Close pg_authid, but keep lock till commit (this is important
+	 * to prevent any risk of deadlock failure while updating flat file)
 	 */
 	heap_close(pg_authid_rel, NoLock);
 
@@ -538,10 +537,9 @@ AlterRole(AlterRoleStmt *stmt)
 		validUntil = strVal(dvalidUntil->arg);
 
 	/*
-	 * Scan the pg_authid relation to be certain the user exists. Note we
-	 * secure exclusive lock to protect our update of the flat auth file.
+	 * Scan the pg_authid relation to be certain the user exists.
 	 */
-	pg_authid_rel = heap_open(AuthIdRelationId, ExclusiveLock);
+	pg_authid_rel = heap_open(AuthIdRelationId, RowExclusiveLock);
 	pg_authid_dsc = RelationGetDescr(pg_authid_rel);
 
 	tuple = SearchSysCache(AUTHNAME,
@@ -697,8 +695,8 @@ AlterRole(AlterRoleStmt *stmt)
 					false);
 
 	/*
-	 * Now we can clean up; but keep lock until commit (to avoid possible
-	 * deadlock when commit code tries to acquire lock).
+	 * Close pg_authid, but keep lock till commit (this is important
+	 * to prevent any risk of deadlock failure while updating flat file)
 	 */
 	heap_close(pg_authid_rel, NoLock);
 
@@ -726,10 +724,6 @@ AlterRoleSet(AlterRoleSetStmt *stmt)
 
 	valuestr = flatten_set_variable_args(stmt->variable, stmt->value);
 
-	/*
-	 * RowExclusiveLock is sufficient, because we don't need to update the
-	 * flat auth file.
-	 */
 	rel = heap_open(AuthIdRelationId, RowExclusiveLock);
 	oldtuple = SearchSysCache(AUTHNAME,
 							  PointerGetDatum(stmt->role),
@@ -799,6 +793,7 @@ AlterRoleSet(AlterRoleSetStmt *stmt)
 	CatalogUpdateIndexes(rel, newtuple);
 
 	ReleaseSysCache(oldtuple);
+	/* needn't keep lock since we won't be updating the flat file */
 	heap_close(rel, RowExclusiveLock);
 }
 
@@ -820,11 +815,9 @@ DropRole(DropRoleStmt *stmt)
 
 	/*
 	 * Scan the pg_authid relation to find the Oid of the role(s) to be
-	 * deleted.  Note we secure exclusive lock on pg_authid, because we need
-	 * to protect our update of the flat auth file.  A regular writer's lock
-	 * on pg_auth_members is sufficient though.
+	 * deleted.
 	 */
-	pg_authid_rel = heap_open(AuthIdRelationId, ExclusiveLock);
+	pg_authid_rel = heap_open(AuthIdRelationId, RowExclusiveLock);
 	pg_auth_members_rel = heap_open(AuthMemRelationId, RowExclusiveLock);
 
 	foreach(item, stmt->roles)
@@ -960,7 +953,7 @@ DropRole(DropRoleStmt *stmt)
 
 	/*
 	 * Now we can clean up; but keep locks until commit (to avoid possible
-	 * deadlock when commit code tries to acquire lock).
+	 * deadlock failure while updating flat file)
 	 */
 	heap_close(pg_auth_members_rel, NoLock);
 	heap_close(pg_authid_rel, NoLock);
@@ -989,8 +982,7 @@ RenameRole(const char *oldname, const char *newname)
 	int			i;
 	Oid			roleid;
 
-	/* ExclusiveLock because we need to update the flat auth file */
-	rel = heap_open(AuthIdRelationId, ExclusiveLock);
+	rel = heap_open(AuthIdRelationId, RowExclusiveLock);
 	dsc = RelationGetDescr(rel);
 
 	oldtuple = SearchSysCache(AUTHNAME,
@@ -1080,6 +1072,11 @@ RenameRole(const char *oldname, const char *newname)
 	CatalogUpdateIndexes(rel, newtuple);
 
 	ReleaseSysCache(oldtuple);
+
+	/*
+	 * Close pg_authid, but keep lock till commit (this is important
+	 * to prevent any risk of deadlock failure while updating flat file)
+	 */
 	heap_close(rel, NoLock);
 
 	/*
@@ -1108,11 +1105,8 @@ GrantRole(GrantRoleStmt *stmt)
 
 	grantee_ids = roleNamesToIds(stmt->grantee_roles);
 
-	/*
-	 * Even though this operation doesn't change pg_authid, we must secure
-	 * exclusive lock on it to protect our update of the flat auth file.
-	 */
-	pg_authid_rel = heap_open(AuthIdRelationId, ExclusiveLock);
+	/* AccessShareLock is enough since we aren't modifying pg_authid */
+	pg_authid_rel = heap_open(AuthIdRelationId, AccessShareLock);
 
 	/*
 	 * Step through all of the granted roles and add/remove entries for the
@@ -1136,6 +1130,10 @@ GrantRole(GrantRoleStmt *stmt)
 						stmt->admin_opt);
 	}
 
+	/*
+	 * Close pg_authid, but keep lock till commit (this is important
+	 * to prevent any risk of deadlock failure while updating flat file)
+	 */
 	heap_close(pg_authid_rel, NoLock);
 
 	/*
@@ -1237,8 +1235,7 @@ roleNamesToIds(List *memberNames)
  * grantorId: who is granting the membership
  * admin_opt: granting admin option?
  *
- * Note: caller is responsible for holding ExclusiveLock on pg_authid,
- * and for calling auth_file_update_needed().
+ * Note: caller is responsible for calling auth_file_update_needed().
  */
 static void
 AddRoleMems(const char *rolename, Oid roleid,
@@ -1283,7 +1280,6 @@ AddRoleMems(const char *rolename, Oid roleid,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("must be superuser to set grantor")));
 
-	/* We need only regular writer's lock on pg_auth_members */
 	pg_authmem_rel = heap_open(AuthMemRelationId, RowExclusiveLock);
 	pg_authmem_dsc = RelationGetDescr(pg_authmem_rel);
 
@@ -1363,8 +1359,8 @@ AddRoleMems(const char *rolename, Oid roleid,
 	}
 
 	/*
-	 * Now we can clean up; but keep lock until commit (to avoid possible
-	 * deadlock when commit code tries to acquire lock).
+	 * Close pg_authmem, but keep lock till commit (this is important
+	 * to prevent any risk of deadlock failure while updating flat file)
 	 */
 	heap_close(pg_authmem_rel, NoLock);
 }
@@ -1378,8 +1374,7 @@ AddRoleMems(const char *rolename, Oid roleid,
  * memberIds: OIDs of roles to del
  * admin_opt: remove admin option only?
  *
- * Note: caller is responsible for holding ExclusiveLock on pg_authid,
- * and for calling auth_file_update_needed().
+ * Note: caller is responsible for calling auth_file_update_needed().
  */
 static void
 DelRoleMems(const char *rolename, Oid roleid,
@@ -1418,7 +1413,6 @@ DelRoleMems(const char *rolename, Oid roleid,
 							rolename)));
 	}
 
-	/* We need only regular writer's lock on pg_auth_members */
 	pg_authmem_rel = heap_open(AuthMemRelationId, RowExclusiveLock);
 	pg_authmem_dsc = RelationGetDescr(pg_authmem_rel);
 
@@ -1478,8 +1472,8 @@ DelRoleMems(const char *rolename, Oid roleid,
 	}
 
 	/*
-	 * Now we can clean up; but keep lock until commit (to avoid possible
-	 * deadlock when commit code tries to acquire lock).
+	 * Close pg_authmem, but keep lock till commit (this is important
+	 * to prevent any risk of deadlock failure while updating flat file)
 	 */
 	heap_close(pg_authmem_rel, NoLock);
 }

src/backend/storage/lmgr/lmgr.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/storage/lmgr/lmgr.c,v 1.82 2006/03/05 15:58:38 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/storage/lmgr/lmgr.c,v 1.83 2006/05/04 16:07:29 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -478,6 +478,9 @@ LockSharedObject(Oid classid, Oid objid, uint16 objsubid,
 					   objsubid);
 
 	(void) LockAcquire(&tag, false, lockmode, false, false);
+
+	/* Make sure syscaches are up-to-date with any changes we waited for */
+	AcceptInvalidationMessages();
 }
 
 /*

src/backend/utils/init/flatfiles.c

@@ -23,7 +23,7 @@
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/backend/utils/init/flatfiles.c,v 1.17 2006/03/05 15:58:46 momjian Exp $
+ * $PostgreSQL: pgsql/src/backend/utils/init/flatfiles.c,v 1.18 2006/05/04 16:07:29 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -768,25 +768,49 @@ AtEOXact_UpdateFlatFiles(bool isCommit)
 	CommandCounterIncrement();
 
 	/*
-	 * We use ExclusiveLock to ensure that only one backend writes the flat
-	 * file(s) at a time.  That's sufficient because it's okay to allow plain
-	 * reads of the tables in parallel.  There is some chance of a deadlock
-	 * here (if we were triggered by a user update of one of the tables, which
-	 * likely won't have gotten a strong enough lock), so get the locks we
-	 * need before writing anything.
+	 * Open and lock the needed catalog(s).
 	 *
-	 * For writing the auth file, it's sufficient to ExclusiveLock pg_authid;
-	 * we take just regular AccessShareLock on pg_auth_members.
+	 * Even though we only need AccessShareLock, this could theoretically fail
+	 * due to deadlock.  In practice, however, our transaction already holds
+	 * RowExclusiveLock or better (it couldn't have updated the catalog
+	 * without such a lock).  This implies that dbcommands.c and other places
+	 * that force flat-file updates must not follow the common practice of
+	 * dropping catalog locks before commit.
 	 */
 	if (database_file_update_subid != InvalidSubTransactionId)
-		drel = heap_open(DatabaseRelationId, ExclusiveLock);
+		drel = heap_open(DatabaseRelationId, AccessShareLock);
 
 	if (auth_file_update_subid != InvalidSubTransactionId)
 	{
-		arel = heap_open(AuthIdRelationId, ExclusiveLock);
+		arel = heap_open(AuthIdRelationId, AccessShareLock);
 		mrel = heap_open(AuthMemRelationId, AccessShareLock);
 	}
 
+	/*
+	 * Obtain special locks to ensure that two transactions don't try to write
+	 * the same flat file concurrently.  Quite aside from any direct risks of
+	 * corrupted output, the winning writer probably wouldn't have seen the
+	 * other writer's updates.  By taking a lock and holding it till commit,
+	 * we ensure that whichever updater goes second will see the other
+	 * updater's changes as committed, and thus the final state of the file
+	 * will include all updates.
+	 *
+	 * We use a lock on "database 0" to protect writing the pg_database flat
+	 * file, and a lock on "role 0" to protect the auth file.  This is a bit
+	 * ugly but it's not worth inventing any more-general convention.  (Any
+	 * two locktags that are never used for anything else would do.)
+	 *
+	 * This is safe against deadlock as long as these are the very last locks
+	 * acquired during the transaction.
+	 */
+	if (database_file_update_subid != InvalidSubTransactionId)
+		LockSharedObject(DatabaseRelationId, InvalidOid, 0,
+						 AccessExclusiveLock);
+
+	if (auth_file_update_subid != InvalidSubTransactionId)
+		LockSharedObject(AuthIdRelationId, InvalidOid, 0,
+						 AccessExclusiveLock);
+
 	/* Okay to write the files */
 	if (database_file_update_subid != InvalidSubTransactionId)
 	{