From d34a3208f79b2fcf938c830773977e7d8de8a35b Mon Sep 17 00:00:00 2001 From: Julia Kartseva Date: Mon, 4 Nov 2024 23:09:37 +0000 Subject: [PATCH 1/4] fix transaction rollback when file write finalize fails --- ...dataStorageFromPlainObjectStorageOperations.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.cpp b/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.cpp index 62015631aa5..3dac1df2ff8 100644 --- a/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.cpp +++ b/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.cpp @@ -97,20 +97,22 @@ void MetadataStorageFromPlainObjectStorageCreateDirectoryOperation::undo(std::un { auto metadata_object_key = createMetadataObjectKey(object_key_prefix, metadata_key_prefix); - if (write_finalized) + if (write_finalized || write_created) { const auto base_path = path.parent_path(); + size_t erase_count = 0; { std::lock_guard lock(path_map.mutex); - path_map.map.erase(base_path); + erase_count = path_map.map.erase(base_path); + } + if (erase_count) + { + auto metric = object_storage->getMetadataStorageMetrics().directory_map_size; + CurrentMetrics::sub(metric, erase_count); } - auto metric = object_storage->getMetadataStorageMetrics().directory_map_size; - CurrentMetrics::sub(metric, 1); object_storage->removeObjectIfExists(StoredObject(metadata_object_key.serialize(), path / PREFIX_PATH_FILE_NAME)); } - else if (write_created) - object_storage->removeObjectIfExists(StoredObject(metadata_object_key.serialize(), path / PREFIX_PATH_FILE_NAME)); } MetadataStorageFromPlainObjectStorageMoveDirectoryOperation::MetadataStorageFromPlainObjectStorageMoveDirectoryOperation( From 0ce808838255bd57423ef9f7e85720692eaad976 Mon Sep 17 00:00:00 2001 From: Julia Kartseva Date: Fri, 8 Nov 2024 06:29:00 +0000 Subject: [PATCH 2/4] address feedback Introduce removePathIfExists method. --- .../ObjectStorages/InMemoryDirectoryPathMap.h | 6 ++++ ...torageFromPlainObjectStorageOperations.cpp | 28 +++++++------------ 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/src/Disks/ObjectStorages/InMemoryDirectoryPathMap.h b/src/Disks/ObjectStorages/InMemoryDirectoryPathMap.h index 117cbad6203..d8e72d253f3 100644 --- a/src/Disks/ObjectStorages/InMemoryDirectoryPathMap.h +++ b/src/Disks/ObjectStorages/InMemoryDirectoryPathMap.h @@ -57,6 +57,12 @@ struct InMemoryDirectoryPathMap return it->second; } + bool removePathIfExists(const std::filesystem::path & path) + { + std::lock_guard lock(mutex); + return map.erase(path) != 0; + } + mutable SharedMutex mutex; #ifdef OS_LINUX diff --git a/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.cpp b/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.cpp index 3dac1df2ff8..3c7473b50c9 100644 --- a/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.cpp +++ b/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.cpp @@ -100,15 +100,10 @@ void MetadataStorageFromPlainObjectStorageCreateDirectoryOperation::undo(std::un if (write_finalized || write_created) { const auto base_path = path.parent_path(); - size_t erase_count = 0; - { - std::lock_guard lock(path_map.mutex); - erase_count = path_map.map.erase(base_path); - } - if (erase_count) + if (path_map.removePathIfExists(base_path)) { auto metric = object_storage->getMetadataStorageMetrics().directory_map_size; - CurrentMetrics::sub(metric, erase_count); + CurrentMetrics::sub(metric, 1); } object_storage->removeObjectIfExists(StoredObject(metadata_object_key.serialize(), path / PREFIX_PATH_FILE_NAME)); @@ -251,19 +246,16 @@ void MetadataStorageFromPlainObjectStorageRemoveDirectoryOperation::execute(std: auto metadata_object = StoredObject(/*remote_path*/ metadata_object_key.serialize(), /*local_path*/ path / PREFIX_PATH_FILE_NAME); object_storage->removeObjectIfExists(metadata_object); + if (path_map.removePathIfExists(base_path)) { - std::lock_guard lock(path_map.mutex); - auto & map = path_map.map; - map.erase(base_path); + removed = true; + + auto metric = object_storage->getMetadataStorageMetrics().directory_map_size; + CurrentMetrics::sub(metric, 1); + + auto event = object_storage->getMetadataStorageMetrics().directory_removed; + ProfileEvents::increment(event); } - - auto metric = object_storage->getMetadataStorageMetrics().directory_map_size; - CurrentMetrics::sub(metric, 1); - - removed = true; - - auto event = object_storage->getMetadataStorageMetrics().directory_removed; - ProfileEvents::increment(event); } void MetadataStorageFromPlainObjectStorageRemoveDirectoryOperation::undo(std::unique_lock &) From 4ac4098d2b3eda81ba7fd8cdeb2caf84bd3d7961 Mon Sep 17 00:00:00 2001 From: Julia Kartseva Date: Thu, 21 Nov 2024 01:44:23 +0000 Subject: [PATCH 3/4] add failpoint and test --- src/Common/FailPoint.cpp | 3 +- ...torageFromPlainObjectStorageOperations.cpp | 15 +++++-- ...aStorageFromPlainObjectStorageOperations.h | 1 - .../03008_s3_plain_rewritable_fault.reference | 1 + .../03008_s3_plain_rewritable_fault.sh | 43 +++++++++++++++++++ 5 files changed, 58 insertions(+), 5 deletions(-) create mode 100644 tests/queries/0_stateless/03008_s3_plain_rewritable_fault.reference create mode 100755 tests/queries/0_stateless/03008_s3_plain_rewritable_fault.sh diff --git a/src/Common/FailPoint.cpp b/src/Common/FailPoint.cpp index ef9e6bc96a9..a640523ac52 100644 --- a/src/Common/FailPoint.cpp +++ b/src/Common/FailPoint.cpp @@ -41,7 +41,7 @@ static struct InitFiu REGULAR(use_delayed_remote_source) \ REGULAR(cluster_discovery_faults) \ REGULAR(replicated_sends_failpoint) \ - REGULAR(stripe_log_sink_write_fallpoint)\ + REGULAR(stripe_log_sink_write_fallpoint) \ ONCE(smt_commit_merge_mutate_zk_fail_after_op) \ ONCE(smt_commit_merge_mutate_zk_fail_before_op) \ ONCE(smt_commit_write_zk_fail_after_op) \ @@ -77,6 +77,7 @@ static struct InitFiu REGULAR(replicated_merge_tree_all_replicas_stale) \ REGULAR(zero_copy_lock_zk_fail_before_op) \ REGULAR(zero_copy_lock_zk_fail_after_op) \ + REGULAR(plain_object_storage_write_fail_on_directory_create) \ namespace FailPoints diff --git a/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.cpp b/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.cpp index 3c7473b50c9..f2b9e5161b8 100644 --- a/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.cpp +++ b/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -18,8 +19,14 @@ namespace ErrorCodes extern const int FILE_DOESNT_EXIST; extern const int FILE_ALREADY_EXISTS; extern const int INCORRECT_DATA; +extern const int FAULT_INJECTED; }; +namespace FailPoints +{ +extern const char plain_object_storage_write_fail_on_directory_create[]; +} + namespace { @@ -72,8 +79,6 @@ void MetadataStorageFromPlainObjectStorageCreateDirectoryOperation::execute(std: /* buf_size */ DBMS_DEFAULT_BUFFER_SIZE, /* settings */ {}); - write_created = true; - { std::lock_guard lock(path_map.mutex); auto & map = path_map.map; @@ -85,6 +90,9 @@ void MetadataStorageFromPlainObjectStorageCreateDirectoryOperation::execute(std: CurrentMetrics::add(metric, 1); writeString(path.string(), *buf); + fiu_do_on(FailPoints::plain_object_storage_write_fail_on_directory_create, { + throw Exception(ErrorCodes::FAULT_INJECTED, "Injecting fault when creating '{}' directory", path); + }); buf->finalize(); write_finalized = true; @@ -97,8 +105,9 @@ void MetadataStorageFromPlainObjectStorageCreateDirectoryOperation::undo(std::un { auto metadata_object_key = createMetadataObjectKey(object_key_prefix, metadata_key_prefix); - if (write_finalized || write_created) + if (write_finalized) { + LOG_TRACE(getLogger("MetadataStorageFromPlainObjectStorageCreateDirectoryOperation"), "Undoing '{}' directory creation", path); const auto base_path = path.parent_path(); if (path_map.removePathIfExists(base_path)) { diff --git a/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.h b/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.h index 565d4429548..0a26dfed3e2 100644 --- a/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.h +++ b/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.h @@ -19,7 +19,6 @@ private: const std::string metadata_key_prefix; const std::string object_key_prefix; - bool write_created = false; bool write_finalized = false; public: diff --git a/tests/queries/0_stateless/03008_s3_plain_rewritable_fault.reference b/tests/queries/0_stateless/03008_s3_plain_rewritable_fault.reference new file mode 100644 index 00000000000..573541ac970 --- /dev/null +++ b/tests/queries/0_stateless/03008_s3_plain_rewritable_fault.reference @@ -0,0 +1 @@ +0 diff --git a/tests/queries/0_stateless/03008_s3_plain_rewritable_fault.sh b/tests/queries/0_stateless/03008_s3_plain_rewritable_fault.sh new file mode 100755 index 00000000000..274e7c148c2 --- /dev/null +++ b/tests/queries/0_stateless/03008_s3_plain_rewritable_fault.sh @@ -0,0 +1,43 @@ +#!/usr/bin/env bash +# Tags: no-fasttest, no-shared-merge-tree +# Tag no-fasttest: requires S3 +# Tag no-shared-merge-tree: does not support replication + +CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=../shell_config.sh +. "$CUR_DIR"/../shell_config.sh + +on_exit() { + ${CLICKHOUSE_CLIENT} --query " + SYSTEM DISABLE FAILPOINT plain_object_storage_write_fail_on_directory_create +" +} + +trap on_exit EXIT + +${CLICKHOUSE_CLIENT} --query "DROP TABLE IF EXISTS test_s3_mt_fault" + +${CLICKHOUSE_CLIENT} --query " +CREATE TABLE test_s3_mt_fault (a Int32, b Int64) engine = MergeTree() ORDER BY tuple(a, b) +SETTINGS disk = disk( + name = 03008_s3_plain_rewritable_fault, + type = s3_plain_rewritable, + endpoint = 'http://localhost:11111/test/03008_test_s3_mt_fault/', + access_key_id = clickhouse, + secret_access_key = clickhouse); +" + +${CLICKHOUSE_CLIENT} --query " +SYSTEM ENABLE FAILPOINT plain_object_storage_write_fail_on_directory_create +" + +${CLICKHOUSE_CLIENT} --query " +INSERT INTO test_s3_mt_fault (*) select number, number from numbers_mt(100)'" 2>&1 | grep -Fq "FAULT_INJECTED" + +${CLICKHOUSE_CLIENT} --query "SELECT count(*) from test_s3_mt_fault;" + +${CLICKHOUSE_CLIENT} --query "DROP TABLE test_s3_mt_fault;" + +${CLICKHOUSE_CLIENT} --query " +SYSTEM DISABLE FAILPOINT plain_object_storage_write_fail_on_directory_create +" From 318e9b4f33b3eb0424d6fcc5b435c4e4dd531ad4 Mon Sep 17 00:00:00 2001 From: Julia Kartseva Date: Thu, 21 Nov 2024 03:29:07 +0000 Subject: [PATCH 4/4] add plain_object_storage_write_fail_on_directory_move fault injection --- src/Common/FailPoint.cpp | 1 + ...torageFromPlainObjectStorageOperations.cpp | 8 ++--- ...aStorageFromPlainObjectStorageOperations.h | 1 - .../03008_s3_plain_rewritable_fault.reference | 13 ++++++- .../03008_s3_plain_rewritable_fault.sh | 34 ++++++++++++++----- 5 files changed, 43 insertions(+), 14 deletions(-) diff --git a/src/Common/FailPoint.cpp b/src/Common/FailPoint.cpp index a640523ac52..027cc347386 100644 --- a/src/Common/FailPoint.cpp +++ b/src/Common/FailPoint.cpp @@ -78,6 +78,7 @@ static struct InitFiu REGULAR(zero_copy_lock_zk_fail_before_op) \ REGULAR(zero_copy_lock_zk_fail_after_op) \ REGULAR(plain_object_storage_write_fail_on_directory_create) \ + REGULAR(plain_object_storage_write_fail_on_directory_move) \ namespace FailPoints diff --git a/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.cpp b/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.cpp index f2b9e5161b8..96832c4dfb1 100644 --- a/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.cpp +++ b/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.cpp @@ -25,6 +25,7 @@ extern const int FAULT_INJECTED; namespace FailPoints { extern const char plain_object_storage_write_fail_on_directory_create[]; +extern const char plain_object_storage_write_fail_on_directory_move[]; } namespace @@ -190,8 +191,10 @@ void MetadataStorageFromPlainObjectStorageMoveDirectoryOperation::execute(std::u getLogger("MetadataStorageFromPlainObjectStorageMoveDirectoryOperation"), "Moving directory '{}' to '{}'", path_from, path_to); auto write_buf = createWriteBuf(path_from, path_to, /* validate_content */ true); - write_created = true; writeString(path_to.string(), *write_buf); + fiu_do_on(FailPoints::plain_object_storage_write_fail_on_directory_move, { + throw Exception(ErrorCodes::FAULT_INJECTED, "Injecting fault when moving from '{}' to '{}'", path_from, path_to); + }); write_buf->finalize(); /// parent_path() removes the trailing '/'. @@ -216,10 +219,7 @@ void MetadataStorageFromPlainObjectStorageMoveDirectoryOperation::undo(std::uniq std::lock_guard lock(path_map.mutex); auto & map = path_map.map; map.emplace(path_from.parent_path(), map.extract(path_to.parent_path()).mapped()); - } - if (write_created) - { auto write_buf = createWriteBuf(path_to, path_from, /* verify_content */ false); writeString(path_from.string(), *write_buf); write_buf->finalize(); diff --git a/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.h b/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.h index 0a26dfed3e2..393fe21a4c3 100644 --- a/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.h +++ b/src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.h @@ -42,7 +42,6 @@ private: ObjectStoragePtr object_storage; const std::string metadata_key_prefix; - bool write_created = false; bool write_finalized = false; std::unique_ptr diff --git a/tests/queries/0_stateless/03008_s3_plain_rewritable_fault.reference b/tests/queries/0_stateless/03008_s3_plain_rewritable_fault.reference index 573541ac970..2f87357245f 100644 --- a/tests/queries/0_stateless/03008_s3_plain_rewritable_fault.reference +++ b/tests/queries/0_stateless/03008_s3_plain_rewritable_fault.reference @@ -1 +1,12 @@ -0 +1 2 +2 2 +3 1 +4 7 +5 10 +6 12 +1 2 +2 2 +3 1 +4 7 +5 10 +6 12 diff --git a/tests/queries/0_stateless/03008_s3_plain_rewritable_fault.sh b/tests/queries/0_stateless/03008_s3_plain_rewritable_fault.sh index 274e7c148c2..89ed24e2b43 100755 --- a/tests/queries/0_stateless/03008_s3_plain_rewritable_fault.sh +++ b/tests/queries/0_stateless/03008_s3_plain_rewritable_fault.sh @@ -1,15 +1,17 @@ #!/usr/bin/env bash -# Tags: no-fasttest, no-shared-merge-tree +# Tags: no-fasttest, no-shared-merge-tree, no-parallel # Tag no-fasttest: requires S3 # Tag no-shared-merge-tree: does not support replication +# Tag no-parallel: uses failpoints CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) # shellcheck source=../shell_config.sh . "$CUR_DIR"/../shell_config.sh on_exit() { - ${CLICKHOUSE_CLIENT} --query " - SYSTEM DISABLE FAILPOINT plain_object_storage_write_fail_on_directory_create + ${CLICKHOUSE_CLIENT} -m --query " + SYSTEM DISABLE FAILPOINT plain_object_storage_write_fail_on_directory_create; + SYSTEM DISABLE FAILPOINT plain_object_storage_write_fail_on_directory_move; " } @@ -27,17 +29,33 @@ SETTINGS disk = disk( secret_access_key = clickhouse); " +${CLICKHOUSE_CLIENT} --query " +INSERT INTO test_s3_mt_fault (*) VALUES (1, 2), (2, 2), (3, 1), (4, 7), (5, 10), (6, 12); +OPTIMIZE TABLE test_s3_mt_fault FINAL; +" + ${CLICKHOUSE_CLIENT} --query " SYSTEM ENABLE FAILPOINT plain_object_storage_write_fail_on_directory_create " ${CLICKHOUSE_CLIENT} --query " -INSERT INTO test_s3_mt_fault (*) select number, number from numbers_mt(100)'" 2>&1 | grep -Fq "FAULT_INJECTED" +INSERT INTO test_s3_mt_fault (*) select number, number from numbers_mt(100)" 2>&1 | grep -Fq "FAULT_INJECTED" -${CLICKHOUSE_CLIENT} --query "SELECT count(*) from test_s3_mt_fault;" - -${CLICKHOUSE_CLIENT} --query "DROP TABLE test_s3_mt_fault;" +${CLICKHOUSE_CLIENT} --query "SELECT * FROM test_s3_mt_fault;" ${CLICKHOUSE_CLIENT} --query " -SYSTEM DISABLE FAILPOINT plain_object_storage_write_fail_on_directory_create +SYSTEM DISABLE FAILPOINT plain_object_storage_write_fail_on_directory_create; +SYSTEM ENABLE FAILPOINT plain_object_storage_write_fail_on_directory_move; " + +${CLICKHOUSE_CLIENT} --query " +INSERT INTO test_s3_mt_fault (*) select number, number from numbers_mt(100); +" 2>&1 | grep -Fq "FAULT_INJECTED" + +${CLICKHOUSE_CLIENT} --query "SELECT * FROM test_s3_mt_fault;" + +${CLICKHOUSE_CLIENT} --query " +SYSTEM DISABLE FAILPOINT plain_object_storage_write_fail_on_directory_move; +" + +${CLICKHOUSE_CLIENT} --query "DROP TABLE test_s3_mt_fault"