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
Make StorageSet::getSet() thread safe #52352
Conversation
This is an automated comment for commit 59874af with description of existing statuses. It's updated for the latest CI running
|
Ok, but the code is difficult to understand because it does not explain why atomic_load is needed there. We should:
|
All integration tests with Thread Sanitizer have failed. You can check the results of the integration tests. Download the archive, and you can probably find a TSan report there. |
PS. I strongly advice replacing everything with a mutex to make the code easier to read. |
Suddenly tsan integration tests passed =) |
Looks not related to PR |
All tests will be flaky since it is DR and very hard to reproduce.
Added
Since it is protecting only accessing to the member (set), I would suggest to use atomic_load/atomic_store instead of mutex, where it points that setting/loading the var is atomic and thread safe, SetPtr itself is not thread safe and every callers to getSet() should use it only for read access. This is why suggesting to make it And sorry for some delays and some mess-up in comments. First commit missed atomic_store and it was confusing, yes. |
0fb2f54
to
4ae91e7
Compare
We have these tests, they are named |
Added a functional test which reproduces DR issue quite often. |
319932a
to
01aa5c7
Compare
|
Not related to PR:
|
759a565
to
19d0f43
Compare
Found by TSan. Happened when CREATE TABLE ... ENGINE = Set() and TRUNCATE TABLE is called DB::StorageSet::truncate DB::InterpreterDropQuery::executeToTableImpl ... DB::InterpreterDropQuery::execute and mutations are happening DB::ExpressionAnalyzer::isPlainStorageSetInSubquery calls `return storage_set->getSet();` DB::ExpressionAnalyzer::tryMakeSetForIndexFromSubquery ... DB::MergeTreeDataMergerMutator::mutatePartToTemporaryPart DB::StorageMergeTree::mutateSelectedPart DB::StorageMergeTree::scheduleDataProcessingJob
|
I remember (maybe I'm wrong) that atomic shared_ptrs in C++ are "fake", and while reading the diff, I'm thinking - maybe there is a mutex already inside this class, so we can just use it to make it obvious. But I didn't read the surrounding code. |
Nevertheless, the requirement to have a comment in the code is not fulfilled... |
Ok, I checked - we use atomic ops on shared_ptr in some other places.
are called concurrently with |
Is there a link to TSan report? |
Continued in #55621. |
Found by TSan.
Happened when CREATE TABLE ... ENGINE = Set()
and TRUNCATE TABLE is called
and mutations are happening
Changelog category (leave one):
Changelog category (leave one):