-
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: implement non-block read when coprocessor meets the lock of a large transaction #11986
Conversation
…large transaction
Codecov Report
@@ Coverage Diff @@
## master #11986 +/- ##
===============================================
+ Coverage 80.2295% 80.317% +0.0874%
===============================================
Files 472 472
Lines 114843 115135 +292
===============================================
+ Hits 92138 92473 +335
+ Misses 15466 15421 -45
- Partials 7239 7241 +2 |
PTAL @coocood @crazycs520 |
@@ -367,6 +367,7 @@ type copIteratorWorker struct { | |||
respChan chan<- *copResponse | |||
finishCh <-chan struct{} | |||
vars *kv.Variables | |||
clientHelper |
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.
Is clientHelper
going to be used in other places?
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.
I'm not sure, but I can export it when it's used.
/rebuild |
@@ -0,0 +1,101 @@ | |||
// Copyright 2019-present, PingCAP, Inc. |
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.
I like this -present
.
store/tikv/lock_resolver.go
Outdated
@@ -277,23 +277,24 @@ func (lr *LockResolver) BatchResolveLocks(bo *Backoffer, locks []*Lock, loc Regi | |||
// commit status. | |||
// 3) Send `ResolveLock` cmd to the lock's region to resolve all locks belong to | |||
// the same transaction. | |||
func (lr *LockResolver) ResolveLocks(bo *Backoffer, callerStartTS uint64, locks []*Lock) (int64, error) { | |||
func (lr *LockResolver) ResolveLocks(bo *Backoffer, callerStartTS uint64, locks []*Lock) (int64, []uint64, error) { |
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.
It is better to add some comments for the return values.
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.
rest LGTM
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
} | ||
} else { | ||
tikvLockResolverCountWithNotExpired.Inc() | ||
// If the lock is valid, the txn may be a pessimistic transaction. | ||
// Update the txn expire time. | ||
msBeforeLockExpired := lr.store.GetOracle().UntilExpired(l.TxnID, status.ttl) | ||
msBeforeTxnExpired.update(msBeforeLockExpired) | ||
if callerStartTS > 0 { | ||
pushed = append(pushed, l.TxnID) | ||
} |
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.
do we have rolling upgrade problem here?
old TiDB B write some locks with lock.minCommitTS == 0
, new TiDB A read same key with callerStartTS > 0
...
so those lock append but not be pushed? @tiancaiamao @coocood
tidb/store/mockstore/mocktikv/mvcc_leveldb.go
Line 1074 in 8dc46b2
if lock.minCommitTS > 0 { |
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.
Good catch, we should try to push the transaction even if minCommitTS == 0
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.
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.
Addressed.
We don't push minCommitTS == 0
and this information is returned, ResolveLocks
will not set the pushed
result in this case (thus the caller has to wait).
/run-all-tests |
1 similar comment
/run-all-tests |
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
|
||
// ResolveLocks wraps the ResolveLocks function and store the resolved result. | ||
func (ch *clientHelper) ResolveLocks(bo *Backoffer, callerStartTS uint64, locks []*Lock) (int64, error) { | ||
msBeforeTxnExpired, resolvedLocks, err := ch.LockResolver.ResolveLocks(bo, callerStartTS, locks) |
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.
s/resolvedLocks/pushed
store/tikv/2pc.go
Outdated
@@ -128,6 +127,9 @@ type twoPhaseCommitter struct { | |||
regionTxnSize map[uint64]int | |||
// Used by pessimistic transaction and large transaction. | |||
ttlManager | |||
|
|||
// Used in the test code. | |||
zeroCommitTS bool |
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.
zeroMinCommitTS
store/tikv/2pc.go
Outdated
@@ -489,6 +491,14 @@ func (c *twoPhaseCommitter) buildPrewriteRequest(batch batchKeys, txnSize uint64 | |||
isPessimisticLock[i] = true | |||
} | |||
} | |||
var minCommitTS uint64 | |||
if !c.zeroCommitTS { |
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.
Can we add a failpoint to set minCommitTS to 0?
use failpoint to cover test
/bench |
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
Benchmark Report
@@ Benchmark Diff @@
================================================================================
--- tidb: 7f02792c5babab607ee6a86ef3ff6c2d85651b72
+++ tidb: 72eef5bac723e5f27b91cc000f038783c9a21ff9
tikv: 18bf609e78f81360e804ce5ce4ebf7d1a625e3d2
pd: 08e0143e83a2c42955bdfcbed4862f0339b9befc
================================================================================
oltp_update_non_index:
* QPS: 4713.20 ± 0.04% (std=1.22) delta: -0.50% (p=0.025)
* Latency p50: 27.16 ± 0.02% (std=0.00) delta: 0.51%
* Latency p99: 43.79 ± 2.72% (std=0.88) delta: 6.54%
oltp_insert:
* QPS: 4720.41 ± 0.18% (std=5.82) delta: -0.22% (p=0.033)
* Latency p50: 27.11 ± 0.18% (std=0.03) delta: 0.21%
* Latency p99: 48.34 ± 0.00% (std=0.00) delta: 4.09%
oltp_read_write:
* QPS: 15388.86 ± 0.10% (std=11.29) delta: 0.59% (p=0.051)
* Latency p50: 166.66 ± 0.10% (std=0.12) delta: -0.63%
* Latency p99: 306.13 ± 2.72% (std=6.16) delta: -5.70%
oltp_update_index:
* QPS: 4234.61 ± 0.02% (std=0.50) delta: -0.12% (p=0.337)
* Latency p50: 30.23 ± 0.02% (std=0.00) delta: 0.12%
* Latency p99: 55.35 ± 4.44% (std=1.64) delta: 3.24%
oltp_point_select:
* QPS: 39807.85 ± 0.47% (std=161.74) delta: 0.20% (p=0.594)
* Latency p50: 3.21 ± 0.54% (std=0.01) delta: -0.16%
* Latency p99: 9.65 ± 0.88% (std=0.08) delta: -0.46%
|
/merge |
/run-all-tests |
@tiancaiamao merge failed. |
…large transaction (pingcap#11986)
What problem does this PR solve?
The lock of the large transaction should not block reading.
When a reader meets a lock, it pushes forward the minCommitTS and reads the data of a previous version.
What is changed and how it works?
This commit implements the read operation through coprocessor.
Scan
andReverseScan
operation, to be exact.A
resolvedLock
is passed as parameter to skip the lock.The first time a read operation meets a lock, it will resolve lock. And then the second request will carry this information to skip the conflicting locks.
Read operation using PointGet (Get or BatchGet) is not included in this PR.
I'll add that later in another commit.
Check List
Tests
Depends on the new implementation of ResolveLock