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

executor: change pessimistic lock wait start for one statement #13922

Merged
merged 6 commits into from
Dec 6, 2019

Conversation

cfzjywxk
Copy link
Contributor

@cfzjywxk cfzjywxk commented Dec 5, 2019

What problem does this PR solve?

When retry is needed, like write conflict or single statement rollback for pessimistic transactions, the Pessimistic transaction lock wait start time will be reset to time.Now() in ActionPessimisitcLock.HandleBatch.

What is changed and how it works?

Add one field in stmtCtx recording the pessimistic lock wait start time, and reuse this value when doing retry

Check List

Tests

  • Manual test (add detailed scripts or steps below)
  1. Change innodb_lock_wait_timeout to 10 seconds
  2. Start a pessimistic transaction in one session
  3. Start an optimistic transaction which will conflict with that in 2
  4. Start another pessimistic transaction in another session, try to lock the same key
  5. After 2 and 4 both retry work done, one will got the pessimistic lock, the other one timed out but not wait another 10 seconds

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change

Side effects

  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch

Release note

@codecov
Copy link

codecov bot commented Dec 5, 2019

Codecov Report

Merging #13922 into master will decrease coverage by 0.1271%.
The diff coverage is 77.2727%.

@@               Coverage Diff                @@
##             master     #13922        +/-   ##
================================================
- Coverage   80.2726%   80.1454%   -0.1272%     
================================================
  Files           480        480                
  Lines        120898     120043       -855     
================================================
- Hits          97048      96209       -839     
- Misses        16143      16151         +8     
+ Partials       7707       7683        -24

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

Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

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

LGTM, can we add a test case via failpoint to return WriteConflictError once?

@cfzjywxk
Copy link
Contributor Author

cfzjywxk commented Dec 5, 2019

LGTM, can we add a test case via failpoint to return WriteConflictError once?

I'll add one

@coocood
Copy link
Member

coocood commented Dec 5, 2019

LGTM

@cfzjywxk
Copy link
Contributor Author

cfzjywxk commented Dec 6, 2019

/run-all-tests

@cfzjywxk
Copy link
Contributor Author

cfzjywxk commented Dec 6, 2019

/run-all-tests

@cfzjywxk
Copy link
Contributor Author

cfzjywxk commented Dec 6, 2019

@jackysp @lysu PTAL

@jackysp jackysp added the status/can-merge Indicates a PR has been approved by a committer. label Dec 6, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Dec 6, 2019

/run-all-tests

@sre-bot sre-bot merged commit 01a7d00 into pingcap:master Dec 6, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Dec 6, 2019

cherry pick to release-3.0 failed

@sre-bot
Copy link
Contributor

sre-bot commented Apr 7, 2020

It seems that, not for sure, we failed to cherry-pick this commit to release-3.0. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @cfzjywxk PTAL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/transaction SIG:Transaction status/can-merge Indicates a PR has been approved by a committer. type/compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants