Merge pull request #39098 from ClickHouse/backport/22.5/39030

Backport #39030 to 22.5: Remove logging from OvercommitTracker to avoid deadlocks

src/Common/OvercommitTracker.cpp

@@ -20,28 +20,14 @@ OvercommitTracker::OvercommitTracker(std::mutex & global_mutex_)
     , allow_release(true)
 {}
 
-#define LOG_DEBUG_SAFE(...)                                                                               \
-    do {                                                                                                  \
-        OvercommitTrackerBlockerInThread blocker;                                                         \
-        try                                                                                               \
-        {                                                                                                 \
-            ALLOW_ALLOCATIONS_IN_SCOPE;                                                                   \
-            LOG_DEBUG(__VA_ARGS__);                                                                       \
-        }                                                                                                 \
-        catch (...)                                                                                       \
-        {                                                                                                 \
-            if (fprintf(stderr, "Allocation failed during writing to log in OvercommitTracker\n") != -1)  \
-                ;                                                                                         \
-        }                                                                                                 \
-    } while (false)
-
 void OvercommitTracker::setMaxWaitTime(UInt64 wait_time)
 {
     std::lock_guard guard(overcommit_m);
     max_wait_time = wait_time * 1us;
 }
 
 bool OvercommitTracker::needToStopQuery(MemoryTracker * tracker, Int64 amount)
+
 {
     DENY_ALLOCATIONS_IN_SCOPE;
 
@@ -91,7 +77,6 @@ bool OvercommitTracker::needToStopQuery(MemoryTracker * tracker, Int64 amount)
     {
         return id < id_to_release || cancellation_state == QueryCancellationState::NONE;
     });
-    LOG_DEBUG_SAFE(getLogger(), "Memory was{} freed within timeout", (timeout ? " not" : ""));
 
     required_memory -= amount;
     bool still_need = !(id < id_to_release); // True if thread wasn't released
@@ -131,8 +116,6 @@ void OvercommitTracker::onQueryStop(MemoryTracker * tracker)
     std::unique_lock<std::mutex> lk(overcommit_m);
     if (picked_tracker == tracker)
     {
-        LOG_DEBUG_SAFE(getLogger(), "Picked query stopped");
-
         reset();
         cv.notify_all();
     }
@@ -158,7 +141,6 @@ void UserOvercommitTracker::pickQueryToExcludeImpl()
     // At this moment query list must be read only.
     // This is guaranteed by locking global_mutex in OvercommitTracker::needToStopQuery.
     auto & queries = user_process_list->queries;
-    LOG_DEBUG_SAFE(logger, "Trying to choose query to stop from {} queries", queries.size());
     for (auto const & query : queries)
     {
         if (query.second->isKilled())
@@ -169,15 +151,12 @@ void UserOvercommitTracker::pickQueryToExcludeImpl()
             continue;
 
         auto ratio = memory_tracker->getOvercommitRatio();
-        LOG_DEBUG_SAFE(logger, "Query has ratio {}/{}", ratio.committed, ratio.soft_limit);
         if (ratio.soft_limit != 0 && current_ratio < ratio)
         {
             query_tracker = memory_tracker;
             current_ratio   = ratio;
         }
     }
-    LOG_DEBUG_SAFE(logger, "Selected to stop query with overcommit ratio {}/{}",
-        current_ratio.committed, current_ratio.soft_limit);
     picked_tracker = query_tracker;
 }
 
@@ -192,7 +171,6 @@ void GlobalOvercommitTracker::pickQueryToExcludeImpl()
     OvercommitRatio current_ratio{0, 0};
     // At this moment query list must be read only.
     // This is guaranteed by locking global_mutex in OvercommitTracker::needToStopQuery.
-    LOG_DEBUG_SAFE(logger, "Trying to choose query to stop from {} queries", process_list->size());
     for (auto const & query : process_list->processes)
     {
         if (query.isKilled())
@@ -208,15 +186,12 @@ void GlobalOvercommitTracker::pickQueryToExcludeImpl()
         if (!memory_tracker)
             continue;
         auto ratio = memory_tracker->getOvercommitRatio(user_soft_limit);
-        LOG_DEBUG_SAFE(logger, "Query has ratio {}/{}", ratio.committed, ratio.soft_limit);
         if (current_ratio < ratio)
         {
             query_tracker = memory_tracker;
             current_ratio   = ratio;
         }
     }
-    LOG_DEBUG_SAFE(logger, "Selected to stop query with overcommit ratio {}/{}",
-        current_ratio.committed, current_ratio.soft_limit);
     picked_tracker = query_tracker;
 }
 

src/Common/OvercommitTracker.h

@@ -80,8 +80,6 @@ struct OvercommitTracker : boost::noncopyable
     // overcommit tracker is in SELECTED state.
     MemoryTracker * picked_tracker;
 
-    virtual Poco::Logger * getLogger() = 0;
-
 private:
 
     void pickQueryToExclude()
@@ -139,10 +137,8 @@ struct UserOvercommitTracker : OvercommitTracker
 protected:
     void pickQueryToExcludeImpl() override;
 
-    Poco::Logger * getLogger() override final { return logger; }
 private:
     DB::ProcessListForUser * user_process_list;
-    Poco::Logger * logger = &Poco::Logger::get("UserOvercommitTracker");
 };
 
 struct GlobalOvercommitTracker : OvercommitTracker
@@ -154,10 +150,8 @@ struct GlobalOvercommitTracker : OvercommitTracker
 protected:
     void pickQueryToExcludeImpl() override;
 
-    Poco::Logger * getLogger() override final { return logger; }
 private:
     DB::ProcessList * process_list;
-    Poco::Logger * logger = &Poco::Logger::get("GlobalOvercommitTracker");
 };
 
 // This class is used to disallow tracking during logging to avoid deadlocks.