From e91c8bfea3903d0cb4ab294721d9a598c9348e24 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Thu, 6 Apr 2023 14:43:52 +0200 Subject: [PATCH] Fix deadlock due to debug tracking of memory allocations Since memory tracker should be blocked globally (by default it is blocked only for VariableContext::User).
stacktrace * thread 11, name = 'Collector', stop reason = trace frame 1: 0x00007ffff7e37ea2 libc.so.6`pthread_mutex_lock + 274 frame 6: 0x000000001ee10298 clickhouse`toStringCached(pointers=0x00007fffedfef068, offset=0, size=45) + 88 at StackTrace.cpp:425 frame 7: 0x000000001ee1128e clickhouse`StackTrace::toString(this=0x00007fffedfef058) const + 46 at StackTrace.cpp:444 frame 8: 0x000000001ede58ff clickhouse`(anonymous namespace)::debugLogBigAllocationWithoutCheck(size=25187648) + 479 at MemoryTracker.cpp:98 frame 9: 0x000000001ede5481 clickhouse`MemoryTracker::allocImpl(this=0x0000000030729e68, size=25187648, throw_if_memory_exceeded=false, query_tracker=0x00007ffff3c22440) + 3937 at MemoryTracker.cpp:352 frame 10: 0x000000001ede477f clickhouse`MemoryTracker::allocImpl(this=0x00007ffff3c1e530, size=25187648, throw_if_memory_exceeded=false, query_tracker=0x00007ffff3c22440) + 607 at MemoryTracker.cpp:198 frame 11: 0x000000001ede477f clickhouse`MemoryTracker::allocImpl(this=0x00007ffff3c22440, size=25187648, throw_if_memory_exceeded=false, query_tracker=0x0000000000000000) + 607 at MemoryTracker.cpp:198 frame 12: 0x000000001ede477f clickhouse`MemoryTracker::allocImpl(this=0x00007fffedff3bf0, size=25187648, throw_if_memory_exceeded=false, query_tracker=0x0000000000000000) + 607 at MemoryTracker.cpp:198 frame 13: 0x000000001ed6c4c9 clickhouse`CurrentMemoryTracker::allocImpl(size=25165824, throw_if_memory_exceeded=false) + 265 at CurrentMemoryTracker.cpp:58 frame 14: 0x000000001ed6c6a1 clickhouse`CurrentMemoryTracker::allocNoThrow(size=25165824) + 33 at CurrentMemoryTracker.cpp:91 frame 15: 0x000000001ed63079 clickhouse`operator new(unsigned long) [inlined] void Memory::trackMemory<>(size=25165824) + 89 at memory.h:177 frame 16: 0x000000001ed63034 clickhouse`operator new(size=25165824) + 20 at new_delete.cpp:74 ... frame 27: 0x000000001ee15895 clickhouse`DB::(anonymous namespace)::collectSymbols(info=0x00007ffff7062000, (null)=, data_ptr=0x00007fffec2b7000) + 53 at SymbolIndex.cpp:483 frame 28: 0x000000002e1a5d57 clickhouse`::dl_iterate_phdr(callback=(clickhouse`DB::(anonymous namespace)::collectSymbols(dl_phdr_info*, unsigned long, void*) at SymbolIndex.cpp:480), data=0x00007fffec2b7000)(dl_phdr_info *, size_t, void *), void *) + 183 at phdr_cache.cpp:76 frame 29: 0x000000001ee15549 clickhouse`DB::SymbolIndex::update(this=0x00007fffec2b7000) + 41 at SymbolIndex.cpp:515 frame 30: 0x000000001ee1be35 clickhouse`DB::SymbolIndex::SymbolIndex(this=) + 53 at SymbolIndex.h:23 frame 31: 0x000000001ee17a6c clickhouse`DB::SymbolIndex::instanceImpl() + 108 at SymbolIndex.cpp:555 frame 32: 0x000000001ee17b4a clickhouse`DB::SymbolIndex::instance() + 10 at SymbolIndex.cpp:561 frame 33: 0x000000001ee1070f clickhouse`void toStringEveryLineImpl const&, unsigned long, unsigned long)::$_0>(fatal=false, stack_trace=0x00007fffedff1ef8, callback=0x00007fffedff1ee0)::$_0&&) + 207 at StackTrace.cpp:349 frame 34: 0x000000001ee1040b clickhouse`toStringCached(pointers=0x00007fffedff2928, offset=0, size=43) + 459 at StackTrace.cpp:436 frame 35: 0x000000001ee1128e clickhouse`StackTrace::toString(this=0x00007fffedff2918) const + 46 at StackTrace.cpp:444 frame 36: 0x000000001ede58ff clickhouse`(anonymous namespace)::debugLogBigAllocationWithoutCheck(size=25165824) + 479 at MemoryTracker.cpp:98 frame 37: 0x000000001ede5481 clickhouse`MemoryTracker::allocImpl(this=0x00007fffedff3bf0, size=25165824, throw_if_memory_exceeded=false, query_tracker=0x0000000000000000) + 3937 at MemoryTracker.cpp:352 frame 38: 0x000000001ed6c4c9 clickhouse`CurrentMemoryTracker::allocImpl(size=25165824, throw_if_memory_exceeded=false) + 265 at CurrentMemoryTracker.cpp:58 frame 39: 0x000000001ed6c6a1 clickhouse`CurrentMemoryTracker::allocNoThrow(size=25165824) + 33 at CurrentMemoryTracker.cpp:91 frame 40: 0x000000001ed63079 clickhouse`operator new(unsigned long) [inlined] void Memory::trackMemory<>(size=25165824) + 89 at memory.h:177 frame 41: 0x000000001ed63034 clickhouse`operator new(size=25165824) + 20 at new_delete.cpp:74 ... frame 55: 0x0000000025badace clickhouse`DB::WriteBufferFromS3::nextImpl(this=0x00007ffff3c58f00) + 366 at WriteBufferFromS3.cpp:110
Signed-off-by: Azat Khuzhin --- src/Common/MemoryTracker.cpp | 40 ++++++++++++++++++------------------ src/Common/MemoryTracker.h | 2 ++ 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/Common/MemoryTracker.cpp b/src/Common/MemoryTracker.cpp index ca5c4a745cd..674d8d469af 100644 --- a/src/Common/MemoryTracker.cpp +++ b/src/Common/MemoryTracker.cpp @@ -82,26 +82,6 @@ inline std::string_view toDescription(OvercommitResult result) } } -inline void debugLogBigAllocationWithoutCheck(Int64 size [[maybe_unused]]) -{ - /// Big allocations through allocNoThrow (without checking memory limits) may easily lead to OOM (and it's hard to debug). - /// Let's find them. -#ifdef ABORT_ON_LOGICAL_ERROR - if (size < 0) - return; - - constexpr Int64 threshold = 16 * 1024 * 1024; /// The choice is arbitrary (maybe we should decrease it) - if (size < threshold) - return; - - MemoryTrackerBlockerInThread blocker; - LOG_TEST(&Poco::Logger::get("MemoryTracker"), "Too big allocation ({} bytes) without checking memory limits, " - "it may lead to OOM. Stack trace: {}", size, StackTrace().toString()); -#else - return; /// Avoid trash logging in release builds -#endif -} - } namespace ProfileEvents @@ -175,6 +155,26 @@ void MemoryTracker::injectFault() const description ? description : ""); } +void MemoryTracker::debugLogBigAllocationWithoutCheck(Int64 size [[maybe_unused]]) +{ + /// Big allocations through allocNoThrow (without checking memory limits) may easily lead to OOM (and it's hard to debug). + /// Let's find them. +#ifdef ABORT_ON_LOGICAL_ERROR + if (size < 0) + return; + + constexpr Int64 threshold = 16 * 1024 * 1024; /// The choice is arbitrary (maybe we should decrease it) + if (size < threshold) + return; + + MemoryTrackerBlockerInThread blocker(VariableContext::Global); + LOG_TEST(&Poco::Logger::get("MemoryTracker"), "Too big allocation ({} bytes) without checking memory limits, " + "it may lead to OOM. Stack trace: {}", size, StackTrace().toString()); +#else + return; /// Avoid trash logging in release builds +#endif +} + void MemoryTracker::allocImpl(Int64 size, bool throw_if_memory_exceeded, MemoryTracker * query_tracker) { if (size < 0) diff --git a/src/Common/MemoryTracker.h b/src/Common/MemoryTracker.h index 66b56730b75..0d7748856bd 100644 --- a/src/Common/MemoryTracker.h +++ b/src/Common/MemoryTracker.h @@ -215,6 +215,8 @@ public: /// Prints info about peak memory consumption into log. void logPeakMemoryUsage(); + + void debugLogBigAllocationWithoutCheck(Int64 size [[maybe_unused]]); }; extern MemoryTracker total_memory_tracker;