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

server: let select for update could be recorded for slow log and statements #16743

Merged
merged 4 commits into from
Apr 28, 2020

Conversation

cfzjywxk
Copy link
Contributor

What problem does this PR solve?

Problem Summary:
The pessimistic select for update statements could not be logged as slow query, also other things RecordSet.Close will do

What is changed and how it works?

Proposal: xxx

What's Changed:

Change the close function of chunkRowRecordSet

How it Works:

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)
    Start two sessions let the second select for update statement wait lock for sometime, the slow query should recorded like
# Conn_ID: 2
# Query_time: 8.580836624
# Parse_time: 0.000062642
# Compile_time: 0.000029461
# LockKeys_time: 8.580678652
# DB: test
# Is_internal: false
# Digest: 0cef963ebac4619e5a5c4562d83626a3abb5df2b0501a4dbeae157f7e5356cff
# Num_cop_tasks: 0
# Prepared: false
# Has_more_results: false
# Succ: true
# Plan: tidb_decode_plan('JIwwCTM2XzEJMAkxCXRhYmxlOnQxLCBoYW5kbGU6MiwgbG9jawo=')
# Plan_digest: 3f5625f5b5b7a079571f7844fe4e63a6587a0674dace590987cdb159574bdd22
select * from t1 where c1 = 2 for update;

Side effects

Release note

@cfzjywxk cfzjywxk requested a review from a team as a code owner April 23, 2020 04:07
@ghost ghost requested review from XuHuaiyu and removed request for a team April 23, 2020 04:07
@codecov
Copy link

codecov bot commented Apr 23, 2020

Codecov Report

Merging #16743 into master will decrease coverage by 0.0086%.
The diff coverage is 100.0000%.

@@               Coverage Diff                @@
##             master     #16743        +/-   ##
================================================
- Coverage   80.3228%   80.3142%   -0.0087%     
================================================
  Files           507        507                
  Lines        137220     137358       +138     
================================================
+ Hits         110219     110318        +99     
- Misses        18334      18367        +33     
- Partials       8667       8673         +6     

@github-actions github-actions bot added the sig/execution SIG execution label Apr 23, 2020
@cfzjywxk
Copy link
Contributor Author

@jackysp @coocood @lysu @tiancaiamao PTAL

@jackysp
Copy link
Member

jackysp commented Apr 24, 2020

Was it introduced by #15890 ?

@@ -437,6 +438,12 @@ func (c *chunkRowRecordSet) NewChunk() *chunk.Chunk {
}

func (c *chunkRowRecordSet) Close() error {
c.execStmt.FinishExecuteStmt(c.execStmt.Ctx.GetSessionVars().TxnCtx.StartTS, true, false)
Copy link
Member

@coocood coocood Apr 26, 2020

Choose a reason for hiding this comment

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

Can we extract the lines to a method in ExecStmt?

@cfzjywxk
Copy link
Contributor Author

cfzjywxk commented Apr 26, 2020

Was it introduced by #15890 ?

The original logic will log each pessimistic retry, it was fixed in #15890, but the statement does not get logged

@coocood
Copy link
Member

coocood commented Apr 27, 2020

LGTM

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
Copy link
Member

jackysp commented Apr 28, 2020

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 28, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Apr 28, 2020

/run-all-tests

@sre-bot sre-bot merged commit b32e834 into pingcap:master Apr 28, 2020
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 28, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Apr 28, 2020

cherry pick to release-3.0 in PR #16884

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 28, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Apr 28, 2020

cherry pick to release-3.1 in PR #16885

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 28, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Apr 28, 2020

cherry pick to release-4.0 in PR #16886

@cfzjywxk
Copy link
Contributor Author

cherry pick to release-4.0 in PR #16897

@cfzjywxk
Copy link
Contributor Author

cherry pick to release-3.1 in PR #16901

@cfzjywxk
Copy link
Contributor Author

cherry pick to release-3.0 in PR #16903

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/server sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants