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

session: fix log slow log succ field #11867

Merged
merged 5 commits into from
Aug 27, 2019

Conversation

crazycs520
Copy link
Contributor

What problem does this PR solve?

The succ field in the slow log was wrong when logging slow log for DDL/DML and admin show slow.

What is changed and how it works?

Check List

Tests

  • manual test.

Code changes

  • Has exported function/method change

Side effects

Related changes

  • Need to cherry-pick to the release branch(affect version: v2.1.16, v3.0.2)

Release note

  • Fix the issue that succ field in slow log and admin show low was wrong.

@crazycs520 crazycs520 added type/bugfix This PR fixes a bug. type/usability labels Aug 26, 2019
@crazycs520
Copy link
Contributor Author

@jackysp @zz-jason PTAL

session/tidb.go Outdated
@@ -214,7 +214,7 @@ func runStmt(ctx context.Context, sctx sessionctx.Context, s sqlexec.Statement)
// If it is not a select statement, we record its slow log here,
// then it could include the transaction commit time.
if rs == nil {
s.(*executor.ExecStmt).LogSlowQuery(se.GetSessionVars().TxnCtx.StartTS, err != nil)
s.(*executor.ExecStmt).LogSlowQuery(se.GetSessionVars().TxnCtx.StartTS, err == nil)
Copy link
Member

Choose a reason for hiding this comment

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

please add a UT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's hard to add UT to test this..., any suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

how about duplicated primary key error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

? To test this problem, we need the slow log output and get the SQL corresponding slow log, then check the succ field.

The above method is hard to implement in UT.

@codecov
Copy link

codecov bot commented Aug 26, 2019

Codecov Report

Merging #11867 into master will increase coverage by 0.159%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             master     #11867       +/-   ##
===============================================
+ Coverage   81.2595%   81.4186%   +0.159%     
===============================================
  Files           438        438               
  Lines         94635      94993      +358     
===============================================
+ Hits          76900      77342      +442     
+ Misses        12238      12174       -64     
+ Partials       5497       5477       -20

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp jackysp added needs-cherry-pick-2.1 status/LGT1 Indicates that a PR has LGTM 1. labels Aug 26, 2019
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

@crazycs520
Copy link
Contributor Author

/run-all-tests

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 27, 2019
@zz-jason zz-jason merged commit e15e66b into pingcap:master Aug 27, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Aug 27, 2019

cherry pick to release-2.1 in PR #11886

@sre-bot
Copy link
Contributor

sre-bot commented Aug 27, 2019

cherry pick to release-3.0 in PR #11887

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug. type/usability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants