Lambda erase_from_active() captures the item (TaskRuntimeDataPtr), most
of the code path is OK, since it explicitly reset the item->task.
However one is not, when it moves the item to pending list, which will
be cleaned up when the table will be DROP/DETACH'ed, from
MergeTreeBackgroundExecutor::removeTasksCorrespondingToStorage(),
and in this case if IStorage will be already removed, then it will lead
to use-after-free on destroying the lambda, since it captures the item
by value.
And I belive that CI founds this issue here [1]:
<details>
<summary>stack trace</summary>
4 0x268d1354 in DB::ReplicatedMergeTreeQueue::CurrentlyExecuting::~CurrentlyExecuting() build_docker/../src/Storages/MergeTree/ReplicatedMergeTreeQueue.cpp:1510:33
5 0x268ead1a in std::__1::default_delete<DB::ReplicatedMergeTreeQueue::CurrentlyExecuting>::operator()(DB::ReplicatedMergeTreeQueue::CurrentlyExecuting*) const build_docker/../contrib/libcxx/include/__memory/unique_ptr.h:54:5
6 0x268ead1a in std::__1::unique_ptr<DB::ReplicatedMergeTreeQueue::CurrentlyExecuting, std::__1::default_delete<DB::ReplicatedMergeTreeQueue::CurrentlyExecuting> >::reset(DB::ReplicatedMergeTreeQueue::CurrentlyExecuting*) build_docker/../contrib/libcxx/include/__memory/unique_ptr.h:315:7
7 0x268ead1a in std::__1::unique_ptr<DB::ReplicatedMergeTreeQueue::CurrentlyExecuting, std::__1::default_delete<DB::ReplicatedMergeTreeQueue::CurrentlyExecuting> >::~unique_ptr() build_docker/../contrib/libcxx/include/__memory/unique_ptr.h:269:19
8 0x268ead1a in DB::ReplicatedMergeTreeQueue::SelectedEntry::~SelectedEntry() build_docker/../src/Storages/MergeTree/ReplicatedMergeTreeQueue.h:351:12
9 0x268ead1a in void std::__1::__destroy_at<DB::ReplicatedMergeTreeQueue::SelectedEntry, 0>(DB::ReplicatedMergeTreeQueue::SelectedEntry*) build_docker/../contrib/libcxx/include/__memory/construct_at.h:56:13
...
16 0x265e9abb in DB::MergeTreeBackgroundExecutor<DB::MergeMutateRuntimeQueue>::routine(std::__1::shared_ptr<DB::TaskRuntimeData>)::'lambda'()::~() build_docker/../src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp:127:30
17 0x265e9abb in DB::MergeTreeBackgroundExecutor<DB::MergeMutateRuntimeQueue>::routine(std::__1::shared_ptr<DB::TaskRuntimeData>) build_docker/../src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp:235:1
18 0x265ea730 in DB::MergeTreeBackgroundExecutor<DB::MergeMutateRuntimeQueue>::threadFunction() build_docker/../src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp:261:13
</details>
[1]: https://s3.amazonaws.com/clickhouse-test-reports/0/533c7888038453c047df816f3f65292cca05a54f/stateless_tests__ubsan__actions_.html
And I also I think that the following issue will also be fixed [2]:
<details>
<summary>stack trace</summary>
[ 680 ] {} <Fatal> : Logical error: 'Memory tracker: allocations not allowed.'.
...
[ 23430 ] {} <Fatal> BaseDaemon: 23. /build/build_docker/../src/Common/formatReadable.cpp:46: formatReadableSizeWithBinarySuffix(double, int) @ 0x1713bb98 in /usr/bin/clickhouse
...
[ 23430 ] {} <Fatal> BaseDaemon: 31. ../src/Common/MemoryTracker.cpp:112: MemoryTracker::logPeakMemoryUsage() @ 0x170e0ab9 in /usr/bin/clickhouse
[ 23430 ] {} <Fatal> BaseDaemon: 32. /build/build_docker/../src/Common/MemoryTracker.cpp:98: MemoryTracker::~MemoryTracker() @ 0x170e063a in /usr/bin/clickhouse
[ 23430 ] {} <Fatal> BaseDaemon: 33. /build/build_docker/../src/Storages/MergeTree/MergeList.cpp:144: DB::MergeListElement::~MergeListElement() @ 0x279fb290 in /usr/bin/clickhouse
...
[ 23430 ] {} <Fatal> BaseDaemon: 38. /build/build_docker/../src/Storages/MergeTree/BackgroundProcessList.h:41: DB::BackgroundProcessListEntry<DB::MergeListElement, DB::MergeInfo>::~BackgroundProcessListEntry() @ 0x276ce6c7 in /usr/bin/clickhouse
[ 23430 ] {} <Fatal> BaseDaemon: 39. /build/build_docker/../contrib/libcxx/include/__memory/unique_ptr.h:54: std::__1::default_delete<DB::BackgroundProcessListEntry<DB::MergeListElement, DB::MergeInfo> >::operator()(DB::BackgroundProcessListEntry<DB::MergeListElement, DB::MergeInfo>*) const @ 0x276ce60b in /usr/bin/clickhouse
[ 23430 ] {} <Fatal> BaseDaemon: 40. /build/build_docker/../contrib/libcxx/include/__memory/unique_ptr.h:316: std::__1::unique_ptr<DB::BackgroundProcessListEntry<DB::MergeListElement, DB::MergeInfo>, std::__1::default_delete<DB::BackgroundProcessListEntry<DB::MergeListElement, DB::MergeInfo> > >::reset(DB::BackgroundProcessListEntry<DB::MergeListElement, DB::MergeInfo>*) @ 0x276ce57c in /usr/bin/clickhouse
[ 23430 ] {} <Fatal> BaseDaemon: 41. /build/build_docker/../contrib/libcxx/include/__memory/unique_ptr.h:269: std::__1::unique_ptr<DB::BackgroundProcessListEntry<DB::MergeListElement, DB::MergeInfo>, std::__1::default_delete<DB::BackgroundProcessListEntry<DB::MergeListElement, DB::MergeInfo> > >::~unique_ptr() @ 0x276ce399 in /usr/bin/clickhouse
[ 23430 ] {} <Fatal> BaseDaemon: 42. /build/build_docker/../src/Storages/MergeTree/MutatePlainMergeTreeTask.h:22: DB::MutatePlainMergeTreeTask::~MutatePlainMergeTreeTask() @ 0x27defceb in /usr/bin/clickhouse
[ 23430 ] {} <Fatal> BaseDaemon: 43. /build/build_docker/../contrib/libcxx/include/__memory/construct_at.h:57: void std::__1::__destroy_at<DB::MutatePlainMergeTreeTask, 0>(DB::MutatePlainMergeTreeTask*) @ 0x27dd69c1 in /usr/bin/clickhouse
[ 23430 ] {} <Fatal> BaseDaemon: 44. /build/build_docker/../contrib/libcxx/include/__memory/construct_at.h:82: void std::__1::destroy_at<DB::MutatePlainMergeTreeTask, 0>(DB::MutatePlainMergeTreeTask*) @ 0x27dd6955 in /usr/bin/clickhouse
[ 23430 ] {} <Fatal> BaseDaemon: Integrity check of the executable skipped because the reference checksum could not be read. (calculated checksum: 91F5937571C11255DFE73230B52CE9C0)
[ 602 ] {} <Fatal> Application: Child process was terminated by signal 6.
</details>
[2]: https://s3.amazonaws.com/clickhouse-test-reports/39222/a068c397dfd7943359a8b554566c3c70b78baf8d/stateless_tests__debug__actions__%5B1/3%5D.html
Refs: https://github.com/ClickHouse/ClickHouse/pull/29614#discussion_r720455032 (cc @nikitamikhaylov)
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Current implementation of NOEXCEPT_SCOPE will not work, you cannot
rethrow exception outside the catch block, this will simply terminate
(via std::terminate) the program.
In other words NOEXCEPT_SCOPE macro will simply call std::terminate on
exception and will lost original exception.
But if NOEXCEPT_SCOPE will accept the code that should be runned w/o
exceptions, then it can catch exception and log it, rewrite it in this
way.
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
A simple HelloWorld program with zero includes except iostream triggers
a build of ca. 2000 source files. The reason is that ClickHouse's
top-level CMakeLists.txt overrides "add_executable()" to link all
binaries against "clickhouse_new_delete". This links against
"clickhouse_common_io", which in turn has lots of 3rd party library
dependencies ... Without linking "clickhouse_new_delete", the number of
compiled files for "HelloWorld" goes down to ca. 70.
As an example, the self-extracting-executable needs none of its current
dependencies but other programs may also benefit.
In order to restore access to the original "add_executable()", the
overriding version is now prefixed. There is precedence for a
"clickhouse_" prefix (as opposed to "ch_"), for example
"clickhouse_split_debug_symbols". In general prefixing makes sense also
because overriding CMake commands relies on undocumented behavior and is
considered not-so-great practice (*).
(*) https://crascit.com/2018/09/14/do-not-redefine-cmake-commands/
Before it produces the following error:
2022.07.07 23:40:03.131044 [ 44869 ] {} <Error> default.rep_data: auto DB::StorageReplicatedMergeTree::processQueueEntry(ReplicatedMergeTreeQueue::SelectedEntryPtr)::(anonymous class)::operator()(DB::StorageReplicatedMergeTree::LogEntryPtr &) const: Code: 107. DB::ErrnoException: Cannot open file /var/lib/clickhouse/data/tmp-fetch_36_25458_25458_0, errno: 2, strerror: No such file or directory. (FILE_DOESNT_EXIST), Stack trace (when copying this message, always include the lines below):
0. Poco::Exception::Exception() @ 0x1764634c in /usr/bin/clickhouse
1. DB::Exception::Exception() @ 0xb42caba in /usr/bin/clickhouse
2. DB::throwFromErrnoWithPath() @ 0xb42d79b in /usr/bin/clickhouse
3. DB::LocalDirectorySyncGuard::LocalDirectorySyncGuard() @ 0x1404bbd4 in /usr/bin/clickhouse
4. DB::DiskLocal::getDirectorySyncGuard() const @ 0x140466a4 in /usr/bin/clickhouse
5. DB::DataPartsExchange::Fetcher::downloadPartToDisk() @ 0x15122af1 in /usr/bin/clickhouse
6. DB::DataPartsExchange::Fetcher::fetchPart() @ 0x1511de39 in /usr/bin/clickhouse
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Before this patch, and after transaction support had been added #24258,
if the server had not removed old parts on shutdown, then they will left
forever.
Fixes: #24258 (cc @tavplubix)
v2: use Tx::PrehistoricTID over removePartsFromWorkingSet() from ctor
v3: apply black to new test
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
- TSA is a static analyzer build by Google which finds race conditions
and deadlocks at compile time.
- It works by associating a shared member variable with a
synchronization primitive that protects it. The compiler can then
check at each access if proper locking happened before. A good
introduction are [0] and [1].
- TSA requires some help by the programmer via annotations. Luckily,
LLVM's libcxx already has annotations for std::mutex, std::lock_guard,
std::shared_mutex and std::scoped_lock. This commit enables them
(--> contrib/libcxx-cmake/CMakeLists.txt).
- Further, this commit adds convenience macros for the low-level
annotations for use in ClickHouse (--> base/defines.h). For
demonstration, they are leveraged in a few places.
- As we compile with "-Wall -Wextra -Weverything", the required compiler
flag "-Wthread-safety-analysis" was already enabled. Negative checks
are an experimental feature of TSA and disabled
(--> cmake/warnings.cmake). Compile times did not increase noticeably.
- TSA is used in a few places with simple locking. I tried TSA also
where locking is more complex. The problem was usually that it is
unclear which data is protected by which lock :-(. But there was
definitely some weird code where locking looked broken. So there is
some potential to find bugs.
*** Limitations of TSA besides the ones listed in [1]:
- The programmer needs to know which lock protects which piece of shared
data. This is not always easy for large classes.
- Two synchronization primitives used in ClickHouse are not annotated in
libcxx:
(1) std::unique_lock: A releaseable lock handle often together with
std::condition_variable, e.g. in solve producer-consumer problems.
(2) std::recursive_mutex: A re-entrant mutex variant. Its usage can be
considered a design flaw + typically it is slower than a standard
mutex. In this commit, one std::recursive_mutex was converted to
std::mutex and annotated with TSA.
- For free-standing functions (e.g. helper functions) which are passed
shared data members, it can be tricky to specify the associated lock.
This is because the annotations use the normal C++ rules for symbol
resolution.
[0] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
[1] https://static.googleusercontent.com/media/research.google.com/en//pubs/archive/42958.pdf
v2: use real column name instead of aliases from GROUP BY
Fixes the following error in 01710_projection_aggregation_in_order:
Not found column a in block. There are only columns: toStartOfHour(ts), sum(value). (NOT_FOUND_COLUMN_IN_BLOCK)
v2.1: Get back support for projected and non-projected parts
v2.2: merge tests and rename
v3: Reduce copy-paste for optimize_aggregation_in_order for projections
v4: rebase on top of QueryPlanResourceHolder
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
v2: fill AggregateColumnsConstData only for only_merge
(fixes 01291_aggregation_in_order and some other tests)
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
