From be58d5d1af9a755ccead05df60d0b755410d1cfb Mon Sep 17 00:00:00 2001 From: alesapin Date: Mon, 30 Jan 2023 17:00:28 +0100 Subject: [PATCH 1/3] Fix bug in tables drop which can lead to potential query hung --- src/Storages/MergeTree/MergeTreeData.cpp | 8 +++ src/Storages/StorageDistributed.cpp | 8 +++ src/Storages/StorageJoin.cpp | 6 +- src/Storages/StorageSet.cpp | 6 +- .../test_drop_no_local_path/__init__.py | 1 + .../configs/remote_servers.xml | 12 ++++ .../test_drop_no_local_path/test.py | 58 +++++++++++++++++++ 7 files changed, 97 insertions(+), 2 deletions(-) create mode 100644 tests/integration/test_drop_no_local_path/__init__.py create mode 100644 tests/integration/test_drop_no_local_path/configs/remote_servers.xml create mode 100644 tests/integration/test_drop_no_local_path/test.py diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index 485f95f12cb..b2e0c14489a 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -2613,6 +2613,14 @@ void MergeTreeData::dropAllData() if (disk->isBroken()) continue; + /// It can naturally happen if we cannot drop table from the first time + /// i.e. get exceptions after remove recursive + if (!disk->exists(relative_data_path)) + { + LOG_INFO(log, "dropAllData: path {} is already removed from disk {}", relative_data_path, disk->getName()); + continue; + } + LOG_INFO(log, "dropAllData: remove format_version.txt, detached, moving and write ahead logs"); disk->removeFileIfExists(fs::path(relative_data_path) / FORMAT_VERSION_FILE_NAME); diff --git a/src/Storages/StorageDistributed.cpp b/src/Storages/StorageDistributed.cpp index 94e52059daa..740ad67cc95 100644 --- a/src/Storages/StorageDistributed.cpp +++ b/src/Storages/StorageDistributed.cpp @@ -1156,7 +1156,15 @@ void StorageDistributed::drop() auto disks = data_volume->getDisks(); for (const auto & disk : disks) + { + if (!disk->exists(relative_data_path)) + { + LOG_INFO(log, "Path {} is already removed from disk {}", relative_data_path, disk->getName()); + continue; + } + disk->removeRecursive(relative_data_path); + } LOG_DEBUG(log, "Removed"); } diff --git a/src/Storages/StorageJoin.cpp b/src/Storages/StorageJoin.cpp index d6cc5199331..f4cf0875059 100644 --- a/src/Storages/StorageJoin.cpp +++ b/src/Storages/StorageJoin.cpp @@ -89,7 +89,11 @@ void StorageJoin::truncate(const ASTPtr &, const StorageMetadataPtr &, ContextPt std::lock_guard mutate_lock(mutate_mutex); TableLockHolder holder = tryLockTimedWithContext(rwlock, RWLockImpl::Write, context); - disk->removeRecursive(path); + if (disk->exists(path)) + disk->removeRecursive(path); + else + LOG_INFO(&Poco::Logger::get("StorageJoin"), "Path {} is already removed from disk {}", path, disk->getName()); + disk->createDirectories(path); disk->createDirectories(path + "tmp/"); diff --git a/src/Storages/StorageSet.cpp b/src/Storages/StorageSet.cpp index b715a8e059b..7c5ba497ec9 100644 --- a/src/Storages/StorageSet.cpp +++ b/src/Storages/StorageSet.cpp @@ -162,7 +162,11 @@ std::optional StorageSet::totalBytes(const Settings &) const { return se void StorageSet::truncate(const ASTPtr &, const StorageMetadataPtr & metadata_snapshot, ContextPtr, TableExclusiveLockHolder &) { - disk->removeRecursive(path); + if (disk->exists(path)) + disk->removeRecursive(path); + else + LOG_INFO(&Poco::Logger::get("StorageSet"), "Path {} is already removed from disk {}", path, disk->getName()); + disk->createDirectories(path); disk->createDirectories(fs::path(path) / "tmp/"); diff --git a/tests/integration/test_drop_no_local_path/__init__.py b/tests/integration/test_drop_no_local_path/__init__.py new file mode 100644 index 00000000000..e5a0d9b4834 --- /dev/null +++ b/tests/integration/test_drop_no_local_path/__init__.py @@ -0,0 +1 @@ +#!/usr/bin/env python3 diff --git a/tests/integration/test_drop_no_local_path/configs/remote_servers.xml b/tests/integration/test_drop_no_local_path/configs/remote_servers.xml new file mode 100644 index 00000000000..c5e0e7ee366 --- /dev/null +++ b/tests/integration/test_drop_no_local_path/configs/remote_servers.xml @@ -0,0 +1,12 @@ + + + + + + instance + 9000 + + + + + diff --git a/tests/integration/test_drop_no_local_path/test.py b/tests/integration/test_drop_no_local_path/test.py new file mode 100644 index 00000000000..407d58021e9 --- /dev/null +++ b/tests/integration/test_drop_no_local_path/test.py @@ -0,0 +1,58 @@ +#!/usr/bin/env python3 + +import pytest +from helpers.cluster import ClickHouseCluster + +cluster = ClickHouseCluster(__file__) +instance = cluster.add_instance("instance", main_configs=["configs/remote_servers.xml"]) + + +@pytest.fixture(scope="module") +def setup_nodes(): + try: + cluster.start() + yield cluster + + finally: + cluster.shutdown() + + +def drop_table_directory(table_name): + data_path = instance.query( + f"SELECT data_paths[1] FROM system.tables where name = '{table_name}'" + ).strip() + print("Data path", data_path) + instance.exec_in_container( + ["bash", "-c", f"rm -fr {data_path}"], privileged=True, user="root" + ) + + +def test_drop_no_local_path(setup_nodes): + instance.query( + "CREATE TABLE merge_tree_table (key UInt64) ENGINE = MergeTree() ORDER BY tuple()" + ) + instance.query("INSERT INTO merge_tree_table VALUES (1)") + drop_table_directory("merge_tree_table") + instance.query("DROP TABLE merge_tree_table SYNC", timeout=10) + + instance.query( + "CREATE TABLE merge_tree_table (key UInt64) ENGINE = MergeTree() ORDER BY tuple()" + ) + + instance.query( + "CREATE TABLE distributed_table (key UInt64) ENGINE = Distributed(test_cluster, default, merge_tree_table, key)" + ) + instance.query("INSERT INTO distributed_table VALUES(0)") + drop_table_directory("distributed_table") + instance.query("DROP TABLE distributed_table SYNC", timeout=10) + + instance.query("DROP TABLE merge_tree_table SYNC", timeout=10) + + instance.query( + "CREATE TABLE join_table(`id` UInt64, `val` String) ENGINE = Join(ANY, LEFT, id)" + ) + instance.query("INSERT INTO join_table VALUES (1, 'a')") + + drop_table_directory("join_table") + + instance.query("TRUNCATE TABLE join_table") From 0908ce8e1473a6f8075778792f3a342596c73799 Mon Sep 17 00:00:00 2001 From: alesapin Date: Mon, 30 Jan 2023 17:02:58 +0100 Subject: [PATCH 2/3] Add timeout --- tests/integration/test_drop_no_local_path/test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_drop_no_local_path/test.py b/tests/integration/test_drop_no_local_path/test.py index 407d58021e9..6e587f0a050 100644 --- a/tests/integration/test_drop_no_local_path/test.py +++ b/tests/integration/test_drop_no_local_path/test.py @@ -55,4 +55,4 @@ def test_drop_no_local_path(setup_nodes): drop_table_directory("join_table") - instance.query("TRUNCATE TABLE join_table") + instance.query("TRUNCATE TABLE join_table", timeout=10) From 9ecf6e70a9d31a0298ff622155b58e3f2d7bd11d Mon Sep 17 00:00:00 2001 From: alesapin Date: Mon, 30 Jan 2023 17:16:07 +0100 Subject: [PATCH 3/3] Make disk object storage compatible with other storages --- src/Disks/ObjectStorages/DiskObjectStorageTransaction.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Disks/ObjectStorages/DiskObjectStorageTransaction.cpp b/src/Disks/ObjectStorages/DiskObjectStorageTransaction.cpp index 6b2c4cdb86f..c1366639e0c 100644 --- a/src/Disks/ObjectStorages/DiskObjectStorageTransaction.cpp +++ b/src/Disks/ObjectStorages/DiskObjectStorageTransaction.cpp @@ -294,7 +294,9 @@ struct RemoveRecursiveObjectStorageOperation final : public IDiskObjectStorageOp void execute(MetadataTransactionPtr tx) override { - removeMetadataRecursive(tx, path); + /// Similar to DiskLocal and https://en.cppreference.com/w/cpp/filesystem/remove + if (metadata_storage.exists(path)) + removeMetadataRecursive(tx, path); } void undo() override