mvcc: abstract a general forward scanner #6218

sticnarf · 2019-12-11T07:28:27Z

What have you changed?

In order to reduce repeated code for forward scanner, txn entry scanner and delta scanner, we want to reuse the forward scanner code.

I introduce ScanPolicy trait to define the behavior when the scanner meets a lock or a key.

Now this PR is a work in progress. It's just a refactor of the existing forward scanner.

What is the type of the changes?

Improvement (a change which is an improvement to an existing feature)

How is the PR tested?

Unit test
Integration test

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

Does this PR affect `tidb-ansible`?

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Any examples? (optional)

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

sticnarf · 2019-12-11T08:33:50Z

/bench +tpch

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

sticnarf · 2019-12-11T10:12:03Z

/test

sre-bot · 2019-12-11T14:11:25Z

Benchmark Report

Run TPC-H 10G Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
tidb: a94cff903cd1e7f3b050db782da84273ef5592f4
--- tikv: 134c5663340c8e868f23bc2f3273b84168aed862
+++ tikv: 066a950cf6078d7b273e27a76b3cef177da18dc5
pd: 13ea87a019a51d143e4c44365b39c49a87047d72
================================================================================
01.sql duration: 17933.71 ms ± 0.59% (std=62.06) delta: -4.40% (p=0.205)
02.sql duration: 6755.99 ms ± 3.32% (std=122.95) delta: 0.68% (p=0.716)
03.sql duration: 17070.60 ms ± 2.93% (std=282.66) delta: -4.86% (p=0.000)
04.sql duration: 7305.43 ms ± 3.29% (std=136.09) delta: -2.57% (p=0.007)
06.sql duration: 9517.13 ms ± 8.15% (std=331.54) delta: -2.08% (p=0.496)
07.sql duration: 14008.61 ms ± 1.96% (std=153.83) delta: -1.93% (p=0.003)
08.sql duration: 12217.36 ms ± 2.21% (std=176.95) delta: -1.11% (p=0.055)
09.sql duration: 35329.91 ms ± 1.80% (std=350.91) delta: 0.26% (p=0.830)
10.sql duration: 9804.63 ms ± 2.42% (std=130.00) delta: -0.38% (p=0.889)
11.sql duration: 3555.22 ms ± 4.13% (std=77.16) delta: 2.94% (p=0.319)
12.sql duration: 11943.37 ms ± 4.52% (std=271.64) delta: 0.56% (p=0.930)
13.sql duration: 10187.38 ms ± 1.77% (std=125.55) delta: 1.18% (p=0.453)
14.sql duration: 10752.69 ms ± 0.47% (std=31.11) delta: -3.78% (p=0.003)
15.sql duration: 19747.68 ms ± 2.82% (std=346.68) delta: -3.38% (p=0.004)
16.sql duration: 3597.96 ms ± 5.34% (std=112.53) delta: 0.08% (p=0.576)
17.sql duration: 34445.30 ms ± 1.36% (std=250.00) delta: -1.99% (p=0.000)
18.sql duration: 49621.62 ms ± 6.84% (std=1681.65) delta: -0.45% (p=0.265)
19.sql duration: 15204.40 ms ± 3.45% (std=316.35) delta: -1.67% (p=0.070)
20.sql duration: 10860.65 ms ± 4.71% (std=239.24) delta: -2.37% (p=0.151)
21.sql duration: 28671.07 ms ± 1.26% (std=201.60) delta: -1.08% (p=0.005)
22.sql duration: 5295.88 ms ± 3.02% (std=82.92) delta: 1.13% (p=0.590)

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

…canner Signed-off-by: Yilin Chen <sticnarf@gmail.com>

overvenus

Rest LGTM

overvenus · 2019-12-12T08:12:11Z

src/storage/mvcc/reader/scanner/forward.rs

@@ -140,73 +233,44 @@ impl<S: Snapshot> ForwardScanner<S> {
                    }
                };

-                // Use `from_encoded_slice` to reserve space for ts, so later we can append ts to
-                // the key or its clones without reallocation.
+                // // Use `from_encoded_slice` to reserve space for ts, so later we can append ts to


Double //?

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

…canner

Connor1996 · 2019-12-12T15:12:54Z

src/storage/mvcc/reader/scanner/backward.rs

-/// Use `ScannerBuilder` to build `BackwardScanner`.
-pub struct BackwardScanner<S: Snapshot> {
+/// Use `ScannerBuilder` to build `BackwardKvScanner`.
+pub struct BackwardKvScanner<S: Snapshot> {


KV is better?

According to Rust's naming conventions, acronyms count as one word.

src/storage/mvcc/reader/scanner/forward.rs

Connor1996 · 2019-12-12T16:19:18Z

src/storage/mvcc/reader/scanner/forward.rs

+        // Even if there is a lock error, we still need to step the cursor for future
+        // calls.
+        if result.is_err() {
+            cursors.move_write_cursor_to_next_user_key(&current_user_key, statistics)?;


when encountering conflict lock, now it moves write cursor to next user key no matter whether it has write for current user key.
seems it doesn't follow the previous behavior. It may skip a user key for the latter call?

oh, I misread the code. it will check current_user_key

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

…/tikv into general-forward-scanner

Connor1996

LGTM

sticnarf · 2019-12-13T03:16:18Z

@overvenus PTAL

breezewish · 2019-12-13T05:26:24Z

src/storage/mvcc/reader/scanner/forward.rs

-                    if self.cfg.omit_value {
-                        return Ok(Some(vec![]));
+                    if cfg.omit_value {
+                        break Some(vec![]);


This is the first time I know this syntax.

breezewish · 2019-12-13T05:27:54Z

@MyonKeminta PTAL

sre-bot · 2019-12-13T06:43:15Z

/run-all-tests

sticnarf · 2019-12-13T08:58:49Z

/merge

sre-bot · 2019-12-13T08:59:13Z

/run-all-tests

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

* mvcc: abstract a general forward scanner (#6218) Signed-off-by: Yilin Chen <sticnarf@gmail.com> * backup: support incremental backup (#6222) Signed-off-by: Yilin Chen <sticnarf@gmail.com> * adapt to 3.1 Signed-off-by: Yilin Chen <sticnarf@gmail.com> * fix Signed-off-by: Yilin Chen <sticnarf@gmail.com> Co-authored-by: Neil Shen <overvenus@gmail.com>

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

abstract a general forward scanner

8a69727

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

sticnarf added sig/transaction SIG: Transaction S: WIP labels Dec 11, 2019

sticnarf added 3 commits December 11, 2019 15:48

fix clippy

1d23cd0

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

fix move_write_cursor_to_ts bool

f093186

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

fix

066a950

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

some cleanup

87bf551

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

sticnarf removed the S: WIP label Dec 11, 2019

sticnarf changed the title ~~[WIP] mvcc: abstract a general forward scanner~~ mvcc: abstract a general forward scanner Dec 11, 2019

sticnarf requested a review from breezewish December 11, 2019 10:28

sticnarf mentioned this pull request Dec 11, 2019

backup: support incremental backup #6222

Merged

sticnarf requested a review from AndreMouche December 11, 2019 13:01

sticnarf added 2 commits December 12, 2019 11:01

update some comments

18dccfe

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

Merge branch 'master' into general-forward-scanner

8f78fc8

sticnarf requested a review from Connor1996 December 12, 2019 03:03

Merge remote-tracking branch 'upstream/master' into general-forward-s…

43b750a

…canner Signed-off-by: Yilin Chen <sticnarf@gmail.com>

sticnarf force-pushed the general-forward-scanner branch from 4d24a24 to 43b750a Compare December 12, 2019 09:47

overvenus reviewed Dec 12, 2019

View reviewed changes

sticnarf added 2 commits December 12, 2019 19:08

fix comment

b30155c

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

Merge remote-tracking branch 'upstream/master' into general-forward-s…

207a4f1

…canner

Connor1996 reviewed Dec 12, 2019

View reviewed changes

Merge branch 'master' into general-forward-scanner

3a6fbfc

Connor1996 reviewed Dec 12, 2019

View reviewed changes

sticnarf added 2 commits December 13, 2019 01:34

fix comments

5a6c011

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

Merge branch 'general-forward-scanner' of https://github.com/sticnarf…

e697207

…/tikv into general-forward-scanner

Connor1996 approved these changes Dec 13, 2019

View reviewed changes

breezewish approved these changes Dec 13, 2019

View reviewed changes

sticnarf added the status/can-merge Status: Can merge to base branch label Dec 13, 2019

Merge branch 'master' into general-forward-scanner

3a41419

sre-bot merged commit 321c286 into tikv:master Dec 13, 2019

zhang555 pushed a commit to zhang555/tikv that referenced this pull request Dec 16, 2019

mvcc: abstract a general forward scanner (tikv#6218)

94d40d1

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

sticnarf added a commit to sticnarf/tikv that referenced this pull request Dec 19, 2019

mvcc: abstract a general forward scanner (tikv#6218)

10b47ae

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

sticnarf mentioned this pull request Dec 19, 2019

implement incremental backup (#6218) (#6222) #6286

Merged

c1ay pushed a commit to c1ay/tikv that referenced this pull request May 9, 2020

mvcc: abstract a general forward scanner (tikv#6218)

9bf3991

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mvcc: abstract a general forward scanner #6218

mvcc: abstract a general forward scanner #6218

sticnarf commented Dec 11, 2019

sticnarf commented Dec 11, 2019

sticnarf commented Dec 11, 2019

sre-bot commented Dec 11, 2019

overvenus left a comment

overvenus Dec 12, 2019

sticnarf Dec 12, 2019

Connor1996 Dec 12, 2019

sticnarf Dec 12, 2019

Connor1996 Dec 12, 2019 •

edited

Connor1996 Dec 13, 2019

Connor1996 left a comment

sticnarf commented Dec 13, 2019

breezewish Dec 13, 2019

breezewish commented Dec 13, 2019

sre-bot commented Dec 13, 2019

sticnarf commented Dec 13, 2019

sre-bot commented Dec 13, 2019

mvcc: abstract a general forward scanner #6218

mvcc: abstract a general forward scanner #6218

Conversation

sticnarf commented Dec 11, 2019

What have you changed?

What is the type of the changes?

How is the PR tested?

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

Does this PR affect tidb-ansible?

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Any examples? (optional)

sticnarf commented Dec 11, 2019

sticnarf commented Dec 11, 2019

sre-bot commented Dec 11, 2019

Benchmark Report

overvenus left a comment

Choose a reason for hiding this comment

overvenus Dec 12, 2019

Choose a reason for hiding this comment

sticnarf Dec 12, 2019

Choose a reason for hiding this comment

Connor1996 Dec 12, 2019

Choose a reason for hiding this comment

sticnarf Dec 12, 2019

Choose a reason for hiding this comment

Connor1996 Dec 12, 2019 • edited

Choose a reason for hiding this comment

Connor1996 Dec 13, 2019

Choose a reason for hiding this comment

Connor1996 left a comment

Choose a reason for hiding this comment

sticnarf commented Dec 13, 2019

breezewish Dec 13, 2019

Choose a reason for hiding this comment

breezewish commented Dec 13, 2019

sre-bot commented Dec 13, 2019

sticnarf commented Dec 13, 2019

sre-bot commented Dec 13, 2019

Does this PR affect `tidb-ansible`?

Connor1996 Dec 12, 2019 •

edited