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

executer: add Cop_time in slow log #16873

Merged
merged 8 commits into from
Apr 28, 2020

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Apr 27, 2020

What problem does this PR solve?

Before this PR, TiDB Slow log only record Process_time, but it is the total process time of cop task spend on TiKV, we still don't know the related Cop_time spend in tidb, as below:

image

What is changed and how it works?

Add Cop_time in slow_query and slow log. The Cop_time contain the time duration as below:

  • Send Cop request.
  • Send request to TiKV network latency duration.
  • TiKV handle cop request duration.
  • Send result to TiDB network latency.
  • Recieve result latency.
  • If meet error, maybe retry upper steps.
mysql>select query_time,cop_time,query from `CLUSTER_SLOW_QUERY` where cop_time>0 limit 3;
+-------------+-------------+---------------------------------------------------------------------------------------------------+
| query_time  | cop_time    | query                                                                                             |
+-------------+-------------+---------------------------------------------------------------------------------------------------+
| 0.382663125 | 0.382427668 | select count(*) from t2;                                                                          |
| 0.329472658 | 0.329190264 | select count(*) from t2;                                                                          |
| 0.929105778 | 0.400684833 | select count(*) from t2 where c2 >=0 and c1 in (select c1 from t2 where c7!=now()) and c8!=now(); |
+-------------+-------------+---------------------------------------------------------------------------------------------------+

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM

Release note

@crazycs520 crazycs520 requested a review from a team as a code owner April 27, 2020 15:28
@ghost ghost requested review from wshwsh12 and removed request for a team April 27, 2020 15:28
Signed-off-by: crazycs520 <[email protected]>
@codecov
Copy link

codecov bot commented Apr 27, 2020

Codecov Report

Merging #16873 into master will decrease coverage by 0.1124%.
The diff coverage is 90.0000%.

@@               Coverage Diff                @@
##             master     #16873        +/-   ##
================================================
- Coverage   80.4616%   80.3491%   -0.1125%     
================================================
  Files           507        507                
  Lines        137913     137409       -504     
================================================
- Hits         110967     110407       -560     
- Misses        18295      18338        +43     
- Partials       8651       8664        +13     

@crazycs520
Copy link
Contributor Author

/rebuild

@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Apr 27, 2020
@SunRunAway
Copy link
Contributor

I think that's a great approaching, would cop_time be involved in statement summary table?

@crazycs520
Copy link
Contributor Author

@SunRunAway Ye, maybe in next PR add cop_time to statement summary table.

@qw4990
Copy link
Contributor

qw4990 commented Apr 28, 2020

@crazycs520 Should we cherry pick this PR to 4.0?

@crazycs520
Copy link
Contributor Author

@qw4990 Yes, I want to cherry-pick to 4.0.

Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

LGTM

@SunRunAway
Copy link
Contributor

@crazycs520 we should add documentation update in Related changes section.

@SunRunAway SunRunAway added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 28, 2020
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM

@qw4990 qw4990 added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 28, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Apr 28, 2020

/run-all-tests

Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

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

Is it compatible with the older versions?
e.g. Older version reads slow log in newer version.
Or, newer version reads slow log in older version.

Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

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

Did you discuss the name with PM? I think the name is confusing, I don't know whether it's time consumed in TiDB or TiKV or both by the name. I can only read the document to figure it out.

@sre-bot
Copy link
Contributor

sre-bot commented Apr 28, 2020

@crazycs520 merge failed.

@crazycs520
Copy link
Contributor Author

@djshow832, It's ok, new version can still read the old version tidb slow log, and old version can read the new tidb slow log, but can't read the Cop_time field value.

@crazycs520
Copy link
Contributor Author

/run-integration-copr-test

@crazycs520
Copy link
Contributor Author

/run-unit-test

@crazycs520 crazycs520 merged commit 3a07cf7 into pingcap:master Apr 28, 2020
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 28, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Apr 28, 2020

cherry pick to release-4.0 in PR #16904

@crazycs520 crazycs520 deleted the refine-slow-log branch April 28, 2020 12:58
zz-jason pushed a commit that referenced this pull request Apr 30, 2020
@SunRunAway
Copy link
Contributor

@crazycs520 Please update the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/infoschema sig/execution SIG execution sig/sql-infra SIG: SQL Infra 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.

5 participants