From c036af0261b972cbb6f3bf6585d7672dca778816 Mon Sep 17 00:00:00 2001 From: alesapin Date: Thu, 21 May 2020 17:11:56 +0300 Subject: [PATCH 1/4] Fix deadlock after clickhouse-server update (with changes in one of system log tables structure) during startup between concurrent merge and table rename. --- src/Interpreters/Context.cpp | 56 +++++++++++-------- src/Interpreters/Context.h | 10 ++-- src/Interpreters/SystemLog.h | 4 +- src/Storages/MergeTree/MergeTreeData.cpp | 2 +- src/Storages/StorageDistributed.cpp | 4 +- src/Storages/System/StorageSystemDisks.cpp | 4 +- .../System/StorageSystemStoragePolicies.cpp | 4 +- 7 files changed, 47 insertions(+), 37 deletions(-) diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 6739aab134f..3b285e77305 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -295,6 +295,10 @@ struct ContextShared mutable std::mutex embedded_dictionaries_mutex; mutable std::mutex external_dictionaries_mutex; mutable std::mutex external_models_mutex; + /// Separate mutex for storage policies. During server startup we may + /// initialize some important storages (system tables with MergeTree engine) + /// under context lock. + mutable std::mutex storage_policies_mutex; /// Separate mutex for re-initialization of zookeeper session. This operation could take a long time and must not interfere with another operations. mutable std::mutex zookeeper_mutex; @@ -567,7 +571,7 @@ void Context::setPath(const String & path) VolumeJBODPtr Context::setTemporaryStorage(const String & path, const String & policy_name) { - auto lock = getLock(); + std::lock_guard lock(shared->storage_policies_mutex); if (policy_name.empty()) { @@ -580,7 +584,7 @@ VolumeJBODPtr Context::setTemporaryStorage(const String & path, const String & p } else { - StoragePolicyPtr tmp_policy = getStoragePolicySelector()->get(policy_name); + StoragePolicyPtr tmp_policy = getStoragePolicySelector(lock)->get(policy_name); if (tmp_policy->getVolumes().size() != 1) throw Exception("Policy " + policy_name + " is used temporary files, such policy should have exactly one volume", ErrorCodes::NO_ELEMENTS_IN_CONFIG); shared->tmp_volume = tmp_policy->getVolume(0); @@ -1688,18 +1692,37 @@ CompressionCodecPtr Context::chooseCompressionCodec(size_t part_size, double par DiskPtr Context::getDisk(const String & name) const { - auto lock = getLock(); + std::lock_guard lock(shared->storage_policies_mutex); - auto disk_selector = getDiskSelector(); + auto disk_selector = getDiskSelector(lock); return disk_selector->get(name); } - -DiskSelectorPtr Context::getDiskSelector() const +StoragePolicyPtr Context::getStoragePolicy(const String & name) const { - auto lock = getLock(); + std::lock_guard lock(shared->storage_policies_mutex); + auto policy_selector = getStoragePolicySelector(lock); + + return policy_selector->get(name); +} + + +const std::map & Context::getDisksMap() const +{ + std::lock_guard lock(shared->storage_policies_mutex); + return getDiskSelector(lock)->getDisksMap(); +} + +const std::map & Context::getPoliciesMap() const +{ + std::lock_guard lock(shared->storage_policies_mutex); + return getStoragePolicySelector(lock)->getPoliciesMap(); +} + +DiskSelectorPtr Context::getDiskSelector(std::lock_guard & /* lock */) const +{ if (!shared->merge_tree_disk_selector) { constexpr auto config_name = "storage_configuration.disks"; @@ -1710,27 +1733,14 @@ DiskSelectorPtr Context::getDiskSelector() const return shared->merge_tree_disk_selector; } - -StoragePolicyPtr Context::getStoragePolicy(const String & name) const +StoragePolicySelectorPtr Context::getStoragePolicySelector(std::lock_guard & lock) const { - auto lock = getLock(); - - auto policy_selector = getStoragePolicySelector(); - - return policy_selector->get(name); -} - - -StoragePolicySelectorPtr Context::getStoragePolicySelector() const -{ - auto lock = getLock(); - if (!shared->merge_tree_storage_policy_selector) { constexpr auto config_name = "storage_configuration.policies"; const auto & config = getConfigRef(); - shared->merge_tree_storage_policy_selector = std::make_shared(config, config_name, getDiskSelector()); + shared->merge_tree_storage_policy_selector = std::make_shared(config, config_name, getDiskSelector(lock)); } return shared->merge_tree_storage_policy_selector; } @@ -1738,7 +1748,7 @@ StoragePolicySelectorPtr Context::getStoragePolicySelector() const void Context::updateStorageConfiguration(const Poco::Util::AbstractConfiguration & config) { - auto lock = getLock(); + std::lock_guard lock(shared->storage_policies_mutex); if (shared->merge_tree_disk_selector) shared->merge_tree_disk_selector = shared->merge_tree_disk_selector->updateFromConfig(config, "storage_configuration.disks", *this); diff --git a/src/Interpreters/Context.h b/src/Interpreters/Context.h index 4ba135d0b1a..65c9ed6749f 100644 --- a/src/Interpreters/Context.h +++ b/src/Interpreters/Context.h @@ -541,14 +541,12 @@ public: /// Lets you select the compression codec according to the conditions described in the configuration file. std::shared_ptr chooseCompressionCodec(size_t part_size, double part_size_ratio) const; - DiskSelectorPtr getDiskSelector() const; /// Provides storage disks DiskPtr getDisk(const String & name) const; - DiskPtr getDefaultDisk() const { return getDisk("default"); } - - StoragePolicySelectorPtr getStoragePolicySelector() const; + const std::map & getPoliciesMap() const; + const std::map & getDisksMap() const; void updateStorageConfiguration(const Poco::Util::AbstractConfiguration & config); /// Provides storage politics schemes @@ -626,6 +624,10 @@ private: EmbeddedDictionaries & getEmbeddedDictionariesImpl(bool throw_on_error) const; void checkCanBeDropped(const String & database, const String & table, const size_t & size, const size_t & max_size_to_drop) const; + + StoragePolicySelectorPtr getStoragePolicySelector(std::lock_guard & lock) const; + + DiskSelectorPtr getDiskSelector(std::lock_guard & /* lock */) const; }; diff --git a/src/Interpreters/SystemLog.h b/src/Interpreters/SystemLog.h index b2a4eec7883..f9b1154f442 100644 --- a/src/Interpreters/SystemLog.h +++ b/src/Interpreters/SystemLog.h @@ -199,8 +199,8 @@ SystemLog::SystemLog(Context & context_, size_t flush_interval_milliseconds_) : context(context_) , table_id(database_name_, table_name_) - , storage_def(storage_def_), - flush_interval_milliseconds(flush_interval_milliseconds_) + , storage_def(storage_def_) + , flush_interval_milliseconds(flush_interval_milliseconds_) { assert(database_name_ == DatabaseCatalog::SYSTEM_DATABASE); log = &Logger::get("SystemLog (" + database_name_ + "." + table_name_ + ")"); diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index de9d3f6e981..64c95b8b8a5 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -866,7 +866,7 @@ void MergeTreeData::loadDataParts(bool skip_sanity_checks) for (const auto & disk_ptr : disks) defined_disk_names.insert(disk_ptr->getName()); - for (const auto & [disk_name, disk] : global_context.getDiskSelector()->getDisksMap()) + for (const auto & [disk_name, disk] : global_context.getDisksMap()) { if (defined_disk_names.count(disk_name) == 0 && disk->exists(relative_data_path)) { diff --git a/src/Storages/StorageDistributed.cpp b/src/Storages/StorageDistributed.cpp index eb75f53ef9c..8d85b5a1db2 100644 --- a/src/Storages/StorageDistributed.cpp +++ b/src/Storages/StorageDistributed.cpp @@ -334,7 +334,7 @@ void StorageDistributed::createStorage() } else { - auto policy = global_context.getStoragePolicySelector()->get(storage_policy); + auto policy = global_context.getStoragePolicy(storage_policy); if (policy->getVolumes().size() != 1) throw Exception("Policy for Distributed table, should have exactly one volume", ErrorCodes::BAD_ARGUMENTS); volume = policy->getVolume(0); @@ -628,7 +628,7 @@ StoragePolicyPtr StorageDistributed::getStoragePolicy() const { if (storage_policy.empty()) return {}; - return global_context.getStoragePolicySelector()->get(storage_policy); + return global_context.getStoragePolicy(storage_policy); } void StorageDistributed::createDirectoryMonitors(const std::string & disk) diff --git a/src/Storages/System/StorageSystemDisks.cpp b/src/Storages/System/StorageSystemDisks.cpp index 5ddf7de9ec8..cadaaff6cbc 100644 --- a/src/Storages/System/StorageSystemDisks.cpp +++ b/src/Storages/System/StorageSystemDisks.cpp @@ -39,9 +39,7 @@ Pipes StorageSystemDisks::read( MutableColumnPtr col_total = ColumnUInt64::create(); MutableColumnPtr col_keep = ColumnUInt64::create(); - const auto & disk_selector = context.getDiskSelector(); - - for (const auto & [disk_name, disk_ptr] : disk_selector->getDisksMap()) + for (const auto & [disk_name, disk_ptr] : context.getDisksMap()) { col_name->insert(disk_name); col_path->insert(disk_ptr->getPath()); diff --git a/src/Storages/System/StorageSystemStoragePolicies.cpp b/src/Storages/System/StorageSystemStoragePolicies.cpp index 81b6ddd465a..474d2b5bcc4 100644 --- a/src/Storages/System/StorageSystemStoragePolicies.cpp +++ b/src/Storages/System/StorageSystemStoragePolicies.cpp @@ -44,9 +44,9 @@ Pipes StorageSystemStoragePolicies::read( MutableColumnPtr col_max_part_size = ColumnUInt64::create(); MutableColumnPtr col_move_factor = ColumnFloat32::create(); - const auto & policy_selector = context.getStoragePolicySelector(); + const auto & policies_map = context.getPoliciesMap(); - for (const auto & [policy_name, policy_ptr] : policy_selector->getPoliciesMap()) + for (const auto & [policy_name, policy_ptr] : policies_map) { const auto & volumes = policy_ptr->getVolumes(); for (size_t i = 0; i != volumes.size(); ++i) From 37fa26971e0bf4da290cf7542d8e54efcdbcb653 Mon Sep 17 00:00:00 2001 From: alesapin Date: Thu, 21 May 2020 21:49:31 +0300 Subject: [PATCH 2/4] boop ci --- src/Interpreters/Context.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 3b285e77305..e099721acdc 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -296,7 +296,7 @@ struct ContextShared mutable std::mutex external_dictionaries_mutex; mutable std::mutex external_models_mutex; /// Separate mutex for storage policies. During server startup we may - /// initialize some important storages (system tables with MergeTree engine) + /// initialize some important storages (system logs with MergeTree engine) /// under context lock. mutable std::mutex storage_policies_mutex; /// Separate mutex for re-initialization of zookeeper session. This operation could take a long time and must not interfere with another operations. From 98ffefd90c161b8073e019546d84a607b9c46950 Mon Sep 17 00:00:00 2001 From: alesapin Date: Fri, 22 May 2020 13:09:24 +0300 Subject: [PATCH 3/4] Fix bug with return type --- src/Interpreters/Context.cpp | 4 ++-- src/Interpreters/Context.h | 4 ++-- src/Storages/System/StorageSystemStoragePolicies.cpp | 4 +--- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index e099721acdc..f6aa2890816 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -1709,13 +1709,13 @@ StoragePolicyPtr Context::getStoragePolicy(const String & name) const } -const std::map & Context::getDisksMap() const +std::map Context::getDisksMap() const { std::lock_guard lock(shared->storage_policies_mutex); return getDiskSelector(lock)->getDisksMap(); } -const std::map & Context::getPoliciesMap() const +std::map Context::getPoliciesMap() const { std::lock_guard lock(shared->storage_policies_mutex); return getStoragePolicySelector(lock)->getPoliciesMap(); diff --git a/src/Interpreters/Context.h b/src/Interpreters/Context.h index 65c9ed6749f..1f59648a525 100644 --- a/src/Interpreters/Context.h +++ b/src/Interpreters/Context.h @@ -545,8 +545,8 @@ public: /// Provides storage disks DiskPtr getDisk(const String & name) const; - const std::map & getPoliciesMap() const; - const std::map & getDisksMap() const; + std::map getPoliciesMap() const; + std::map getDisksMap() const; void updateStorageConfiguration(const Poco::Util::AbstractConfiguration & config); /// Provides storage politics schemes diff --git a/src/Storages/System/StorageSystemStoragePolicies.cpp b/src/Storages/System/StorageSystemStoragePolicies.cpp index c4f43b0a5bf..acbc9d72a20 100644 --- a/src/Storages/System/StorageSystemStoragePolicies.cpp +++ b/src/Storages/System/StorageSystemStoragePolicies.cpp @@ -45,9 +45,7 @@ Pipes StorageSystemStoragePolicies::read( MutableColumnPtr col_max_part_size = ColumnUInt64::create(); MutableColumnPtr col_move_factor = ColumnFloat32::create(); - const auto & policies_map = context.getPoliciesMap(); - - for (const auto & [policy_name, policy_ptr] : policies_map) + for (const auto & [policy_name, policy_ptr] : context.getPoliciesMap()) { const auto & volumes = policy_ptr->getVolumes(); for (size_t i = 0; i != volumes.size(); ++i) From 70fe1194261ea839cc333019e12722170a12e64f Mon Sep 17 00:00:00 2001 From: alesapin Date: Fri, 22 May 2020 13:33:57 +0300 Subject: [PATCH 4/4] Add clear names --- src/Disks/DiskSelector.h | 6 ++++-- src/Disks/StoragePolicy.h | 5 +++-- src/Interpreters/Context.cpp | 4 ++-- src/Interpreters/Context.h | 6 ++++-- 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/Disks/DiskSelector.h b/src/Disks/DiskSelector.h index 8ae8de3be61..3f19dfba381 100644 --- a/src/Disks/DiskSelector.h +++ b/src/Disks/DiskSelector.h @@ -9,9 +9,11 @@ namespace DB { + class Context; class DiskSelector; using DiskSelectorPtr = std::shared_ptr; +using DisksMap = std::map; /// Parse .xml configuration and store information about disks /// Mostly used for introspection. @@ -28,14 +30,14 @@ public: DiskPtr get(const String & name) const; /// Get all disks with names - const auto & getDisksMap() const { return disks; } + const DisksMap & getDisksMap() const { return disks; } void addToDiskMap(String name, DiskPtr disk) { disks.emplace(name, disk); } private: - std::map disks; + DisksMap disks; }; } diff --git a/src/Disks/StoragePolicy.h b/src/Disks/StoragePolicy.h index 00ac2c2c9bb..a8cba9abaeb 100644 --- a/src/Disks/StoragePolicy.h +++ b/src/Disks/StoragePolicy.h @@ -99,6 +99,7 @@ private: class StoragePolicySelector; using StoragePolicySelectorPtr = std::shared_ptr; +using StoragePoliciesMap = std::map; /// Parse .xml configuration and store information about policies /// Mostly used for introspection. @@ -113,10 +114,10 @@ public: StoragePolicyPtr get(const String & name) const; /// All policies - const std::map & getPoliciesMap() const { return policies; } + const StoragePoliciesMap & getPoliciesMap() const { return policies; } private: - std::map policies; + StoragePoliciesMap policies; }; } diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index f6aa2890816..45eeb47c035 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -1709,13 +1709,13 @@ StoragePolicyPtr Context::getStoragePolicy(const String & name) const } -std::map Context::getDisksMap() const +DisksMap Context::getDisksMap() const { std::lock_guard lock(shared->storage_policies_mutex); return getDiskSelector(lock)->getDisksMap(); } -std::map Context::getPoliciesMap() const +StoragePoliciesMap Context::getPoliciesMap() const { std::lock_guard lock(shared->storage_policies_mutex); return getStoragePolicySelector(lock)->getPoliciesMap(); diff --git a/src/Interpreters/Context.h b/src/Interpreters/Context.h index 1f59648a525..864468c0663 100644 --- a/src/Interpreters/Context.h +++ b/src/Interpreters/Context.h @@ -97,8 +97,10 @@ class IDisk; using DiskPtr = std::shared_ptr; class DiskSelector; using DiskSelectorPtr = std::shared_ptr; +using DisksMap = std::map; class StoragePolicy; using StoragePolicyPtr = std::shared_ptr; +using StoragePoliciesMap = std::map; class StoragePolicySelector; using StoragePolicySelectorPtr = std::shared_ptr; @@ -545,8 +547,8 @@ public: /// Provides storage disks DiskPtr getDisk(const String & name) const; - std::map getPoliciesMap() const; - std::map getDisksMap() const; + StoragePoliciesMap getPoliciesMap() const; + DisksMap getDisksMap() const; void updateStorageConfiguration(const Poco::Util::AbstractConfiguration & config); /// Provides storage politics schemes