From 2e2e2b91904e2e6c50cf10aad9431607d9a160a9 Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Thu, 7 Apr 2022 18:17:43 +0200 Subject: [PATCH] fix a race condition --- src/Interpreters/MergeTreeTransaction.cpp | 26 +++++++++++++------ src/Interpreters/MergeTreeTransaction.h | 3 +++ .../01172_transaction_counters.reference | 3 ++- 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/Interpreters/MergeTreeTransaction.cpp b/src/Interpreters/MergeTreeTransaction.cpp index dd235d23bfe..7c1feb579e2 100644 --- a/src/Interpreters/MergeTreeTransaction.cpp +++ b/src/Interpreters/MergeTreeTransaction.cpp @@ -248,6 +248,24 @@ bool MergeTreeTransaction::rollback() noexcept for (const auto & table_and_mutation : mutations_to_kill) table_and_mutation.first->killMutation(table_and_mutation.second); + /// Discard changes in active parts set + /// Remove parts that were created, restore parts that were removed (except parts that were created by this transaction too) + for (const auto & part : parts_to_remove) + { + if (part->version.isRemovalTIDLocked()) + { + /// Don't need to remove part from working set if it was created and removed by this transaction + assert(part->version.removal_tid_lock == tid.getHash()); + continue; + } + /// FIXME do not lock removal_tid when rolling back part creation, it's ugly + const_cast(part->storage).removePartsFromWorkingSet(NO_TRANSACTION_RAW, {part}, true); + } + + for (const auto & part : parts_to_activate) + if (part->version.getCreationTID() != tid) + const_cast(part->storage).restoreAndActivatePart(part); + /// Kind of optimization: cleanup thread can remove these parts immediately for (const auto & part : parts_to_remove) { @@ -264,14 +282,6 @@ bool MergeTreeTransaction::rollback() noexcept part->version.unlockRemovalTID(tid, TransactionInfoContext{part->storage.getStorageID(), part->name}); } - /// Discard changes in active parts set - /// Remove parts that were created, restore parts that were removed (except parts that were created by this transaction too) - for (const auto & part : parts_to_remove) - const_cast(part->storage).removePartsFromWorkingSet(NO_TRANSACTION_RAW, {part}, true); - - for (const auto & part : parts_to_activate) - if (part->version.getCreationTID() != tid) - const_cast(part->storage).restoreAndActivatePart(part); assert([&]() { diff --git a/src/Interpreters/MergeTreeTransaction.h b/src/Interpreters/MergeTreeTransaction.h index 301434ac712..7ebea450dd0 100644 --- a/src/Interpreters/MergeTreeTransaction.h +++ b/src/Interpreters/MergeTreeTransaction.h @@ -16,6 +16,9 @@ class IMergeTreeDataPart; using DataPartPtr = std::shared_ptr; using DataPartsVector = std::vector; +/// This object is responsible for tracking all changes that some transaction is making in MergeTree tables. +/// It collects all changes that queries of current transaction made in data part sets of all MergeTree tables +/// to ether make them visible when transaction commits or undo when transaction rolls back. class MergeTreeTransaction : public std::enable_shared_from_this, private boost::noncopyable { friend class TransactionLog; diff --git a/tests/queries/0_stateless/01172_transaction_counters.reference b/tests/queries/0_stateless/01172_transaction_counters.reference index 3a167e76817..1aabf8a2a38 100644 --- a/tests/queries/0_stateless/01172_transaction_counters.reference +++ b/tests/queries/0_stateless/01172_transaction_counters.reference @@ -6,7 +6,7 @@ 3 all_1_1_0 0 3 all_3_3_0 1 4 all_1_1_0 1 (0,0,'00000000-0000-0000-0000-000000000000') 0 -4 all_2_2_0 18446744073709551615 (0,0,'00000000-0000-0000-0000-000000000000') 0 +4 all_2_2_0 18446744073709551615 (1,1,'00000000-0000-0000-0000-000000000000') 0 4 all_3_3_0 0 (0,0,'00000000-0000-0000-0000-000000000000') 0 5 1 6 all_1_1_0 0 @@ -19,6 +19,7 @@ 1 1 AddPart 1 1 1 1 all_1_1_0 2 1 Begin 1 1 1 1 2 1 AddPart 1 1 1 1 all_2_2_0 +1 1 LockPart 1 1 1 1 all_2_2_0 2 1 Rollback 1 1 1 1 3 1 Begin 1 1 1 1 3 1 AddPart 1 1 1 1 all_3_3_0