Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
***************
*** 1316,1321 **** index_drop(Oid indexId, bool concurrent)
--- 1316,1326 ----
  	 * table lock strong enough to prevent all queries on the table from
  	 * proceeding until we commit and send out a shared-cache-inval notice
  	 * that will make them update their index lists.
+ 	 *
+ 	 * All predicate locks on the index are about to be made invalid. Promote
+ 	 * them to relation locks on the heap. For correctness this must be done
+ 	 * after the index was last seen with indisready = true and before it is
+ 	 * seen with indisvalid = false.
  	 */
  	heapId = IndexGetRelation(indexId, false);
  	if (concurrent)
***************
*** 1349,1354 **** index_drop(Oid indexId, bool concurrent)
--- 1354,1361 ----
  		 */
  		indexRelation = heap_open(IndexRelationId, RowExclusiveLock);
  
+ 		TransferPredicateLocksToHeapRelation(userIndexRelation);
+ 
  		tuple = SearchSysCacheCopy1(INDEXRELID,
  									ObjectIdGetDatum(indexId));
  		if (!HeapTupleIsValid(tuple))
***************
*** 1438,1449 **** 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);
  
  	/*
  	 * Schedule physical removal of the files
--- 1445,1452 ----
  		userHeapRelation = heap_open(heapId, ShareUpdateExclusiveLock);
  		userIndexRelation = index_open(indexId, AccessExclusiveLock);
  	}
! 	else
! 		TransferPredicateLocksToHeapRelation(userIndexRelation);
  
  	/*
  	 * Schedule physical removal of the files

*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
***************
*** 1316,1321 **** index_drop(Oid indexId, bool concurrent)
--- 1316,1333 ----
  	 * table lock strong enough to prevent all queries on the table from
  	 * proceeding until we commit and send out a shared-cache-inval notice
  	 * that will make them update their index lists.
+ 	 *
+ 	 * 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)
***************
*** 1349,1354 **** index_drop(Oid indexId, bool concurrent)
--- 1361,1368 ----
  		 */
  		indexRelation = heap_open(IndexRelationId, RowExclusiveLock);
  
+ 		TransferPredicateLocksToHeapRelation(userIndexRelation);
+ 
  		tuple = SearchSysCacheCopy1(INDEXRELID,
  									ObjectIdGetDatum(indexId));
  		if (!HeapTupleIsValid(tuple))
***************
*** 1438,1449 **** 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);
  
  	/*
  	 * Schedule physical removal of the files
--- 1452,1459 ----
  		userHeapRelation = heap_open(heapId, ShareUpdateExclusiveLock);
  		userIndexRelation = index_open(indexId, AccessExclusiveLock);
  	}
! 	else
! 		TransferPredicateLocksToHeapRelation(userIndexRelation);
  
  	/*
  	 * Schedule physical removal of the files

From 037daa464d3fc63dbc943b13dd90f477a4fc9aba Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 25 Sep 2012 01:41:29 +0200
Subject: [PATCH 1/2] Fix concurrency issues in concurrent index drops

Previously a DROP INDEX CONCURRENTLY started with unsetting indisvalid *and*
indisready. Thats problematic if some transaction is still looking at the index
and another transction makes changes. See the example below.

Now we do the drop in three stages, just as a concurrent index build. First
unset indivalid, wait, unset indisready, wait, drop index.

Example:

Session 1:
CREATE TABLE test_drop_concurrently(id serial primary key, data int);
INSERT INTO test_drop_concurrently(data) SELECT * FROM generate_series(1,
100000);
CREATE INDEX test_drop_concurrently_data ON test_drop_concurrently(data);
BEGIN;
EXPLAIN ANALYZE SELECT * FROM test_drop_concurrently WHERE data = 34343;
SELECT * FROM test_drop_concurrently WHERE data = 34343;
(1 row)

Session 2:
BEGIN;
SELECT * FROM test_drop_concurrently WHERE data = 34343;

Session 3:
DROP INDEX CONCURRENTLY test_drop_concurrently_data;
(in-progress)

Session 2:
INSERT INTO test_drop_concurrently(data) SELECT * FROM generate_series(1, 100000);
COMMIT;

Session 1:
SELECT * FROM test_drop_concurrently WHERE data = 34343;
(1 row)
SET enable_bitmapscan = false;
SET enable_indexscan = false;
SELECT * FROM test_drop_concurrently WHERE data = 34343;
(2 rows)
---
 src/backend/catalog/index.c |   99 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 84 insertions(+), 15 deletions(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 464950b..b39536e 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1316,6 +1316,10 @@ index_drop(Oid indexId, bool concurrent)
 	 * table lock strong enough to prevent all queries on the table from
 	 * proceeding until we commit and send out a shared-cache-inval notice
 	 * that will make them update their index lists.
+	 *
+	 * 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
+	 * fledged AccessExlusiveLock yet.
 	 */
 	heapId = IndexGetRelation(indexId, false);
 	if (concurrent)
@@ -1336,7 +1340,19 @@ index_drop(Oid indexId, bool concurrent)
 
 	/*
 	 * Drop Index concurrently is similar in many ways to creating an index
-	 * concurrently, so some actions are similar to DefineIndex()
+	 * concurrently, so some actions are similar to DefineIndex() just in the
+	 * reverse order.
+	 *
+	 * First we unset indisvalid so queries starting afterwards don't use the
+	 * index to answer queries anymore. We have to keep indisready = true
+	 * though so transactions that are still using the index can continue to
+	 * see valid index contents. E.g. when they are using READ COMMITTED mode,
+	 * and another transactions that started later commits makes changes and
+	 * commits, they need to see those new tuples in the index.
+	 *
+	 * After all transactions that could possibly have used it for queries
+	 * ended we can unset indisready and wait till nobody could be updating it
+	 * anymore.
 	 */
 	if (concurrent)
 	{
@@ -1355,21 +1371,14 @@ index_drop(Oid indexId, bool concurrent)
 			elog(ERROR, "cache lookup failed for index %u", indexId);
 		indexForm = (Form_pg_index) GETSTRUCT(tuple);
 
-		indexForm->indisvalid = false;	/* make unusable for queries */
-		indexForm->indisready = false;	/* make invisible to changes */
+		indexForm->indisvalid = false;	/* make unusable for new queries */
+		/* we keep indisready == true so it still gets updated */
 
 		simple_heap_update(indexRelation, &tuple->t_self, tuple);
 		CatalogUpdateIndexes(indexRelation, tuple);
 
 		heap_close(indexRelation, RowExclusiveLock);
 
-		/*
-		 * Invalidate the relcache for the table, so that after this
-		 * transaction we will refresh the index list. Forgetting just the
-		 * index is not enough.
-		 */
-		CacheInvalidateRelcache(userHeapRelation);
-
 		/* save lockrelid and locktag for below, then close but keep locks */
 		heaprelid = userHeapRelation->rd_lockInfo.lockRelId;
 		SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
@@ -1381,8 +1390,8 @@ index_drop(Oid indexId, bool concurrent)
 		/*
 		 * For a concurrent drop, it's important to make the catalog entries
 		 * visible to other transactions before we drop the index. The index
-		 * will be marked not indisvalid, so that no one else tries to either
-		 * insert into it or use it for queries.
+		 * will be marked not indisvalid, so that no one else tries to use it
+		 * for queries.
 		 *
 		 * We must commit our current transaction so that the index update
 		 * becomes visible; then start another.  Note that all the data
@@ -1397,6 +1406,7 @@ index_drop(Oid indexId, bool concurrent)
 		LockRelationIdForSession(&heaprelid, ShareUpdateExclusiveLock);
 		LockRelationIdForSession(&indexrelid, ShareUpdateExclusiveLock);
 
+		/* XXX: Why do we have to do that here, who pushed which snap? */
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 		StartTransactionCommand();
@@ -1429,11 +1439,70 @@ index_drop(Oid indexId, bool concurrent)
 		}
 
 		/*
+		 * 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
+		 * ->indisready and wait till nobody could update the index anymore.
+		 */
+		indexRelation = heap_open(IndexRelationId, RowExclusiveLock);
+
+		userHeapRelation = heap_open(heapId, ShareUpdateExclusiveLock);
+		userIndexRelation = index_open(indexId, ShareUpdateExclusiveLock);
+
+		tuple = SearchSysCacheCopy1(INDEXRELID,
+									ObjectIdGetDatum(indexId));
+		if (!HeapTupleIsValid(tuple))
+			elog(ERROR, "cache lookup failed for index %u", indexId);
+		indexForm = (Form_pg_index) GETSTRUCT(tuple);
+
+		/* we hold a lock, but make sure we haven't screwed anything up */
+		Assert(indexForm->indisvalid == false);
+		indexForm->indisready = false;	/* don't update index anymore */
+
+		simple_heap_update(indexRelation, &tuple->t_self, tuple);
+		CatalogUpdateIndexes(indexRelation, tuple);
+
+		heap_close(indexRelation, RowExclusiveLock);
+
+		/*
+		 * we close the relations again, were still holding the session lock
+		 * from above though!
+		 */
+		heap_close(userHeapRelation, NoLock);
+		index_close(userIndexRelation, NoLock);
+
+		/*
+		 * Invalidate the relcache for the table, so that after this
+		 * transaction we will refresh the index list. Forgetting just the
+		 * index is not enough.
+		 */
+		CacheInvalidateRelcache(userHeapRelation);
+
+		/*
+		 * just as with indisvalid = false we need to make sure indisready =
+		 * false is visible for everyone.
+		 */
+		CommitTransactionCommand();
+		StartTransactionCommand();
+
+		/*
+		 * Wait till everyone that saw indisready = true finished so we can
+		 * finally really remove the index. The logic here is the same as
+		 * above.
+		 */
+		old_lockholders = GetLockConflicts(&heaplocktag, AccessExclusiveLock);
+
+		while (VirtualTransactionIdIsValid(*old_lockholders))
+		{
+			VirtualXactLock(*old_lockholders, true);
+			old_lockholders++;
+		}
+
+		/*
 		 * Re-open relations to allow us to complete our actions.
 		 *
-		 * At this point, nothing should be accessing the index, but lets
-		 * leave nothing to chance and grab AccessExclusiveLock on the index
-		 * before the physical deletion.
+		 * At this point, nothing should be accessing the index, but lets leave
+		 * nothing to chance and grab AccessExclusiveLock on the index before
+		 * the physical deletion.
 		 */
 		userHeapRelation = heap_open(heapId, ShareUpdateExclusiveLock);
 		userIndexRelation = index_open(indexId, AccessExclusiveLock);
-- 
1.7.10.4

From 251b31f6ae4a7af500b14107f55d253b72bf9bee Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 25 Sep 2012 14:58:32 +0200
Subject: [PATCH 2/2] Fix that DROP INDEX CONCURRENT could leave behind an
 index without dependencies in case of error

DROP INDEX CONCURRENT needs to commit internal transactions to achieve its
aim. It prevents problems due to that by forbidding doing any DROP that
involves more than one object. It forgot about it's own dependencies to its own
table though. Fix that by removing those only in the transaction the index is
actually removed not in the ones invalidating it.
---
 src/backend/catalog/dependency.c |   49 +++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 87d6f02..b9cfee2 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -355,12 +355,7 @@ performMultipleDeletions(const ObjectAddresses *objects,
 	/* And clean up */
 	free_object_addresses(targetObjects);
 
-	/*
-	 * We closed depRel earlier in deleteOneObject if doing a drop
-	 * concurrently
-	 */
-	if ((flags & PERFORM_DELETION_CONCURRENTLY) != PERFORM_DELETION_CONCURRENTLY)
-		heap_close(depRel, RowExclusiveLock);
+	heap_close(depRel, RowExclusiveLock);
 }
 
 /*
@@ -1000,6 +995,7 @@ deleteOneObject(const ObjectAddress *object, Relation depRel, int flags)
 	int			nkeys;
 	SysScanDesc scan;
 	HeapTuple	tup;
+	Oid			depRelOid = depRel->rd_id;
 
 	/* DROP hook of the objects being removed */
 	if (object_access_hook)
@@ -1012,7 +1008,33 @@ deleteOneObject(const ObjectAddress *object, Relation depRel, int flags)
 	}
 
 	/*
-	 * First remove any pg_depend records that link from this object to
+	 * Close depRel if we are doing a drop concurrently. The individual
+	 * deletion has to commit the transaction and we don't want dangling
+	 * references.
+	 */
+	if (flags & PERFORM_DELETION_CONCURRENTLY)
+		heap_close(depRel, RowExclusiveLock);
+
+	/*
+	 * Delete the object itself, in an object-type-dependent way.
+	 *
+	 * Do this before removing outgoing dependencies as deletions can be
+	 * happening in concurrent mode. That will only happen for a single object
+	 * at once and if so the object will be invalidated inside a separate
+	 * transaction and only dropped inside a transaction thats in-progress when
+	 * doDeletion returns. This way no observer can see dangling dependency
+	 * entries.
+	 */
+	doDeletion(object, flags);
+
+	/*
+	 * Reopen depRel if we closed it before
+	 */
+	if (flags & PERFORM_DELETION_CONCURRENTLY)
+		depRel = heap_open(depRelOid, RowExclusiveLock);
+
+	/*
+	 * Then remove any pg_depend records that link from this object to
 	 * others.	(Any records linking to this object should be gone already.)
 	 *
 	 * When dropping a whole object (subId = 0), remove all pg_depend records
@@ -1054,17 +1076,6 @@ deleteOneObject(const ObjectAddress *object, Relation depRel, int flags)
 	deleteSharedDependencyRecordsFor(object->classId, object->objectId,
 									 object->objectSubId);
 
-	/*
-	 * Close depRel if we are doing a drop concurrently because it commits the
-	 * transaction, so we don't want dangling references.
-	 */
-	if ((flags & PERFORM_DELETION_CONCURRENTLY) == PERFORM_DELETION_CONCURRENTLY)
-		heap_close(depRel, RowExclusiveLock);
-
-	/*
-	 * Now delete the object itself, in an object-type-dependent way.
-	 */
-	doDeletion(object, flags);
 
 	/*
 	 * Delete any comments or security labels associated with this object.
@@ -1247,7 +1258,7 @@ AcquireDeletionLock(const ObjectAddress *object, int flags)
 {
 	if (object->classId == RelationRelationId)
 	{
-		if ((flags & PERFORM_DELETION_CONCURRENTLY) == PERFORM_DELETION_CONCURRENTLY)
+		if (flags & PERFORM_DELETION_CONCURRENTLY)
 			LockRelationOid(object->objectId, ShareUpdateExclusiveLock);
 		else
 			LockRelationOid(object->objectId, AccessExclusiveLock);
-- 
1.7.10.4

*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
***************
*** 1320,1325 **** index_drop(Oid indexId, bool concurrent)
--- 1320,1337 ----
  	 * 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
  	 * fledged AccessExlusiveLock 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)
***************
*** 1439,1444 **** index_drop(Oid indexId, bool concurrent)
--- 1451,1464 ----
  		}
  
  		/*
+ 		 * 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
  		 * ->indisready and wait till nobody could update the index anymore.
***************
*** 1507,1518 **** 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);
  
  	/*
  	 * Schedule physical removal of the files
--- 1527,1534 ----
  		userHeapRelation = heap_open(heapId, ShareUpdateExclusiveLock);
  		userIndexRelation = index_open(indexId, AccessExclusiveLock);
  	}
! 	else
! 		TransferPredicateLocksToHeapRelation(userIndexRelation);
  
  	/*
  	 * Schedule physical removal of the files

*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
***************
*** 1320,1325 **** index_drop(Oid indexId, bool concurrent)
--- 1320,1337 ----
  	 * 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)
***************
*** 1445,1450 **** index_drop(Oid indexId, bool concurrent)
--- 1457,1470 ----
  		}
  
  		/*
+ 		 * 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
  		 * indisready and wait till nobody could update the index anymore.
***************
*** 1514,1525 **** 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);
  
  	/*
  	 * Schedule physical removal of the files
--- 1534,1541 ----
  		userHeapRelation = heap_open(heapId, ShareUpdateExclusiveLock);
  		userIndexRelation = index_open(indexId, AccessExclusiveLock);
  	}
! 	else
! 		TransferPredicateLocksToHeapRelation(userIndexRelation);
  
  	/*
  	 * Schedule physical removal of the files