From 8556929f50f1d642609453872e344fafe20df5fa Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Thu, 26 Oct 2023 08:46:06 +0200 Subject: [PATCH] Fix mixing of send_timeout and receive_timeout I've noticed this in one of production setups, where lots of queries are executed with distributed_group_by_no_merge=2 (automatically vai optimize_skip_unused_shards optimization) and optimize_aggregation_in_order=1, with this two settings initiator may not read from some shards (i.e. X) for quite long period of time, because it reads from other shards (i.e. Y) and it does not need any data from X yet (due to it read everything in order), and this will lead to query timeout, so timeouts had been increased. Previously both timeouts had been tuned, but this leads to connection hungs in case of abnormal machine reboots. So it is better to tune only send_timeout (and this should be enough, since the only problem is the sender) and this will allow to see that the connection is broken on the initiator once it will read from this shard. but after changing only send_timeout, the query still timedout, and the reason is this place, which swaps this timeouts. It had been introduced in 134efcd, with a comment: NOTE: We use send_timeout for the receive timeout and vice versa (change arguments ordering in TimeoutSetter), because send_timeout is client-side setting which has opposite meaning on the server side. But it sounds odd to me, it may only make sense with the clickhouse-client, since any other driver does not implement any server settings. Signed-off-by: Azat Khuzhin --- src/Server/TCPHandler.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Server/TCPHandler.cpp b/src/Server/TCPHandler.cpp index 871606c6298..5082d8e4f3b 100644 --- a/src/Server/TCPHandler.cpp +++ b/src/Server/TCPHandler.cpp @@ -377,10 +377,7 @@ void TCPHandler::runImpl() extractConnectionSettingsFromContext(query_context); /// Sync timeouts on client and server during current query to avoid dangling queries on server - /// NOTE: We use send_timeout for the receive timeout and vice versa (change arguments ordering in TimeoutSetter), - /// because send_timeout is client-side setting which has opposite meaning on the server side. - /// NOTE: these settings are applied only for current connection (not for distributed tables' connections) - state.timeout_setter = std::make_unique(socket(), receive_timeout, send_timeout); + state.timeout_setter = std::make_unique(socket(), send_timeout, receive_timeout); /// Should we send internal logs to client? const auto client_logs_level = query_context->getSettingsRef().send_logs_level;