Fix max_distributed_connections by azat · Pull Request #9673 · ClickHouse/ClickHouse

azat · 2020-03-15T21:55:58Z

Changelog category (leave one):

Bug Fix

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix max_distributed_connections (w/ and w/o Processors)

qoega · 2020-03-16T07:07:21Z

Probably we should find better way to test it:

2020-03-16 02:27:51 01085_max_distributed_connections:                                      [ FAIL ] 1.87 sec. - return code 124

azat · 2020-03-16T07:36:35Z

Probably we should find better way to test it:

Actually the test fails correctly without processors, digging

KochetovNicolai · 2020-03-16T09:43:31Z

Let me clarify what's going on. When we build query pipeline, there are two parameters we are interested in: the number of streams we build (max_streams) and the number of threads we use (max_threads) to execute query.

Usually, max_streams = max_threads = settings.max_threads. Let's look at the cases when it is not:

Storage returned less streams then settings.max_threads and query is quite simple. For example, select * from system.numbers. Then max_threads = 1 < max_streams = settings.max_threads.
Setting settings.max_streams_to_max_threads_ratio > 1. Then max_streams = settings.max_threads * settings.max_streams_to_max_threads_ratio, max_threads = settings.max_threads.
Query is remote, setting settings.max_distributed_connections is used instead of settings.max_threads.

In order to implement 1., QueryPipeline calculates the maximum number of streams used at each step (see QueryPipeline::Streams::maxParallelStreams()). The number of threads is limited by QueryPipeline::getNumThreads() at TCPHandler.

In order to support 2., the number of threads was additionally limited by settings.max_threads in TCPHandler. But it is not good, because 3 is not correct after this (we use less threads then expected).

This pr fixes 3., but broke 2, because max_threads be settings.max_threads * settings.max_streams_to_max_threads_ratio.

It is easy to fix - set query_pipeline.setMaxThreads() not in InterpreterSelectQuery::executeWithProcessors, but in InterpreterSelectQuery::executeFetchColumns, when settings max_streams_to_max_threads_ratio and max_distributed_connections are checked. Then it will be possible to get the number of threads directly from query pipeline, which is quite logical.

dbms/programs/server/TCPHandler.cpp

KochetovNicolai · 2020-03-16T09:51:04Z

Please also look here. I have forgotten to limit the number of threads for http case. It must also use QueryPipeline::getNumThreads().

alexey-milovidov · 2020-03-16T10:22:09Z

Added "bugfix" label because this was significant performance regression that will be visible on large clusters like those we have in Yandex Metrica (and sadly enough is not visible on any testing environments).

azat · 2020-03-16T19:11:53Z

Please also look here

@KochetovNicolai it had been fixed already

KochetovNicolai · 2020-03-16T19:13:49Z

dbms/src/Interpreters/InterpreterSelectWithUnionQuery.cpp

I suppose using minimum is not the best choice. Imagine query like select number from system.number union all select number from system.numbers. It could have been executed in in 2 threads, but after this change it it will use only one.

I think we should think more about it.

Indeed (I also though like this), and this will go away, already testing better approach

Hm, I though that I found better alternative but I didn't (too much interpreters:) )

As for union all:

if max_threads=1 will be specified than it should use one thread (like before)

if max_threads=2 will be specified than it will be runned in parallel (like before, since each interpreter will have max_streams=2)

@KochetovNicolai what did I miss?

FWIW:

$ time clickhouse-client --max_threads 1 -q 'select sleep(1) union all select sleep(1)' 0 0 real 0m2.322s $ time clickhouse-client --max_threads 2 -q 'select sleep(1) union all select sleep(1)' 0 0 real 0m1.327s $ time clickhouse-client --max_threads 2 -q 'select sleep(1) union all select sleep(1)' --experimental_use_processors 0 0 0 real 0m1.419s $ time clickhouse-client --max_threads 1 -q 'select sleep(1) union all select sleep(1)' --experimental_use_processors 0 0 0 real 0m2.421s

KochetovNicolai · 2020-03-16T19:15:56Z

@KochetovNicolai it had been fixed already

Thank you!

azat · 2020-03-17T07:15:23Z

Tests failures looks unrelated:

00834_kill_mutation_replicated_zookeeper
SELECT arraySlice(arrayFill(x -> (((x.1) % 2) = 0), arrayMap(x -> (x, toString(x)), range(100000000))), 1, 10) (perf)

azat · 2020-03-17T07:38:30Z

Please also look here. I have forgotten to limit the number of threads for http case. It must also use QueryPipeline::getNumThreads().

@KochetovNicolai added a test to avoid regression - 7c24cee9b79878d20278be89c3994ba59d5aa1ca

dbms/tests/queries/0_stateless/01085_max_distributed_connections_http.sh

dbms/src/Interpreters/InterpreterSelectQuery.cpp

KochetovNicolai · 2020-03-19T09:15:09Z

01085_max_distributed_connections_http fails :(

KochetovNicolai · 2020-03-19T09:26:07Z

Actually, it seems that reference file is wrong.

KochetovNicolai · 2020-03-19T09:27:40Z

        if (max_streams > 1 && !is_remote)
            max_streams *= settings.max_streams_to_max_threads_ratio;

I think we need to limit the number of threads here too.

azat · 2020-03-19T09:30:41Z

Actually, it seems that reference file is wrong.

Yep, you are right!

azat · 2020-03-19T09:31:12Z

I think we need to limit the number of threads here too.

So revert the last change?

KochetovNicolai · 2020-03-19T09:45:34Z

So revert the last change?

Well, let's not revert anything.
Let's just fix you test and merge this pr. Because it solves the issue and all tests are fine.
Then I check how max_streams_to_max_threads_ratio works and fix it if something wrong separetly.

azat · 2020-03-19T21:32:14Z

Functional stateless tests (thread) — fail: 20, passed: 1522, skipped: 8

00111_shard_external_sort_distributed breaks everything after, but I cannot find a lot from the logs (maybe clickhouse-server had been killed by OOM, especially TSAN version)

P.S. recent changes passes all tests except for 01085_max_distributed_connections_http, where wrong .reference file had been provided)

azat · 2020-03-20T10:41:07Z

Functional stateless tests (thread) — fail: 20, passed: 1522, skipped: 8

00965_live_view_watch_heartbeat (does not looks relevant)

P.S. I can rebase to force CI

With Processors max_distributed_connections does not used anymore, since it always uses not more then max_threads (see TCPHandler::processOrdinaryQueryWithProcessors()/ QueryPipeline::execute()/PipelineExecutor::execute()) So just use QueryPipeline::getNumThreads(), that takes max_distributed_connections into account.

azat · 2020-03-21T06:50:24Z

Functional stateless tests (address)

Looks flacky:

01006_simpod_empty_part_single_column_write
01019_alter_materialized_view_consistent

alexey-milovidov · 2020-03-24T05:31:06Z

Unfortunately, these tests clearly indicate a bug (most likely not in your PR).

…ions Fix max_distributed_connections (cherry picked from commit 9a2760d)

With prefer_localhost_replica=1 max_distributed_connections uses max_threads before this patch. Fixes: ClickHouse#14936 Previous fix: ClickHouse#9673

qoega added no-docs-needed pr-improvement Pull request with some product improvements labels Mar 16, 2020

KochetovNicolai requested changes Mar 16, 2020

View reviewed changes

dbms/programs/server/TCPHandler.cpp Outdated Show resolved Hide resolved

alexey-milovidov added the pr-bugfix Pull request with bugfix, not backported by default label Mar 16, 2020

KochetovNicolai reviewed Mar 16, 2020

View reviewed changes

azat force-pushed the processors-max_distributed_connections branch from af92dc6 to 7659d44 Compare March 16, 2020 19:44

azat changed the title ~~Fix max_distributed_connections under Processors~~ Fix max_distributed_connections Mar 16, 2020

azat force-pushed the processors-max_distributed_connections branch from 7659d44 to afbe87c Compare March 16, 2020 20:42

azat commented Mar 17, 2020

View reviewed changes

dbms/tests/queries/0_stateless/01085_max_distributed_connections_http.sh Outdated Show resolved Hide resolved

azat requested a review from KochetovNicolai March 17, 2020 18:57

KochetovNicolai reviewed Mar 18, 2020

View reviewed changes

dbms/src/Interpreters/InterpreterSelectQuery.cpp Outdated Show resolved Hide resolved

azat force-pushed the processors-max_distributed_connections branch from 7c24cee to 3d126e4 Compare March 18, 2020 17:32

KochetovNicolai approved these changes Mar 19, 2020

View reviewed changes

alesapin added the can be tested label Mar 19, 2020

azat force-pushed the processors-max_distributed_connections branch from 3d126e4 to b90d094 Compare March 19, 2020 17:22

azat added 5 commits March 20, 2020 20:51

Add missing space in StorageDistributed logging (sharding key)

62b60a0

Test for max_distributed_connections (via TCP)

3910540

Test for max_distributed_connections via HTTP

977849d

Fix max_distributed_connections w/o Processors

550cfef

azat force-pushed the processors-max_distributed_connections branch from b90d094 to 550cfef Compare March 20, 2020 17:56

alexey-milovidov approved these changes Mar 24, 2020

View reviewed changes

alexey-milovidov merged commit 9a2760d into ClickHouse:master Mar 24, 2020

azat deleted the processors-max_distributed_connections branch March 24, 2020 06:43

alesapin pushed a commit that referenced this pull request Mar 26, 2020

Merge pull request #9673 from azat/processors-max_distributed_connect…

cb49e8b

…ions Fix max_distributed_connections (cherry picked from commit 9a2760d)

alesapin added v20.3 labels Mar 26, 2020

azat mentioned this pull request May 14, 2020

Distributed table is slower than a query containing union all for each remote servers. #6464

Closed

akuzm added v20.3-backported and removed v20.3 labels Jun 5, 2020

azat added a commit to azat/ClickHouse that referenced this pull request Dec 6, 2020

Fix max_distributed_connections (one more time)

c3a4e5e

With prefer_localhost_replica=1 max_distributed_connections uses max_threads before this patch. Fixes: ClickHouse#14936 Previous fix: ClickHouse#9673

Conversation

azat commented Mar 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qoega commented Mar 16, 2020

Uh oh!

azat commented Mar 16, 2020

Uh oh!

KochetovNicolai commented Mar 16, 2020

Uh oh!

Uh oh!

KochetovNicolai commented Mar 16, 2020

Uh oh!

alexey-milovidov commented Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azat commented Mar 16, 2020

Uh oh!

KochetovNicolai Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

azat Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

azat Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

azat Mar 17, 2020

Choose a reason for hiding this comment

Uh oh!

KochetovNicolai commented Mar 16, 2020

Uh oh!

azat commented Mar 17, 2020

Uh oh!

azat commented Mar 17, 2020

Uh oh!

Uh oh!

Uh oh!

KochetovNicolai commented Mar 19, 2020

Uh oh!

KochetovNicolai commented Mar 19, 2020

Uh oh!

KochetovNicolai commented Mar 19, 2020

Uh oh!

azat commented Mar 19, 2020

Uh oh!

azat commented Mar 19, 2020

Uh oh!

KochetovNicolai commented Mar 19, 2020

Uh oh!

azat commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azat commented Mar 20, 2020

Uh oh!

azat commented Mar 21, 2020

Uh oh!

alexey-milovidov commented Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

azat commented Mar 15, 2020 •

edited

Loading

alexey-milovidov commented Mar 16, 2020 •

edited

Loading

azat commented Mar 19, 2020 •

edited

Loading

alexey-milovidov commented Mar 24, 2020 •

edited

Loading