fix a race condition

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<MergeTreeData &>(part->storage).removePartsFromWorkingSet(NO_TRANSACTION_RAW, {part}, true);
+    }
+
+    for (const auto & part : parts_to_activate)
+        if (part->version.getCreationTID() != tid)
+            const_cast<MergeTreeData &>(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<MergeTreeData &>(part->storage).removePartsFromWorkingSet(NO_TRANSACTION_RAW, {part}, true);
-
-    for (const auto & part : parts_to_activate)
-        if (part->version.getCreationTID() != tid)
-            const_cast<MergeTreeData &>(part->storage).restoreAndActivatePart(part);
 
     assert([&]()
     {

src/Interpreters/MergeTreeTransaction.h

@@ -16,6 +16,9 @@ class IMergeTreeDataPart;
 using DataPartPtr = std::shared_ptr<const IMergeTreeDataPart>;
 using DataPartsVector = std::vector<DataPartPtr>;
 
+/// 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<MergeTreeTransaction>, private boost::noncopyable
 {
     friend class TransactionLog;

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