diff --git a/dbms/src/Storages/MergeTree/MergeTreeData.cpp b/dbms/src/Storages/MergeTree/MergeTreeData.cpp index 94e42c34d0f..9d12a9ee6ea 100644 --- a/dbms/src/Storages/MergeTree/MergeTreeData.cpp +++ b/dbms/src/Storages/MergeTree/MergeTreeData.cpp @@ -87,6 +87,8 @@ namespace ErrorCodes extern const int CANNOT_MUNMAP; extern const int CANNOT_MREMAP; extern const int BAD_TTL_EXPRESSION; + extern const int INCORRECT_FILE_NAME; + extern const int BAD_DATA_PART_NAME; } @@ -2583,11 +2585,21 @@ MergeTreeData::getDetachedParts() const res.emplace_back(); auto & part = res.back(); - DetachedPartInfo::tryParseDetachedPartName(dir_name, &part, format_version); + DetachedPartInfo::tryParseDetachedPartName(dir_name, part, format_version); } return res; } +void MergeTreeData::validateDetachedPartName(const String & name) const +{ + if (name.find('/') != std::string::npos || name == "." || name == "..") + throw DB::Exception("Invalid part name", ErrorCodes::INCORRECT_FILE_NAME); + + Poco::File detached_part_dir(full_path + "detached/" + name); + if (!detached_part_dir.exists()) + throw DB::Exception("Detached part \"" + name + "\" not found" , ErrorCodes::BAD_DATA_PART_NAME); +} + MergeTreeData::DataParts MergeTreeData::getDataParts(const DataPartStates & affordable_states) const { DataParts res; diff --git a/dbms/src/Storages/MergeTree/MergeTreeData.h b/dbms/src/Storages/MergeTree/MergeTreeData.h index cec3651652b..2333135d53e 100644 --- a/dbms/src/Storages/MergeTree/MergeTreeData.h +++ b/dbms/src/Storages/MergeTree/MergeTreeData.h @@ -389,6 +389,8 @@ public: /// Returns all detached parts std::vector getDetachedParts() const; + void validateDetachedPartName(const String & name) const; + /// Returns Committed parts DataParts getDataParts() const; DataPartsVector getDataPartsVector() const; diff --git a/dbms/src/Storages/MergeTree/MergeTreeDataPart.cpp b/dbms/src/Storages/MergeTree/MergeTreeDataPart.cpp index 865aaf80ed1..24bc5cd2463 100644 --- a/dbms/src/Storages/MergeTree/MergeTreeDataPart.cpp +++ b/dbms/src/Storages/MergeTree/MergeTreeDataPart.cpp @@ -356,16 +356,18 @@ void MergeTreeDataPart::remove(bool force_recursive /*= false*/) const * - rename directory to temporary name; * - remove it recursive. * - * For temporary name we use "delete_tmp_" prefix. + * For temporary name we use "detached/deleting_" prefix. * - * NOTE: We cannot use "tmp_delete_" prefix, because there is a second thread, + * NOTE: We cannot use "tmp_*" prefix, because there is a second thread, * that calls "clearOldTemporaryDirectories" and removes all directories, that begin with "tmp_" and are old enough. * But when we removing data part, it can be old enough. And rename doesn't change mtime. * And a race condition can happen that will lead to "File not found" error here. + * We move directory to detached/, because if an attempt to remove directory after renaming failed for some reason + * there would be no way to remove directory from storage.full_path (except manually). */ String from = storage.full_path + relative_path; - String to = storage.full_path + "delete_tmp_" + name; + String to = storage.full_path + getRelativePathForDetachedPart("deleting_"); Poco::File from_dir{from}; Poco::File to_dir{to}; diff --git a/dbms/src/Storages/MergeTree/MergeTreePartInfo.cpp b/dbms/src/Storages/MergeTree/MergeTreePartInfo.cpp index de7150e4cea..a9e31a988b3 100644 --- a/dbms/src/Storages/MergeTree/MergeTreePartInfo.cpp +++ b/dbms/src/Storages/MergeTree/MergeTreePartInfo.cpp @@ -188,35 +188,29 @@ String MergeTreePartInfo::getPartNameV0(DayNum left_date, DayNum right_date) con return wb.str(); } -bool DetachedPartInfo::tryParseDetachedPartName(const String & dir_name, DetachedPartInfo * part_info, +bool DetachedPartInfo::tryParseDetachedPartName(const String & dir_name, DetachedPartInfo & part_info, MergeTreeDataFormatVersion format_version) { + part_info.dir_name = dir_name; + /// First, try to parse as . - if (MergeTreePartInfo::tryParsePartName(dir_name, part_info, format_version)) - return part_info->valid_name = true; + if (MergeTreePartInfo::tryParsePartName(dir_name, &part_info, format_version)) + return part_info.valid_name = true; /// Next, as _. Use entire name as prefix if it fails. - part_info->prefix = dir_name; + part_info.prefix = dir_name; const auto first_separator = dir_name.find_first_of('_'); if (first_separator == String::npos) - return part_info->valid_name = false; + return part_info.valid_name = false; // TODO what if contains '_'? const auto part_name = dir_name.substr(first_separator + 1, dir_name.size() - first_separator - 1); - if (!MergeTreePartInfo::tryParsePartName(part_name, part_info, format_version)) - return part_info->valid_name = false; + if (!MergeTreePartInfo::tryParsePartName(part_name, &part_info, format_version)) + return part_info.valid_name = false; - part_info->prefix = dir_name.substr(0, first_separator); - return part_info->valid_name = true; + part_info.prefix = dir_name.substr(0, first_separator); + return part_info.valid_name = true; } -String DetachedPartInfo::fullDirName() const -{ - if (!valid_name) - return prefix; - if (prefix.empty()) - return getPartName(); - return prefix + "_" + getPartName(); -} } diff --git a/dbms/src/Storages/MergeTree/MergeTreePartInfo.h b/dbms/src/Storages/MergeTree/MergeTreePartInfo.h index 7d0fb446ee3..25cf46ad46d 100644 --- a/dbms/src/Storages/MergeTree/MergeTreePartInfo.h +++ b/dbms/src/Storages/MergeTree/MergeTreePartInfo.h @@ -92,15 +92,14 @@ struct MergeTreePartInfo /// addition to the above fields. struct DetachedPartInfo : public MergeTreePartInfo { + /// Suddenly, name of detached part may contain suffix (such as _tryN), which is ignored by MergeTreePartInfo::tryParsePartName(...) + String dir_name; String prefix; - /// If false, prefix contains full directory name and MergeTreePartInfo may be in invalid state - /// (directory name was not successfully parsed). + /// If false, MergeTreePartInfo is in invalid state (directory name was not successfully parsed). bool valid_name; - String fullDirName() const; - - static bool tryParseDetachedPartName(const String & dir_name, DetachedPartInfo * part_info, MergeTreeDataFormatVersion format_version); + static bool tryParseDetachedPartName(const String & dir_name, DetachedPartInfo & part_info, MergeTreeDataFormatVersion format_version); }; } diff --git a/dbms/src/Storages/StorageMergeTree.cpp b/dbms/src/Storages/StorageMergeTree.cpp index 0c1503347c0..ad4d0cd933f 100644 --- a/dbms/src/Storages/StorageMergeTree.cpp +++ b/dbms/src/Storages/StorageMergeTree.cpp @@ -36,7 +36,6 @@ namespace ErrorCodes extern const int INCORRECT_FILE_NAME; extern const int CANNOT_ASSIGN_OPTIMIZE; extern const int INCOMPATIBLE_COLUMNS; - extern const int BAD_DATA_PART_NAME; } namespace ActionLocks @@ -1003,7 +1002,7 @@ void StorageMergeTree::dropDetached(const ASTPtr & partition, bool part, const C validateDetachedPartName(part_id); DetachedPartInfo info; - DetachedPartInfo::tryParseDetachedPartName(part_id, &info, format_version); + DetachedPartInfo::tryParseDetachedPartName(part_id, info, format_version); MergeTreeDataPart detached_part(*this, part_id, info); detached_part.relative_path = "detached/" + part_id; @@ -1038,7 +1037,8 @@ void StorageMergeTree::attachPartition(const ASTPtr & partition, bool attach_par { const String & name = it.name(); MergeTreePartInfo part_info; - /// Parts with prefix in name (e.g. attaching_1_3_3_0, delete_tmp_1_3_3_0) will be ignored + /// Parts with prefix in name (e.g. attaching_1_3_3_0, deleting_1_3_3_0) will be ignored + // TODO what if name contains "_tryN" suffix? if (!MergeTreePartInfo::tryParsePartName(name, &part_info, format_version) || part_info.partition_id != partition_id) { @@ -1163,15 +1163,6 @@ void StorageMergeTree::replacePartitionFrom(const StoragePtr & source_table, con } } -void StorageMergeTree::validateDetachedPartName(const String & name) const -{ - if (name.find('/') != std::string::npos || name == "." || name == "..") - throw DB::Exception("Invalid part name", ErrorCodes::INCORRECT_FILE_NAME); - - Poco::File detached_part_dir(full_path + "detached/" + name); - if (!detached_part_dir.exists()) - throw DB::Exception("Detached part \"" + name + "\" not found" , ErrorCodes::BAD_DATA_PART_NAME); -} ActionLock StorageMergeTree::getActionLock(StorageActionBlockType action_type) { diff --git a/dbms/src/Storages/StorageMergeTree.h b/dbms/src/Storages/StorageMergeTree.h index 42061894a8e..fa2561e4ab2 100644 --- a/dbms/src/Storages/StorageMergeTree.h +++ b/dbms/src/Storages/StorageMergeTree.h @@ -124,7 +124,6 @@ private: void clearColumnInPartition(const ASTPtr & partition, const Field & column_name, const Context & context); void attachPartition(const ASTPtr & partition, bool part, const Context & context); void replacePartitionFrom(const StoragePtr & source_table, const ASTPtr & partition, bool replace, const Context & context); - void validateDetachedPartName(const String & name) const; friend class MergeTreeBlockOutputStream; friend class MergeTreeData; diff --git a/dbms/src/Storages/StorageReplicatedMergeTree.cpp b/dbms/src/Storages/StorageReplicatedMergeTree.cpp index 5f91c304e98..192384602eb 100644 --- a/dbms/src/Storages/StorageReplicatedMergeTree.cpp +++ b/dbms/src/Storages/StorageReplicatedMergeTree.cpp @@ -3554,6 +3554,7 @@ void StorageReplicatedMergeTree::attachPartition(const ASTPtr & partition, bool Strings parts; if (attach_part) { + validateDetachedPartName(partition_id); parts.push_back(partition_id); } else @@ -3566,6 +3567,7 @@ void StorageReplicatedMergeTree::attachPartition(const ASTPtr & partition, bool { String name = it.name(); MergeTreePartInfo part_info; + // TODO what if name contains "_tryN" suffix? if (!MergeTreePartInfo::tryParsePartName(name, &part_info, format_version)) continue; if (part_info.partition_id != partition_id) diff --git a/dbms/src/Storages/System/StorageSystemDetachedParts.cpp b/dbms/src/Storages/System/StorageSystemDetachedParts.cpp index 9b32f1fb29b..e27c7945670 100644 --- a/dbms/src/Storages/System/StorageSystemDetachedParts.cpp +++ b/dbms/src/Storages/System/StorageSystemDetachedParts.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -31,13 +32,12 @@ protected: setColumns(ColumnsDescription{{ {"database", std::make_shared()}, {"table", std::make_shared()}, - {"partition_id", std::make_shared()}, + {"partition_id", std::make_shared(std::make_shared())}, {"name", std::make_shared()}, - {"reason", std::make_shared()}, - {"min_block_number", std::make_shared()}, - {"max_block_number", std::make_shared()}, - {"level", std::make_shared()}, - {"directory_name", std::make_shared()} + {"reason", std::make_shared(std::make_shared())}, + {"min_block_number", std::make_shared(std::make_shared())}, + {"max_block_number", std::make_shared(std::make_shared())}, + {"level", std::make_shared(std::make_shared())} }}); } @@ -63,13 +63,12 @@ protected: int i = 0; columns[i++]->insert(info.database); columns[i++]->insert(info.table); - columns[i++]->insert(p.valid_name ? p.partition_id : ""); - columns[i++]->insert(p.valid_name ? p.getPartName() : ""); - columns[i++]->insert(p.prefix); - columns[i++]->insert(p.min_block); - columns[i++]->insert(p.max_block); - columns[i++]->insert(p.level); - columns[i++]->insert(p.fullDirName()); + columns[i++]->insert(p.valid_name ? p.partition_id : Field()); + columns[i++]->insert(p.dir_name); + columns[i++]->insert(p.valid_name ? p.prefix : Field()); + columns[i++]->insert(p.valid_name ? p.min_block : Field()); + columns[i++]->insert(p.valid_name ? p.max_block : Field()); + columns[i++]->insert(p.valid_name ? p.level : Field()); } } diff --git a/dbms/tests/queries/0_stateless/00974_attach_invalid_parts.reference b/dbms/tests/queries/0_stateless/00974_attach_invalid_parts.reference index d44f46779ca..42a04fe5666 100644 --- a/dbms/tests/queries/0_stateless/00974_attach_invalid_parts.reference +++ b/dbms/tests/queries/0_stateless/00974_attach_invalid_parts.reference @@ -13,5 +13,5 @@ OK 16 120 === detached === 0_5_5_0 -delete_tmp_0_7_7 -attaching_0_6_6 +deleting_0_7_7_0 +attaching_0_6_6_0 diff --git a/dbms/tests/queries/0_stateless/00974_attach_invalid_parts.sh b/dbms/tests/queries/0_stateless/00974_attach_invalid_parts.sh index 89a6be183d2..4e9efa64ad1 100755 --- a/dbms/tests/queries/0_stateless/00974_attach_invalid_parts.sh +++ b/dbms/tests/queries/0_stateless/00974_attach_invalid_parts.sh @@ -27,15 +27,18 @@ $CLICKHOUSE_CLIENT --query="INSERT INTO attach_partitions SELECT number FROM sys $CLICKHOUSE_CLIENT --query="INSERT INTO attach_partitions SELECT number FROM system.numbers WHERE number % 2 = 1 LIMIT 8"; $CLICKHOUSE_CLIENT --query="ALTER TABLE attach_partitions DETACH PARTITION 0"; -mkdir $ch_dir/data/$cur_db/attach_partitions/detached/0_5_5_0/ # broken part -cp -r $ch_dir/data/$cur_db/attach_partitions/detached/0_1_1_0/ $ch_dir/data/$cur_db/attach_partitions/detached/attaching_0_6_6_0/ -cp -r $ch_dir/data/$cur_db/attach_partitions/detached/0_3_3_0/ $ch_dir/data/$cur_db/attach_partitions/detached/delete_tmp_0_7_7_0/ +sudo -n mkdir $ch_dir/data/$cur_db/attach_partitions/detached/0_5_5_0/ 2>/dev/null || \ + mkdir $ch_dir/data/$cur_db/attach_partitions/detached/0_5_5_0/ # broken part +sudo -n cp -r $ch_dir/data/$cur_db/attach_partitions/detached/0_1_1_0/ $ch_dir/data/$cur_db/attach_partitions/detached/attaching_0_6_6_0/ 2>/dev/null || \ + cp -r $ch_dir/data/$cur_db/attach_partitions/detached/0_1_1_0/ $ch_dir/data/$cur_db/attach_partitions/detached/attaching_0_6_6_0/ +sudo -n cp -r $ch_dir/data/$cur_db/attach_partitions/detached/0_3_3_0/ $ch_dir/data/$cur_db/attach_partitions/detached/deleting_0_7_7_0/ 2>/dev/null || \ + cp -r $ch_dir/data/$cur_db/attach_partitions/detached/0_3_3_0/ $ch_dir/data/$cur_db/attach_partitions/detached/deleting_0_7_7_0/ $CLICKHOUSE_CLIENT --query="ALTER TABLE attach_partitions ATTACH PARTITION 0"; $CLICKHOUSE_CLIENT --query="SElECT name FROM system.parts WHERE table='attach_partitions' AND database='${cur_db}' ORDER BY name FORMAT TSV"; $CLICKHOUSE_CLIENT --query="SElECT count(), sum(n) FROM attach_partitions FORMAT TSV"; echo '=== detached ==='; -$CLICKHOUSE_CLIENT --query="SELECT directory_name FROM system.detached_parts WHERE table='attach_partitions' AND database='${cur_db}' FORMAT TSV"; +$CLICKHOUSE_CLIENT --query="SELECT name FROM system.detached_parts WHERE table='attach_partitions' AND database='${cur_db}' FORMAT TSV"; $CLICKHOUSE_CLIENT --query="DROP TABLE attach_partitions"; $CLICKHOUSE_CLIENT --query="SYSTEM START MERGES"; diff --git a/dbms/tests/queries/0_stateless/00975_drop_detached.sh b/dbms/tests/queries/0_stateless/00975_drop_detached.sh index 9f831560bdc..3a5e920da75 100755 --- a/dbms/tests/queries/0_stateless/00975_drop_detached.sh +++ b/dbms/tests/queries/0_stateless/00975_drop_detached.sh @@ -15,16 +15,19 @@ $CLICKHOUSE_CLIENT --query="INSERT INTO drop_detached SELECT number FROM system. $CLICKHOUSE_CLIENT --query="INSERT INTO drop_detached SELECT number FROM system.numbers WHERE number % 2 = 1 LIMIT 8"; $CLICKHOUSE_CLIENT --query="ALTER TABLE drop_detached DETACH PARTITION 0"; -mkdir $ch_dir/data/$cur_db/drop_detached/detached/attaching_0_6_6_0/ -mkdir $ch_dir/data/$cur_db/drop_detached/detached/delete_tmp_0_7_7_0/ -mkdir $ch_dir/data/$cur_db/drop_detached/detached/any_other_name/ +sudo -n mkdir $ch_dir/data/$cur_db/drop_detached/detached/attaching_0_6_6_0/ 2>/dev/null || \ + mkdir $ch_dir/data/$cur_db/drop_detached/detached/attaching_0_6_6_0/ +sudo -n mkdir $ch_dir/data/$cur_db/drop_detached/detached/deleting_0_7_7_0/ 2>/dev/null || \ + mkdir $ch_dir/data/$cur_db/drop_detached/detached/deleting_0_7_7_0/ +sudo -n mkdir $ch_dir/data/$cur_db/drop_detached/detached/any_other_name/ 2>/dev/null || \ + mkdir $ch_dir/data/$cur_db/drop_detached/detached/any_other_name/ $CLICKHOUSE_CLIENT --allow_drop_detached=1 --query="ALTER TABLE drop_detached DROP DETACHED PART '../1_2_2_0'" 2>&1 | grep "Invalid part name" > /dev/null && echo 'OK' $CLICKHOUSE_CLIENT --allow_drop_detached=1 --query="ALTER TABLE drop_detached DROP DETACHED PART '0_1_1_0'" $CLICKHOUSE_CLIENT --allow_drop_detached=1 --query="ALTER TABLE drop_detached DROP DETACHED PART 'attaching_0_6_6_0'" -$CLICKHOUSE_CLIENT --allow_drop_detached=1 --query="ALTER TABLE drop_detached DROP DETACHED PART 'delete_tmp_0_7_7_0'" +$CLICKHOUSE_CLIENT --allow_drop_detached=1 --query="ALTER TABLE drop_detached DROP DETACHED PART 'deleting_0_7_7_0'" $CLICKHOUSE_CLIENT --allow_drop_detached=1 --query="ALTER TABLE drop_detached DROP DETACHED PART 'any_other_name'" -$CLICKHOUSE_CLIENT --query="SElECT directory_name FROM system.detached_parts WHERE table='drop_detached' AND database='${cur_db}' FORMAT TSV"; +$CLICKHOUSE_CLIENT --query="SElECT name FROM system.detached_parts WHERE table='drop_detached' AND database='${cur_db}' FORMAT TSV"; $CLICKHOUSE_CLIENT --query="DROP TABLE drop_detached"; $CLICKHOUSE_CLIENT --query="SYSTEM START MERGES";