Skip to content

Commit 2528e72

Browse files
author
Vitaly Baranov
authored
Merge pull request #10587 from vitlibar/database-with-dictionary-init-in-constructor
Move initialization of DatabaseWithDictionaries to constructor.
2 parents 4d8660f + dd34cd7 commit 2528e72

7 files changed

Lines changed: 56 additions & 54 deletions

‎src/Databases/DatabaseOrdinary.cpp‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,6 @@ void DatabaseOrdinary::loadStoredObjects(
179179
startupTables(pool);
180180

181181
/// Attach dictionaries.
182-
attachToExternalDictionariesLoader(context);
183182
for (const auto & [name, query] : file_names)
184183
{
185184
auto create_query = query->as<const ASTCreateQuery &>();

‎src/Databases/DatabaseWithDictionaries.cpp‎

Lines changed: 32 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
#include <Storages/StorageDictionary.h>
1212
#include <IO/WriteBufferFromFile.h>
1313
#include <Poco/File.h>
14-
#include <ext/scope_guard.h>
14+
#include <boost/smart_ptr/make_shared_object.hpp>
1515

1616

1717
namespace CurrentStatusInfo
@@ -61,9 +61,9 @@ void DatabaseWithDictionaries::attachDictionary(const String & dictionary_name,
6161

6262
CurrentStatusInfo::set(CurrentStatusInfo::DictionaryStatus, full_name, static_cast<Int8>(ExternalLoaderStatus::NOT_LOADED));
6363

64-
/// ExternalLoader::reloadConfig() will find out that the dictionary's config has been added
65-
/// and in case `dictionaries_lazy_load == false` it will load the dictionary.
66-
external_loader->reloadConfig(getDatabaseName(), full_name);
64+
/// We want ExternalLoader::reloadConfig() to find out that the dictionary's config
65+
/// has been added and in case `dictionaries_lazy_load == false` to load the dictionary.
66+
reloadDictionaryConfig(full_name);
6767
}
6868

6969
void DatabaseWithDictionaries::detachDictionary(const String & dictionary_name)
@@ -98,9 +98,9 @@ void DatabaseWithDictionaries::detachDictionaryImpl(const String & dictionary_na
9898

9999
CurrentStatusInfo::unset(CurrentStatusInfo::DictionaryStatus, full_name);
100100

101-
/// ExternalLoader::reloadConfig() will find out that the dictionary's config has been removed
102-
/// and therefore it will unload the dictionary.
103-
external_loader->reloadConfig(getDatabaseName(), full_name);
101+
/// We want ExternalLoader::reloadConfig() to find out that the dictionary's config
102+
/// has been removed and to unload the dictionary.
103+
reloadDictionaryConfig(full_name);
104104
}
105105

106106
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
122122

123123
/// A dictionary with the same full name could be defined in *.xml config files.
124124
String full_name = getDatabaseName() + "." + dictionary_name;
125-
if (external_loader->getCurrentStatus(full_name) != ExternalLoader::Status::NOT_EXIST)
125+
if (external_loader.getCurrentStatus(full_name) != ExternalLoader::Status::NOT_EXIST)
126126
throw Exception(
127127
"Dictionary " + backQuote(getDatabaseName()) + "." + backQuote(dictionary_name) + " already exists.",
128128
ErrorCodes::DICTIONARY_ALREADY_EXISTS);
@@ -153,15 +153,15 @@ void DatabaseWithDictionaries::createDictionary(const Context & context, const S
153153

154154
/// Add a temporary repository containing the dictionary.
155155
/// We need this temp repository to try loading the dictionary before actually attaching it to the database.
156-
auto temp_repository = external_loader->addConfigRepository(std::make_unique<ExternalLoaderTempConfigRepository>(
156+
auto temp_repository = external_loader.addConfigRepository(std::make_unique<ExternalLoaderTempConfigRepository>(
157157
getDatabaseName(), dictionary_metadata_tmp_path, getDictionaryConfigurationFromAST(query->as<const ASTCreateQuery &>())));
158158

159159
bool lazy_load = context.getConfigRef().getBool("dictionaries_lazy_load", true);
160160
if (!lazy_load)
161161
{
162162
/// load() is called here to force loading the dictionary, wait until the loading is finished,
163163
/// and throw an exception if the loading is failed.
164-
external_loader->load(full_name);
164+
external_loader.load(full_name);
165165
}
166166

167167
auto config = getDictionaryConfigurationFromAST(query->as<const ASTCreateQuery &>());
@@ -176,8 +176,8 @@ void DatabaseWithDictionaries::createDictionary(const Context & context, const S
176176
Poco::File(dictionary_metadata_tmp_path).renameTo(dictionary_metadata_path);
177177

178178
/// ExternalDictionariesLoader doesn't know we renamed the metadata path.
179-
/// So we have to manually call reloadConfig() here.
180-
external_loader->reloadConfig(getDatabaseName(), full_name);
179+
/// That's why we have to call ExternalLoader::reloadConfig() here.
180+
reloadDictionaryConfig(full_name);
181181

182182
/// Everything's ok.
183183
succeeded = true;
@@ -291,28 +291,38 @@ bool DatabaseWithDictionaries::empty() const
291291
return tables.empty() && dictionaries.empty();
292292
}
293293

294+
void DatabaseWithDictionaries::reloadDictionaryConfig(const String & full_name)
295+
{
296+
/// Ensure that this database is attached to ExternalLoader as a config repository.
297+
if (!database_as_config_repo_for_external_loader.load())
298+
database_as_config_repo_for_external_loader = boost::make_shared<ext::scope_guard>(
299+
external_loader.addConfigRepository(std::make_unique<ExternalLoaderDatabaseConfigRepository>(*this)));
300+
301+
external_loader.reloadConfig(getDatabaseName(), full_name);
302+
}
303+
304+
294305
void DatabaseWithDictionaries::shutdown()
295306
{
296307
{
297308
std::lock_guard lock(mutex);
298309
dictionaries.clear();
299310
}
300-
detachFromExternalDictionariesLoader();
311+
312+
/// Invoke removing the database from ExternalLoader.
313+
database_as_config_repo_for_external_loader = nullptr;
314+
301315
DatabaseOnDisk::shutdown();
302316
}
303317

304-
DatabaseWithDictionaries::~DatabaseWithDictionaries() = default;
305318

306-
void DatabaseWithDictionaries::attachToExternalDictionariesLoader(Context & context)
319+
DatabaseWithDictionaries::DatabaseWithDictionaries(
320+
const String & name, const String & metadata_path_, const String & data_path_, const String & logger, const Context & context)
321+
: DatabaseOnDisk(name, metadata_path_, data_path_, logger, context)
322+
, external_loader(context.getExternalDictionariesLoader())
307323
{
308-
external_loader = &context.getExternalDictionariesLoader();
309-
database_as_config_repo_for_external_loader
310-
= external_loader->addConfigRepository(std::make_unique<ExternalLoaderDatabaseConfigRepository>(*this, context));
311324
}
312325

313-
void DatabaseWithDictionaries::detachFromExternalDictionariesLoader()
314-
{
315-
database_as_config_repo_for_external_loader = {};
316-
}
326+
DatabaseWithDictionaries::~DatabaseWithDictionaries() = default;
317327

318328
}

‎src/Databases/DatabaseWithDictionaries.h‎

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#include <Databases/DatabaseOnDisk.h>
2+
#include <boost/smart_ptr/atomic_shared_ptr.hpp>
23
#include <ext/scope_guard.h>
34

45
namespace DB
@@ -33,22 +34,18 @@ class DatabaseWithDictionaries : public DatabaseOnDisk
3334
~DatabaseWithDictionaries() override;
3435

3536
protected:
36-
DatabaseWithDictionaries(const String & name, const String & metadata_path_, const String & data_path_, const String & logger, const Context & context)
37-
: DatabaseOnDisk(name, metadata_path_, data_path_, logger, context) {}
37+
DatabaseWithDictionaries(const String & name, const String & metadata_path_, const String & data_path_, const String & logger, const Context & context);
3838

39-
void attachToExternalDictionariesLoader(Context & context);
40-
void detachFromExternalDictionariesLoader();
41-
42-
void detachDictionaryImpl(const String & dictionary_name, DictionaryAttachInfo & attach_info);
43-
44-
ASTPtr getCreateDictionaryQueryImpl(const String & dictionary_name,
45-
bool throw_on_error) const override;
39+
ASTPtr getCreateDictionaryQueryImpl(const String & dictionary_name, bool throw_on_error) const override;
4640

4741
std::unordered_map<String, DictionaryAttachInfo> dictionaries;
4842

4943
private:
50-
ExternalDictionariesLoader * external_loader = nullptr;
51-
ext::scope_guard database_as_config_repo_for_external_loader;
44+
void detachDictionaryImpl(const String & dictionary_name, DictionaryAttachInfo & attach_info);
45+
void reloadDictionaryConfig(const String & full_name);
46+
47+
const ExternalDictionariesLoader & external_loader;
48+
boost::atomic_shared_ptr<ext::scope_guard> database_as_config_repo_for_external_loader;
5249
};
5350

5451
}

‎src/Interpreters/ExternalLoader.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1260,7 +1260,7 @@ ExternalLoader::ExternalLoader(const String & type_name_, Logger * log_)
12601260

12611261
ExternalLoader::~ExternalLoader() = default;
12621262

1263-
ext::scope_guard ExternalLoader::addConfigRepository(std::unique_ptr<IExternalLoaderConfigRepository> repository)
1263+
ext::scope_guard ExternalLoader::addConfigRepository(std::unique_ptr<IExternalLoaderConfigRepository> repository) const
12641264
{
12651265
auto * ptr = repository.get();
12661266
String name = ptr->getName();

‎src/Interpreters/ExternalLoader.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ class ExternalLoader
8686
virtual ~ExternalLoader();
8787

8888
/// Adds a repository which will be used to read configurations from.
89-
ext::scope_guard addConfigRepository(std::unique_ptr<IExternalLoaderConfigRepository> config_repository);
89+
ext::scope_guard addConfigRepository(std::unique_ptr<IExternalLoaderConfigRepository> config_repository) const;
9090

9191
void setConfigSettings(const ExternalLoaderConfigSettings & settings_);
9292

‎src/Interpreters/ExternalLoaderDatabaseConfigRepository.cpp‎

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,49 +12,46 @@ namespace ErrorCodes
1212

1313
namespace
1414
{
15-
String trimDatabaseName(const std::string & loadable_definition_name, const IDatabase & database)
15+
String trimDatabaseName(const std::string & loadable_definition_name, const String & database_name)
1616
{
17-
const auto & dbname = database.getDatabaseName();
18-
if (!startsWith(loadable_definition_name, dbname))
17+
if (!startsWith(loadable_definition_name, database_name))
1918
throw Exception(
20-
"Loadable '" + loadable_definition_name + "' is not from database '" + database.getDatabaseName(), ErrorCodes::UNKNOWN_DICTIONARY);
19+
"Loadable '" + loadable_definition_name + "' is not from database '" + database_name, ErrorCodes::UNKNOWN_DICTIONARY);
2120
/// dbname.loadable_name
2221
///--> remove <---
23-
return loadable_definition_name.substr(dbname.length() + 1);
22+
return loadable_definition_name.substr(database_name.length() + 1);
2423
}
2524
}
2625

2726

28-
ExternalLoaderDatabaseConfigRepository::ExternalLoaderDatabaseConfigRepository(IDatabase & database_, const Context & context_)
29-
: name(database_.getDatabaseName())
27+
ExternalLoaderDatabaseConfigRepository::ExternalLoaderDatabaseConfigRepository(IDatabase & database_)
28+
: database_name(database_.getDatabaseName())
3029
, database(database_)
31-
, context(context_)
3230
{
3331
}
3432

3533
LoadablesConfigurationPtr ExternalLoaderDatabaseConfigRepository::load(const std::string & loadable_definition_name)
3634
{
37-
return database.getDictionaryConfiguration(trimDatabaseName(loadable_definition_name, database));
35+
return database.getDictionaryConfiguration(trimDatabaseName(loadable_definition_name, database_name));
3836
}
3937

4038
bool ExternalLoaderDatabaseConfigRepository::exists(const std::string & loadable_definition_name)
4139
{
42-
return database.isDictionaryExist(trimDatabaseName(loadable_definition_name, database));
40+
return database.isDictionaryExist(trimDatabaseName(loadable_definition_name, database_name));
4341
}
4442

4543
Poco::Timestamp ExternalLoaderDatabaseConfigRepository::getUpdateTime(const std::string & loadable_definition_name)
4644
{
47-
return database.getObjectMetadataModificationTime(trimDatabaseName(loadable_definition_name, database));
45+
return database.getObjectMetadataModificationTime(trimDatabaseName(loadable_definition_name, database_name));
4846
}
4947

5048
std::set<std::string> ExternalLoaderDatabaseConfigRepository::getAllLoadablesDefinitionNames()
5149
{
5250
std::set<std::string> result;
53-
const auto & dbname = database.getDatabaseName();
5451
auto itr = database.getDictionariesIterator();
5552
while (itr && itr->isValid())
5653
{
57-
result.insert(dbname + "." + itr->name());
54+
result.insert(database_name + "." + itr->name());
5855
itr->next();
5956
}
6057
return result;

‎src/Interpreters/ExternalLoaderDatabaseConfigRepository.h‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
#include <Interpreters/IExternalLoaderConfigRepository.h>
44
#include <Databases/IDatabase.h>
5-
#include <Interpreters/Context.h>
5+
66

77
namespace DB
88
{
@@ -12,9 +12,9 @@ namespace DB
1212
class ExternalLoaderDatabaseConfigRepository : public IExternalLoaderConfigRepository
1313
{
1414
public:
15-
ExternalLoaderDatabaseConfigRepository(IDatabase & database_, const Context & context_);
15+
ExternalLoaderDatabaseConfigRepository(IDatabase & database_);
1616

17-
const std::string & getName() const override { return name; }
17+
const std::string & getName() const override { return database_name; }
1818

1919
std::set<std::string> getAllLoadablesDefinitionNames() override;
2020

@@ -25,9 +25,8 @@ class ExternalLoaderDatabaseConfigRepository : public IExternalLoaderConfigRepos
2525
LoadablesConfigurationPtr load(const std::string & loadable_definition_name) override;
2626

2727
private:
28-
const String name;
28+
const String database_name;
2929
IDatabase & database;
30-
Context context;
3130
};
3231

3332
}

0 commit comments

Comments
 (0)