On Thursday, October 18, 2012 06:12:02 AM Kevin Grittner wrote:
> Kevin Grittner wrote:
> > Hmm. The comment is probably better now, but I've been re-checking
> > the code, and I think my actual code change is completely wrong.
> > Give me a bit to sort this out.
>
> I'm having trouble seeing a way to make this work without rearranging
> the code for concurrent drop to get to a state where it has set
> indisvalid = false, made that visible to all processes, and ensured
> that all scans of the index are complete -- while indisready is still
> true. That is the point where TransferPredicateLocksToHeapRelation()
> could be safely called. Then we would need to set indisready = false,
> make that visible to all processes, and ensure that all access to the
> index is complete. I can't see where it works to set both flags at
> the same time. I want to sleep on it to see if I can come up with any
> other way, but right now that's the only way I'm seeing to make DROP
> INDEX CONCURRENTLY compatible with SERIALIZABLE transactions. :-(
In a nearby bug I had to restructure the code that in a way thats similar to
this anyway, so that seems fine. Maybe you can fix the bug ontop of the two
attached patches?
Greetings,
Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
From 037daa464d3fc63dbc943b13dd90f477a4fc9aba Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@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 <and...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
