Change the relation_open protocol so that we obtain lock on a relation

(table or index) before trying to open its relcache entry.  This fixes
race conditions in which someone else commits a change to the relation's
catalog entries while we are in process of doing relcache load.  Problems
of that ilk have been reported sporadically for years, but it was not
really practical to fix until recently --- for instance, the recent
addition of WAL-log support for in-place updates helped.

Along the way, remove pg_am.amconcurrent: all AMs are now expected to support
concurrent update.

Author: tglsfdc

contrib/userlock/user_locks.c

@@ -35,8 +35,7 @@ user_lock(uint32 id1, uint32 id2, LOCKMODE lockmode)
 
 	SET_LOCKTAG_USERLOCK(tag, id1, id2);
 
-	return (LockAcquire(&tag, false,
-						lockmode, true, true) != LOCKACQUIRE_NOT_AVAIL);
+	return (LockAcquire(&tag, lockmode, true, true) != LOCKACQUIRE_NOT_AVAIL);
 }
 
 int

doc/src/sgml/catalogs.sgml

@@ -1,4 +1,4 @@
-<!-- $PostgreSQL: pgsql/doc/src/sgml/catalogs.sgml,v 2.128 2006/07/27 08:30:41 petere Exp $ -->
+<!-- $PostgreSQL: pgsql/doc/src/sgml/catalogs.sgml,v 2.129 2006/07/31 20:08:55 tgl Exp $ -->
 <!--
  Documentation of the system catalogs, directed toward PostgreSQL developers
  -->
@@ -401,13 +401,6 @@
       <entry>Can index storage data type differ from column data type?</entry>
      </row>
 
-     <row>
-      <entry><structfield>amconcurrent</structfield></entry>
-      <entry><type>bool</type></entry>
-      <entry></entry>
-      <entry>Does the access method support concurrent updates?</entry>
-     </row>
-
      <row>
       <entry><structfield>amclusterable</structfield></entry>
       <entry><type>bool</type></entry>

doc/src/sgml/indexam.sgml

@@ -1,4 +1,4 @@
-<!-- $PostgreSQL: pgsql/doc/src/sgml/indexam.sgml,v 2.15 2006/07/03 22:45:36 tgl Exp $ -->
+<!-- $PostgreSQL: pgsql/doc/src/sgml/indexam.sgml,v 2.16 2006/07/31 20:08:59 tgl Exp $ -->
 
 <chapter id="indexam">
  <title>Index Access Method Interface Definition</title>
@@ -94,8 +94,7 @@
   <para>
    Some of the flag columns of <structname>pg_am</structname> have nonobvious
    implications.  The requirements of <structfield>amcanunique</structfield>
-   are discussed in <xref linkend="index-unique-checks">, and those of
-   <structfield>amconcurrent</structfield> in <xref linkend="index-locking">.
+   are discussed in <xref linkend="index-unique-checks">.
    The <structfield>amcanmulticol</structfield> flag asserts that the
    access method supports multicolumn indexes, while
    <structfield>amoptionalkey</structfield> asserts that it allows scans
@@ -474,11 +473,7 @@ amrestrpos (IndexScanDesc scan);
    a concurrent delete may or may not be reflected in the results of a scan.
    What is important is that insertions or deletions not cause the scan to
    miss or multiply return entries that were not themselves being inserted or
-   deleted.  (For an index type that does not set
-   <structname>pg_am</>.<structfield>amconcurrent</>, it is sufficient to
-   handle these cases for insertions or deletions performed by the same
-   backend that's doing the scan.  But when <structfield>amconcurrent</> is
-   true, insertions or deletions from other backends must be handled as well.)
+   deleted.
   </para>
 
   <para>
@@ -506,31 +501,16 @@ amrestrpos (IndexScanDesc scan);
   <title>Index Locking Considerations</title>
 
   <para>
-   An index access method can choose whether it supports concurrent updates
-   of the index by multiple processes.  If the method's
-   <structname>pg_am</>.<structfield>amconcurrent</> flag is true, then
-   the core <productname>PostgreSQL</productname> system obtains
+   Index access methods must handle concurrent updates
+   of the index by multiple processes.
+   The core <productname>PostgreSQL</productname> system obtains
    <literal>AccessShareLock</> on the index during an index scan, and
-   <literal>RowExclusiveLock</> when updating the index.  Since these lock
+   <literal>RowExclusiveLock</> when updating the index (including plain
+   <command>VACUUM</>).  Since these lock
    types do not conflict, the access method is responsible for handling any
    fine-grained locking it may need.  An exclusive lock on the index as a whole
-   will be taken only during index creation, destruction, or
-   <literal>REINDEX</>.  When <structfield>amconcurrent</> is false,
-   <productname>PostgreSQL</productname> still obtains
-   <literal>AccessShareLock</> during index scans, but it obtains
-   <literal>AccessExclusiveLock</> during any update.  This ensures that
-   updaters have sole use of the index.  Note that this implicitly assumes
-   that index scans are read-only; an access method that might modify the
-   index during a scan will still have to do its own locking to handle the
-   case of concurrent scans.
-  </para>
-
-  <para>
-   Recall that a backend's own locks never conflict; therefore, even a
-   non-concurrent index type must be prepared to handle the case where
-   a backend is inserting or deleting entries in an index that it is itself
-   scanning.  (This is of course necessary to support an <command>UPDATE</>
-   that uses the index to find the rows to be updated.)
+   will be taken only during index creation, destruction,
+   <command>REINDEX</>, or <command>VACUUM FULL</>.
   </para>
 
   <para>
@@ -567,7 +547,7 @@ amrestrpos (IndexScanDesc scan);
      </listitem>
      <listitem>
       <para>
-       For concurrent index types, an index scan must maintain a pin
+       An index scan must maintain a pin
        on the index page holding the item last returned by
        <function>amgettuple</>, and <function>ambulkdelete</> cannot delete
        entries from pages that are pinned by other backends.  The need
@@ -576,11 +556,10 @@ amrestrpos (IndexScanDesc scan);
      </listitem>
     </itemizedlist>
 
-   If an index is concurrent then it is possible for an index reader to
+   Without the third rule, it is possible for an index reader to
    see an index entry just before it is removed by <command>VACUUM</>, and
    then to arrive at the corresponding heap entry after that was removed by
-   <command>VACUUM</>.  (With a nonconcurrent index, this is not possible
-   because of the conflicting index-level locks that will be taken out.)
+   <command>VACUUM</>.
    This creates no serious problems if that item
    number is still unused when the reader reaches it, since an empty
    item slot will be ignored by <function>heap_fetch()</>.  But what if a

src/backend/access/gin/ginvacuum.c

@@ -8,7 +8,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *          $PostgreSQL: pgsql/src/backend/access/gin/ginvacuum.c,v 1.4 2006/07/14 14:52:16 momjian Exp $
+ *          $PostgreSQL: pgsql/src/backend/access/gin/ginvacuum.c,v 1.5 2006/07/31 20:08:59 tgl Exp $
  *-------------------------------------------------------------------------
  */
 
@@ -572,7 +572,7 @@ ginvacuumcleanup(PG_FUNCTION_ARGS) {
 	IndexVacuumInfo *info = (IndexVacuumInfo *) PG_GETARG_POINTER(0);
 	IndexBulkDeleteResult *stats = (IndexBulkDeleteResult *) PG_GETARG_POINTER(1);
 	Relation    index = info->index;
-	bool	 needLock = !RELATION_IS_LOCAL(index);
+	bool	 needLock;
     BlockNumber npages,
 				blkno;
 	BlockNumber nFreePages,
@@ -591,10 +591,14 @@ ginvacuumcleanup(PG_FUNCTION_ARGS) {
 	 */
 	stats->num_index_tuples = info->num_heap_tuples;
 
-	if (info->vacuum_full) {
-		LockRelation(index, AccessExclusiveLock);
+	/*
+	 * If vacuum full, we already have exclusive lock on the index.
+	 * Otherwise, need lock unless it's local to this backend.
+	 */
+	if (info->vacuum_full)
 		needLock = false;
-	}
+	else
+		needLock = !RELATION_IS_LOCAL(index);
 
 	if (needLock)
 		LockRelationForExtension(index, ExclusiveLock);
@@ -653,9 +657,6 @@ ginvacuumcleanup(PG_FUNCTION_ARGS) {
 	if (needLock)
 		UnlockRelationForExtension(index, ExclusiveLock);
 
-	if (info->vacuum_full)
-		UnlockRelation(index, AccessExclusiveLock);
-
 	PG_RETURN_POINTER(stats);
 }
 

src/backend/access/gist/gistvacuum.c

@@ -8,7 +8,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/gist/gistvacuum.c,v 1.25 2006/07/14 14:52:16 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/gist/gistvacuum.c,v 1.26 2006/07/31 20:08:59 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -517,7 +517,7 @@ gistvacuumcleanup(PG_FUNCTION_ARGS)
 		GistVacuum	gv;
 		ArrayTuple	res;
 
-		LockRelation(rel, AccessExclusiveLock);
+		/* note: vacuum.c already acquired AccessExclusiveLock on index */
 
 		gv.index = rel;
 		initGISTstate(&(gv.giststate), rel);
@@ -543,8 +543,12 @@ gistvacuumcleanup(PG_FUNCTION_ARGS)
 				(errmsg("index \"%s\" needs VACUUM FULL or REINDEX to finish crash recovery",
 						RelationGetRelationName(rel))));
 
+	/*
+	 * If vacuum full, we already have exclusive lock on the index.
+	 * Otherwise, need lock unless it's local to this backend.
+	 */
 	if (info->vacuum_full)
-		needLock = false;		/* relation locked with AccessExclusiveLock */
+		needLock = false;
 	else
 		needLock = !RELATION_IS_LOCAL(rel);
 
@@ -613,9 +617,6 @@ gistvacuumcleanup(PG_FUNCTION_ARGS)
 	if (needLock)
 		UnlockRelationForExtension(rel, ExclusiveLock);
 
-	if (info->vacuum_full)
-		UnlockRelation(rel, AccessExclusiveLock);
-
 	PG_RETURN_POINTER(stats);
 }
 

src/backend/access/heap/heapam.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/heap/heapam.c,v 1.217 2006/07/14 14:52:17 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/heap/heapam.c,v 1.218 2006/07/31 20:08:59 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
@@ -51,6 +51,7 @@
 #include "pgstat.h"
 #include "storage/procarray.h"
 #include "utils/inval.h"
+#include "utils/lsyscache.h"
 #include "utils/relcache.h"
 
 
@@ -687,15 +688,16 @@ relation_open(Oid relationId, LOCKMODE lockmode)
 
 	Assert(lockmode >= NoLock && lockmode < MAX_LOCKMODES);
 
+	/* Get the lock before trying to open the relcache entry */
+	if (lockmode != NoLock)
+		LockRelationOid(relationId, lockmode);
+
 	/* The relcache does all the real work... */
 	r = RelationIdGetRelation(relationId);
 
 	if (!RelationIsValid(r))
 		elog(ERROR, "could not open relation with OID %u", relationId);
 
-	if (lockmode != NoLock)
-		LockRelation(r, lockmode);
-
 	return r;
 }
 
@@ -713,26 +715,38 @@ conditional_relation_open(Oid relationId, LOCKMODE lockmode, bool nowait)
 
 	Assert(lockmode >= NoLock && lockmode < MAX_LOCKMODES);
 
-	/* The relcache does all the real work... */
-	r = RelationIdGetRelation(relationId);
-
-	if (!RelationIsValid(r))
-		elog(ERROR, "could not open relation with OID %u", relationId);
-
+	/* Get the lock before trying to open the relcache entry */
 	if (lockmode != NoLock)
 	{
 		if (nowait)
 		{
-			if (!ConditionalLockRelation(r, lockmode))
-				ereport(ERROR,
-						(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
-						 errmsg("could not obtain lock on relation \"%s\"",
-								RelationGetRelationName(r))));
+			if (!ConditionalLockRelationOid(relationId, lockmode))
+			{
+				/* try to throw error by name; relation could be deleted... */
+				char   *relname = get_rel_name(relationId);
+
+				if (relname)
+					ereport(ERROR,
+							(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
+							 errmsg("could not obtain lock on relation \"%s\"",
+									relname)));
+				else
+					ereport(ERROR,
+							(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
+							 errmsg("could not obtain lock on relation with OID %u",
+									relationId)));
+			}
 		}
 		else
-			LockRelation(r, lockmode);
+			LockRelationOid(relationId, lockmode);
 	}
 
+	/* The relcache does all the real work... */
+	r = RelationIdGetRelation(relationId);
+
+	if (!RelationIsValid(r))
+		elog(ERROR, "could not open relation with OID %u", relationId);
+
 	return r;
 }
 
@@ -749,12 +763,12 @@ relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
 
 	/*
 	 * Check for shared-cache-inval messages before trying to open the
-	 * relation.  This is needed to cover the case where the name identifies a
-	 * rel that has been dropped and recreated since the start of our
+	 * relation.  This is needed to cover the case where the name identifies
+	 * a rel that has been dropped and recreated since the start of our
 	 * transaction: if we don't flush the old syscache entry then we'll latch
-	 * onto that entry and suffer an error when we do LockRelation. Note that
-	 * relation_open does not need to do this, since a relation's OID never
-	 * changes.
+	 * onto that entry and suffer an error when we do RelationIdGetRelation.
+	 * Note that relation_open does not need to do this, since a relation's
+	 * OID never changes.
 	 *
 	 * We skip this if asked for NoLock, on the assumption that the caller has
 	 * already ensured some appropriate lock is held.
@@ -772,7 +786,7 @@ relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
 /* ----------------
  *		relation_close - close any relation
  *
- *		If lockmode is not "NoLock", we first release the specified lock.
+ *		If lockmode is not "NoLock", we then release the specified lock.
  *
  *		Note that it is often sensible to hold a lock beyond relation_close;
  *		in that case, the lock is released automatically at xact end.
@@ -781,13 +795,15 @@ relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
 void
 relation_close(Relation relation, LOCKMODE lockmode)
 {
-	Assert(lockmode >= NoLock && lockmode < MAX_LOCKMODES);
+	LockRelId	relid = relation->rd_lockInfo.lockRelId;
 
-	if (lockmode != NoLock)
-		UnlockRelation(relation, lockmode);
+	Assert(lockmode >= NoLock && lockmode < MAX_LOCKMODES);
 
 	/* The relcache does the real work... */
 	RelationClose(relation);
+
+	if (lockmode != NoLock)
+		UnlockRelationId(&relid, lockmode);
 }