Fix possibile deadlock when dropping partitions.

heap_drop_with_catalog and RangeVarCallbackForDropRelation should
lock the parent before locking the target relation.

Amit Langote

Discussion: http://postgr.es/m/29588799-a8ce-b0a2-3dae-f39ff6d35922@lab.ntt.co.jp

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index a264f1b9eb99d0e6e4a176de2274e12bed89e7d3..4a5f545dc67228159de76fdb11f5638478b68422 100644 (file)
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1759,29 +1759,33 @@ void
 heap_drop_with_catalog(Oid relid)
 {
        Relation        rel;
+       HeapTuple       tuple;
        Oid                     parentOid;
        Relation        parent = NULL;
 
        /*
-        * Open and lock the relation.
+        * To drop a partition safely, we must grab exclusive lock on its parent,
+        * because another backend might be about to execute a query on the parent
+        * table.  If it relies on previously cached partition descriptor, then
+        * it could attempt to access the just-dropped relation as its partition.
+        * We must therefore take a 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.
         */
-       rel = relation_open(relid, AccessExclusiveLock);
-
-       /*
-        * If the relation is a partition, we must grab exclusive lock on its
-        * parent because we need to update its partition descriptor. We must take
-        * a table lock strong enough to prevent all queries on the parent from
-        * proceeding until we commit and send out a shared-cache-inval notice
-        * that will make them update their partition descriptor. Sometimes, doing
-        * this is cycles spent uselessly, especially if the parent will be
-        * dropped as part of the same command anyway.
-        */
-       if (rel->rd_rel->relispartition)
+       tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+       if (((Form_pg_class) GETSTRUCT(tuple))->relispartition)
        {
                parentOid = get_partition_parent(relid);
                parent = heap_open(parentOid, AccessExclusiveLock);
        }
 
+       ReleaseSysCache(tuple);
+
+       /*
+        * Open and lock the relation.
+        */
+       rel = relation_open(relid, AccessExclusiveLock);
+
        /*
         * There can no longer be anyone *else* touching the relation, but we
         * might still have open queries or cursors, or pending trigger events, in

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index abb262b851cc90bfb75c80b887c47bfe9b4f91e6..a6a9f54b13edd9ec0c8ece93f6e117c27c9337a4 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -271,6 +271,7 @@ struct DropRelationCallbackState
 {
        char            relkind;
        Oid                     heapOid;
+       Oid                     partParentOid;
        bool            concurrent;
 };
 
@@ -1049,6 +1050,7 @@ RemoveRelations(DropStmt *drop)
                /* Look up the appropriate relation using namespace search. */
                state.relkind = relkind;
                state.heapOid = InvalidOid;
+               state.partParentOid = InvalidOid;
                state.concurrent = drop->concurrent;
                relOid = RangeVarGetRelidExtended(rel, lockmode, true,
                                                                                  false,
@@ -1078,6 +1080,8 @@ RemoveRelations(DropStmt *drop)
 /*
  * Before acquiring a table lock, check whether we have sufficient rights.
  * In the case of DROP INDEX, also try to lock the table before the index.
+ * Also, if the table to be dropped is a partition, we try to lock the parent
+ * first.
  */
 static void
 RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid, Oid oldRelOid,
@@ -1087,6 +1091,7 @@ RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid, Oid oldRelOid,
        struct DropRelationCallbackState *state;
        char            relkind;
        char            expected_relkind;
+       bool            is_partition;
        Form_pg_class classform;
        LOCKMODE        heap_lockmode;
 
@@ -1106,6 +1111,17 @@ RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid, Oid oldRelOid,
                state->heapOid = InvalidOid;
        }
 
+       /*
+        * Similarly, if we previously locked some other partition's heap, and
+        * the name we're looking up no longer refers to that relation, release
+        * the now-useless lock.
+        */
+       if (relOid != oldRelOid && OidIsValid(state->partParentOid))
+       {
+               UnlockRelationOid(state->partParentOid, AccessExclusiveLock);
+               state->partParentOid = InvalidOid;
+       }
+
        /* Didn't find a relation, so no need for locking or permission checks. */
        if (!OidIsValid(relOid))
                return;
@@ -1114,6 +1130,7 @@ RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid, Oid oldRelOid,
        if (!HeapTupleIsValid(tuple))
                return;                                 /* concurrently dropped, so nothing to do */
        classform = (Form_pg_class) GETSTRUCT(tuple);
+       is_partition = classform->relispartition;
 
        /*
         * Both RELKIND_RELATION and RELKIND_PARTITIONED_TABLE are OBJECT_TABLE,
@@ -1157,6 +1174,19 @@ RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid, Oid oldRelOid,
                if (OidIsValid(state->heapOid))
                        LockRelationOid(state->heapOid, heap_lockmode);
        }
+
+       /*
+        * Similarly, if the relation is a partition, we must acquire lock on its
+        * parent before locking the partition.  That's because queries lock the
+        * parent before its partitions, so we risk deadlock it we do it the other
+        * way around.
+        */
+       if (is_partition && relOid != oldRelOid)
+       {
+               state->partParentOid = get_partition_parent(relOid);
+               if (OidIsValid(state->partParentOid))
+                       LockRelationOid(state->partParentOid, AccessExclusiveLock);
+       }
 }
 
 /*