From dd34cd73e217d94360fe374055b6f6912ed5501a Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Thu, 30 Apr 2020 01:38:22 +0300 Subject: [PATCH] Move initialization of DatabaseWithDictionaries to constructor. --- src/Databases/DatabaseOrdinary.cpp | 1 - src/Databases/DatabaseWithDictionaries.cpp | 60 +++++++++++-------- src/Databases/DatabaseWithDictionaries.h | 19 +++--- src/Interpreters/ExternalLoader.cpp | 2 +- src/Interpreters/ExternalLoader.h | 2 +- ...ExternalLoaderDatabaseConfigRepository.cpp | 23 ++++--- .../ExternalLoaderDatabaseConfigRepository.h | 9 ++- 7 files changed, 59 insertions(+), 57 deletions(-) diff --git a/src/Databases/DatabaseOrdinary.cpp b/src/Databases/DatabaseOrdinary.cpp index f424a114e4c..352b9db2672 100644 --- a/src/Databases/DatabaseOrdinary.cpp +++ b/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(); diff --git a/src/Databases/DatabaseWithDictionaries.cpp b/src/Databases/DatabaseWithDictionaries.cpp index 2ca4a7b3395..0d49078bd08 100644 --- a/src/Databases/DatabaseWithDictionaries.cpp +++ b/src/Databases/DatabaseWithDictionaries.cpp @@ -11,7 +11,7 @@ #include #include #include -#include +#include namespace CurrentStatusInfo @@ -61,9 +61,9 @@ void DatabaseWithDictionaries::attachDictionary(const String & dictionary_name, CurrentStatusInfo::set(CurrentStatusInfo::DictionaryStatus, full_name, static_cast(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( + auto temp_repository = external_loader.addConfigRepository(std::make_unique( getDatabaseName(), dictionary_metadata_tmp_path, getDictionaryConfigurationFromAST(query->as()))); 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()); @@ -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( + external_loader.addConfigRepository(std::make_unique(*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(*this, context)); -} - -void DatabaseWithDictionaries::detachFromExternalDictionariesLoader() -{ - database_as_config_repo_for_external_loader = {}; -} - } diff --git a/src/Databases/DatabaseWithDictionaries.h b/src/Databases/DatabaseWithDictionaries.h index 48ec92aa3d0..a02b68a56da 100644 --- a/src/Databases/DatabaseWithDictionaries.h +++ b/src/Databases/DatabaseWithDictionaries.h @@ -1,4 +1,5 @@ #include +#include #include 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 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 database_as_config_repo_for_external_loader; }; } diff --git a/src/Interpreters/ExternalLoader.cpp b/src/Interpreters/ExternalLoader.cpp index ccea5262213..acde799de6a 100644 --- a/src/Interpreters/ExternalLoader.cpp +++ b/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 repository) +ext::scope_guard ExternalLoader::addConfigRepository(std::unique_ptr repository) const { auto * ptr = repository.get(); String name = ptr->getName(); diff --git a/src/Interpreters/ExternalLoader.h b/src/Interpreters/ExternalLoader.h index bcf01eb6625..9f9fa97b156 100644 --- a/src/Interpreters/ExternalLoader.h +++ b/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 config_repository); + ext::scope_guard addConfigRepository(std::unique_ptr config_repository) const; void setConfigSettings(const ExternalLoaderConfigSettings & settings_); diff --git a/src/Interpreters/ExternalLoaderDatabaseConfigRepository.cpp b/src/Interpreters/ExternalLoaderDatabaseConfigRepository.cpp index f850762e962..5f8f6f7c431 100644 --- a/src/Interpreters/ExternalLoaderDatabaseConfigRepository.cpp +++ b/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 ExternalLoaderDatabaseConfigRepository::getAllLoadablesDefinitionNames() { std::set 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; diff --git a/src/Interpreters/ExternalLoaderDatabaseConfigRepository.h b/src/Interpreters/ExternalLoaderDatabaseConfigRepository.h index 2afff035d9d..d800db7a8e1 100644 --- a/src/Interpreters/ExternalLoaderDatabaseConfigRepository.h +++ b/src/Interpreters/ExternalLoaderDatabaseConfigRepository.h @@ -2,7 +2,7 @@ #include #include -#include + 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 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; }; }