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.

3 files changed, 22 insertions, 62 deletions

diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index 11f2ae20f5e..c879de3bbc1 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/executor/execUtils.c,v 1.137 2006/07/14 14:52:19 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/executor/execUtils.c,v 1.138 2006/07/31 20:09:04 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -795,12 +795,6 @@ ExecCloseScanRelation(Relation scanrel)
  *
  *		At entry, caller has already opened and locked
  *		resultRelInfo->ri_RelationDesc.
- *
- *		This used to be horribly ugly code, and slow too because it
- *		did a sequential scan of pg_index.	Now we rely on the relcache
- *		to cache a list of the OIDs of the indices associated with any
- *		specific relation, and we use the pg_index syscache to get the
- *		entries we need from pg_index.
  * ----------------------------------------------------------------
  */
 void
@@ -840,6 +834,7 @@ ExecOpenIndices(ResultRelInfo *resultRelInfo)
 
 	/*
 	 * For each index, open the index relation and save pg_index info.
+	 * We acquire RowExclusiveLock, signifying we will update the index.
 	 */
 	i = 0;
 	foreach(l, indexoidlist)
@@ -848,31 +843,7 @@ ExecOpenIndices(ResultRelInfo *resultRelInfo)
 		Relation	indexDesc;
 		IndexInfo  *ii;
 
-		/*
-		 * Open and lock the index relation
-		 *
-		 * If the index AM supports concurrent updates, obtain
-		 * RowExclusiveLock to signify that we are updating the index.	This
-		 * locks out only operations that need exclusive access, such as
-		 * relocating the index to a new tablespace.
-		 *
-		 * If the index AM is not safe for concurrent updates, obtain an
-		 * exclusive lock on the index to lock out other updaters as well as
-		 * readers (index_beginscan places AccessShareLock on the index).
-		 *
-		 * If there are multiple not-concurrent-safe indexes, all backends
-		 * must lock the indexes in the same order or we will get deadlocks
-		 * here. This is guaranteed by RelationGetIndexList(), which promises
-		 * to return the index list in OID order.
-		 *
-		 * The locks will be released in ExecCloseIndices.
-		 */
-		indexDesc = index_open(indexOid);
-
-		if (indexDesc->rd_am->amconcurrent)
-			LockRelation(indexDesc, RowExclusiveLock);
-		else
-			LockRelation(indexDesc, AccessExclusiveLock);
+		indexDesc = index_open(indexOid, RowExclusiveLock);
 
 		/* extract index key information from the index's pg_index info */
 		ii = BuildIndexInfo(indexDesc);
@@ -907,12 +878,7 @@ ExecCloseIndices(ResultRelInfo *resultRelInfo)
 			continue;			/* shouldn't happen? */
 
 		/* Drop lock acquired by ExecOpenIndices */
-		if (indexDescs[i]->rd_am->amconcurrent)
-			UnlockRelation(indexDescs[i], RowExclusiveLock);
-		else
-			UnlockRelation(indexDescs[i], AccessExclusiveLock);
-
-		index_close(indexDescs[i]);
+		index_close(indexDescs[i], RowExclusiveLock);
 	}
 
 	/*
diff --git a/src/backend/executor/nodeBitmapIndexscan.c b/src/backend/executor/nodeBitmapIndexscan.c
index 7555b5a22ca..6a0303cddd1 100644
--- a/src/backend/executor/nodeBitmapIndexscan.c
+++ b/src/backend/executor/nodeBitmapIndexscan.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/executor/nodeBitmapIndexscan.c,v 1.19 2006/05/30 14:01:58 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/executor/nodeBitmapIndexscan.c,v 1.20 2006/07/31 20:09:04 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -201,7 +201,7 @@ ExecEndBitmapIndexScan(BitmapIndexScanState *node)
 	 * close the index relation
 	 */
 	index_endscan(indexScanDesc);
-	index_close(indexRelationDesc);
+	index_close(indexRelationDesc, NoLock);
 }
 
 /* ----------------------------------------------------------------
@@ -258,8 +258,14 @@ ExecInitBitmapIndexScan(BitmapIndexScan *node, EState *estate, int eflags)
 
 	/*
 	 * Open the index relation.
+	 *
+	 * If the parent table is one of the target relations of the query, then
+	 * InitPlan already opened and write-locked the index, so we can avoid
+	 * taking another lock here.  Otherwise we need a normal reader's lock.
 	 */
-	indexstate->biss_RelationDesc = index_open(node->indexid);
+	relistarget = ExecRelationIsTargetRelation(estate, node->scan.scanrelid);
+	indexstate->biss_RelationDesc = index_open(node->indexid,
+									relistarget ? NoLock : AccessShareLock);
 
 	/*
 	 * Initialize index-specific scan state
@@ -303,18 +309,9 @@ ExecInitBitmapIndexScan(BitmapIndexScan *node, EState *estate, int eflags)
 
 	/*
 	 * Initialize scan descriptor.
-	 *
-	 * Note we acquire no locks here; the index machinery does its own locks
-	 * and unlocks.  (We rely on having a lock on the parent table to
-	 * ensure the index won't go away!)  Furthermore, if the parent table
-	 * is one of the target relations of the query, then InitPlan already
-	 * opened and write-locked the index, so we can tell the index machinery
-	 * not to bother getting an extra lock.
 	 */
-	relistarget = ExecRelationIsTargetRelation(estate, node->scan.scanrelid);
 	indexstate->biss_ScanDesc =
 		index_beginscan_multi(indexstate->biss_RelationDesc,
-							  !relistarget,
 							  estate->es_snapshot,
 							  indexstate->biss_NumScanKeys,
 							  indexstate->biss_ScanKeys);
diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c
index 02f83667a45..84ee56beb0f 100644
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/executor/nodeIndexscan.c,v 1.115 2006/07/14 14:52:19 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/executor/nodeIndexscan.c,v 1.116 2006/07/31 20:09:04 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -415,7 +415,7 @@ ExecEndIndexScan(IndexScanState *node)
 	 * close the index relation
 	 */
 	index_endscan(indexScanDesc);
-	index_close(indexRelationDesc);
+	index_close(indexRelationDesc, NoLock);
 
 	/*
 	 * close the heap relation.
@@ -517,8 +517,14 @@ ExecInitIndexScan(IndexScan *node, EState *estate, int eflags)
 
 	/*
 	 * Open the index relation.
+	 *
+	 * If the parent table is one of the target relations of the query, then
+	 * InitPlan already opened and write-locked the index, so we can avoid
+	 * taking another lock here.  Otherwise we need a normal reader's lock.
 	 */
-	indexstate->iss_RelationDesc = index_open(node->indexid);
+	relistarget = ExecRelationIsTargetRelation(estate, node->scan.scanrelid);
+	indexstate->iss_RelationDesc = index_open(node->indexid,
+									relistarget ? NoLock : AccessShareLock);
 
 	/*
 	 * Initialize index-specific scan state
@@ -561,18 +567,9 @@ ExecInitIndexScan(IndexScan *node, EState *estate, int eflags)
 
 	/*
 	 * Initialize scan descriptor.
-	 *
-	 * Note we acquire no locks here; the index machinery does its own locks
-	 * and unlocks.  (We rely on having a lock on the parent table to
-	 * ensure the index won't go away!)  Furthermore, if the parent table
-	 * is one of the target relations of the query, then InitPlan already
-	 * opened and write-locked the index, so we can tell the index machinery
-	 * not to bother getting an extra lock.
 	 */
-	relistarget = ExecRelationIsTargetRelation(estate, node->scan.scanrelid);
 	indexstate->iss_ScanDesc = index_beginscan(currentRelation,
 											   indexstate->iss_RelationDesc,
-											   !relistarget,
 											   estate->es_snapshot,
 											   indexstate->iss_NumScanKeys,
 											   indexstate->iss_ScanKeys);