From c1036f0b8ee3ca73eee5e2c335011d180a3aa3ae Mon Sep 17 00:00:00 2001 From: save-my-heart Date: Tue, 25 Jan 2022 22:43:44 +0800 Subject: [PATCH 1/9] ensure signal_pipe_buf_size is <= PIPE_BUF --- base/daemon/BaseDaemon.cpp | 47 ++++++++++++++++++-------------------- src/Common/StackTrace.cpp | 4 ++-- src/Common/StackTrace.h | 2 +- 3 files changed, 25 insertions(+), 28 deletions(-) diff --git a/base/daemon/BaseDaemon.cpp b/base/daemon/BaseDaemon.cpp index f3026d7c87a..6ac552e1284 100644 --- a/base/daemon/BaseDaemon.cpp +++ b/base/daemon/BaseDaemon.cpp @@ -79,18 +79,14 @@ static void call_default_signal_handler(int sig) raise(sig); } -static constexpr size_t max_query_id_size = 127; - static const size_t signal_pipe_buf_size = sizeof(int) + sizeof(siginfo_t) - + sizeof(ucontext_t) + + sizeof(mcontext_t) + sizeof(StackTrace) + sizeof(UInt32) - + max_query_id_size + 1 /// query_id + varint encoded length + sizeof(void*); - using signal_function = void(int, siginfo_t*, void*); static void writeSignalIDtoSignalPipe(int sig) @@ -132,15 +128,11 @@ static void signalHandler(int sig, siginfo_t * info, void * context) const ucontext_t signal_context = *reinterpret_cast(context); const StackTrace stack_trace(signal_context); - StringRef query_id = DB::CurrentThread::getQueryId(); /// This is signal safe. - query_id.size = std::min(query_id.size, max_query_id_size); - DB::writeBinary(sig, out); DB::writePODBinary(*info, out); - DB::writePODBinary(signal_context, out); + DB::writePODBinary(signal_context.uc_mcontext, out); DB::writePODBinary(stack_trace, out); DB::writeBinary(UInt32(getThreadId()), out); - DB::writeStringBinary(query_id, out); DB::writePODBinary(DB::current_thread, out); out.next(); @@ -184,6 +176,8 @@ public: void run() override { + static_assert(PIPE_BUF >= 512); + static_assert(signal_pipe_buf_size <= PIPE_BUF, "Only write of PIPE_BUF to pipe is atomic and the minimal known PIPE_BUF across supported platforms is 512"); char buf[signal_pipe_buf_size]; DB::ReadBufferFromFileDescriptor in(signal_pipe.fds_rw[0], signal_pipe_buf_size, buf); @@ -227,26 +221,24 @@ public: else { siginfo_t info{}; - ucontext_t context{}; + mcontext_t mcontext{}; StackTrace stack_trace(NoCapture{}); UInt32 thread_num{}; - std::string query_id; DB::ThreadStatus * thread_ptr{}; if (sig != SanitizerTrap) { DB::readPODBinary(info, in); - DB::readPODBinary(context, in); + DB::readPODBinary(mcontext, in); } DB::readPODBinary(stack_trace, in); DB::readBinary(thread_num, in); - DB::readBinary(query_id, in); DB::readPODBinary(thread_ptr, in); /// This allows to receive more signals if failure happens inside onFault function. /// Example: segfault while symbolizing stack trace. - std::thread([=, this] { onFault(sig, info, context, stack_trace, thread_num, query_id, thread_ptr); }).detach(); + std::thread([=, this] { onFault(sig, info, mcontext, stack_trace, thread_num, thread_ptr); }).detach(); } } } @@ -279,18 +271,27 @@ private: void onFault( int sig, const siginfo_t & info, - const ucontext_t & context, + const mcontext_t & mcontext, const StackTrace & stack_trace, UInt32 thread_num, - const std::string & query_id, DB::ThreadStatus * thread_ptr) const { DB::ThreadStatus thread_status; + String query_id; + String query; + /// Send logs from this thread to client if possible. /// It will allow client to see failure messages directly. if (thread_ptr) { + query_id = thread_ptr->getQueryId().toString(); + + if (auto thread_group = thread_ptr->getThreadGroup()) + { + query = thread_group->query; + } + if (auto logs_queue = thread_ptr->getInternalTextLogsQueue()) DB::CurrentThread::attachInternalTextLogsQueue(logs_queue, DB::LogsLevel::trace); } @@ -305,15 +306,15 @@ private: } else { - LOG_FATAL(log, "(version {}{}, {}) (from thread {}) (query_id: {}) Received signal {} ({})", + LOG_FATAL(log, "(version {}{}, {}) (from thread {}) (query_id: {}) (query: {}) Received signal {} ({})", VERSION_STRING, VERSION_OFFICIAL, daemon.build_id_info, - thread_num, query_id, strsignal(sig), sig); + thread_num, query_id, query, strsignal(sig), sig); } String error_message; if (sig != SanitizerTrap) - error_message = signalToErrorMessage(sig, info, context); + error_message = signalToErrorMessage(sig, info, mcontext); else error_message = "Sanitizer trap."; @@ -389,20 +390,16 @@ static void sanitizerDeathCallback() const StackTrace stack_trace; - StringRef query_id = DB::CurrentThread::getQueryId(); - query_id.size = std::min(query_id.size, max_query_id_size); - int sig = SignalListener::SanitizerTrap; DB::writeBinary(sig, out); DB::writePODBinary(stack_trace, out); DB::writeBinary(UInt32(getThreadId()), out); - DB::writeStringBinary(query_id, out); DB::writePODBinary(DB::current_thread, out); out.next(); /// The time that is usually enough for separate thread to print info into log. - sleepForSeconds(10); + sleepForSeconds(20); } #endif diff --git a/src/Common/StackTrace.cpp b/src/Common/StackTrace.cpp index 18e2e223744..35d590d7508 100644 --- a/src/Common/StackTrace.cpp +++ b/src/Common/StackTrace.cpp @@ -19,7 +19,7 @@ # include #endif -std::string signalToErrorMessage(int sig, const siginfo_t & info, [[maybe_unused]] const ucontext_t & context) +std::string signalToErrorMessage(int sig, const siginfo_t & info, [[maybe_unused]] const mcontext_t & mcontext) { std::stringstream error; // STYLE_CHECK_ALLOW_STD_STRING_STREAM error.exceptions(std::ios::failbit); @@ -34,7 +34,7 @@ std::string signalToErrorMessage(int sig, const siginfo_t & info, [[maybe_unused error << "Address: " << info.si_addr; #if defined(__x86_64__) && !defined(__FreeBSD__) && !defined(__APPLE__) && !defined(__arm__) && !defined(__powerpc__) - auto err_mask = context.uc_mcontext.gregs[REG_ERR]; + auto err_mask = mcontext.gregs[REG_ERR]; if ((err_mask & 0x02)) error << " Access: write."; else diff --git a/src/Common/StackTrace.h b/src/Common/StackTrace.h index 06a17e73091..697574900d5 100644 --- a/src/Common/StackTrace.h +++ b/src/Common/StackTrace.h @@ -75,4 +75,4 @@ protected: FramePointers frame_pointers{}; }; -std::string signalToErrorMessage(int sig, const siginfo_t & info, const ucontext_t & context); +std::string signalToErrorMessage(int sig, const siginfo_t & info, const mcontext_t & mcontext); From 99c8736f00533c8879fe522e61073fcd63524ebd Mon Sep 17 00:00:00 2001 From: save-my-heart Date: Wed, 26 Jan 2022 22:18:17 +0800 Subject: [PATCH 2/9] fix build error on freebsd & aarch --- base/daemon/BaseDaemon.cpp | 16 ++++++++-------- src/Common/StackTrace.cpp | 4 ++-- src/Common/StackTrace.h | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/base/daemon/BaseDaemon.cpp b/base/daemon/BaseDaemon.cpp index 6ac552e1284..861a872a9f8 100644 --- a/base/daemon/BaseDaemon.cpp +++ b/base/daemon/BaseDaemon.cpp @@ -82,7 +82,7 @@ static void call_default_signal_handler(int sig) static const size_t signal_pipe_buf_size = sizeof(int) + sizeof(siginfo_t) - + sizeof(mcontext_t) + + sizeof(ucontext_t*) + sizeof(StackTrace) + sizeof(UInt32) + sizeof(void*); @@ -125,12 +125,12 @@ static void signalHandler(int sig, siginfo_t * info, void * context) char buf[signal_pipe_buf_size]; DB::WriteBufferFromFileDescriptorDiscardOnFailure out(signal_pipe.fds_rw[1], signal_pipe_buf_size, buf); - const ucontext_t signal_context = *reinterpret_cast(context); - const StackTrace stack_trace(signal_context); + const ucontext_t * signal_context = reinterpret_cast(context); + const StackTrace stack_trace(*signal_context); DB::writeBinary(sig, out); DB::writePODBinary(*info, out); - DB::writePODBinary(signal_context.uc_mcontext, out); + DB::writePODBinary(signal_context, out); DB::writePODBinary(stack_trace, out); DB::writeBinary(UInt32(getThreadId()), out); DB::writePODBinary(DB::current_thread, out); @@ -221,7 +221,7 @@ public: else { siginfo_t info{}; - mcontext_t mcontext{}; + ucontext_t * context{}; StackTrace stack_trace(NoCapture{}); UInt32 thread_num{}; DB::ThreadStatus * thread_ptr{}; @@ -238,7 +238,7 @@ public: /// This allows to receive more signals if failure happens inside onFault function. /// Example: segfault while symbolizing stack trace. - std::thread([=, this] { onFault(sig, info, mcontext, stack_trace, thread_num, thread_ptr); }).detach(); + std::thread([=, this] { onFault(sig, info, *context, stack_trace, thread_num, thread_ptr); }).detach(); } } } @@ -271,7 +271,7 @@ private: void onFault( int sig, const siginfo_t & info, - const mcontext_t & mcontext, + const ucontext_t & context, const StackTrace & stack_trace, UInt32 thread_num, DB::ThreadStatus * thread_ptr) const @@ -314,7 +314,7 @@ private: String error_message; if (sig != SanitizerTrap) - error_message = signalToErrorMessage(sig, info, mcontext); + error_message = signalToErrorMessage(sig, info, context); else error_message = "Sanitizer trap."; diff --git a/src/Common/StackTrace.cpp b/src/Common/StackTrace.cpp index 35d590d7508..18e2e223744 100644 --- a/src/Common/StackTrace.cpp +++ b/src/Common/StackTrace.cpp @@ -19,7 +19,7 @@ # include #endif -std::string signalToErrorMessage(int sig, const siginfo_t & info, [[maybe_unused]] const mcontext_t & mcontext) +std::string signalToErrorMessage(int sig, const siginfo_t & info, [[maybe_unused]] const ucontext_t & context) { std::stringstream error; // STYLE_CHECK_ALLOW_STD_STRING_STREAM error.exceptions(std::ios::failbit); @@ -34,7 +34,7 @@ std::string signalToErrorMessage(int sig, const siginfo_t & info, [[maybe_unused error << "Address: " << info.si_addr; #if defined(__x86_64__) && !defined(__FreeBSD__) && !defined(__APPLE__) && !defined(__arm__) && !defined(__powerpc__) - auto err_mask = mcontext.gregs[REG_ERR]; + auto err_mask = context.uc_mcontext.gregs[REG_ERR]; if ((err_mask & 0x02)) error << " Access: write."; else diff --git a/src/Common/StackTrace.h b/src/Common/StackTrace.h index 697574900d5..06a17e73091 100644 --- a/src/Common/StackTrace.h +++ b/src/Common/StackTrace.h @@ -75,4 +75,4 @@ protected: FramePointers frame_pointers{}; }; -std::string signalToErrorMessage(int sig, const siginfo_t & info, const mcontext_t & mcontext); +std::string signalToErrorMessage(int sig, const siginfo_t & info, const ucontext_t & context); From 2c37b572d41dda9faafe52e92243e9a5135d545d Mon Sep 17 00:00:00 2001 From: save-my-heart Date: Wed, 26 Jan 2022 22:21:18 +0800 Subject: [PATCH 3/9] fix --- base/daemon/BaseDaemon.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/base/daemon/BaseDaemon.cpp b/base/daemon/BaseDaemon.cpp index 861a872a9f8..71e842876be 100644 --- a/base/daemon/BaseDaemon.cpp +++ b/base/daemon/BaseDaemon.cpp @@ -229,7 +229,7 @@ public: if (sig != SanitizerTrap) { DB::readPODBinary(info, in); - DB::readPODBinary(mcontext, in); + DB::readPODBinary(context, in); } DB::readPODBinary(stack_trace, in); From 62ba4833df3ae38dd53f7670cf229521b0c831e4 Mon Sep 17 00:00:00 2001 From: save-my-heart Date: Wed, 26 Jan 2022 22:55:19 +0800 Subject: [PATCH 4/9] fix dereference null pointer --- base/daemon/BaseDaemon.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/base/daemon/BaseDaemon.cpp b/base/daemon/BaseDaemon.cpp index 71e842876be..bd3209280f7 100644 --- a/base/daemon/BaseDaemon.cpp +++ b/base/daemon/BaseDaemon.cpp @@ -238,7 +238,7 @@ public: /// This allows to receive more signals if failure happens inside onFault function. /// Example: segfault while symbolizing stack trace. - std::thread([=, this] { onFault(sig, info, *context, stack_trace, thread_num, thread_ptr); }).detach(); + std::thread([=, this] { onFault(sig, info, context, stack_trace, thread_num, thread_ptr); }).detach(); } } } @@ -271,7 +271,7 @@ private: void onFault( int sig, const siginfo_t & info, - const ucontext_t & context, + ucontext_t * context, const StackTrace & stack_trace, UInt32 thread_num, DB::ThreadStatus * thread_ptr) const @@ -314,7 +314,7 @@ private: String error_message; if (sig != SanitizerTrap) - error_message = signalToErrorMessage(sig, info, context); + error_message = signalToErrorMessage(sig, info, *context); else error_message = "Sanitizer trap."; From cc3cbc65033b9052571ceaa825efd596875196a6 Mon Sep 17 00:00:00 2001 From: save-my-heart Date: Wed, 26 Jan 2022 23:05:07 +0800 Subject: [PATCH 5/9] remove whitespace to pass style check --- base/daemon/BaseDaemon.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/base/daemon/BaseDaemon.cpp b/base/daemon/BaseDaemon.cpp index bd3209280f7..91bb007e78a 100644 --- a/base/daemon/BaseDaemon.cpp +++ b/base/daemon/BaseDaemon.cpp @@ -286,7 +286,7 @@ private: if (thread_ptr) { query_id = thread_ptr->getQueryId().toString(); - + if (auto thread_group = thread_ptr->getThreadGroup()) { query = thread_group->query; From eae0bc7c04aeb4c54ef39305253f1178e9e67c75 Mon Sep 17 00:00:00 2001 From: kssenii Date: Wed, 2 Feb 2022 18:19:33 +0300 Subject: [PATCH 6/9] add query id for delayed interactive --- src/Client/ClientBase.cpp | 36 ++++++++++++++++++++++++------------ src/Client/ClientBase.h | 2 ++ 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/Client/ClientBase.cpp b/src/Client/ClientBase.cpp index 27deace416d..1958b1d81ce 100644 --- a/src/Client/ClientBase.cpp +++ b/src/Client/ClientBase.cpp @@ -1485,6 +1485,25 @@ String ClientBase::prompt() const } +void ClientBase::initQueryIdFormats() +{ + if (!query_id_formats.empty()) + return; + + /// Initialize query_id_formats if any + if (config().has("query_id_formats")) + { + Poco::Util::AbstractConfiguration::Keys keys; + config().keys("query_id_formats", keys); + for (const auto & name : keys) + query_id_formats.emplace_back(name + ":", config().getString("query_id_formats." + name)); + } + + if (query_id_formats.empty()) + query_id_formats.emplace_back("Query id:", " {query_id}\n"); +} + + void ClientBase::runInteractive() { if (config().has("query_id")) @@ -1492,6 +1511,8 @@ void ClientBase::runInteractive() if (print_time_to_stderr) throw Exception("time option could be specified only in non-interactive mode", ErrorCodes::BAD_ARGUMENTS); + initQueryIdFormats(); + /// Initialize DateLUT here to avoid counting time spent here as query execution time. const auto local_tz = DateLUT::instance().getTimeZone(); @@ -1512,18 +1533,6 @@ void ClientBase::runInteractive() home_path = home_path_cstr; } - /// Initialize query_id_formats if any - if (config().has("query_id_formats")) - { - Poco::Util::AbstractConfiguration::Keys keys; - config().keys("query_id_formats", keys); - for (const auto & name : keys) - query_id_formats.emplace_back(name + ":", config().getString("query_id_formats." + name)); - } - - if (query_id_formats.empty()) - query_id_formats.emplace_back("Query id:", " {query_id}\n"); - /// Load command history if present. if (config().has("history_file")) history_file = config().getString("history_file"); @@ -1632,6 +1641,9 @@ void ClientBase::runInteractive() void ClientBase::runNonInteractive() { + if (delayed_interactive) + initQueryIdFormats(); + if (!queries_files.empty()) { auto process_multi_query_from_file = [&](const String & file) diff --git a/src/Client/ClientBase.h b/src/Client/ClientBase.h index 89e0770182b..e2cd91d1e5f 100644 --- a/src/Client/ClientBase.h +++ b/src/Client/ClientBase.h @@ -138,6 +138,8 @@ private: void updateSuggest(const ASTCreateQuery & ast_create); + void initQueryIdFormats(); + protected: bool is_interactive = false; /// Use either interactive line editing interface or batch mode. bool is_multiquery = false; From 2e8adc797c1be512bd7a30dce3c493673c629e58 Mon Sep 17 00:00:00 2001 From: Amos Bird Date: Sat, 5 Feb 2022 22:05:05 +0800 Subject: [PATCH 7/9] Better handle pre-inputs before client start --- contrib/replxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/replxx b/contrib/replxx index c745b3fb012..9460e5e0fc1 160000 --- a/contrib/replxx +++ b/contrib/replxx @@ -1 +1 @@ -Subproject commit c745b3fb012ee5ae762fbc8cd7a40c4dc3fe15df +Subproject commit 9460e5e0fc10f78f460af26a6bd928798cac864d From 35235d2d7f2dae3775cf9ea0b206fafb13208e04 Mon Sep 17 00:00:00 2001 From: Maksim Kita Date: Sat, 5 Feb 2022 16:11:36 +0000 Subject: [PATCH 8/9] Added additional performance test --- tests/performance/map_populate_series.xml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/performance/map_populate_series.xml b/tests/performance/map_populate_series.xml index a050be6f3a8..db40cf09455 100644 --- a/tests/performance/map_populate_series.xml +++ b/tests/performance/map_populate_series.xml @@ -1,4 +1,6 @@ SELECT mapPopulateSeries(range(number), range(number)) FROM numbers(5000) FORMAT Null; SELECT mapPopulateSeries(range(number), range(number), 2500) FROM numbers(5000) FORMAT Null; + SELECT mapPopulateSeries(map(0, 0, number, 5)) FROM numbers(5000) FORMAT Null; + SELECT mapPopulateSeries(map(0, 0, number, 5), 2500) FROM numbers(5000) FORMAT Null; From f2725b1b2201c3dd42ed2ff72511b1791ba959d1 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Sat, 5 Feb 2022 18:26:43 +0100 Subject: [PATCH 9/9] Fix wrong OK_SKIP labels --- tests/ci/run_check.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ci/run_check.py b/tests/ci/run_check.py index dc3f1da94cd..a2403e61ac1 100644 --- a/tests/ci/run_check.py +++ b/tests/ci/run_check.py @@ -18,7 +18,7 @@ TRUSTED_ORG_IDS = { 54801242, # clickhouse } -OK_SKIP_LABELS = {"release", "pr-documentation", "pr-doc-fix"} +OK_SKIP_LABELS = {"release", "pr-backport", "pr-cherrypick"} CAN_BE_TESTED_LABEL = "can be tested" DO_NOT_TEST_LABEL = "do not test" FORCE_TESTS_LABEL = "force tests"