txn_types: Add GC fence feild to Write records #9207

MyonKeminta · 2020-12-09T09:05:13Z

Signed-off-by: MyonKeminta MyonKeminta@users.noreply.github.com

What problem does this PR solve?

Issue Number:

Adds a gc_fence field to Write records which will be used to resolve the compatibility issue between async commit and gc compaction filter/cdc/tiflash.

What is changed and how it works?

Added a gc_fence field.

Related changes

Check List

Tests

Unit test

Side effects

Release note

No release note.

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

ti-srebot · 2020-12-09T09:08:22Z

@hicqu, Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments. See the corresponding SIG page for more information. Related SIG: transaction(slack).

youjiali1995 · 2020-12-10T02:48:17Z

components/txn_types/src/write.rs

@@ -68,6 +70,47 @@ pub struct Write {
    /// Also note that `has_overlapped_rollback` field is only necessary when the Rollback record
    /// should be protected.
    pub has_overlapped_rollback: bool,


Seems it can be replaced by gc_fence?

If it's produced because commit_ts == rollback_ts in the lock, it doesn't need a gc fence.

sticnarf

If it's produced because commit_ts == rollback_ts in the lock, it doesn't need a gc fence.

sticnarf · 2020-12-10T02:54:31Z

components/txn_types/src/write.rs

@@ -21,6 +21,7 @@ const FLAG_LOCK: u8 = b'L';
 const FLAG_ROLLBACK: u8 = b'R';

 const FLAG_OVERLAPPED_ROLLBACK: u8 = b'R';
+const FLAG_GC_FENCE: u8 = b'F';


Is it better to call it a prefix instead of a flag because it has a timestamp payload?

sticnarf · 2020-12-10T03:09:33Z

components/txn_types/src/write.rs

+    ///   doesn't have an newer version.
+    /// * `Some(ts)`: A commit record that has been rewritten due to overlapping rollback,
+    ///   and it's next version's `commit_ts` is `ts`
+    pub gc_fence: Option<TimeStamp>,
 }

 impl std::fmt::Debug for Write {


Debug needs updating

sticnarf · 2020-12-10T03:12:47Z

components/txn_types/src/write.rs

@@ -217,6 +277,10 @@ impl WriteRef<'_> {
        if self.has_overlapped_rollback {
            b.push(FLAG_OVERLAPPED_ROLLBACK);
        }
+        if let Some(ts) = self.gc_fence {


Maybe we can calculate a better capacity estimation like https://github.com/tikv/tikv/blob/6b4466ba8a/components/txn_types/src/lock.rs#L161

nrc

lgtm, one query about the comment

nrc · 2020-12-10T03:22:26Z

components/txn_types/src/write.rs

+    /// * `Some(0)`: A commit record that has been rewritten due to overlapping rollback, but it
+    ///   doesn't have an newer version.
+    /// * `Some(ts)`: A commit record that has been rewritten due to overlapping rollback,
+    ///   and it's next version's `commit_ts` is `ts`


This is a great comment! 👍👍👍

nrc · 2020-12-10T03:26:01Z

components/txn_types/src/write.rs

+    /// record, add a special field `gc_fence` on it. If there is a newer version after the record
+    /// being rewritten, the next version's `commit_ts` will be recorded. When MVCC reading finds
+    /// a commit record with a GC fence timestamp but the corresponding version that matches that ts
+    /// doesn't exest, the current version will be believed to be already GC-ed and ignored.


Could you say when and what the gc fence would be set to in the example?

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

sticnarf

The rest lgtm.

components/txn_types/src/write.rs

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

hicqu · 2020-12-10T05:45:00Z

/merge

ti-srebot · 2020-12-10T05:45:03Z

@hicqu Oops! auto merge is restricted to Committers of the SIG.See the corresponding SIG page for more information. Related SIG: transaction(slack).

sticnarf · 2020-12-10T05:48:17Z

/merge

ti-srebot · 2020-12-10T05:50:50Z

/run-all-tests

youjiali1995 · 2020-12-10T06:04:16Z

components/txn_types/src/write.rs

@@ -217,9 +291,25 @@ impl WriteRef<'_> {
        if self.has_overlapped_rollback {
            b.push(FLAG_OVERLAPPED_ROLLBACK);
        }
+        if let Some(ts) = self.gc_fence {
+            b.push(GC_FENCE_PREFIX);
+            b.encode_u64(ts.into_inner()).unwrap();


Why not encode_var_u64?

VarInt saves space when ts is 0. But VarInt occupies more space (9 bytes vs 8 bytes) and is slower when ts is a real timestamp.
It has little difference to me because overlapping happens rarely...

I thought it doesn't matter much which encoding to use here, but encode_u64 looks faster than var_u64

But it's strange to encode start_ts in var int and this ts in u64.

You can see in Lock's encoding, everything new is using encode_u64...

ti-srebot · 2020-12-10T06:04:49Z

@MyonKeminta merge failed.

hicqu · 2020-12-10T06:05:59Z

/run-all-tests

MyonKeminta · 2020-12-10T06:53:58Z

/merge

ti-srebot · 2020-12-10T06:56:20Z

/run-all-tests

- tikv/tikv#8349 and tikv/tikv#9207 has introduced new behavior in txt write. - When meet `GC fence` flag, it is definitely a rewriting record and there must be a complete row has been written, just ignore it in tiflash. Signed-off-by: Tong Zhigao <tongzhigao@pingcap.com>

txn_types: Add GC fence feild to Write records

6b4466b

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta added the sig/transaction SIG: Transaction label Dec 9, 2020

MyonKeminta requested review from hicqu, youjiali1995 and sticnarf December 9, 2020 09:05

hicqu approved these changes Dec 9, 2020

View reviewed changes

MyonKeminta requested a review from nrc December 9, 2020 09:10

youjiali1995 reviewed Dec 10, 2020

View reviewed changes

sticnarf reviewed Dec 10, 2020

View reviewed changes

nrc approved these changes Dec 10, 2020

View reviewed changes

ti-srebot added the status/LGT1 Status: PR - There is already 1 approval label Dec 10, 2020

Address comments

ae871ec

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

sticnarf approved these changes Dec 10, 2020

View reviewed changes

ti-srebot removed the status/LGT1 Status: PR - There is already 1 approval label Dec 10, 2020

ti-srebot approved these changes Dec 10, 2020

View reviewed changes

ti-srebot added the status/LGT2 Status: PR - There are already 2 approvals label Dec 10, 2020

sticnarf reviewed Dec 10, 2020

View reviewed changes

components/txn_types/src/write.rs Outdated Show resolved Hide resolved

Address comments

bbc5b0c

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

ti-srebot added the status/can-merge Status: Can merge to base branch label Dec 10, 2020

Merge branch 'master' into m/add-gc-fence-field

7ae0c9f

youjiali1995 reviewed Dec 10, 2020

View reviewed changes

solotzg mentioned this pull request Dec 10, 2020

[Async Commit] Support new behavior about rewriting pingcap/tiflash#1259

Closed

Merge branch 'master' into m/add-gc-fence-field

7fcd438

ti-srebot merged commit 18b7b7a into tikv:master Dec 10, 2020

MyonKeminta deleted the m/add-gc-fence-field branch December 10, 2020 07:28

solotzg mentioned this pull request Dec 10, 2020

[Async Commit] Support new behavior about rewriting pingcap/tiflash#1278

Merged

MyonKeminta mentioned this pull request Dec 10, 2020

storage/mvcc: Check GC Fence when reading #9214

Merged

20 tasks

hicqu mentioned this pull request Dec 11, 2020

txn: append GC fence with overlapped rollback flag if necessary #9222

Merged

sticnarf added this to In progress in Async Commit via automation Dec 29, 2020

sticnarf moved this from In progress to Done in Async Commit Dec 29, 2020

ti-srebot mentioned this pull request Jan 27, 2021

storage/mvcc: Check GC Fence when reading (#9214) #9581

Closed

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

txn_types: Add GC fence feild to Write records #9207

txn_types: Add GC fence feild to Write records #9207

MyonKeminta commented Dec 9, 2020

ti-srebot commented Dec 9, 2020

youjiali1995 Dec 10, 2020

sticnarf Dec 10, 2020

sticnarf left a comment

sticnarf Dec 10, 2020 •

edited

sticnarf Dec 10, 2020

sticnarf Dec 10, 2020

nrc left a comment

nrc Dec 10, 2020

nrc Dec 10, 2020

sticnarf left a comment

hicqu commented Dec 10, 2020

ti-srebot commented Dec 10, 2020

sticnarf commented Dec 10, 2020

ti-srebot commented Dec 10, 2020

youjiali1995 Dec 10, 2020

sticnarf Dec 10, 2020

MyonKeminta Dec 10, 2020

youjiali1995 Dec 10, 2020

MyonKeminta Dec 10, 2020

ti-srebot commented Dec 10, 2020

hicqu commented Dec 10, 2020

MyonKeminta commented Dec 10, 2020

ti-srebot commented Dec 10, 2020

txn_types: Add GC fence feild to Write records #9207

txn_types: Add GC fence feild to Write records #9207

Conversation

MyonKeminta commented Dec 9, 2020

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

ti-srebot commented Dec 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sticnarf left a comment

Choose a reason for hiding this comment

sticnarf Dec 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sticnarf left a comment

Choose a reason for hiding this comment

hicqu commented Dec 10, 2020

ti-srebot commented Dec 10, 2020

sticnarf commented Dec 10, 2020

ti-srebot commented Dec 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-srebot commented Dec 10, 2020

hicqu commented Dec 10, 2020

MyonKeminta commented Dec 10, 2020

ti-srebot commented Dec 10, 2020

sticnarf Dec 10, 2020 •

edited