Merge pull request #10587 from vitlibar/database-with-dictionary-init-in-constructor

Move initialization of DatabaseWithDictionaries to constructor.

src/Databases/DatabaseOrdinary.cpp

@@ -179,7 +179,6 @@ void DatabaseOrdinary::loadStoredObjects(
     startupTables(pool);
 
     /// Attach dictionaries.
-    attachToExternalDictionariesLoader(context);
     for (const auto & [name, query] : file_names)
     {
         auto create_query = query->as<const ASTCreateQuery &>();

src/Databases/DatabaseWithDictionaries.cpp

@@ -11,7 +11,7 @@
 #include <Storages/StorageDictionary.h>
 #include <IO/WriteBufferFromFile.h>
 #include <Poco/File.h>
-#include <ext/scope_guard.h>
+#include <boost/smart_ptr/make_shared_object.hpp>
 
 
 namespace CurrentStatusInfo
@@ -61,9 +61,9 @@ void DatabaseWithDictionaries::attachDictionary(const String & dictionary_name,
 
     CurrentStatusInfo::set(CurrentStatusInfo::DictionaryStatus, full_name, static_cast<Int8>(ExternalLoaderStatus::NOT_LOADED));
 
-    /// ExternalLoader::reloadConfig() will find out that the dictionary's config has been added
-    /// and in case `dictionaries_lazy_load == false` it will load the dictionary.
-    external_loader->reloadConfig(getDatabaseName(), full_name);
+    /// We want ExternalLoader::reloadConfig() to find out that the dictionary's config
+    /// has been added and in case `dictionaries_lazy_load == false` to load the dictionary.
+    reloadDictionaryConfig(full_name);
 }
 
 void DatabaseWithDictionaries::detachDictionary(const String & dictionary_name)
@@ -98,9 +98,9 @@ void DatabaseWithDictionaries::detachDictionaryImpl(const String & dictionary_na
 
     CurrentStatusInfo::unset(CurrentStatusInfo::DictionaryStatus, full_name);
 
-    /// ExternalLoader::reloadConfig() will find out that the dictionary's config has been removed
-    /// and therefore it will unload the dictionary.
-    external_loader->reloadConfig(getDatabaseName(), full_name);
+    /// We want ExternalLoader::reloadConfig() to find out that the dictionary's config
+    /// has been removed and to unload the dictionary.
+    reloadDictionaryConfig(full_name);
 }
 
 void DatabaseWithDictionaries::createDictionary(const Context & context, const String & dictionary_name, const ASTPtr & query)
@@ -122,7 +122,7 @@ void DatabaseWithDictionaries::createDictionary(const Context & context, const S
 
     /// A dictionary with the same full name could be defined in *.xml config files.
     String full_name = getDatabaseName() + "." + dictionary_name;
-    if (external_loader->getCurrentStatus(full_name) != ExternalLoader::Status::NOT_EXIST)
+    if (external_loader.getCurrentStatus(full_name) != ExternalLoader::Status::NOT_EXIST)
         throw Exception(
                 "Dictionary " + backQuote(getDatabaseName()) + "." + backQuote(dictionary_name) + " already exists.",
                 ErrorCodes::DICTIONARY_ALREADY_EXISTS);
@@ -153,7 +153,7 @@ void DatabaseWithDictionaries::createDictionary(const Context & context, const S
 
     /// Add a temporary repository containing the dictionary.
     /// We need this temp repository to try loading the dictionary before actually attaching it to the database.
-    auto temp_repository = external_loader->addConfigRepository(std::make_unique<ExternalLoaderTempConfigRepository>(
+    auto temp_repository = external_loader.addConfigRepository(std::make_unique<ExternalLoaderTempConfigRepository>(
         getDatabaseName(), dictionary_metadata_tmp_path, getDictionaryConfigurationFromAST(query->as<const ASTCreateQuery &>())));
 
     bool lazy_load = context.getConfigRef().getBool("dictionaries_lazy_load", true);
@@ -161,7 +161,7 @@ void DatabaseWithDictionaries::createDictionary(const Context & context, const S
     {
         /// load() is called here to force loading the dictionary, wait until the loading is finished,
         /// and throw an exception if the loading is failed.
-        external_loader->load(full_name);
+        external_loader.load(full_name);
     }
 
     auto config = getDictionaryConfigurationFromAST(query->as<const ASTCreateQuery &>());
@@ -176,8 +176,8 @@ void DatabaseWithDictionaries::createDictionary(const Context & context, const S
     Poco::File(dictionary_metadata_tmp_path).renameTo(dictionary_metadata_path);
 
     /// ExternalDictionariesLoader doesn't know we renamed the metadata path.
-    /// So we have to manually call reloadConfig() here.
-    external_loader->reloadConfig(getDatabaseName(), full_name);
+    /// That's why we have to call ExternalLoader::reloadConfig() here.
+    reloadDictionaryConfig(full_name);
 
     /// Everything's ok.
     succeeded = true;
@@ -291,28 +291,38 @@ bool DatabaseWithDictionaries::empty() const
     return tables.empty() && dictionaries.empty();
 }
 
+void DatabaseWithDictionaries::reloadDictionaryConfig(const String & full_name)
+{
+    /// Ensure that this database is attached to ExternalLoader as a config repository.
+    if (!database_as_config_repo_for_external_loader.load())
+        database_as_config_repo_for_external_loader = boost::make_shared<ext::scope_guard>(
+            external_loader.addConfigRepository(std::make_unique<ExternalLoaderDatabaseConfigRepository>(*this)));
+
+    external_loader.reloadConfig(getDatabaseName(), full_name);
+}
+
+
 void DatabaseWithDictionaries::shutdown()
 {
     {
         std::lock_guard lock(mutex);
         dictionaries.clear();
     }
-    detachFromExternalDictionariesLoader();
+
+    /// Invoke removing the database from ExternalLoader.
+    database_as_config_repo_for_external_loader = nullptr;
+
     DatabaseOnDisk::shutdown();
 }
 
+
+DatabaseWithDictionaries::DatabaseWithDictionaries(
+    const String & name, const String & metadata_path_, const String & data_path_, const String & logger, const Context & context)
+    : DatabaseOnDisk(name, metadata_path_, data_path_, logger, context)
+    , external_loader(context.getExternalDictionariesLoader())
+{
+}
+
 DatabaseWithDictionaries::~DatabaseWithDictionaries() = default;
 
-void DatabaseWithDictionaries::attachToExternalDictionariesLoader(Context & context)
-{
-    external_loader = &context.getExternalDictionariesLoader();
-    database_as_config_repo_for_external_loader
-        = external_loader->addConfigRepository(std::make_unique<ExternalLoaderDatabaseConfigRepository>(*this, context));
-}
-
-void DatabaseWithDictionaries::detachFromExternalDictionariesLoader()
-{
-    database_as_config_repo_for_external_loader = {};
-}
-
 }

src/Databases/DatabaseWithDictionaries.h

@@ -1,4 +1,5 @@
 #include <Databases/DatabaseOnDisk.h>
+#include <boost/smart_ptr/atomic_shared_ptr.hpp>
 #include <ext/scope_guard.h>
 
 namespace DB
@@ -33,22 +34,18 @@ public:
     ~DatabaseWithDictionaries() override;
 
 protected:
-    DatabaseWithDictionaries(const String & name, const String & metadata_path_, const String & data_path_, const String & logger, const Context & context)
-        : DatabaseOnDisk(name, metadata_path_, data_path_, logger, context) {}
+    DatabaseWithDictionaries(const String & name, const String & metadata_path_, const String & data_path_, const String & logger, const Context & context);
 
-    void attachToExternalDictionariesLoader(Context & context);
-    void detachFromExternalDictionariesLoader();
-
-    void detachDictionaryImpl(const String & dictionary_name, DictionaryAttachInfo & attach_info);
-
-    ASTPtr getCreateDictionaryQueryImpl(const String & dictionary_name,
-                                        bool throw_on_error) const override;
+    ASTPtr getCreateDictionaryQueryImpl(const String & dictionary_name, bool throw_on_error) const override;
 
     std::unordered_map<String, DictionaryAttachInfo> dictionaries;
 
 private:
-    ExternalDictionariesLoader * external_loader = nullptr;
-    ext::scope_guard database_as_config_repo_for_external_loader;
+    void detachDictionaryImpl(const String & dictionary_name, DictionaryAttachInfo & attach_info);
+    void reloadDictionaryConfig(const String & full_name);
+
+    const ExternalDictionariesLoader & external_loader;
+    boost::atomic_shared_ptr<ext::scope_guard> database_as_config_repo_for_external_loader;
 };
 
 }

src/Interpreters/ExternalLoader.cpp

@@ -1260,7 +1260,7 @@ ExternalLoader::ExternalLoader(const String & type_name_, Logger * log_)
 
 ExternalLoader::~ExternalLoader() = default;
 
-ext::scope_guard ExternalLoader::addConfigRepository(std::unique_ptr<IExternalLoaderConfigRepository> repository)
+ext::scope_guard ExternalLoader::addConfigRepository(std::unique_ptr<IExternalLoaderConfigRepository> repository) const
 {
     auto * ptr = repository.get();
     String name = ptr->getName();

src/Interpreters/ExternalLoader.h

@@ -86,7 +86,7 @@ public:
     virtual ~ExternalLoader();
 
     /// Adds a repository which will be used to read configurations from.
-    ext::scope_guard addConfigRepository(std::unique_ptr<IExternalLoaderConfigRepository> config_repository);
+    ext::scope_guard addConfigRepository(std::unique_ptr<IExternalLoaderConfigRepository> config_repository) const;
 
     void setConfigSettings(const ExternalLoaderConfigSettings & settings_);
 

src/Interpreters/ExternalLoaderDatabaseConfigRepository.cpp

@@ -12,49 +12,46 @@ namespace ErrorCodes
 
 namespace
 {
-    String trimDatabaseName(const std::string & loadable_definition_name, const IDatabase & database)
+    String trimDatabaseName(const std::string & loadable_definition_name, const String & database_name)
     {
-        const auto & dbname = database.getDatabaseName();
-        if (!startsWith(loadable_definition_name, dbname))
+        if (!startsWith(loadable_definition_name, database_name))
             throw Exception(
-                "Loadable '" + loadable_definition_name + "' is not from database '" + database.getDatabaseName(), ErrorCodes::UNKNOWN_DICTIONARY);
+                "Loadable '" + loadable_definition_name + "' is not from database '" + database_name, ErrorCodes::UNKNOWN_DICTIONARY);
         ///    dbname.loadable_name
         ///--> remove <---
-        return loadable_definition_name.substr(dbname.length() + 1);
+        return loadable_definition_name.substr(database_name.length() + 1);
     }
 }
 
 
-ExternalLoaderDatabaseConfigRepository::ExternalLoaderDatabaseConfigRepository(IDatabase & database_, const Context & context_)
-    : name(database_.getDatabaseName())
+ExternalLoaderDatabaseConfigRepository::ExternalLoaderDatabaseConfigRepository(IDatabase & database_)
+    : database_name(database_.getDatabaseName())
     , database(database_)
-    , context(context_)
 {
 }
 
 LoadablesConfigurationPtr ExternalLoaderDatabaseConfigRepository::load(const std::string & loadable_definition_name)
 {
-    return database.getDictionaryConfiguration(trimDatabaseName(loadable_definition_name, database));
+    return database.getDictionaryConfiguration(trimDatabaseName(loadable_definition_name, database_name));
 }
 
 bool ExternalLoaderDatabaseConfigRepository::exists(const std::string & loadable_definition_name)
 {
-    return database.isDictionaryExist(trimDatabaseName(loadable_definition_name, database));
+    return database.isDictionaryExist(trimDatabaseName(loadable_definition_name, database_name));
 }
 
 Poco::Timestamp ExternalLoaderDatabaseConfigRepository::getUpdateTime(const std::string & loadable_definition_name)
 {
-    return database.getObjectMetadataModificationTime(trimDatabaseName(loadable_definition_name, database));
+    return database.getObjectMetadataModificationTime(trimDatabaseName(loadable_definition_name, database_name));
 }
 
 std::set<std::string> ExternalLoaderDatabaseConfigRepository::getAllLoadablesDefinitionNames()
 {
     std::set<std::string> result;
-    const auto & dbname = database.getDatabaseName();
     auto itr = database.getDictionariesIterator();
     while (itr && itr->isValid())
     {
-        result.insert(dbname + "." + itr->name());
+        result.insert(database_name + "." + itr->name());
         itr->next();
     }
     return result;

src/Interpreters/ExternalLoaderDatabaseConfigRepository.h

@@ -2,7 +2,7 @@
 
 #include <Interpreters/IExternalLoaderConfigRepository.h>
 #include <Databases/IDatabase.h>
-#include <Interpreters/Context.h>
+
 
 namespace DB
 {
@@ -12,9 +12,9 @@ namespace DB
 class ExternalLoaderDatabaseConfigRepository : public IExternalLoaderConfigRepository
 {
 public:
-    ExternalLoaderDatabaseConfigRepository(IDatabase & database_, const Context & context_);
+    ExternalLoaderDatabaseConfigRepository(IDatabase & database_);
 
-    const std::string & getName() const override { return name; }
+    const std::string & getName() const override { return database_name; }
 
     std::set<std::string> getAllLoadablesDefinitionNames() override;
 
@@ -25,9 +25,8 @@ public:
     LoadablesConfigurationPtr load(const std::string & loadable_definition_name) override;
 
 private:
-    const String name;
+    const String database_name;
     IDatabase & database;
-    Context context;
 };
 
 }