Update dictionary's comment in the same safe manner too.

ClickHouse · Mar 17, 2024 · 0620d8d · 0620d8d
1 parent 4747af5
commit 0620d8d
Show file tree

Hide file tree

Showing 4 changed files with 27 additions and 24 deletions.
diff --git a/src/Dictionaries/DictionaryFactory.cpp b/src/Dictionaries/DictionaryFactory.cpp
@@ -55,11 +55,7 @@ DictionaryPtr DictionaryFactory::create(
         if (found != registered_layouts.end())
         {
             const auto & layout_creator = found->second.layout_create_function;
-            auto result = layout_creator(name, dict_struct, config, config_prefix, std::move(source_ptr), global_context, created_from_ddl);
-            if (config.hasProperty(config_prefix + ".comment"))
-                result->setDictionaryComment(config.getString(config_prefix + ".comment"));
-
-            return result;
+            return layout_creator(name, dict_struct, config, config_prefix, std::move(source_ptr), global_context, created_from_ddl);
         }
     }
 

diff --git a/src/Dictionaries/IDictionary.h b/src/Dictionaries/IDictionary.h
@@ -332,12 +332,6 @@ class IDictionary : public IExternalLoadable, public IKeyValueEntity
         return std::static_pointer_cast<const IDictionary>(IExternalLoadable::shared_from_this());
     }
 
-    void setDictionaryComment(String new_comment)
-    {
-        std::lock_guard lock{mutex};
-        dictionary_comment = std::move(new_comment);
-    }
-
     String getDictionaryComment() const
     {
         std::lock_guard lock{mutex};
@@ -454,6 +448,14 @@ class IDictionary : public IExternalLoadable, public IKeyValueEntity
         dictionary_id = new_dictionary_id;
     }
 
+    /// Internally called by ExternalDictionariesLoader.
+    /// In order to update the dictionary comment change its configuration first and then call ExternalDictionariesLoader::reloadConfig().
+    void updateDictionaryComment(const String & new_dictionary_comment)
+    {
+        std::lock_guard lock{mutex};
+        dictionary_comment = new_dictionary_comment;
+    }
+
 private:
     mutable std::mutex mutex;
     StorageID dictionary_id TSA_GUARDED_BY(mutex);

diff --git a/src/Interpreters/ExternalDictionariesLoader.cpp b/src/Interpreters/ExternalDictionariesLoader.cpp
@@ -45,14 +45,17 @@ ExternalLoader::LoadablePtr ExternalDictionariesLoader::create(
 bool ExternalDictionariesLoader::doesConfigChangeRequiresReloadingObject(const Poco::Util::AbstractConfiguration & old_config, const String & old_key_in_config,
                                                                          const Poco::Util::AbstractConfiguration & new_config, const String & new_key_in_config) const
 {
+    std::unordered_set<std::string_view> ignore_keys;
+    ignore_keys.insert("comment"); /// We always can change the comment without reloading a dictionary.
+
     /// If the database is atomic then a dictionary can be renamed without reloading.
     if (!old_config.getString(old_key_in_config + ".uuid", "").empty() && !new_config.getString(new_key_in_config + ".uuid", "").empty())
     {
-        static const std::unordered_set<std::string_view> ignore_keys{"name", "database"};
-        bool only_name_or_database_may_differ = isSameConfigurationIgnoringKeys(old_config, old_key_in_config, new_config, new_key_in_config, ignore_keys);
-        return !only_name_or_database_may_differ;
+        ignore_keys.insert("name");
+        ignore_keys.insert("database");
     }
-    return true;
+
+    return isSameConfigurationIgnoringKeys(old_config, old_key_in_config, new_config, new_key_in_config, ignore_keys);
 }
 
 void ExternalDictionariesLoader::updateObjectFromConfigWithoutReloading(IExternalLoadable & object, const Poco::Util::AbstractConfiguration & config, const String & key_in_config) const
@@ -67,6 +70,8 @@ void ExternalDictionariesLoader::updateObjectFromConfigWithoutReloading(IExterna
         if ((new_dictionary_id.uuid == old_dictionary_id.uuid) && (new_dictionary_id.uuid != UUIDHelpers::Nil))
             dict.updateDictionaryID(new_dictionary_id);
     }
+
+    dict.updateDictionaryComment(config.getString("dictionary.comment", ""));
 }
 
 ExternalDictionariesLoader::DictPtr ExternalDictionariesLoader::getDictionary(const std::string & dictionary_name, ContextPtr local_context) const

diff --git a/src/Storages/StorageDictionary.cpp b/src/Storages/StorageDictionary.cpp
@@ -288,19 +288,19 @@ void StorageDictionary::alter(const AlterCommands & params, ContextPtr alter_con
 
     auto new_comment = getInMemoryMetadataPtr()->comment;
 
-    auto storage_id = getStorageID();
-    const auto & external_dictionaries_loader = getContext()->getExternalDictionariesLoader();
-    auto result = external_dictionaries_loader.getLoadResult(storage_id.getInternalDictionaryName());
+    /// It's better not to update an associated `IDictionary` directly here because it can be not loaded yet or
+    /// it can be in the process of loading or reloading right now.
+    /// The correct way is to update the dictionary's configuration first and then ask ExternalDictionariesLoader to reload our dictionary.
 
-    if (result.object)
     {
-        auto dictionary = std::static_pointer_cast<const IDictionary>(result.object);
-        auto * dictionary_non_const = const_cast<IDictionary *>(dictionary.get());
-        dictionary_non_const->setDictionaryComment(new_comment);
+        std::lock_guard lock(dictionary_config_mutex);
+        auto new_configuration = makeNewConfiguration();
+        new_configuration->setString("dictionary.comment", new_comment);
+        configuration = new_configuration;
     }
 
-    std::lock_guard lock(dictionary_config_mutex);
-    configuration->setString("dictionary.comment", new_comment);
+    const auto & external_dictionaries_loader = getContext()->getExternalDictionariesLoader();
+    external_dictionaries_loader.reloadConfig(getStorageID().getInternalDictionaryName());
 }
 
 void registerStorageDictionary(StorageFactory & factory)