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

Add tidb_low_resolution_tso session scope variable on master #10428

Merged
merged 6 commits into from
May 22, 2019
Merged

Add tidb_low_resolution_tso session scope variable on master #10428

merged 6 commits into from
May 22, 2019

Conversation

sunxiaoguang
Copy link
Contributor

Low resolution timestamp is updated once every two seconds which can be used if reading slightly old version of data is acceptable. Like tidb_snapshot read, transaction is readonly when tidb_low_resolution_tso is enabled.

What problem does this PR solve?

Introduce a session scope variable tidb_low_resolution_tso which can use periodically updated
timestamp from TSO when transaction starts. This can save one TSO request roundtrip time per transaction which is beneficial for small readonly queries in terms of latency. As a side effect, there will be less TSO requests hence less pressure on PD.

What is changed and how it works?

Add a new tidb_low_resolution_tso session scope variable. When transaction starts and about to get start timestamp from TSO, call low resolution timestamp interface when tidb_low_resolution_tso is enabled. Like tidb_snapshot read, transaction is readonly when tidb_low_resolution_tso is enabled.

Tests

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

Related changes

  • Need to update the documentation

Low resolution timestamp is updated once every two seconds which can
be used if reading slightly old version of data is acceptable. Like
tidb_snapshot read, transaction is readonly when tidb_low_resolution_tso
is enabled.

Signed-off-by: Xiaoguang Sun <[email protected]>
@codecov
Copy link

codecov bot commented May 13, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@52c9d03). Click here to learn what that means.
The diff coverage is 60.7142%.

@@             Coverage Diff             @@
##             master     #10428   +/-   ##
===========================================
  Coverage          ?   77.2622%           
===========================================
  Files             ?        413           
  Lines             ?      86992           
  Branches          ?          0           
===========================================
  Hits              ?      67212           
  Misses            ?      14615           
  Partials          ?       5165

@jackysp jackysp added the contribution This PR is from a community contributor. label May 13, 2019
@zz-jason zz-jason requested review from tiancaiamao and jackysp May 13, 2019 10:47
type lowResolutionTsFuture uint64

// Wait implements the oracle.Future interface.
func (f lowResolutionTsFuture) Wait() (uint64, error) {
Copy link
Member

Choose a reason for hiding this comment

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

how about abbreviate Resolution to Rez or Reso? Found in https://www.abbreviations.com/abbreviation/RESOLUTION

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does look verbose now. But i'm not sure about Rez, looks strange to me though. Let's see what other people think about it.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to keep the whole word.

@@ -140,3 +140,21 @@ func (o *pdOracle) updateTS(ctx context.Context, interval time.Duration) {
func (o *pdOracle) Close() {
close(o.quit)
}

type lowResolutionTsFuture uint64
Copy link
Member

Choose a reason for hiding this comment

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

please add some comment about this future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

sessionctx/variable/session.go Outdated Show resolved Hide resolved
session/txn.go Outdated Show resolved Hide resolved
@sunxiaoguang
Copy link
Contributor Author

It's strange that I have signed CLA however it kept complaining it's not done yet.

@sunxiaoguang
Copy link
Contributor Author

Finally got CLA passed after merging master

@tiancaiamao
Copy link
Contributor

/run-all-tests

@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added status/all tests passed status/LGT1 Indicates that a PR has LGTM 1. labels May 20, 2019
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason merged commit abd013c into pingcap:master May 22, 2019
@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 22, 2019
db-storage pushed a commit to db-storage/tidb that referenced this pull request May 29, 2019
db-storage pushed a commit to db-storage/tidb that referenced this pull request May 29, 2019
@sunxiaoguang sunxiaoguang deleted the low_resolution_tso_on_master branch June 4, 2019 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants