-
Notifications
You must be signed in to change notification settings - Fork 6.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not take lock for shared context in setTempDataOnDisk #48219
Conversation
src/Interpreters/Context.cpp
Outdated
void Context::setTempDataOnDisk(TemporaryDataOnDiskScopePtr temp_data_on_disk_) | ||
{ | ||
auto lock = getLock(); | ||
/// It's set from `ProcessList::insert` in `executeQueryImpl` before query execution | ||
/// so no races with `getTempDataOnDisk` which is called from query execution. | ||
this->temp_data_on_disk = std::move(temp_data_on_disk_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about other functions that use temp_data_on_disk
? For example
ClickHouse/src/Interpreters/Context.cpp
Lines 739 to 743 in 714b54b
/// TODO: remove, use `getTempDataOnDisk` | |
VolumePtr Context::getTemporaryVolume() const | |
{ | |
auto lock = getLock(); | |
if (shared->temp_data_on_disk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, we have two different temp_data_on_disk
one in shared part of context and one in current 🤔 I'll check why do we need two and when it was introduced. Probably shared is global and in current context is for query execution. Global one is set on server startup
ClickHouse/programs/server/Server.cpp
Lines 1048 to 1060 in ceef48a
if (!server_settings.tmp_policy.value.empty()) | |
{ | |
global_context->setTemporaryStoragePolicy(server_settings.tmp_policy, server_settings.max_temporary_data_on_disk_size); | |
} | |
else if (!server_settings.temporary_data_in_cache.value.empty()) | |
{ | |
global_context->setTemporaryStorageInCache(server_settings.temporary_data_in_cache, server_settings.max_temporary_data_on_disk_size); | |
} | |
else | |
{ | |
std::string temporary_path = config().getString("tmp_path", path / "tmp/"); | |
global_context->setTemporaryStoragePath(temporary_path, server_settings.max_temporary_data_on_disk_size); | |
} |
Anyway, better naming and comments are required, so I'm going to investigate how to make it better and continue this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tavplubix
I renamed temp_data_on_disk
in shared context and added comments.
Also added locks for get/set this variable in shared context (not sure if it's mandatory since it's set once at Server::main
), but in other places accessing shared parts of context we do (e.g. Context::setFlagsPath
)
9fc368b
to
7fe06f8
Compare
src/Interpreters/Context.cpp
Outdated
auto lock = getLock(); | ||
|
||
if (shared->root_temp_data_on_disk) | ||
throw Exception(ErrorCodes::LOGICAL_ERROR, "Temporary storage is already set"); | ||
|
||
StoragePolicyPtr tmp_policy = getStoragePolicySelector(lock)->get(policy_name); | ||
std::lock_guard storage_policies_lock(shared->storage_policies_mutex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean that we cannot access Context
while holding storage_policies_mutex
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reduced scope of storage_policies_mutex
not to overlap with getLock()
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Documentation entry for user-facing changes