Merge pull request #32165 from azat/query-profiler-reset-fix

Fix QueryProfiler (query_profiler_{cpu,real}_time_period_ns) reset

base/daemon/BaseDaemon.cpp

@@ -104,7 +104,7 @@ static void writeSignalIDtoSignalPipe(int sig)
     errno = saved_errno;
 }
 
-/** Signal handler for HUP / USR1 */
+/** Signal handler for HUP */
 static void closeLogsSignalHandler(int sig, siginfo_t *, void *)
 {
     DENY_ALLOCATIONS_IN_SCOPE;
@@ -161,7 +161,7 @@ __attribute__((__weak__)) void collectCrashLog(
 
 
 /** The thread that read info about signal or std::terminate from pipe.
-  * On HUP / USR1, close log files (for new files to be opened later).
+  * On HUP, close log files (for new files to be opened later).
   * On information about std::terminate, write it to log.
   * On other signals, write info to log.
   */
@@ -201,7 +201,7 @@ public:
                 LOG_INFO(log, "Stop SignalListener thread");
                 break;
             }
-            else if (sig == SIGHUP || sig == SIGUSR1)
+            else if (sig == SIGHUP)
             {
                 LOG_DEBUG(log, "Received signal to close logs.");
                 BaseDaemon::instance().closeLogs(BaseDaemon::instance().logger());
@@ -832,7 +832,7 @@ void BaseDaemon::initializeTerminationAndSignalProcessing()
     /// SIGTSTP is added for debugging purposes. To output a stack trace of any running thread at anytime.
 
     addSignalHandler({SIGABRT, SIGSEGV, SIGILL, SIGBUS, SIGSYS, SIGFPE, SIGPIPE, SIGTSTP, SIGTRAP}, signalHandler, &handled_signals);
-    addSignalHandler({SIGHUP, SIGUSR1}, closeLogsSignalHandler, &handled_signals);
+    addSignalHandler({SIGHUP}, closeLogsSignalHandler, &handled_signals);
     addSignalHandler({SIGINT, SIGQUIT, SIGTERM}, terminateRequestedSignalHandler, &handled_signals);
 
 #if defined(SANITIZER)
@@ -1006,7 +1006,7 @@ void BaseDaemon::setupWatchdog()
 
         /// Forward signals to the child process.
         addSignalHandler(
-            {SIGHUP, SIGUSR1, SIGINT, SIGQUIT, SIGTERM},
+            {SIGHUP, SIGINT, SIGQUIT, SIGTERM},
             [](int sig, siginfo_t *, void *)
             {
                 /// Forward all signals except INT as it can be send by terminal to the process group when user press Ctrl+C,

src/Common/QueryProfiler.cpp

@@ -25,6 +25,12 @@ namespace
 {
 #if defined(OS_LINUX)
     thread_local size_t write_trace_iteration = 0;
+    /// Even after timer_delete() the signal can be delivered,
+    /// since it does not do anything with pending signals.
+    ///
+    /// And so to overcome this flag is exists,
+    /// to ignore delivered signals after timer_delete().
+    thread_local bool signal_handler_disarmed = true;
 #endif
 
     void writeTraceInfo(TraceType trace_type, int /* sig */, siginfo_t * info, void * context)
@@ -117,10 +123,8 @@ QueryProfilerBase<ProfilerImpl>::QueryProfilerBase(const UInt64 thread_id, const
     if (sigaddset(&sa.sa_mask, pause_signal))
         throwFromErrno("Failed to add signal to mask for query profiler", ErrorCodes::CANNOT_MANIPULATE_SIGSET);
 
-    struct sigaction local_previous_handler;
+    if (sigaction(pause_signal, &sa, nullptr))
-    if (sigaction(pause_signal, &sa, &local_previous_handler))
         throwFromErrno("Failed to setup signal handler for query profiler", ErrorCodes::CANNOT_SET_SIGNAL_HANDLER);
-    previous_handler.emplace(local_previous_handler);
 
     try
     {
@@ -160,6 +164,8 @@ QueryProfilerBase<ProfilerImpl>::QueryProfilerBase(const UInt64 thread_id, const
         struct itimerspec timer_spec = {.it_interval = interval, .it_value = offset};
         if (timer_settime(*timer_id, 0, &timer_spec, nullptr))
             throwFromErrno("Failed to set thread timer period", ErrorCodes::CANNOT_SET_TIMER_PERIOD);
+
+        signal_handler_disarmed = false;
     }
     catch (...)
     {
@@ -179,11 +185,14 @@ template <typename ProfilerImpl>
 void QueryProfilerBase<ProfilerImpl>::tryCleanup()
 {
 #if USE_UNWIND
-    if (timer_id.has_value() && timer_delete(*timer_id))
+    if (timer_id.has_value())
-        LOG_ERROR(log, "Failed to delete query profiler timer {}", errnoToString(ErrorCodes::CANNOT_DELETE_TIMER));
+    {
+        if (timer_delete(*timer_id))
+            LOG_ERROR(log, "Failed to delete query profiler timer {}", errnoToString(ErrorCodes::CANNOT_DELETE_TIMER));
+        timer_id.reset();
+    }
 
-    if (previous_handler.has_value() && sigaction(pause_signal, &*previous_handler, nullptr))
+    signal_handler_disarmed = true;
-        LOG_ERROR(log, "Failed to restore signal handler after query profiler {}", errnoToString(ErrorCodes::CANNOT_SET_SIGNAL_HANDLER));
 #endif
 }
 
@@ -196,6 +205,9 @@ QueryProfilerReal::QueryProfilerReal(const UInt64 thread_id, const UInt32 period
 
 void QueryProfilerReal::signalHandler(int sig, siginfo_t * info, void * context)
 {
+    if (signal_handler_disarmed)
+        return;
+
     DENY_ALLOCATIONS_IN_SCOPE;
     writeTraceInfo(TraceType::Real, sig, info, context);
 }
@@ -206,6 +218,9 @@ QueryProfilerCPU::QueryProfilerCPU(const UInt64 thread_id, const UInt32 period)
 
 void QueryProfilerCPU::signalHandler(int sig, siginfo_t * info, void * context)
 {
+    if (signal_handler_disarmed)
+        return;
+
     DENY_ALLOCATIONS_IN_SCOPE;
     writeTraceInfo(TraceType::CPU, sig, info, context);
 }

src/Common/QueryProfiler.h

@@ -46,9 +46,6 @@ private:
 
     /// Pause signal to interrupt threads to get traces
     int pause_signal;
-
-    /// Previous signal handler to restore after query profiler exits
-    std::optional<struct sigaction> previous_handler;
 };
 
 /// Query profiler with timer based on real clock