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

infoschema/slow_query: fix token too long #10328

Merged
merged 18 commits into from May 9, 2019

Conversation

Projects
None yet
6 participants
@crazycs520
Copy link
Contributor

commented Apr 30, 2019

What problem does this PR solve?

This PR try to fix If there is row length more than bufio.MaxScanTokenSize, parse slow log will get error: bufio.Scanner: token too long.

What is changed and how it works?

Adjust bufio.Buffer before parse slow log and refine error msg.

  • Add QueryLogMaxLenRecord to record the max QueryLogMaxLen.

Attention: If the user set @@tidb_query_log_max_len=65536 and write a long slow query to slow log, then the tidb-restart. Because the QueryLogMaxLenRecord doesn't persistent Stores, so it will be the
QueryLogMaxLen default value 2048, and then query INFORMATION_SCHEMA.SLOW_QUERY will also get error: "read file buffer overflow, please try to enlarge the variable 'tidb_query_log_max_len', but I think the error message is more clear now, and just enlarge tidb_query_log_max_len then retry.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change
    Side effects

Related changes

  • Need to cherry-pick to the release branch
@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@codecov

This comment has been minimized.

Copy link

commented Apr 30, 2019

Codecov Report

Merging #10328 into master will increase coverage by 0.0183%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #10328        +/-   ##
================================================
+ Coverage   77.6692%   77.6875%   +0.0183%     
================================================
  Files           411        411                
  Lines         85447      85450         +3     
================================================
+ Hits          66366      66384        +18     
+ Misses        14118      14111         -7     
+ Partials       4963       4955         -8
@codecov

This comment has been minimized.

Copy link

commented Apr 30, 2019

Codecov Report

Merging #10328 into master will increase coverage by 0.0235%.
The diff coverage is 91.3043%.

@@               Coverage Diff                @@
##             master     #10328        +/-   ##
================================================
+ Coverage   77.3236%   77.3471%   +0.0235%     
================================================
  Files           412        412                
  Lines         85825      85839        +14     
================================================
+ Hits          66363      66394        +31     
+ Misses        14412      14402        -10     
+ Partials       5050       5043         -7
if atomic.LoadUint64(&config.QueryLogMaxLenRecord) > (bufio.MaxScanTokenSize - 100) {
maxBuf := int(atomic.LoadUint64(&config.QueryLogMaxLenRecord) + 100)
scanner.Buffer([]byte{}, maxBuf)
}

This comment has been minimized.

Copy link
@eurekaka

eurekaka Apr 30, 2019

Contributor

Can we use bufio.Reader.ReadLine to completely avoid this error?

This comment has been minimized.

Copy link
@winkyao

winkyao Apr 30, 2019

Member

We can use ReadLine to implement a new scanner

This comment has been minimized.

Copy link
@crazycs520

crazycs520 Apr 30, 2019

Author Contributor

Great idea. Other question, should we add a max-single-line-length-limit? If user executes a long query.

At first, I think if we don't limit the max-single-line-length, then parse slow log may consume a lot of memory and then may make TiDB-server killed by the system.

But after careful consideration, I think no need to add max-single-line-length, the first reason is we have log rotate, the other reason is executing the long query may make tidb-server killed in executing. 😂 What do you think? @eurekaka @winkyao

This comment has been minimized.

This comment has been minimized.

Copy link
@crazycs520

crazycs520 May 5, 2019

Author Contributor

done. PTAL @eurekaka @winkyao

@zhouqiang-cl

This comment has been minimized.

Copy link
Member

commented May 5, 2019

/run-all-tests

@crazycs520 crazycs520 force-pushed the crazycs520:fix-slow-log-token-too-long branch from ab58560 to b4e610e May 5, 2019

crazycs520 added some commits May 5, 2019

@winkyao
Copy link
Member

left a comment

LGTM

crazycs520 added some commits May 5, 2019

@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

/run-all-tests

@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

@eurekaka PTAL

@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

/run-all-tests

lineByte = append(lineByte, tempLine...)

// Use the max value of max_allowed_packet to check the single line length.
if len(lineByte) > int(variable.MaxOfMaxAllowedPacket) {

This comment has been minimized.

Copy link
@eurekaka

eurekaka May 8, 2019

Contributor

Why using MaxOfMaxAllowedPacket instead of MaxAllowedPacket?

This comment has been minimized.

Copy link
@crazycs520

crazycs520 May 8, 2019

Author Contributor

Because MaxAllowedPacket may has been changed.

Show resolved Hide resolved infoschema/slow_log.go Outdated

crazycs520 added some commits May 8, 2019

@eurekaka
Copy link
Contributor

left a comment

LGTM

@eurekaka eurekaka added the status/LGT2 label May 8, 2019

@CLAassistant

This comment has been minimized.

Copy link

commented May 8, 2019

CLA assistant check
All committers have signed the CLA.

@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

/run-all-tests

@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

/run-all-tests

1 similar comment
@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

/run-all-tests

@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

/run-all-tests

2 similar comments
@zhouqiang-cl

This comment has been minimized.

Copy link
Member

commented May 9, 2019

/run-all-tests

@zhouqiang-cl

This comment has been minimized.

Copy link
Member

commented May 9, 2019

/run-all-tests

@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

/run-all-tests

@crazycs520 crazycs520 merged commit 5617809 into pingcap:master May 9, 2019

8 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 91.3043% of diff hit (target 0%)
Details
codecov/project 77.3471% (+0.0235%) compared to 834d8fb
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/build_check_race Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev_2 Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

crazycs520 added a commit to crazycs520/tidb that referenced this pull request May 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.