diff --git a/src/Storages/MergeTree/MergeTreeSink.cpp b/src/Storages/MergeTree/MergeTreeSink.cpp index 2f860b34fd5..7021d1ab51f 100644 --- a/src/Storages/MergeTree/MergeTreeSink.cpp +++ b/src/Storages/MergeTree/MergeTreeSink.cpp @@ -139,6 +139,9 @@ void MergeTreeSink::finishDelayedChunk() bool added = false; + /// It's important to create it outside of lock scope because + /// otherwise it can lock parts in desctructor and deadlock is possible. + MergeTreeData::Transaction transaction(storage, context->getCurrentTransaction().get()); { auto lock = storage.lockParts(); storage.fillNewPartName(part, lock); @@ -155,19 +158,15 @@ void MergeTreeSink::finishDelayedChunk() } else { - MergeTreeData::Transaction transaction(storage, context->getCurrentTransaction().get()); added = storage.renameTempPartAndAdd(part, transaction, lock); transaction.commit(&lock); - } } else { - MergeTreeData::Transaction transaction(storage, context->getCurrentTransaction().get()); added = storage.renameTempPartAndAdd(part, transaction, lock); transaction.commit(&lock); } - } /// Part can be deduplicated, so increment counters and add to part log only if it's really added diff --git a/src/Storages/MergeTree/ReplicatedMergeTreeSink.cpp b/src/Storages/MergeTree/ReplicatedMergeTreeSink.cpp index 07e21def184..473dad075b9 100644 --- a/src/Storages/MergeTree/ReplicatedMergeTreeSink.cpp +++ b/src/Storages/MergeTree/ReplicatedMergeTreeSink.cpp @@ -471,6 +471,8 @@ void ReplicatedMergeTreeSink::commitPart( /// Information about the part. storage.getCommitPartOps(ops, part, block_id_path); + /// It's important to create it outside of lock scope because + /// otherwise it can lock parts in desctructor and deadlock is possible. MergeTreeData::Transaction transaction(storage, NO_TRANSACTION_RAW); /// If you can not add a part to ZK, we'll remove it back from the working set. bool renamed = false; diff --git a/src/Storages/StorageMergeTree.cpp b/src/Storages/StorageMergeTree.cpp index 5fe7214194a..f6e62656c83 100644 --- a/src/Storages/StorageMergeTree.cpp +++ b/src/Storages/StorageMergeTree.cpp @@ -1540,9 +1540,11 @@ PartitionCommandsResultInfo StorageMergeTree::attachPartition( loaded_parts[i]->storeVersionMetadata(); String old_name = renamed_parts.old_and_new_names[i].old_name; + /// It's important to create it outside of lock scope because + /// otherwise it can lock parts in desctructor and deadlock is possible. + MergeTreeData::Transaction transaction(*this, local_context->getCurrentTransaction().get()); { auto lock = lockParts(); - MergeTreeData::Transaction transaction(*this, local_context->getCurrentTransaction().get()); fillNewPartName(loaded_parts[i], lock); renameTempPartAndAdd(loaded_parts[i], transaction, lock); transaction.commit(&lock); @@ -1797,13 +1799,18 @@ CheckResults StorageMergeTree::checkData(const ASTPtr & query, ContextPtr local_ void StorageMergeTree::attachRestoredParts(MutableDataPartsVector && parts) { + for (auto part : parts) { - auto lock = lockParts(); + /// It's important to create it outside of lock scope because + /// otherwise it can lock parts in desctructor and deadlock is possible. MergeTreeData::Transaction transaction(*this, NO_TRANSACTION_RAW); - fillNewPartName(part, lock); - renameTempPartAndAdd(part, transaction, lock); - transaction.commit(&lock); + { + auto lock = lockParts(); + fillNewPartName(part, lock); + renameTempPartAndAdd(part, transaction, lock); + transaction.commit(&lock); + } } }