Acquire read lock in LockKeys to avoid data race #585
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cfzjywxk
reviewed
Sep 15, 2022
| @@ -86,6 +86,7 @@ func (actionPessimisticRollback) tiKVTxnRegionsNumHistogram() prometheus.Observe | |||
| func (action actionPessimisticLock) handleSingleBatch(c *twoPhaseCommitter, bo *retry.Backoffer, batch batchMutations) error { | |||
| m := batch.mutations | |||
| mutations := make([]*kvrpcpb.Mutation, m.Len()) | |||
| c.txn.GetMemBuffer().RLock() | |||
There was a problem hiding this comment.
If the contention happens here would it be possible that actionPessimisticLock tasks are processed one by one according to the feature of the RWLock? I'm not quite sure if it may introduce extra risks.
There was a problem hiding this comment.
Do you mean that all later RLock requests will wait for a queuing write lock request? Could you point out which write lock may cause it to run one by one?
There was a problem hiding this comment.
For example:
Update // Try to acquire the write lock.
Some possible parallel executors(like parallel projection in the early issue)
selectLockExec // Try to acquire read locks.
The update execution may try to Set new key-value pairs into the memory buffer which needs to acquire the write lock, while the parallel actionPessimisticLock operations triggered by LockKeys are being processed. If an actionPessimisticLock gets the read lock successfully first and then the write lock acquisition is pending, other actionPessimisticLock requests may have to wait.
There was a problem hiding this comment.
Got it. Yet I think it will have little impact if all these operations are fast. And the goroutines that are blocked after the write lock will then acquire the read lock at the same time, so they can run concurrently as before.
I will try to design a worst case to see how much impact it will bring.
|
We may need a test case to verify that the panic could be avoided, some benchmark tests are needed too. |
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
sticnarf
force-pushed
the
lock-keys-rlock
branch
from
dd2de13
to
9a1aa06
Compare
|
Table schema: CREATE TABLE `t` (
`id` bigint(20) NOT NULL,
`v` bigint(20) DEFAULT NULL,
PRIMARY KEY (`id`) /*T![clustered_index] CLUSTERED */
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin
PARTITION BY HASH (`id`) PARTITIONS 10Workload: BEGIN;
UPDATE t SET v = v + 1 WHERE id BETWEEN {x} AND {x} + 1000
ROLLBACK;Execution plan: I revert pingcap/tidb#30290 to allow concurrency for the projection executor in this plan. I limit the CPU of TiDB to only 4 cores to let TiDB be a bottleneck.
In benchbot TPC-C 400 threads test, I find no regression but the TiDB CPU is not 100% used. This PR: 69682 QPS, Baseline: 68184. (It should not improve performance, it should be just small environment error). I'll also benchmark against other cases when TiDB is the bottleneck. |
|
I also test sysbench read_write in the benchbot, which almost runs out of TiDB CPU. (Sorry, the links below are not publicly accessible) There seems no big difference
|
cfzjywxk
approved these changes
Oct 10, 2022
sticnarf
added a commit
to sticnarf/client-go
that referenced
this pull request
Oct 12, 2022
Signed-off-by: Yilin Chen <sticnarf@gmail.com> Signed-off-by: Yilin Chen <sticnarf@gmail.com>
cfzjywxk
pushed a commit
that referenced
this pull request
Oct 12, 2022
* Clear intersecting regions in the cache when inserting a region (#566) Signed-off-by: Yilin Chen <sticnarf@gmail.com> * Acquire read lock in LockKeys to avoid data race (#585) Signed-off-by: Yilin Chen <sticnarf@gmail.com> Signed-off-by: Yilin Chen <sticnarf@gmail.com> Signed-off-by: Yilin Chen <sticnarf@gmail.com>
This was referenced Oct 12, 2022
guo-shaoge
pushed a commit
to guo-shaoge/client-go
that referenced
this pull request
Oct 13, 2022
Signed-off-by: Yilin Chen <sticnarf@gmail.com> Signed-off-by: Yilin Chen <sticnarf@gmail.com> Signed-off-by: guo-shaoge <shaoge1994@163.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR tries to reduce the risk of data race in memDB like pingcap/tidb#26832, by acquiring the read lock when it needs to read the memDB in the execution path of
LockKeys. The sameRWMutexis used inmemDB.set, so we won't have concurrent write and read.Throughout our observations about similar problems, most of them are connected with locking keys while updating the membuf. So, although this PR does not thoroughly solve the problem of memDB data race, I think it will largely reduce its possibility in the real world.
The granularity of locking is relatively coarse to minimize the performance impact as much as possible.
extractKeyExistsErris not in the critical path, so the lock is acquired in each call.