Disable logic max_threads=max_distributed_connections when async_socket_for_remote=1

[filimonov]

Changelog category (leave one):

• Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Distributed queries executed in async_socket_for_remote mode (default) now respect max_threads limit. Previously, some queries could create excessive threads (up to max_distributed_connections), causing server performance issues.

Documentation entry for user-facing changes

See #53287

[robot-ch-test-poll]

This is an automated comment for commit aa2bf5a with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Successful checks

Check name	Description	Status
AST fuzzer	Runs randomly generated queries to catch program errors. The build type is optionally given in parenthesis. If it fails, ask a maintainer for help	✅ success
CI running	A meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR	✅ success
ClickHouse build check	Builds ClickHouse in various configurations for use in further steps. You have to fix the builds that fail. Build logs often has enough information to fix the error, but you might have to reproduce the failure locally. The cmake options can be found in the build log, grepping for cmake. Use these options and follow the general build process	✅ success
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	✅ success
Docker image for servers	The check to build and optionally push the mentioned image to docker hub	✅ success
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	✅ success
Flaky tests	Checks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integrational tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc	✅ success
Install packages	Checks that the built packages are installable in a clear environment	✅ success
Mergeable Check	Checks if all other necessary checks are successful	✅ success
Performance Comparison	Measure changes in query performance. The performance test report is described in detail here. In square brackets are the optional part/total tests	✅ success
Push to Dockerhub	The check for building and pushing the CI related docker images to docker hub	✅ success
SQLTest	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
SQLancer	Fuzzing tests that detect logical bugs with SQLancer tool	✅ success
Sqllogic	Run clickhouse on the sqllogic test set against sqlite and checks that all statements are passed	✅ success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Style Check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	✅ success
Unit tests	Runs the unit tests for different release types	✅ success

Check name	Description	Status
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	❌ failure
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	❌ failure
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	❌ failure
Upgrade check	Runs stress tests on server version from last release and then tries to upgrade it to the version from the PR. It checks if the new server can successfully startup without any errors, crashes or sanitizer asserts	❌ failure

[den-crane]

# cat xxx.sql.j2

select count() from remote('127.0.0.1:9000', view(

{% for n in range(10) -%}
SELECT * FROM system.one {{ "UNION ALL" if not loop.last }}
{% endfor -%}

)) SETTINGS log_comment = 'check_concurrency_in_remote_queries1'

# jinja2 xxx.sql.j2


select count() from remote('127.0.0.1:9000', view(

SELECT * FROM system.one UNION ALL
SELECT * FROM system.one UNION ALL
SELECT * FROM system.one UNION ALL
SELECT * FROM system.one UNION ALL
SELECT * FROM system.one UNION ALL
SELECT * FROM system.one UNION ALL
SELECT * FROM system.one UNION ALL
SELECT * FROM system.one UNION ALL
SELECT * FROM system.one UNION ALL
SELECT * FROM system.one
)) SETTINGS log_comment = 'check_concurrency_in_remote_queries1'

[alexey-milovidov]

Does it also require hedged_requests to avoid slowdown?

@Avogar

[Avogar]

Maybe also worth checking async_query_sending_for_remote setting

[azat]

I thought about this as well (and also about use_hedged_requests), but it seems that it should not be required, since only in case of async_socket_for_remote the result will be received in parallel (regardless of max_distributed_connections)

[alexey-milovidov]

@filimonov, would you mind fixing the tests and addressing the review comments?

[filimonov]

Was on short vacations. Getting back to that.

[filimonov]

select count() from remote('127.0.0.1:9000', view( SELECT * FROM system.one UNION ALL SELECT * FROM system.one)) settings allow_experimental_analyzer=0;

┌─count()─┐
│       2 │
└─────────┘


select count() from remote('127.0.0.1:9000', view( SELECT * FROM system.one UNION ALL SELECT * FROM system.one)) settings allow_experimental_analyzer=1;

┌─count()─┐
│       1 │
└─────────┘

[alexey-milovidov]

A test has failed.

[filimonov]

A test has failed.

Yep. I've wanted to demonstrate to @azat the issue discussed in that comment: #53504 (comment)

 +too many threads: 37

37 is much more than max_threads (5) + 2. But that problem is unrelated to that story. I'll just relax that limit for now, and put the comment.

[azat]

Yep. I've wanted to demonstrate to @azat the issue discussed in that comment: #53504 (comment)

Thanks.

+too many threads: 37
37 is much more than max_threads (5) + 2. But that problem is unrelated to that story. I'll just relax that limit for now, and put the comment.

Yes, but still maxIntersection does not gives correct results so it is not correct to use it in the test, but peak_threads_usage is exactly what you need (though you are right there will be problems with backporting the test at least), since it will correctly account only "simultaneously running" threads.

And just to double check, here is some output for the test on my machine - https://gist.github.com/azat/024a9253190db9f50e78d219c266c731

That query uses 7 threads, like it should, but as you can see in query_threads_log there are more entries, but this is just due to multiple attach/detach to a query thread group.

And also sometimes it fails in case of settings randomization, because you need to specify some options explicitly (at least prefer_localhost_replica - https://gist.github.com/azat/d377fb50d934d5c2b5b082666f332a53), or disable settings randomization

And anyway simply using max_threads*8 in the test for the patch that addresses the problem of number of threads is simply not right.

[filimonov]

@azat https://github.com/ClickHouse/ClickHouse/actions/runs/6326161975/job/17179570559?pr=53504#step:6:8558

[azat]

There is something else - https://s3.amazonaws.com/clickhouse-test-reports/53504/bbba89977803200b83d7f35637304be886311b4d/stateless_tests__debug__s3_storage__[5_6].html

2023-10-02 16:41:31  prefer_localhost_replica=1, remote query with read in order
2023-10-02 16:41:31 -ok
2023-10-02 16:41:31 +too many threads: 12

[filimonov]

It's S3 storage test, so may be remote_filesystem_read_method=read similar to local_filesystem_read_method='pread'? (default is threadpool).

I'll try it.

[filimonov]

all failures seem unrelated now

[Avogar]

Need to check timeouts for tests 01085_max_distributed_connections and 01104_distributed_numbers_test in debug mode

UPD: run these tests locally many times, looks ok