Automatic decision of nº of parallel replicas by Algunenano · Pull Request #51692 · ClickHouse/ClickHouse

Algunenano · 2023-06-30T15:39:33Z

Changelog category (leave one):

Improvement

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

Add the ability to tune the number of parallel replicas used in a query execution based on the estimation of rows to read.

Documentation entry for user-facing changes

Removes the setting parallel_replicas_min_number_of_granules_to_enable.
Introduces a new setting parallel_replicas_min_number_of_rows_per_replica to decide dynamically how many replicas to use when executing the query. The decision is row estimation / parallel_replicas_min_number_of_rows_per_replica (integer division).
If the estimation for nº of replicas is 1 or less, the parallel replicas feature is deactivated completely. There won't be any setup or any other servers involved (contrary to parallel_replicas_min_number_of_granules_to_enable).

~~Still pending~~:

Clean up the implementation. For it to work and actually prune granules and partitions (it didn't work before) I had to copy some of the actions (prewhere, added_filter_nodes) from further down the pipeline so the estimation was done with all the information. Now the code is duplicated and ugly.
Support for the analyzer.
Tests with JOINs and multiple subqueries that might each use parallel replicas independently.
Work with trivial LIMIT.
Ignore non mergetree tables

robot-ch-test-poll1 · 2023-06-30T15:41:00Z

This is an automated comment for commit a4f5e9e 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
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

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

Algunenano · 2023-07-06T11:18:53Z

@nikitamikhaylov The only thing missing right now is the analyzer (which needs a completely different implementation). I don't know how hard it will be to add to the analyzer, but if you want to review the interpreter part we could improve and merge this part separately

alifirat · 2023-07-06T14:43:11Z

Hey @Algunenano can we expect with this PR to exclude a specific replicas when using parallel replicas feature ?
If I have to give a bit of context, let's imagine I have a node that is handles the insertions and all the replicas serves mainly for reading the data.
I would like to exclude the node in charge of the writes to not add more load on him.

Algunenano · 2023-07-06T14:48:17Z

I would like to exclude the node in charge of the writes to not add more load on him.

No. This PR is only meant to decide how many replicas to use based purely on the estimation of rows to read. But note that you can already do what you want by having a cluster without that specific replica or sets of replicas.

alifirat · 2023-07-06T15:00:24Z

So it means I can configure the same cluster with 2 different sets of replicas but still allow the replication between them ?

Algunenano · 2023-07-06T15:03:18Z

So it means I can configure the same cluster with 2 different sets of replicas but still allow the replication between them ?

Replication is independent on the cluster, but yes, you could have something like this:

    <remote_servers>
        <my_cluster>
            <shard>
                <internal_replication>true</internal_replication>
                <replica>
                    <host>clickhouse-01</host>
                </replica>
                <replica>
                    <host>clickhouse-02</host>
                </replica>
                <replica>
                    <host>clickhouse-03</host>
                </replica>
            </shard>
        </my_cluster>
        <my_cluster_readers>
            <shard>
                <internal_replication>true</internal_replication>
                <replica>
                    <host>clickhouse-02</host>
                </replica>
                <replica>
                    <host>clickhouse-03</host>
                </replica>
            </shard>
        </my_cluster_readers>

And use my_cluster for your ON CLUSTER operations and my_cluster_readers for parallel replicas.

Algunenano · 2023-07-10T08:54:41Z

I've been looking into how to implement this cleanly in the analyzer and I don't see a good way that doesn't involve re-analyzing the query again. To do a proper estimation you need to generate the full optimized pipeline (to get proper prewhere filters and knowledge about affected parts) and at that point changing the parallel replica settings doesn't have the desired effect (be able to disable any remote replica setup if they are not used).

I'm going to do some more tests and fallback to full re-analysis if I don't find a better way forward. Even though this is how the current interpreter works (full pipeline regeneration) I'd expect the analyzer to be able to have a better way to change the pipeline dynamically. Maybe it's just me not knowing how the analyzer works or maybe it's not there yet.

…arallel_replicas_and_empty_header" This reverts commit 2c592a6, reversing changes made to 7a593ed.

Algunenano · 2023-07-11T12:01:12Z

I've added support for the analyzer too. I'm afraid the approach is bad but I couldn't find a better way that didn't involve me spending several weeks learning how the analyzer works and how I could get the optimized filters from the query into the earlier stage (so the estimation of rows is valid).

Note that the test will still fail under the analyzer because it's also testing JOINs with it, and parallel replicas + analyzer + JOIN is not supported.

Marking as ready for review now

…row_estimation

Algunenano · 2023-10-18T09:57:21Z

src/Planner/PlannerJoinTree.cpp

+                {
+                    ActionDAGNodes filter_nodes;
+                    if (table_expression_query_info.filter_actions_dag)
+                        filter_nodes.nodes = table_expression_query_info.filter_actions_dag->getOutputs();


I'm not sure if we need to pass all filters here or we need to filter them more, like getFiltersForPrimaryKeyAnalysis does for the old interpreter.

I'm not sure whether additional table filters setting is supported for the Analyzer...

I'd expect table_expression_query_info.filter_actions_dag to contain all the filter expressions.
And yes, Planner supports additional table filters.

It seems that passing all filters is ok. buildIndexes will do the heavy work of checking primary key and so on so reducing the list doesn't seem necessary.

Algunenano · 2023-10-18T09:58:59Z

tests/analyzer_tech_debt.txt

 02721_url_cluster
 02534_s3_cluster_insert_select_schema_inference
 02765_parallel_replicas_final_modifier
+02784_parallel_replicas_automatic_decision_join


This test needs the analyzer supporting JOINs with pure parallel replicas

We do not usually add anything to this list. It was created to disallow introducing incompatibility.

Parallel replicas with JOINs is not supported with the analyzer yet, so the tests checking the automatic decision on top of that won't work until the underlying feature is implemented. For now I'd rather add tests (with some of those disabled for the analyzer) that not add them at all.

Then you'll need to work on supporting it with the analyzer because it'll be a blocker.

I know. My point is that it was already a blocker, it's just more tests for when I implement the feature on the new planner.

Algunenano · 2023-10-19T14:31:07Z

Failed tests:

`Stress test (debug) — Possible deadlock on shutdown (see gdb.log)
- fatal_messages.txt -> OOM messages
- hung_check.log -> Failed because the client sent a query and CH didn't reply. It didn't reply because it was dead because the OOM killer had sent a signal for it to die.

For some reason it took 45 minutes from children killed by OOM killer to server fully off:

2023.10.18 00:06:24.457028 [ 2166 ] {} <Warning> Application: Child process was stopped by signal 19.
2023.10.18 00:51:28.952026 [ 2166 ] {} <Fatal> Application: Child process was terminated by signal 9 (KILL). If it is not done by 'forcestop' command or manually, the possible cause is OOM Killer (see 'dmesg' and look at the '/var/log/kern.log' for the details).

I guess it might be generating dumps and then gdb connections + explorations. In any case, it's unrelated to the changes. Let's merge!

src/Planner/PlannerJoinTree.cpp

novikd · 2023-10-19T15:08:22Z

src/Planner/PlannerJoinTree.cpp

+                {
+                    ActionDAGNodes filter_nodes;
+                    if (table_expression_query_info.filter_actions_dag)
+                        filter_nodes.nodes = table_expression_query_info.filter_actions_dag->getOutputs();


I'd expect table_expression_query_info.filter_actions_dag to contain all the filter expressions.
And yes, Planner supports additional table filters.

novikd · 2023-10-19T15:10:59Z

tests/analyzer_tech_debt.txt

 02721_url_cluster
 02534_s3_cluster_insert_select_schema_inference
 02765_parallel_replicas_final_modifier
+02784_parallel_replicas_automatic_decision_join


We do not usually add anything to this list. It was created to disallow introducing incompatibility.

…row_estimation

Algunenano · 2023-10-20T10:30:25Z

Integration tests (asan, analyzer) [6/6] — fail: 1, passed: 480 -> Flaky test_delayed_replica_failover #54502
Integration tests (tsan) [6/6] — fail: 1, passed: 483 -> Flaky test_delayed_replica_failover #54502 again
ClickHouse Stateless Tests (release, S3 Storage) -> Covered by Fix 00002_log_and_exception_messages_formatting #55844
ClickHouse Stress Test (debug) -> FileCache::assertCacheCorrectness() Logical error: entry.size == reserved_size #55867

novikd

LGTM, but I checked only the code related to Planner and new infrastructure.

novikd · 2023-10-20T13:42:47Z

tests/analyzer_tech_debt.txt

 02721_url_cluster
 02534_s3_cluster_insert_select_schema_inference
 02765_parallel_replicas_final_modifier
+02784_parallel_replicas_automatic_decision_join


Then you'll need to work on supporting it with the analyzer because it'll be a blocker.

robot-ch-test-poll1 added the pr-feature Pull request with new product feature label Jun 30, 2023

nikitamikhaylov self-assigned this Jun 30, 2023

Algunenano force-pushed the parallel_replicas_row_estimation branch 3 times, most recently from 9eb5208 to 314b800 Compare July 4, 2023 10:44

Algunenano mentioned this pull request Jul 5, 2023

Fix for parallel replicas not completely disabled by granule count threshold #51805

Merged

Algunenano force-pushed the parallel_replicas_row_estimation branch from 314b800 to e026911 Compare July 6, 2023 10:59

Algunenano force-pushed the parallel_replicas_row_estimation branch from e026911 to cd90256 Compare July 11, 2023 11:57

Algunenano added 12 commits July 11, 2023 13:57

Revert "Merge pull request ClickHouse#51805 from ClickHouse/fix_for_p…

9a5f357

…arallel_replicas_and_empty_header" This reverts commit 2c592a6, reversing changes made to 7a593ed.

Interpreter: Control number of parallel replicas based on row estimation

f5e0c1b

Test automatic decision of nº of parallel replicas

b9969e8

Consider trivial LIMIT when deciding on parallel replicas

088b052

Don't attempt to work with not mergetree tables

dacc8cb

Add tests for parallel replicas JOINs

97717e0

Start cleaning up things

d5c3323

Add test from ClickHouse#51805

35b315b

Partially cleanup the TODO

dd39177

Cleanup parallel replica adjustement

0d9888e

More tests and better names

9d0cc56

Add support for the analyzer

b6900c8

Algunenano force-pushed the parallel_replicas_row_estimation branch from cd90256 to b6900c8 Compare July 11, 2023 11:58

Algunenano marked this pull request as ready for review July 11, 2023 11:58

Algunenano added 4 commits October 11, 2023 16:50

Merge remote-tracking branch 'blessed/master' into parallel_replicas_…

0b9bd80

…row_estimation

Clean up diff with master

2fb6a7d

Fix test and try to fix queries not being run

0879cf9

Merge remote-tracking branch 'blessed/master' into parallel_replicas_…

500dab9

…row_estimation

Algunenano force-pushed the parallel_replicas_row_estimation branch from cf02904 to 0fcc27c Compare October 13, 2023 09:45

Algunenano added 2 commits October 13, 2023 15:05

Fix CI

0092c48

Merge remote-tracking branch 'blessed/master' into parallel_replicas_…

c5bd297

…row_estimation

Algunenano force-pushed the parallel_replicas_row_estimation branch from 0fcc27c to c5bd297 Compare October 13, 2023 13:05

Algunenano added 2 commits October 16, 2023 15:07

Merge remote-tracking branch 'blessed/master' into parallel_replicas_…

f799f5d

…row_estimation

Implement replica estimation on top of the analyzer

bfb5b8f

Algunenano force-pushed the parallel_replicas_row_estimation branch from 5fe94e3 to bfb5b8f Compare October 16, 2023 16:35

Algunenano added 2 commits October 17, 2023 11:36

Merge remote-tracking branch 'blessed/master' into parallel_replicas_…

3eaf752

…row_estimation

Tidy

0c49f72

Algunenano commented Oct 18, 2023

View reviewed changes

Algunenano marked this pull request as ready for review October 18, 2023 09:59

Algunenano requested a review from nikitamikhaylov October 18, 2023 09:59

nikitamikhaylov approved these changes Oct 19, 2023

View reviewed changes

novikd reviewed Oct 19, 2023

View reviewed changes

Algunenano added 2 commits October 19, 2023 15:20

Merge remote-tracking branch 'blessed/master' into parallel_replicas_…

4a53943

…row_estimation

Improvements based on PR review

a4f5e9e

Algunenano requested review from nikitamikhaylov and novikd October 20, 2023 10:30

nikitamikhaylov approved these changes Oct 20, 2023

View reviewed changes

novikd approved these changes Oct 20, 2023

View reviewed changes

Algunenano merged commit d08ce13 into ClickHouse:master Oct 20, 2023

KochetovNicolai mentioned this pull request Jan 8, 2024

[Umbrella] Analyzer, Planner migration #42648

Closed

devcrafter mentioned this pull request Mar 7, 2024

Fix: parallel replicas with PREWHERE (ubsan) #61052

Merged

Conversation

Algunenano commented Jun 30, 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):

Documentation entry for user-facing changes

Uh oh!

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

Uh oh!

Uh oh!

Algunenano commented Jul 6, 2023

Uh oh!

alifirat commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Algunenano commented Jul 6, 2023

Uh oh!

alifirat commented Jul 6, 2023

Uh oh!

Algunenano commented Jul 6, 2023

Uh oh!

Algunenano commented Jul 10, 2023

Uh oh!

Algunenano commented Jul 11, 2023

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!

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!

Algunenano commented Oct 19, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Algunenano commented Oct 20, 2023

Uh oh!

novikd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Algunenano commented Jun 30, 2023 •

edited

Loading

robot-ch-test-poll1 commented Jun 30, 2023 •

edited

Loading

alifirat commented Jul 6, 2023 •

edited

Loading