From 4f6c0c0b5408715aceaf2d7ffa4689e901295c34 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Tue, 8 Oct 2024 13:54:23 +0200 Subject: [PATCH] Do not ignore error during moving parts to detached for MergeTree MergeTree is fragile in this case, since the source of truth for it is the filesystem and it will be left in an inconsistent state. Image the following: - during startup you got some broken part, that should be moved to detached - this rename throws exception (i.e. permission error) - but this error is ignored, server continues, and tries to merge something that produces intersecting parts for that broken part - later on restart you will got intersecting parts (since at the time of scanning for intersecting parts, the part is not checked) Signed-off-by: Azat Khuzhin --- src/Storages/MergeTree/IMergeTreeDataPart.cpp | 32 ++++++++++--------- src/Storages/MergeTree/IMergeTreeDataPart.h | 2 +- src/Storages/MergeTree/MergeTreeData.cpp | 15 +++++---- src/Storages/StorageReplicatedMergeTree.cpp | 4 +-- 4 files changed, 28 insertions(+), 25 deletions(-) diff --git a/src/Storages/MergeTree/IMergeTreeDataPart.cpp b/src/Storages/MergeTree/IMergeTreeDataPart.cpp index 41783ffddb0..effa8e85665 100644 --- a/src/Storages/MergeTree/IMergeTreeDataPart.cpp +++ b/src/Storages/MergeTree/IMergeTreeDataPart.cpp @@ -1894,7 +1894,6 @@ bool IMergeTreeDataPart::shallParticipateInMerges(const StoragePolicyPtr & stora } void IMergeTreeDataPart::renameTo(const String & new_relative_path, bool remove_new_dir_if_exists) -try { assertOnDisk(); @@ -1920,18 +1919,6 @@ try for (const auto & [_, part] : projection_parts) part->getDataPartStorage().changeRootPath(old_projection_root_path, new_projection_root_path); } -catch (...) -{ - if (startsWith(new_relative_path, fs::path(MergeTreeData::DETACHED_DIR_NAME) / "")) - { - // Don't throw when the destination is to the detached folder. It might be able to - // recover in some cases, such as fetching parts into multi-disks while some of the - // disks are broken. - tryLogCurrentException(__PRETTY_FUNCTION__); - } - else - throw; -} std::pair IMergeTreeDataPart::canRemovePart() const { @@ -2038,11 +2025,26 @@ std::optional IMergeTreeDataPart::getRelativePathForDetachedPart(const S return {}; } -void IMergeTreeDataPart::renameToDetached(const String & prefix) +void IMergeTreeDataPart::renameToDetached(const String & prefix, bool ignore_error) { auto path_to_detach = getRelativePathForDetachedPart(prefix, /* broken */ false); assert(path_to_detach); - renameTo(path_to_detach.value(), true); + try + { + renameTo(path_to_detach.value(), true); + } + catch (...) + { + if (ignore_error) + { + // Don't throw when the destination is to the detached folder. It might be able to + // recover in some cases, such as fetching parts into multi-disks while some of the + // disks are broken. + tryLogCurrentException(__PRETTY_FUNCTION__); + } + else + throw; + } part_is_probably_removed_from_disk = true; } diff --git a/src/Storages/MergeTree/IMergeTreeDataPart.h b/src/Storages/MergeTree/IMergeTreeDataPart.h index b41a1d840e1..a02b1ea80d1 100644 --- a/src/Storages/MergeTree/IMergeTreeDataPart.h +++ b/src/Storages/MergeTree/IMergeTreeDataPart.h @@ -396,7 +396,7 @@ public: auto getFilesChecksums() const { return checksums.files; } /// Moves a part to detached/ directory and adds prefix to its name - void renameToDetached(const String & prefix); + void renameToDetached(const String & prefix, bool ignore_error = false); /// Makes checks and move part to new directory /// Changes only relative_dir_name, you need to update other metadata (name, is_temp) explicitly diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index 4dd9fb7b228..9bac18c094a 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -2009,9 +2009,10 @@ void MergeTreeData::loadDataParts(bool skip_sanity_checks, std::optional(this) != nullptr; if (!is_static_storage) for (auto & part : broken_parts_to_detach) - part->renameToDetached("broken-on-start"); /// detached parts must not have '_' in prefixes + part->renameToDetached("broken-on-start", /*ignore_error=*/ replicated); /// detached parts must not have '_' in prefixes resetObjectColumnsFromActiveParts(part_lock); resetSerializationHints(part_lock); @@ -2099,6 +2100,7 @@ try ThreadPoolCallbackRunnerLocal runner(getUnexpectedPartsLoadingThreadPool().get(), "UnexpectedParts"); + bool replicated = dynamic_cast(this) != nullptr; for (auto & load_state : unexpected_data_parts) { std::lock_guard lock(unexpected_data_parts_mutex); @@ -2114,9 +2116,7 @@ try chassert(load_state.part); if (load_state.is_broken) - { - load_state.part->renameToDetached("broken-on-start"); /// detached parts must not have '_' in prefixes - } + load_state.part->renameToDetached("broken-on-start", /*ignore_error=*/ replicated); /// detached parts must not have '_' in prefixes }, Priority{}); } runner.waitForAllToFinishAndRethrowFirstError(); @@ -2167,6 +2167,7 @@ try ThreadPoolCallbackRunnerLocal runner(getOutdatedPartsLoadingThreadPool().get(), "OutdatedParts"); + bool replicated = dynamic_cast(this) != nullptr; while (true) { ThreadFuzzer::maybeInjectSleep(); @@ -2207,7 +2208,7 @@ try if (res.is_broken) { forcefullyRemoveBrokenOutdatedPartFromZooKeeperBeforeDetaching(res.part->name); - res.part->renameToDetached("broken-on-start"); /// detached parts must not have '_' in prefixes + res.part->renameToDetached("broken-on-start", /*ignore_error=*/ replicated); /// detached parts must not have '_' in prefixes } else if (res.part->is_duplicate) res.part->remove(); @@ -4530,6 +4531,7 @@ void MergeTreeData::forcefullyMovePartToDetachedAndRemoveFromMemory(const MergeT auto lock = lockParts(); bool removed_active_part = false; bool restored_active_part = false; + bool replicated = dynamic_cast(this) != nullptr; auto it_part = data_parts_by_info.find(part_to_detach->info); if (it_part == data_parts_by_info.end()) @@ -4548,7 +4550,7 @@ void MergeTreeData::forcefullyMovePartToDetachedAndRemoveFromMemory(const MergeT } modifyPartState(it_part, DataPartState::Deleting); - asMutableDeletingPart(part)->renameToDetached(prefix); + asMutableDeletingPart(part)->renameToDetached(prefix, /*ignore_error=*/ replicated); LOG_TEST(log, "forcefullyMovePartToDetachedAndRemoveFromMemory: removing {} from data_parts_indexes", part->getNameWithState()); data_parts_indexes.erase(it_part); @@ -5939,6 +5941,7 @@ MergeTreeData::MutableDataPartPtr MergeTreeData::loadPartRestoredFromBackup(cons .withPartType(MergeTreeDataPartType::Wide) .build(); } + /// Errors should not be ignored RESTORE, since you should not restore to broken disks. part->renameToDetached("broken-from-backup"); }; diff --git a/src/Storages/StorageReplicatedMergeTree.cpp b/src/Storages/StorageReplicatedMergeTree.cpp index 793fd02c656..62c05db1a52 100644 --- a/src/Storages/StorageReplicatedMergeTree.cpp +++ b/src/Storages/StorageReplicatedMergeTree.cpp @@ -1844,9 +1844,7 @@ bool StorageReplicatedMergeTree::checkPartsImpl(bool skip_sanity_checks) /// detached all unexpected data parts after sanity check. for (auto & part_state : unexpected_data_parts) - { - part_state.part->renameToDetached("ignored"); - } + part_state.part->renameToDetached("ignored", /* ignore_error= */ true); unexpected_data_parts.clear(); return true;