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

tikv: use timestamp from TSO instead of MaxVersion in point get retry #22789

Merged
merged 7 commits into from
Feb 22, 2021

Conversation

sticnarf
Copy link
Contributor

@sticnarf sticnarf commented Feb 18, 2021

What problem does this PR solve?

We use MaxVersion to read when doing an auto-commit point select. Such reads can be blocked by any secondary locks or async-commit locks for linearizability.

Before this PR, we try to resolve the lock and backoff before retry. However, these operations can be unnecessary. And in the worst cases, it is possible that the reader keeps meeting different locks after backoff, which increases latency a lot.

What is changed and how it works?

In this PR, before doing a point get with max version, we get a timestamp from the TSO asynchronously. If the point get meets a lock, we use the timestamp from the TSO to retry. It does not add latency if point get does not meet a lock while improving the worst cases.

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

Run two sysbench test concurrently:

  1. update index with zipfian exp=2, 256 threads (with async commit enabled)
  2. point select with zipfian exp=2, 64 threads

So the point select will read the lock very often. Measure the QPS of point select:

Master: 5032.14
This PR: 8809.72

Release note

  • Use TSO timestamp in auto-commit point get retries.

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf changed the title tikv: ignore different locks when using MaxVersion to get tikv: use timestamp from TSO instead of MaxVersion in point get retry Feb 19, 2021
@sticnarf sticnarf marked this pull request as ready for review February 19, 2021 05:03
Copy link
Contributor

@youjiali1995 youjiali1995 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 19, 2021
@youjiali1995
Copy link
Contributor

Run two sysbench test concurrently:
update index with zipfian exp=2, 256 threads
point select with zipfian exp=2, 64 threads
So the point select will read the lock very often. Measure the QPS of point select:
Master: 5032.14
This PR: 8809.72

Is async-commit on? The reason why async commit has some regression using point get in high-conflict scenarios is it's more likely to be blocked by memory locks because they're written faster in memory than the raft store?

@sticnarf
Copy link
Contributor Author

sticnarf commented Feb 20, 2021

Run two sysbench test concurrently:
update index with zipfian exp=2, 256 threads
point select with zipfian exp=2, 64 threads
So the point select will read the lock very often. Measure the QPS of point select:
Master: 5032.14
This PR: 8809.72

Is async-commit on? The reason why async commit has some regression using point get in high-conflict scenarios is it's more likely to be blocked by memory locks because they're written faster in memory than the raft store?

Yes. This PR mitigates one of the regression causes. And my another PR tikv/tikv#9625 deals with the other one you mentioned

Copy link
Contributor

@nrc nrc left a comment

Choose a reason for hiding this comment

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

lgtm

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Feb 21, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Feb 21, 2021
@youjiali1995
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 22833

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 22, 2021
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 2bbec47 into pingcap:master Feb 22, 2021
youjiali1995 added a commit to youjiali1995/tidb that referenced this pull request Apr 1, 2021
Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
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. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants