Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

topsql: fix issue of topsql failed catch the running SQL when topsql is enabled in execution #33861

Merged
merged 55 commits into from
Apr 26, 2022

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Apr 11, 2022

What problem does this PR solve?

Issue Number: close #33859

Problem Summary:

Fix issue of topsql failed catch the running SQL when topsql is enabled in execution

What is changed and how it works?

Always attach the SQL and Plan information whether TopSQL is enabled.

Benchmark Test

Sysbench test

Machine: AWS c4.4xlarge, 16 cpu core, 32G memory.
Topology: 1TiDB, 1TiKV, 1*PD

Disable TopSQL(threads=300) QPS in This PR QPS in Master Impact(%)
Point-Get 37396 37642 0.6535253175
Read-Only 19108 19343 1.214909786
Enable TopSQL(threads=300) QPS in This PR QPS in Master Impact(%)
Point-Get 34671 34643 -0.08082440897
Read-Only 17701 17682 -0.1074539079

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  1. Create table t1 and insert into 10000000 rows data.

  2. Mock some normal OLTP load by loadgen:

bin/loadgen payload normal-oltp --rows=1000000
  1. Execute following SQLs in client-A and client-B at same:
select count(*),sum(a+b),sum(b+d),sum(c+d*2), sum(d*2+a*b), avg(a*b*d), avg(b*c), avg(c*d),avg(a*b*d), avg(2*b*c), avg(3*c*d), sum(a*b*c), sum(b*d) from t1;
select count(*) from t1 as t0 join t1 as t1 on t0.a != t1.a;
  1. enable TopSQL during the upper SQL execution:
set @@global.tidb_enable_top_sql=1;
  1. Check the TopSQL data.

TiDB:

image

TiKV

image

Side effects

  • Performance regression: Consumes more CPU

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Fix issue of TopSQL failed to catch the running SQL when TopSQL is enabled in execution.

Signed-off-by: crazycs520 <crazycs520@gmail.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Apr 11, 2022

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • mornyx
  • zhongzc

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. needs-cherry-pick-6.0 size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 11, 2022
@breezewish
Copy link
Member

breezewish commented Apr 11, 2022

  1. Please update the release note to correctly reflect the bug fix.
  2. Please add automated tests to avoid regression.

Signed-off-by: crazycs520 <crazycs520@gmail.com>
executor/adapter.go Outdated Show resolved Hide resolved
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520
Copy link
Contributor Author

/merge

2 similar comments
@breezewish
Copy link
Member

/merge

@breezewish
Copy link
Member

/merge

@crazycs520
Copy link
Contributor Author

/run-all-tests

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2022
@ti-chi-bot ti-chi-bot removed status/can-merge Indicates a PR has been approved by a committer. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 26, 2022
@breezewish
Copy link
Member

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 3da532e

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 26, 2022
@crazycs520
Copy link
Contributor Author

/merge

Copy link
Member

@breezewish breezewish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A green approval on behalf of the bot.

@breezewish
Copy link
Member

/merge

@ti-chi-bot ti-chi-bot merged commit fb342ff into pingcap:master Apr 26, 2022
@crazycs520 crazycs520 deleted the topsql-catch-running1 branch April 26, 2022 05:25
@sre-bot
Copy link
Contributor

sre-bot commented Apr 26, 2022

TiDB MergeCI notify

🔴 Bad News! New failing [2] after this pr merged.
These new failed integration tests seem to be caused by the current PR, please try to fix these new failed integration tests, thanks!

CI Name Result Duration Compare with Parent commit
idc-jenkins-ci-tidb/integration-ddl-test 🟥 failed 1, success 5, total 6 7 min 43 sec New failing
idc-jenkins-ci-tidb/common-test 🟥 failed 1, success 11, total 12 5 min 32 sec New failing
idc-jenkins-ci-tidb/integration-common-test 🟢 all 11 tests passed 10 min Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2 🟢 all 28 tests passed 5 min 23 sec Existing passed
idc-jenkins-ci-tidb/tics-test 🟢 all 1 tests passed 5 min 16 sec Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1 🟢 all 26 tests passed 4 min 49 sec Existing passed
idc-jenkins-ci-tidb/mybatis-test 🟢 all 1 tests passed 3 min 32 sec Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test 🟢 all 1 tests passed 3 min 31 sec Existing passed
idc-jenkins-ci-tidb/plugin-test 🟢 build success, plugin test success 4min Existing passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

topsql failed catch the running SQL when Top SQL is enabled in execution
6 participants