diff --git a/.unreleased/pr_7600 b/.unreleased/pr_7600 new file mode 100644 index 00000000000..b62a48196f0 --- /dev/null +++ b/.unreleased/pr_7600 @@ -0,0 +1,2 @@ +Fixes: #7600 Fix lock order when dropping index + diff --git a/src/chunk_index.c b/src/chunk_index.c index da11d4a5658..484219b8acf 100644 --- a/src/chunk_index.c +++ b/src/chunk_index.c @@ -573,11 +573,52 @@ typedef struct ChunkIndexDeleteData bool drop_index; } ChunkIndexDeleteData; +/* + * Lock object. + * + * In particular, we need to ensure that we lock the table of an index before + * locking the index, or run the risk of ending up in a deadlock since the + * normal locking order is table first, index second. Since we're not a + * concurrent delete, we take a strong lock for this. + * + * It is also necessary that the parent table is locked first, but we have + * already done that at this stage, so it does not need to be done explicitly. + */ +static bool +chunk_lock_object_for_deletion(ObjectAddress *obj) +{ + /* + * If we're locking an index, we need to lock the table first. See + * RangeVarCallbackForDropRelation() in tablecmds.c. We can ignore + * partition indexes since we're not using that. + */ + char relkind = get_rel_relkind(obj->objectId); + + /* + * If we cannot find the object, it might have been concurrently deleted + * (we do not have locks on objects yet). + */ + if (!relkind) + return false; + if (relkind == RELKIND_INDEX) + { + Oid heapOid = IndexGetRelation(obj->objectId, true); + if (OidIsValid(heapOid)) + LockRelationOid(heapOid, AccessExclusiveLock); + } + + LockRelationOid(obj->objectId, AccessExclusiveLock); + return true; +} + /* Find all internal dependencies to be able to delete all the objects in one * go. We do this by scanning the dependency table and keeping all the tables - * in our internal schema. */ + * in our internal schema. + * + * We also lock the objects in the correct order here to make sure that we do + * not end up with deadlocks. */ static void -chunk_collect_objects_for_deletion(const ObjectAddress *relobj, ObjectAddresses *objects) +chunk_collect_and_lock_objects_for_deletion(const ObjectAddress *relobj, ObjectAddresses *objects) { Relation deprel = table_open(DependRelationId, RowExclusiveLock); ScanKeyData scankey[2]; @@ -608,16 +649,10 @@ chunk_collect_objects_for_deletion(const ObjectAddress *relobj, ObjectAddresses { Form_pg_depend record = (Form_pg_depend) GETSTRUCT(tup); ObjectAddress refobj = { .classId = record->refclassid, .objectId = record->refobjid }; - - switch (record->deptype) - { - case DEPENDENCY_INTERNAL: - add_exact_object_address(&refobj, objects); - break; - default: - continue; /* Do nothing */ - } + if (record->deptype == DEPENDENCY_INTERNAL && chunk_lock_object_for_deletion(&refobj)) + add_exact_object_address(&refobj, objects); } + systable_endscan(scan); table_close(deprel, RowExclusiveLock); } @@ -651,6 +686,12 @@ chunk_index_tuple_delete(TupleInfo *ti, void *data) * internal dependencies and use the function * performMultipleDeletions. * + * We lock the objects to delete first to make sure that the lock + * order is correct. This is done inside RemoveRelations and + * performMultipledeletions expect these locks to be taken + * first. If not, it will take very rudimentary locks, which will + * cause deadlocks in some cases. + * * The function performMultipleDeletions accept a list of objects * and if there are dependencies between any of the objects given * to the function, it will not generate an error for that but @@ -658,7 +699,7 @@ chunk_index_tuple_delete(TupleInfo *ti, void *data) * (internal or not) outside this set of objects, it will still * abort the deletion and print an error. */ ObjectAddresses *objects = new_object_addresses(); - chunk_collect_objects_for_deletion(&idxobj, objects); + chunk_collect_and_lock_objects_for_deletion(&idxobj, objects); performMultipleDeletions(objects, DROP_RESTRICT, 0); free_object_addresses(objects); }