From 6cfd5b2165970a65a551117fe58e4b9d22237b8c Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Thu, 11 Apr 2024 14:10:13 +0200 Subject: [PATCH] Fix possible assertion when size of precommitted_parts <= precommitted_parts_need_rename CI founds [1]: Logical error: 'precommitted_parts.size() >= precommitted_parts_need_rename.size()' [1]: https://s3.amazonaws.com/clickhouse-test-reports/61973/5c1e6a3e956917bdbb7eaa467934e5b75f17a923/stateless_tests__tsan__s3_storage__[5_5].html The problem is that after precommitted_parts cleaned from detached parts it may be less then precommitted_parts_need_rename, so to avoid this, let's just copy it to a new container. Signed-off-by: Azat Khuzhin --- src/Storages/MergeTree/MergeTreeData.cpp | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index 522c9f8dd82..ace28e058d4 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -6638,19 +6638,21 @@ void MergeTreeData::Transaction::rollback(DataPartsLock * lock) for (const auto & part : precommitted_parts) part->version.creation_csn.store(Tx::RolledBackCSN); + auto non_detached_precommitted_parts = precommitted_parts; + /// Remove detached parts from working set. /// /// It is possible to have detached parts here, only when rename (in /// commit()) of detached parts had been broken (i.e. during ATTACH), /// i.e. the part itself is broken. DataPartsVector detached_precommitted_parts; - for (auto it = precommitted_parts.begin(); it != precommitted_parts.end();) + for (auto it = non_detached_precommitted_parts.begin(); it != non_detached_precommitted_parts.end();) { const auto & part = *it; if (part->getDataPartStorage().getParentDirectory() == DETACHED_DIR_NAME) { detached_precommitted_parts.push_back(part); - it = precommitted_parts.erase(it); + it = non_detached_precommitted_parts.erase(it); } else ++it; @@ -6658,7 +6660,7 @@ void MergeTreeData::Transaction::rollback(DataPartsLock * lock) WriteBufferFromOwnString buf; buf << "Removing parts:"; - for (const auto & part : precommitted_parts) + for (const auto & part : non_detached_precommitted_parts) buf << " " << part->getDataPartStorage().getPartDirectory(); buf << "."; if (!detached_precommitted_parts.empty()) @@ -6679,7 +6681,7 @@ void MergeTreeData::Transaction::rollback(DataPartsLock * lock) if (!data.all_data_dropped) { Strings part_names; - for (const auto & part : precommitted_parts) + for (const auto & part : non_detached_precommitted_parts) part_names.emplace_back(part->name); throw Exception(ErrorCodes::LOGICAL_ERROR, "There are some PreActive parts ({}) to rollback, " "but data parts set is empty and table {} was not dropped. It's a bug", @@ -6693,7 +6695,7 @@ void MergeTreeData::Transaction::rollback(DataPartsLock * lock) &our_lock); data.removePartsFromWorkingSet(txn, - DataPartsVector(precommitted_parts.begin(), precommitted_parts.end()), + DataPartsVector(non_detached_precommitted_parts.begin(), non_detached_precommitted_parts.end()), /* clear_without_timeout = */ true, &our_lock); } }