executor: unify executor concurrency by niedhui · Pull Request #16999 · pingcap/tidb

niedhui · 2020-05-06T15:00:05Z

What problem does this PR solve?

Issue Number: part of #15428

Problem Summary:

What is changed and how it works?

What's Changed:

Deprecated existing executor-concurrency related variables, use a single ExecutorConcurreny to control the concurrency of all types of executors.

How it Works:

Introduce a system variable ExecutorConcurrency to control the concurrency of all types of executors.
All deprecated fields of sessionctx/Concurrency were hidden, and use the exported functions to get the value
The functions first try to read the deprecated-field for compatibility, if the value was -1 (which means unset), the value of ExecutorConcurrency was returned.

Updates

We have nine executor-concurrency related variables:

tidb_index_lookup_concurrency
tidb_index_lookup_join_concurrency
tidb_hashagg_partial_concurrency
tidb_hashagg_final_concurrency
tidb_window_concurrency
tidb_projection_concurrency
tidb_hash_join_concurrency
tidb_distsql_scan_concurrency
tidb_index_serial_scan_concurrency

I planed to migrate these variables in the following steps:

In this PR, I only migrate the first 6 variables to adopt ExecutorConcurrency. Because the original default value of these variables is all 4, by making the default value of ExecutorConcurrency to 4 (not the number of cups), we would not break too many tests
I will migrate tidb_hash_join_concurrency and tidb_distsql_scan_concurrency in the following PR, the original default value of these two is 5 and 15, we may discuss the default fixed-value of ExecutorConcurreny in the PR, 4 or 15, and this will break many tests, in TiDB, mysql-tester and copr-test, I would make changes on these repos by setting the fixed value of the concurrency variables.
As we already knew, some tests are in strict mode, we may add the deprecated warning in a separate PR as the last step.
tidb_index_serial_scan_concurrency will be kept since it's used by analyze-table as @SunRunAway suggested.

Known issues about tests currently :
We should only use set @@session.tidb_executor_concurrenly= to control the concurrency of executor, the execution plan may not be the same as it is, which may lead to lots of changes for currency tests.

Check List

All executors use ExecutorConcurrency
Bootstrap from old version
Show variable and values in mysql.global_variables
fix test

Tests

Unit test
Integration test

Release note

Deprecate tidb_index_lookup_concurrency, tidb_index_lookup_join_concurrency, , `tidb_hashagg_partial_concurrency`, `tidb_hashagg_final_concurrency`, `tidb_window_concurrency`, `tidb_projection_concurrency` and use the new tidb_executor_concurrency instead.

sre-bot · 2020-05-06T15:00:09Z

Thanks for your contribution. If your PR get merged, you will be rewarded 300 points.

CLAassistant · 2020-05-06T15:00:13Z

All committers have signed the CLA.

niedhui · 2020-05-12T05:56:54Z

@SunRunAway friendly ping 😀

SunRunAway

Hi, @niedhui

Firstly I apologize for the late reply.

Do you mean remove all the existing concurrency system variable, and make these as user-defined variable for compatibility?

I have to send my second apology to you that I misunderstood what you said and might give the wrong information to you. And the issue description may also lead us to an unexpected way.

It's not related to The user-defined variable.

What we want is,

introduce a session and global variable tidb_executor_concurrency, which defaults to the numbers of CPU cores.
change the default session and global variable tidb_hash_join_concurrency to NULL
if tidb_hash_join_concurrency is NULL, apply the Hash Join concurrency by tidb_executor_concurrency, or by tidb_hash_join_concurrency for compatibility.

We do not need really remove existing concurrency system variables, but only deprecate and hide to new users for tidb v4.0.

For users upgraded from old versions, we may introduce a bootstrap policy,

If all the global concurrency variables are the default value in old versions, we may assume that users haven't changed even one of them. So tidb-server can set all the global concurrency variables to NULL, the default value in new version, to make use of tidb_executor_concurrency in bootstrap.go

niedhui · 2020-05-26T17:53:38Z

🤣 Welcome back, mentor. I'll update this tomorrow

niedhui · 2020-05-27T08:17:33Z

@SunRunAway

but only deprecate and hide to new users for tidb v4.0.

Then will the user still be able to change these deprecated variables using SQL? like

set @@session.tidb_hashagg_partial_concurrency = 1

SunRunAway · 2020-05-28T08:17:23Z

@SunRunAway

but only deprecate and hide to new users for tidb v4.0.

Then will the user still be able to change these deprecated variables using SQL? like
set @@session.tidb_hashagg_partial_concurrency = 1

Yes. And the code is like,

if `tidb_hashagg_partial_concurrency` is not `NULL`:
    concurrency = tidb_hashagg_partial_concurrency
else 
    concurrency = tidb_executor_concurrency

Signed-off-by: niedhui <niedhui@gmail.com>

sre-bot · 2020-05-28T13:51:45Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

SunRunAway

For users upgraded from old versions, we may introduce a bootstrap policy,

If all the global concurrency variables are the default value in old versions, we may assume that users haven't changed even one of them. So tidb-server can set all the global concurrency variables to NULL, the default value in new version, to make use of tidb_executor_concurrency in bootstrap.go

SunRunAway · 2020-05-28T15:10:02Z

And best to update the PR description for your newest code and add a release note for the check_release_note bot.

niedhui · 2020-06-16T14:31:05Z

[2020-06-16T13:07:04.798Z] time="2020-06-16T21:07:04+08:00" level=warning msg="Create new db&{0 {root:@tcp(127.0.0.1:4001)/test?strict=true&time_zone=%27Asia%2FShanghai%27 0x10a1110} 0 {0 0} [0xc000134280] map[] 0 1 0xc00002c2a0 0xc0000ca840 false map[0xc000134280:map[0xc000134280:true]] map[] 0 0 0 <nil> 0 0 0 0x48c430}"

[2020-06-16T13:07:04.798Z] time="2020-06-16T21:07:04+08:00" level=fatal msg="Executing \"SET @@tidb_hash_join_concurrency=2\" err[Warning 1287: 'tidb_hash_join_concurrency' is deprecated and will be removed in a future release. Please use tidb_executor_concurrency instead]"

Seems in strict mode, the warning was treated as error, should I remove the warning temporarily ？

…/unify_executor_concurrency

Signed-off-by: niedhui <niedhui@gmail.com>

niedhui · 2020-06-18T12:54:10Z

@sre-bot /run-all-tests

niedhui · 2020-06-18T13:13:26Z

@sre-bot /run-integration-br-test br=pr/362

Signed-off-by: niedhui <niedhui@gmail.com>

niedhui · 2020-06-18T16:13:38Z

@sre-bot /run-all-tests

niedhui · 2020-06-18T16:24:36Z

@sre-bot /run-unit-test

niedhui · 2020-06-18T16:28:50Z

@sre-bot /run-common-test

SunRunAway · 2020-06-18T16:44:56Z

/run-integration-br-test

SunRunAway · 2020-06-18T17:05:40Z

@kennytm It seems that the BR command-line binary does not use the version in this pull request as a dependency.
This PR seems to require both tidb-server and BR itself to upgrade to this PR version because it changes the default value of tidb_index_lookup_concurrency along with the corresponding validation code.

SunRunAway

LGTM

kennytm · 2020-06-19T06:49:38Z

@SunRunAway We could modify the BR CI to add a replace the github.com/pingcap/tidb dependency by the specific TiDB commit, but the TiDB CI must be modified to tell the BR CI which TiDB commit is used. Someone from SRE may chime in?

Also, even if this trick allows the integration-br-test test to pass, BR itself would still not be fixed until a manual update.

niedhui · 2020-06-19T12:16:00Z

Hi @zz-jason, I add an Updates section in the PR description. TLDR, in order to not break too many tests, I will only adopt some variables to ExecutorConcurrency and will open another PR to adopt the left.
PTAL, thanks.

zz-jason

LGTM

zz-jason · 2020-06-22T07:52:20Z

/merge

ti-srebot · 2020-06-22T07:54:44Z

/run-all-tests

ti-srebot · 2020-06-22T08:02:43Z

@niedhui merge failed.

niedhui requested review from a team as code owners May 6, 2020 15:00

ghost requested review from eurekaka and qw4990 and removed request for a team May 6, 2020 15:00

github-actions Bot added sig/execution SIG execution sig/planner SIG: Planner labels May 6, 2020

niedhui force-pushed the niedhui/unify_executor_concurrency branch 6 times, most recently from b5635f5 to bd39e1a Compare May 7, 2020 03:45

zz-jason requested a review from SunRunAway May 7, 2020 15:43

zz-jason added the contribution This PR is from a community contributor. label May 7, 2020

SunRunAway reviewed May 26, 2020

View reviewed changes

Comment thread sessionctx/variable/tidb_vars.go Outdated

unify executor concurrency

7580be5

Signed-off-by: niedhui <niedhui@gmail.com>

niedhui force-pushed the niedhui/unify_executor_concurrency branch from bd39e1a to 7580be5 Compare May 28, 2020 13:51

SunRunAway reviewed May 28, 2020

View reviewed changes

Comment thread executor/builder.go Outdated

Comment thread sessionctx/variable/session.go Outdated

Comment thread sessionctx/variable/session.go

Comment thread sessionctx/variable/varsutil_test.go

SunRunAway reviewed May 28, 2020

View reviewed changes

Comment thread sessionctx/variable/session.go

Comment thread sessionctx/variable/tidb_vars.go

github-actions Bot added the component/expression label May 28, 2020

niedhui added 2 commits June 18, 2020 19:28

Merge branch 'master' of https://github.com/pingcap/tidb into niedhui…

f2792d0

…/unify_executor_concurrency

rollback hashjoin & distsql concurrency

cd7bed9

Signed-off-by: niedhui <niedhui@gmail.com>

niedhui dismissed zz-jason’s stale review via cd7bed9 June 18, 2020 12:50

niedhui force-pushed the niedhui/unify_executor_concurrency branch from 9c00501 to 00bccb3 Compare June 18, 2020 15:58

remove warning for hashjoin & distsql

b861379

Signed-off-by: niedhui <niedhui@gmail.com>

niedhui force-pushed the niedhui/unify_executor_concurrency branch from 00bccb3 to b861379 Compare June 18, 2020 16:10

SunRunAway reviewed Jun 19, 2020

View reviewed changes

SunRunAway added status/LGT1 Indicates that a PR has LGTM 1. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Jun 19, 2020

SunRunAway requested a review from zz-jason June 19, 2020 05:53

zz-jason approved these changes Jun 22, 2020

View reviewed changes

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 22, 2020

Merge branch 'master' into niedhui/unify_executor_concurrency

a832911

SunRunAway merged commit c98514e into pingcap:master Jun 22, 2020

qw4990 mentioned this pull request Jun 30, 2020

executor: implement parallel apply #18044

Merged

0xPoe mentioned this pull request Oct 18, 2025

variable: mark tidb_index_serial_scan_concurrency as deprecated #64023

Merged

13 tasks

Conversation

niedhui commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

sre-bot commented May 6, 2020

Uh oh!

CLAassistant commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niedhui commented May 12, 2020

Uh oh!

SunRunAway left a comment • 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!

Uh oh!

niedhui commented May 26, 2020

Uh oh!

niedhui commented May 27, 2020

Uh oh!

SunRunAway commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sre-bot commented May 28, 2020

Uh oh!

SunRunAway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SunRunAway commented May 28, 2020

Uh oh!

niedhui commented Jun 16, 2020

Uh oh!

niedhui commented Jun 18, 2020

Uh oh!

niedhui commented Jun 18, 2020

Uh oh!

niedhui commented Jun 18, 2020

Uh oh!

niedhui commented Jun 18, 2020

Uh oh!

niedhui commented Jun 18, 2020

Uh oh!

SunRunAway commented Jun 18, 2020

Uh oh!

SunRunAway commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SunRunAway left a comment

Choose a reason for hiding this comment

Uh oh!

kennytm commented Jun 19, 2020

Uh oh!

niedhui commented Jun 19, 2020

Uh oh!

zz-jason left a comment

Choose a reason for hiding this comment

Uh oh!

zz-jason commented Jun 22, 2020

Uh oh!

ti-srebot commented Jun 22, 2020

Uh oh!

ti-srebot commented Jun 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

niedhui commented May 6, 2020 •

edited

Loading

CLAassistant commented May 6, 2020 •

edited

Loading

SunRunAway left a comment •

edited

Loading

SunRunAway commented May 28, 2020 •

edited

Loading

SunRunAway commented Jun 18, 2020 •

edited

Loading