From 5cc5ca22debcf84eb053006465a0c9eaf11e5f27 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Thu, 15 Sep 2022 11:15:19 +0200 Subject: [PATCH 1/7] Fix possible deadlock with async_socket_for_remote/use_hedged_requests and parallel KILL Right now it is possible to call QueryStatus::addPipelineExecutor() when the executors_mutex already acquired, it is possible when the query was cancelled via KILL QUERY. Here I will show some traces from debugger from a real example, where tons of ProcessList::insert() got deadlocked. Let's look at the lock owner for one of the threads that was deadlocked in ProcessList::insert(): (gdb) p *mutex $2 = { __data = { __owner = 46899, }, } And now let's see the stack trace of the 46899: #0 __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:103 #1 0x00007fb65569b714 in __GI___pthread_mutex_lock (mutex=0x7fb4a9d15298) at ../nptl/pthread_mutex_lock.c:80 #2 0x000000001b6edd91 in pthread_mutex_lock (arg=0x7fb4a9d15298) at ../src/Common/ThreadFuzzer.cpp:317 #3 std::__1::__libcpp_mutex_lock (__m=0x7fb4a9d15298) at ../contrib/libcxx/include/__threading_support:303 #4 std::__1::mutex::lock (this=0x7fb4a9d15298) at ../contrib/libcxx/src/mutex.cpp:33 #5 0x0000000014c7ae63 in std::__1::lock_guard::lock_guard (__m=..., this=) at ../contrib/libcxx/include/__mutex_base:91 #6 DB::QueryStatus::addPipelineExecutor (this=0x7fb4a9d14f90, e=0x80) at ../src/Interpreters/ProcessList.cpp:372 #7 0x0000000015bee4a7 in DB::PipelineExecutor::PipelineExecutor (this=0x7fb4b1e53618, processors=..., elem=) at ../src/Processors/Executors/PipelineExecutor.cpp:54 #12 std::__1::make_shared, std::__1::allocator > >&, DB::QueryStatus*&, void> (__args=@0x7fb63095b9b0: 0x7fb4a9d14f90, __args=@0x7fb63095b9b0: 0x7fb4a9d14f90) at ../contrib/libcxx/include/__memory/shared_ptr.h:963 #13 DB::QueryPipelineBuilder::execute (this=0x7fb63095b8b0) at ../src/QueryPipeline/QueryPipelineBuilder.cpp:552 #14 0x00000000158c6c27 in DB::Connection::sendExternalTablesData (this=0x7fb6545e9d98, data=...) at ../src/Client/Connection.cpp:797 #27 0x0000000014043a81 in DB::RemoteQueryExecutorRoutine::operator() (this=0x7fb63095bf20, sink=...) at ../src/QueryPipeline/RemoteQueryExecutorReadContext.cpp:46 #32 0x000000000a16dd4f in make_fcontext () at ../contrib/boost/libs/context/src/asm/make_x86_64_sysv_elf_gas.S:71 And also in the logs you can see very strange things for this thread: 2022.09.13 14:14:51.228979 [ 51145 ] {1712D4E914EC7C99} Connection (localhost:9000): Sent data for 1 external tables, total 11 rows in 0.00046389 sec., 23688 rows/sec., 3.84 KiB (8.07 MiB/sec.), compressed 1.1070121092649958 times to 3.47 KiB (7.29 MiB/sec.) ... 2022.09.13 14:14:51.719402 [ 46899 ] {7c90ffa4-1dc8-42fd-938c-4e307c244394} executeQuery: (from 10.101.15.181:42478) KILL QUERY WHERE query_id = '1712D4E914EC7C99' (stage: Complete) 2022.09.13 14:14:51.719488 [ 46899 ] {7c90ffa4-1dc8-42fd-938c-4e307c244394} executeQuery: (internal) SELECT query_id, user, query FROM system.processes WHERE query_id = '1712D4E914EC7C99' (stage: Complete) 2022.09.13 14:14:51.719754 [ 46899 ] {7c90ffa4-1dc8-42fd-938c-4e307c244394} ContextAccess (default): Access granted: SELECT(user, query_id, query) ON system.processes 2022.09.13 14:14:51.720544 [ 46899 ] {7c90ffa4-1dc8-42fd-938c-4e307c244394} InterpreterSelectQuery: FetchColumns -> Complete 2022.09.13 14:14:53.228964 [ 46899 ] {7c90ffa4-1dc8-42fd-938c-4e307c244394} Connection (localhost:9000): Sent data for 2 scalars, total 2 rows in 2.6838e-05 sec., 73461 rows/sec., 68.00 B (2.38 MiB/sec.), compressed 0.4594594594594595 times to 148.00 B (5.16 MiB/sec.) How is this possible? The answer is fibers and query cancellation routine. During cancellation of async queries it going into fibers again and try to do this gracefully. However because of this during canceling query it may call QueryStatus::addPipelineExecutor() from QueryStatus::cancelQuery(). Signed-off-by: Azat Khuzhin --- src/Interpreters/ProcessList.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Interpreters/ProcessList.cpp b/src/Interpreters/ProcessList.cpp index e21d3814086..35124696d4c 100644 --- a/src/Interpreters/ProcessList.cpp +++ b/src/Interpreters/ProcessList.cpp @@ -369,6 +369,12 @@ CancellationCode QueryStatus::cancelQuery(bool) void QueryStatus::addPipelineExecutor(PipelineExecutor * e) { + /// In case of asynchronous distributed queries it is possible to call + /// addPipelineExecutor() from the cancelQuery() context, and this will + /// lead to deadlock. + if (is_killed.load()) + throw Exception("Query was cancelled", ErrorCodes::QUERY_WAS_CANCELLED); + std::lock_guard lock(executors_mutex); assert(std::find(executors.begin(), executors.end(), e) == executors.end()); executors.push_back(e); From e8888593962b73086b641a7c3963a343260db39a Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Sun, 18 Sep 2022 22:21:13 +0000 Subject: [PATCH 2/7] Fix: correct sort description for ReadFromMergeTree with read in order optimization --- src/Core/SortDescription.h | 2 ++ src/Processors/QueryPlan/ReadFromMergeTree.cpp | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/src/Core/SortDescription.h b/src/Core/SortDescription.h index 0025e44b489..20a4bef8176 100644 --- a/src/Core/SortDescription.h +++ b/src/Core/SortDescription.h @@ -48,6 +48,8 @@ struct SortColumnDescription bool with_fill; FillColumnDescription fill_description; + SortColumnDescription() = default; + explicit SortColumnDescription( const std::string & column_name_, int direction_ = 1, diff --git a/src/Processors/QueryPlan/ReadFromMergeTree.cpp b/src/Processors/QueryPlan/ReadFromMergeTree.cpp index 60bf8d6a15c..67ff85f89ae 100644 --- a/src/Processors/QueryPlan/ReadFromMergeTree.cpp +++ b/src/Processors/QueryPlan/ReadFromMergeTree.cpp @@ -143,7 +143,12 @@ ReadFromMergeTree::ReadFromMergeTree( { auto const & settings = context->getSettingsRef(); if ((settings.optimize_read_in_order || settings.optimize_aggregation_in_order) && query_info.getInputOrderInfo()) + { output_stream->sort_scope = DataStream::SortScope::Stream; + const auto used_prefix_of_sorting_key_size = query_info.getInputOrderInfo()->used_prefix_of_sorting_key_size; + if (sort_description.size() > used_prefix_of_sorting_key_size) + sort_description.resize(used_prefix_of_sorting_key_size); + } else output_stream->sort_scope = DataStream::SortScope::Chunk; } From 54c2fed9ccb5d5b17a221bb78eb4a3812908d27c Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Mon, 19 Sep 2022 09:31:00 +0000 Subject: [PATCH 3/7] Update: use type explicitly --- src/Processors/QueryPlan/ReadFromMergeTree.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Processors/QueryPlan/ReadFromMergeTree.cpp b/src/Processors/QueryPlan/ReadFromMergeTree.cpp index 67ff85f89ae..91777f7d642 100644 --- a/src/Processors/QueryPlan/ReadFromMergeTree.cpp +++ b/src/Processors/QueryPlan/ReadFromMergeTree.cpp @@ -145,7 +145,7 @@ ReadFromMergeTree::ReadFromMergeTree( if ((settings.optimize_read_in_order || settings.optimize_aggregation_in_order) && query_info.getInputOrderInfo()) { output_stream->sort_scope = DataStream::SortScope::Stream; - const auto used_prefix_of_sorting_key_size = query_info.getInputOrderInfo()->used_prefix_of_sorting_key_size; + const size_t used_prefix_of_sorting_key_size = query_info.getInputOrderInfo()->used_prefix_of_sorting_key_size; if (sort_description.size() > used_prefix_of_sorting_key_size) sort_description.resize(used_prefix_of_sorting_key_size); } From eef638d58e76c3f0e3ea4ba97fee6c6eb1dacd9d Mon Sep 17 00:00:00 2001 From: kssenii Date: Mon, 19 Sep 2022 17:18:53 +0200 Subject: [PATCH 4/7] Disable random settings for s3 && (tsan || debug) --- tests/clickhouse-test | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/clickhouse-test b/tests/clickhouse-test index 14cf4d0674a..56f086aa2bd 100755 --- a/tests/clickhouse-test +++ b/tests/clickhouse-test @@ -2341,4 +2341,7 @@ if __name__ == "__main__": if args.db_engine and args.db_engine == "Ordinary": MESSAGES_TO_RETRY.append(" locking attempt on ") + if args.s3_storage and (BuildFlags.THREAD in args.build_flags or BuildFlags.DEBUG in args.build_flags): + args.no_random_settings = True + main(args) From 3419a47848b26ff0a66f9a0b7f8cd058acd09dce Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Mon, 19 Sep 2022 16:51:43 +0000 Subject: [PATCH 5/7] + test --- ...e_sorting_by_input_stream_properties_explain.reference | 8 ++++++-- ...optimize_sorting_by_input_stream_properties_explain.sh | 5 ++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/queries/0_stateless/02377_optimize_sorting_by_input_stream_properties_explain.reference b/tests/queries/0_stateless/02377_optimize_sorting_by_input_stream_properties_explain.reference index 1ad64150049..a7498e68bc0 100644 --- a/tests/queries/0_stateless/02377_optimize_sorting_by_input_stream_properties_explain.reference +++ b/tests/queries/0_stateless/02377_optimize_sorting_by_input_stream_properties_explain.reference @@ -21,7 +21,6 @@ PartialSortingTransform -- ExpressionStep preserves sort mode -- QUERY: set optimize_read_in_order=1;EXPLAIN PLAN actions=1, header=1, sorting=1 SELECT a FROM optimize_sorting ORDER BY a Sorting (Global): a ASC -Sorting Sorting (Global): a ASC Sorting (Stream): a ASC Sorting (Stream): a ASC @@ -66,7 +65,6 @@ Sorting (Global): a ASC Sorting (Sorting for ORDER BY) Sorting (Global): a ASC Sorting (None) -Sorting Sorting (Global): a ASC Sorting (Stream): a ASC Sorting (Stream): a ASC @@ -87,3 +85,9 @@ Sorting (Sorting for ORDER BY) Sorting (Global): plus(a, 1) ASC Sorting (Chunk): a ASC Sorting (Chunk): a ASC +-- check that correct sorting info is provided in case of only prefix of sorting key is in ORDER BY clause but all sorting key columns returned by query +-- QUERY: set optimize_read_in_order=1;EXPLAIN PLAN sorting=1 SELECT a, b FROM optimize_sorting ORDER BY a +Sorting (Global): a ASC +Sorting (Global): a ASC +Sorting (Stream): a ASC +Sorting (Stream): a ASC diff --git a/tests/queries/0_stateless/02377_optimize_sorting_by_input_stream_properties_explain.sh b/tests/queries/0_stateless/02377_optimize_sorting_by_input_stream_properties_explain.sh index cea76b7d6ea..a308d9bcbc1 100755 --- a/tests/queries/0_stateless/02377_optimize_sorting_by_input_stream_properties_explain.sh +++ b/tests/queries/0_stateless/02377_optimize_sorting_by_input_stream_properties_explain.sh @@ -8,7 +8,7 @@ DISABLE_OPTIMIZATION="set optimize_sorting_by_input_stream_properties=0;set max_ ENABLE_OPTIMIZATION="set optimize_sorting_by_input_stream_properties=1;set max_threads=1" MAKE_OUTPUT_STABLE="set optimize_read_in_order=1" GREP_SORTING="grep 'PartialSortingTransform\|LimitsCheckingTransform\|MergeSortingTransform\|MergingSortedTransform'" -GREP_SORTMODE="grep 'Sorting'" +GREP_SORTMODE="grep 'Sorting ('" TRIM_LEADING_SPACES="sed -e 's/^[ \t]*//'" FIND_SORTING="$GREP_SORTING | $TRIM_LEADING_SPACES" FIND_SORTMODE="$GREP_SORTMODE | $TRIM_LEADING_SPACES" @@ -72,4 +72,7 @@ explain_sortmode "$MAKE_OUTPUT_STABLE;EXPLAIN PLAN actions=1, header=1, sorting= echo "-- actions chain breaks sorting order: input(column a)->sipHash64(column a)->alias(sipHash64(column a), a)->plus(alias a, 1)" explain_sortmode "$MAKE_OUTPUT_STABLE;EXPLAIN PLAN actions=1, header=1, sorting=1 SELECT a, z FROM (SELECT sipHash64(a) AS a, a + 1 AS z FROM (SELECT a FROM optimize_sorting ORDER BY a + 1)) ORDER BY a + 1" +echo "-- check that correct sorting info is provided in case of only prefix of sorting key is in ORDER BY clause but all sorting key columns returned by query" +explain_sortmode "$MAKE_OUTPUT_STABLE;EXPLAIN PLAN sorting=1 SELECT a, b FROM optimize_sorting ORDER BY a" + $CLICKHOUSE_CLIENT -q "drop table if exists optimize_sorting sync" From f3844b5e250ebac519eca1756e8ed2f682db4099 Mon Sep 17 00:00:00 2001 From: Kseniia Sumarokova <54203879+kssenii@users.noreply.github.com> Date: Tue, 20 Sep 2022 12:25:51 +0200 Subject: [PATCH 6/7] Update clickhouse-test --- tests/clickhouse-test | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/clickhouse-test b/tests/clickhouse-test index 56f086aa2bd..b0b03daf3b5 100755 --- a/tests/clickhouse-test +++ b/tests/clickhouse-test @@ -1808,6 +1808,9 @@ def main(args): args, "system", "processes", "is_all_data_sent" ) + if args.s3_storage and (BuildFlags.THREAD in args.build_flags or BuildFlags.DEBUG in args.build_flags): + args.no_random_settings = True + if args.skip: args.skip = set(args.skip) @@ -2341,7 +2344,4 @@ if __name__ == "__main__": if args.db_engine and args.db_engine == "Ordinary": MESSAGES_TO_RETRY.append(" locking attempt on ") - if args.s3_storage and (BuildFlags.THREAD in args.build_flags or BuildFlags.DEBUG in args.build_flags): - args.no_random_settings = True - main(args) From 2dff3077708347bbf8d02f19e912acc11a16200b Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Tue, 20 Sep 2022 13:41:13 +0200 Subject: [PATCH 7/7] Build latest tags ONLY from master branch --- tests/ci/docker_images_check.py | 2 +- tests/ci/docker_test.py | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/ci/docker_images_check.py b/tests/ci/docker_images_check.py index 181555f3a94..773f3ac1b57 100644 --- a/tests/ci/docker_images_check.py +++ b/tests/ci/docker_images_check.py @@ -164,7 +164,7 @@ def gen_versions( # The order is important, PR number is used as cache during the build versions = [str(pr_info.number), pr_commit_version] result_version = pr_commit_version - if pr_info.number == 0: + if pr_info.number == 0 and pr_info.base_name == "master": # First get the latest for cache versions.insert(0, "latest") diff --git a/tests/ci/docker_test.py b/tests/ci/docker_test.py index 32df6d5f1d0..740cae5bc97 100644 --- a/tests/ci/docker_test.py +++ b/tests/ci/docker_test.py @@ -99,6 +99,11 @@ class TestDockerImageCheck(unittest.TestCase): def test_gen_version(self): pr_info = PRInfo(PRInfo.default_event.copy()) + pr_info.base_name = "anything-else" + versions, result_version = di.gen_versions(pr_info, None) + self.assertEqual(versions, ["0", "0-HEAD"]) + self.assertEqual(result_version, "0-HEAD") + pr_info.base_name = "master" versions, result_version = di.gen_versions(pr_info, None) self.assertEqual(versions, ["latest", "0", "0-HEAD"]) self.assertEqual(result_version, "0-HEAD")