Improve reading with prefetch by nickitat · Pull Request #49732 · ClickHouse/ClickHouse

nickitat · 2023-05-10T11:56:50Z

Changelog category (leave one):

Performance Improvement

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

Now we use fixed-size tasks in MergeTreePrefetchedReadPool as in MergeTreeReadPool. Also from now we use connection pool for S3 requests.

Logical continuation of #49287.

robot-ch-test-poll1 · 2023-05-10T12:01:15Z

This is an automated comment for commit 63b9c1a with description of existing statuses. It's updated for the latest CI running
The full report is available here
The overall status of the commit is 🔴 failure

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
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	🟢 success
Mergeable Check	Checks if all other necessary checks are successful	🔴 failure
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
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
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	🟢 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
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	🟢 success

nickitat · 2023-05-10T19:25:02Z

src/Storages/MergeTree/MergeTreePrefetchedReadPool.cpp

this is to actually have moderate const-size tasks. and when we will have code to start new prefetch as soon as some of the previous completed, all tasks will be prefetched as soon as max_streams tasks fit in limits.

nickitat · 2023-05-10T19:27:46Z

src/Storages/MergeTree/MergeTreePrefetchedReadPool.cpp

it doesn't look necessary and to my taste it is fairly fairly random logic

nickitat · 2023-05-10T19:29:51Z

src/Storages/MergeTree/MergeTreePrefetchedReadPool.cpp

changed to debug because on test level we now have literally tons of log messages per query

src/IO/ReadBufferFromS3.cpp

CheSema · 2023-06-03T08:49:09Z

src/Common/PoolBase.h

How about different model here?
Soft and hard limit.
If pool is under soft limit, it allocates more. When some connection if freed, pool closes excess connections.
If pool is close to hard limit. It waits no more than connection timeout when limit hits hard level.

what advantages it would have? from perf standpoint any waiting is bad I think.

With those 2 values you are able to configure system in more than two ways: when it waits a vacant connection, when it doesn't wait any and any option between this extremes. However the most important part that you will never go into infinite connections count. That part is worth the efforts.

from perf standpoint any waiting is bad I think.
What do you have as alternative way?

Ofcourse you could fail request instantly. But it is unlogical, since there is a setting connection_timeout.

Imagine that there are a lot of requests to the S3. If you just fail all the requests above the limit the cluster would stop unable to do a progress, that would be denial of service problem. If you slow down the progress by waiting the connections the progress will continue (there would be the brown zone before the black), that would be performance issue with some ways how to mitigate it by changing load pattern from user side or by changing settings from our side. Sounds like gracefull degradation.

kssenii · 2023-06-15T11:01:02Z

src/IO/S3/PocoHTTPClient.cpp

did not understand why second condition is needed, add a comment?

kssenii · 2023-06-15T11:05:50Z

src/IO/ReadBufferFromS3.cpp

But AsynchronousBoundedReadBuffer has the same check, which means that for the last read range we will never get to this point (as well as to line 178) in code even though the range was all read successfully. I think we can add the same check in destructor before resetting the session and update read_all_range_successfully?

yep, it is a good idea

src/IO/ReadBufferFromS3.cpp

kssenii · 2023-06-15T11:15:37Z

src/Disks/IO/ReadBufferFromRemoteFSGather.cpp

with_cache can now be make const in .h file )

kssenii · 2023-06-15T11:20:34Z

src/Disks/IO/CachedOnDiskReadBufferFromFile.cpp

ok, I need not to forget to change this a bit in PR with background download in cache :) because with background download we should not reset the implementation_buffer if file_segment is in PARTAILLY_DOWNLOADED state.

not necessary to keep it

src/IO/ReadBufferFromS3.cpp

qoega · 2023-06-20T15:10:43Z

Integration tests are related and potentially have to be updated to reflect current behaviour

src/IO/HTTPCommon.cpp

nickitat · 2023-06-26T12:12:21Z

smth happened with zookeeper, need to restart tests (
https://s3.amazonaws.com/clickhouse-test-reports/49732/8f3b3271d3fc8d174b7d5d5e912e9b70c886c96a/stateless_tests__release__databasereplicated__[4_4].html

azat · 2023-07-09T11:57:10Z

src/Common/MemoryTrackerSwitcher.h

+    explicit MemoryTrackerSwitcher(MemoryTracker * new_tracker)
+    {
+        if (!current_thread)
+            throw Exception(ErrorCodes::LOGICAL_ERROR, "current_thread is not initialized");


This breaks clickhouse-disks for S3 disks

Failed to make request to: http://localhost:11111/test?list-type=2&max-keys=1&prefix=clickhouse-disks%2Fdefault%2Ftest.copy: Code: 49. DB::Exception: current_thread is not initialized. (LOGICAL_ERROR), Stack trace (when copying this message, always include the lines below):

So after this change it is not possible to do some operations that requires pools from the main thread.

I will fix it here - b2ea45b (#51448)

Otherwise "current_thread is not initialized" error, that had been introduced in ClickHouse#49732, since it is possible to run this code from non-ClickHouse thread pools. Fixes: ClickHouse#52013 Signed-off-by: Azat Khuzhin <[email protected]>

The code that it uses had been removed in ClickHouse#58845. Introduced in ClickHouse#49732 Signed-off-by: Azat Khuzhin <[email protected]>

nickitat marked this pull request as draft May 10, 2023 11:57

nickitat changed the title ~~Improve prefetch~~ Improve reading with prefetch May 10, 2023

nickitat changed the title ~~Improve reading with prefetch~~ [WIP] Improve reading with prefetch May 10, 2023

ClickHouse deleted a comment from clickhouse-ci bot May 10, 2023

kssenii self-assigned this May 10, 2023

robot-ch-test-poll1 added the pr-performance Pull request with some performance improvements label May 10, 2023

nickitat commented May 10, 2023

View reviewed changes

nickitat changed the title ~~[WIP] Improve reading with prefetch~~ Improve reading with prefetch May 10, 2023

nickitat force-pushed the impr_prefetch branch 3 times, most recently from bc1bf81 to eef6f32 Compare May 20, 2023 21:16

nickitat force-pushed the impr_prefetch branch 4 times, most recently from 1802fb7 to f9c1f09 Compare May 29, 2023 13:30

nickitat force-pushed the impr_prefetch branch from 5f2d2db to 68b60eb Compare June 1, 2023 23:06

nickitat commented Jun 2, 2023

View reviewed changes

src/IO/ReadBufferFromS3.cpp Outdated Show resolved Hide resolved

nickitat force-pushed the impr_prefetch branch from 299ae57 to 9768862 Compare June 2, 2023 11:17

This was referenced Jun 2, 2023

S3: turn on http keep alive, use connection pool #50469

Closed

S3 http_keep_alive_timeout, use ConnectionPool #50217

Closed

CheSema reviewed Jun 3, 2023

View reviewed changes

nickitat force-pushed the impr_prefetch branch 3 times, most recently from 9e8f03c to af59841 Compare June 10, 2023 21:03

nickitat force-pushed the impr_prefetch branch from 6694f17 to 1f16277 Compare June 12, 2023 14:21

nickitat marked this pull request as ready for review June 13, 2023 22:56

nickitat added 2 commits June 14, 2023 12:47

use const-size tasks in prefetch pool

2b40734

cosmetics

e88fc39

kssenii reviewed Jun 15, 2023

View reviewed changes

review fixes + test

b546d8e

nickitat force-pushed the impr_prefetch branch from 1f16277 to b546d8e Compare June 16, 2023 13:33

kssenii approved these changes Jun 16, 2023

View reviewed changes

nickitat commented Jun 16, 2023

View reviewed changes

src/IO/ReadBufferFromS3.cpp Outdated Show resolved Hide resolved

fix ReadBufferFromS3

876d5ae

nickitat force-pushed the impr_prefetch branch from 3100306 to f6a2b95 Compare June 20, 2023 18:28

nickitat commented Jun 21, 2023

View reviewed changes

src/IO/HTTPCommon.cpp Outdated Show resolved Hide resolved

rollback changes in test

1419bb7

nickitat force-pushed the impr_prefetch branch from dd88ba5 to 989cf35 Compare June 21, 2023 18:54

nickitat marked this pull request as draft July 3, 2023 09:29

nickitat force-pushed the impr_prefetch branch 3 times, most recently from 5bd2a08 to f6e4cb3 Compare July 4, 2023 12:44

nickitat added 3 commits July 6, 2023 14:41

don't account session's memory in thread/user mem tracker

c23e29d

rework pool usage

aec7205

add test

63b9c1a

nickitat force-pushed the impr_prefetch branch from e3c20df to 63b9c1a Compare July 6, 2023 16:58

nickitat marked this pull request as ready for review July 7, 2023 19:53

alexey-milovidov merged commit 3d48009 into ClickHouse:master Jul 9, 2023

azat reviewed Jul 9, 2023

View reviewed changes

azat mentioned this pull request Jul 13, 2023

Logical error: 'current_thread is not initialized'. #52013

Closed

azat mentioned this pull request Jul 13, 2023

Fix using of pools for non ClickHouse threads (fixes "current_thread is not initialized" logical error) #52096

Closed

nickitat added the pr-must-backport-cloud label Jul 27, 2023

robot-clickhouse-ci-1 added the pr-backports-created-cloud deprecated label, NOOP label Jul 27, 2023

azat added a commit to azat/ClickHouse that referenced this pull request Mar 25, 2024

Remove PoolBase::AllocateNewBypassingPool

1bfd588

The code that it uses had been removed in ClickHouse#58845. Introduced in ClickHouse#49732 Signed-off-by: Azat Khuzhin <[email protected]>

azat mentioned this pull request Mar 25, 2024

Remove PoolBase::AllocateNewBypassingPool #61866

Merged

Conversation

nickitat commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Uh oh!

robot-ch-test-poll1 commented May 10, 2023 • edited by robot-ch-test-poll Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CheSema Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qoega commented Jun 20, 2023

Uh oh!

Uh oh!

nickitat commented Jun 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nickitat commented May 10, 2023 •

edited

Loading

robot-ch-test-poll1 commented May 10, 2023 •

edited by robot-ch-test-poll

Loading

CheSema Jun 7, 2023 •

edited

Loading