Correct predicate locking for DROP INDEX CONCURRENTLY.

kgrittn · kgrittn · commit 4c9d0901f135 · 2012-10-21T16:35:42.000-05:00
For the non-concurrent case there is an AccessExclusiveLock lock
on both the index and the heap at a time during which no other
process is using either, before which the index is maintained and
used for scans, and after which the index is no longer used or
maintained.  Predicate locks can safely be moved from the index to
the related heap relation under the protection of these locks.
This was done prior to the introductin of DROP INDEX CONCURRENTLY
and continues to be done for non-concurrent index drops.

For concurrent index drops, the predicate locks must be moved when
there are no index scans in progress on that index and no more can
subsequently start, and before heap inserts stop maintaining the
index.  As long as these conditions are guaranteed when the
TransferPredicateLocksToHeapRelation() function is called,
stronger locks are not needed for correctness.

Kevin Grittner based on questions by Tom Lane in reviewing the
DROP INDEX CONCURRENTLY patch and in cooperation with Andres
Freund and Simon Riggs.
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
@@ -1320,6 +1320,18 @@ index_drop(Oid indexId, bool concurrent)
 	 * In the concurrent case we make sure that nobody can be looking at the
 	 * indexes by dropping the index in multiple steps, so we don't need a full
 	 * AccessExclusiveLock yet.
+	 *
+	 * All predicate locks on the index are about to be made invalid. Promote
+	 * them to relation locks on the heap. For correctness the index must not
+	 * be seen with indisvalid = true during query planning after the move
+	 * starts, so that the index will not be used for a scan after the
+	 * predicate lock move, as this could create new predicate locks on the
+	 * index which would not ensure a heap relation lock. Also, the index must
+	 * not be seen during execution of a heap tuple insert with indisready =
+	 * false before the move is complete, since the conflict with the
+	 * predicate lock on the index gap could be missed before the lock on the
+	 * heap relation is in place to detect a conflict based on the heap tuple
+	 * insert.
 	 */
 	heapId = IndexGetRelation(indexId, false);
 	if (concurrent)
@@ -1444,6 +1456,14 @@ index_drop(Oid indexId, bool concurrent)
 			old_lockholders++;
 		}
 
+		/*
+		 * No more predicate locks will be acquired on this index, and we're
+		 * about to stop doing inserts into the index which could show
+		 * conflicts with existing predicate locks, so now is the time to move
+		 * them to the heap relation.
+		 */
+		TransferPredicateLocksToHeapRelation(userIndexRelation);
+
 		/*
 		 * Now we are sure that nobody uses the index for queries, they just
 		 * might have it opened for updating it. So now we can unset
@@ -1514,12 +1534,8 @@ index_drop(Oid indexId, bool concurrent)
 		userHeapRelation = heap_open(heapId, ShareUpdateExclusiveLock);
 		userIndexRelation = index_open(indexId, AccessExclusiveLock);
 	}
-
-	/*
-	 * All predicate locks on the index are about to be made invalid. Promote
-	 * them to relation locks on the heap.
-	 */
-	TransferPredicateLocksToHeapRelation(userIndexRelation);
+	else
+		TransferPredicateLocksToHeapRelation(userIndexRelation);
 
 	/*
 	 * Schedule physical removal of the files
