-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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: resolve all locks no matter they're expired or not in BatchResolveLocks
#12784
Conversation
PTAL @coocood @MyonKeminta @jackysp |
Codecov Report
@@ Coverage Diff @@
## master #12784 +/- ##
===========================================
Coverage 79.9755% 79.9755%
===========================================
Files 465 465
Lines 107294 107294
===========================================
Hits 85809 85809
Misses 15000 15000
Partials 6485 6485 |
Is there any test for it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code LGTM. But I think there need some tests.
LGTM |
Construct a counterexample that does unsafe GC in the unit test is not easy work. @MyonKeminta |
@tiancaiamao How about try to test only the |
Sounds reasonable ... |
PTAL @MyonKeminta |
Ping @MyonKeminta |
PTAL @MyonKeminta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/rebuild |
PTAL @jackysp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Your auto merge job has been accepted, waiting for 12878 |
/run-all-tests |
What problem does this PR solve?
Should GC rollback a transaction even its lock TTL is not expired? That's hard to answer.
If we choose YES, transactions may be killed unexpectedly by GC.
If we choose NO, then the GC processing would be blocked by active transactions.
We're in a dilemma.
After we introduce the support for large transactions, the TTL may be updating.
It's hard to guarantee when GC could run if we let active transactions block GC.
The simpler solution is to let GC rollback transactions, which is the same as the elder version TiDB does.
What is changed and how it works?
In #11999 we choose to not resolve large transaction's lock, that's not a thoughtful change, and has potential correctness issues.
In this commit, BatchResolveLocks would resolve all locks no matter they're expired or not.
Check List
Tests