deadlock in system.text_log#12339
deadlock in system.text_log#12339nikitamikhaylov wants to merge 11 commits intoClickHouse:masterfrom
Conversation
robot-clickhouse
added
the
pr-bugfix
|
It may be interesting to add a scoped class like |
src/Interpreters/SystemLog.h
Outdated
| } | ||
|
|
||
| queue.push_back(element); | ||
| queue.push_back(std::forward<T>(element)); |
There was a problem hiding this comment.
This should avoid copy-construction where possible, but not always -- depends on what kind of reference you pass. Maybe we should require rvalue reference input instead?
Also there is still reallocation of queue buffer itself -- we can probably grow it to max size right away. If you do this, don't forget also initialize the local queue in the flushing thread, that is swapped with this one.
There was a problem hiding this comment.
- I thought the problem was only with
text_log. Other logs will pass lvalue as before. - That is the problem.
|
@akuzm I forgot why we have to avoid allocations in |
| class FunctionPrintToLog : public IFunction | ||
| { | ||
| public: | ||
| static constexpr auto name = "printToLog"; |
There was a problem hiding this comment.
Can I now run select printToLog(repeat('a', 1000)) from system.numbers to bring down the playground? It should require sysadmin priveleges and be disabled in read only mode.
There was a problem hiding this comment.
allow_introspection_functions
There was a problem hiding this comment.
I also wanted this function, see #9434
Details:
- only trace level;
- no quantity parameter (I think it's harmful);
- different name (log is also a verb);
base/loggers/OwnSplitChannel.cpp
Outdated
| std::unique_lock<std::mutex> lock(text_log_mutex); | ||
| if (auto log = text_log.lock()) | ||
| log->add(elem); | ||
| { | ||
| lock.unlock(); | ||
| log->add(std::move(elem)); | ||
| } | ||
|
|
There was a problem hiding this comment.
I can already imagine how I fail to notice the lock.unlock() inside the if, add some code after if and believe that it's under the lock. Let's restructure:
shared_ptr<TextLog> text_log_locked;
{
std::lock_guard<std::mutex> lock(text_log_mutex);
text_log_locked = text_log.lock();
}
if (text_log_locked)
{
text_log_locked->add(elem);
}
Idk, why do we have to use |
|
The new function is probably not something we should backport. So I guess we should have a one-line PR with locking changes and bugfix category, and everything else in another PR. Or do we have to backport it at all? I remember Alexey fixed something, maybe it's enough for the back branches? |
But I think the changes themselves are OK and we can keep them. These logs can be used in surprisingly intensive ways e.g. when tracing all memory allocations -- I had to fix several performance problems before. So saving excessive data copying and allocation is helpful. |
|
|
||
| DataTypePtr getReturnTypeImpl(const DataTypes &) const override | ||
| { | ||
| return std::make_shared<DataTypeString>(); |
There was a problem hiding this comment.
I'd return 0 of type UInt8.
|
The deadlock is fixed. What's left is the print function. @nikitamikhaylov do you want to continue? Maybe we should close this PR and concentrate on something more vital. |
Of course, maybe I will create PR only with function in the near future. |
OK, I'll close this one then. |
4 participants
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Try to avoid deadlock in
system.text_log. Closes #12325