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

store/tikv: set ScanDetail to true #7445

Merged
merged 4 commits into from
Aug 21, 2018
Merged

Conversation

coocood
Copy link
Member

@coocood coocood commented Aug 21, 2018

What problem does this PR solve?

Improve the SLOW_QUERY log, make sure it includes all scan details, so the sum will be accurate.

What is changed and how it works?

Currently ScanDetail is returned only when the scan takes more than one second, set this field to true will make sure the detail always returned.

Before the change, total_keys and processed_keys is missing.

2018/08/21 09:59:11.817 adapter.go:363: [warning] [SLOW_QUERY] cost_time:1.688710205s 
process_time:5.95s wait_time:6.312s request_count:13 succ:true con:1 user:[email protected] 
txn_start_ts:402343002133495809 database:test table_ids:[31],
sql:select count(c) from sbtest1

After the change, total_keys and processed_keys is present.

2018/08/21 10:00:16.495 adapter.go:363: [warning] [SLOW_QUERY] cost_time:1.397274785s 
process_time:5.278s wait_time:6.254s request_count:13 total_keys:3000187 
processed_keys:3000000 succ:true con:1 user:[email protected] 
txn_start_ts:402343019173117957 database:test table_ids:[31],
sql:select count(c) from sbtest1

Check List

Tests

  • Manual test (add detailed scripts or steps below)
  1. start a single instance pd/tikv/tidb.
  2. import 3 million records.
  3. query select count(c) from sbtest1.
  4. observe the SLOW_QUERY log.

Side effects

  • Possible performance regression
    TiKV need to allocate memory to marshal the scan detail information in the response. but the impact is very small.

@coocood
Copy link
Member Author

coocood commented Aug 21, 2018

@zz-jason @shenli PTAL

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@ngaut
Copy link
Member

ngaut commented Aug 21, 2018

/run-all-tests

Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 21, 2018
@shenli
Copy link
Member

shenli commented Aug 21, 2018

Should we evaluate the performance regress with sysbench?

@coocood coocood merged commit eeb6435 into pingcap:master Aug 21, 2018
@coocood coocood deleted the request-scan-detail branch August 21, 2018 08:01
coocood added a commit to coocood/tidb that referenced this pull request Aug 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/metrics status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants