diff --git a/src/Interpreters/MergeTreeTransaction.cpp b/src/Interpreters/MergeTreeTransaction.cpp index 7c1feb579e2..c0d3cdfeb62 100644 --- a/src/Interpreters/MergeTreeTransaction.cpp +++ b/src/Interpreters/MergeTreeTransaction.cpp @@ -250,21 +250,6 @@ bool MergeTreeTransaction::rollback() noexcept /// 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) @@ -274,6 +259,18 @@ bool MergeTreeTransaction::rollback() noexcept part->appendCSNToVersionMetadata(VersionMetadata::CREATION); } + for (const auto & part : parts_to_remove) + { + /// NOTE It's possible that part is already removed from working set in the same transaction + /// (or, even worse, in a separate non-transactional query with PrehistoricTID), + /// but it's not a problem: removePartsFromWorkingSet(...) will do nothing in this case. + 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); + for (const auto & part : parts_to_activate) { /// Clear removal_tid from version metadata file, so we will not need to distinguish TIDs that were not committed diff --git a/tests/queries/0_stateless/01172_transaction_counters.reference b/tests/queries/0_stateless/01172_transaction_counters.reference index 1aabf8a2a38..3a167e76817 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 (1,1,'00000000-0000-0000-0000-000000000000') 0 +4 all_2_2_0 18446744073709551615 (0,0,'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,7 +19,6 @@ 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