server, storage: make flashback compatible with resolved_ts by JmPotato · Pull Request #13823 · tikv/tikv

JmPotato · 2022-11-18T07:21:14Z

Signed-off-by: JmPotato ghzpotato@gmail.com

What is changed and how it works?

Issue Number: ref #13787. Should merge after pingcap/kvproto#1016.

What's Changed:

- Prewrite and commit `self.start_key` independently to prevent `resolved_ts` from advancing during the flashback process.
- Roll back all keys before prewriting `self.start_key`  during the preparing flashback.
- Add a test case for CDC compatibility.

The whole process now looks like these:

In PrepareFlashback, we will:

Roll back all locks.
Prewrite the self.start_key.

In FinishFlashback, we will:

Flashback all writes with 1PC.
Commit the self.start_key.

Check List

Tests

Unit test
Integration test

Release note

None.

ti-chi-bot · 2022-11-18T07:21:16Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Connor1996
overvenus

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

JmPotato · 2022-11-18T13:19:51Z

/cc @overvenus @Connor1996 @sticnarf

Please help review this PR, thx a lot.

src/storage/txn/actions/flashback_to_version.rs

src/storage/txn/commands/flashback_to_version.rs

src/server/service/kv.rs

src/storage/txn/actions/check_txn_status.rs

src/storage/txn/commands/flashback_to_version.rs

src/server/service/kv.rs

components/cdc/tests/integrations/test_cdc.rs

src/storage/txn/commands/flashback_to_version.rs

src/storage/txn/actions/flashback_to_version.rs

sticnarf · 2022-11-22T06:03:09Z

src/storage/txn/commands/flashback_to_version_read_phase.rs

+///   2. Read-and-flashback writes phase:
+///     - Scan all the latest writes and their corresponding values at
+///       `self.version`.
+///     - Write the old MVCC version writes again for all these keys with
+///       `self.commit_ts` excluding the `self.start_key` .
+///   3. Read-and-rollback locks phase:
+///     - Scan all locks.
+///     - Rollback all these locks excluding the `self.start_key` lock we write
+///       at the first phase.


I notice you swap the order of FlashbackLock and FlashbackWrite. Is there a reason for that?

Is it possible that some lock exists while you are writing the new flashbacked records? If so, it seems a bit weird... And I just start wondering what will happen if there is a lock on the start_key before flashback begins...

At first, I wanted to clean up the "placeholder" lock by finally cleaning the lock. Now that we have changed the way, I will change it back. Thx for pointing this out.

And I just start wondering what will happen if there is a lock on the start_key before flashback begins...

Since all the other read/write transactions will be blocked and fail during the flashback, I think this kind of lock doesn't have any side effects. 🤔

I'm just not sure how CDC will react if a lock is rewritten with a different start ts...

I have adjusted the whole process, now it works more reasonably I think.

In PrepareFlashback, we will:

Roll back all locks.

Prewrite the self.start_key.

In FinishFlashback, we will:

Flashback all writes with 1PC.

Commit the self.start_key.

As for the lock, after discussing with @overvenus, as long as the lock is roll backed normally, it won't be a bother to CDC. Also, since we checked the minimal store resolved_ts with the flashback version before sending the request, so it won't break any half-committed transaction either.

src/storage/txn/commands/flashback_to_version.rs

src/storage/txn/actions/flashback_to_version.rs

sticnarf

LGTM

JmPotato · 2022-11-22T08:29:39Z

@overvenus @Connor1996 PTAL also, thx.

src/storage/txn/actions/flashback_to_version.rs

Connor1996 · 2022-11-29T01:55:35Z

/merge

ti-chi-bot · 2022-11-29T01:55:36Z

@Connor1996: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-11-29T01:55:38Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: 470a674

JmPotato · 2022-11-29T02:39:10Z

/hold

Signed-off-by: JmPotato <ghzpotato@gmail.com>

JmPotato · 2022-11-29T02:42:03Z

/unhold

Signed-off-by: JmPotato <ghzpotato@gmail.com>

JmPotato · 2022-11-29T09:14:49Z

/test

Signed-off-by: JmPotato <ghzpotato@gmail.com>

sticnarf · 2022-11-29T13:22:53Z

src/storage/mvcc/reader/reader.rs

    }

+    #[inline(always)]
+    pub fn get_value(&mut self, key: &Key, start_ts: TimeStamp) -> Result<Option<Value>> {


SnapshotReader typically uses its own start_ts for something. If it doesn't need the start_ts of SnapshotReader itself, I suggest to just use the inner MvccReader directly.

Removed. PTAL.

BTW, your suggestion made me realize that there is a mix of SnapshotReader and MvccReader in the flashback commands, and I think we can open another separate PR to improve this situation, after this PR is merged, of course.

address this comment at #13860 @sticnarf @JmPotato

Signed-off-by: JmPotato <ghzpotato@gmail.com>

sticnarf · 2022-11-29T13:53:10Z

/merge

ti-chi-bot · 2022-11-29T13:53:12Z

@sticnarf: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-11-29T13:53:14Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: da39377

ti-chi-bot requested review from Connor1996, overvenus and sticnarf November 18, 2022 13:19

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2022

HuSharp reviewed Nov 18, 2022

View reviewed changes

src/storage/txn/actions/flashback_to_version.rs Outdated Show resolved Hide resolved

src/storage/txn/commands/flashback_to_version.rs Outdated Show resolved Hide resolved

JmPotato force-pushed the flashback_resolved_ts branch from 1cd8f03 to c7276ac Compare November 18, 2022 14:55

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2022

sticnarf reviewed Nov 21, 2022

View reviewed changes

src/server/service/kv.rs Outdated Show resolved Hide resolved

src/storage/txn/actions/check_txn_status.rs Outdated Show resolved Hide resolved

JmPotato requested a review from sticnarf November 21, 2022 13:40

HuSharp reviewed Nov 22, 2022

View reviewed changes

src/storage/txn/commands/flashback_to_version.rs Outdated Show resolved Hide resolved

src/server/service/kv.rs Outdated Show resolved Hide resolved

sticnarf reviewed Nov 22, 2022

View reviewed changes

components/cdc/tests/integrations/test_cdc.rs Outdated Show resolved Hide resolved

src/storage/txn/commands/flashback_to_version.rs Outdated Show resolved Hide resolved

JmPotato force-pushed the flashback_resolved_ts branch from b4cc9d4 to 95d4bb1 Compare November 22, 2022 05:23

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 22, 2022

JmPotato requested review from HuSharp and sticnarf November 22, 2022 05:25

sticnarf reviewed Nov 22, 2022

View reviewed changes

HuSharp reviewed Nov 22, 2022

View reviewed changes

src/storage/txn/commands/flashback_to_version.rs Outdated Show resolved Hide resolved

src/storage/txn/actions/flashback_to_version.rs Outdated Show resolved Hide resolved

JmPotato requested review from HuSharp and sticnarf November 22, 2022 07:38

sticnarf reviewed Nov 22, 2022

View reviewed changes

HuSharp mentioned this pull request Nov 22, 2022

commands: split flashback prewrite and commit. #13835

Closed

overvenus reviewed Nov 23, 2022

View reviewed changes

src/storage/txn/actions/flashback_to_version.rs Outdated Show resolved Hide resolved

ti-chi-bot added the status/LGT2 Indicates that a PR has LGTM 2. label Nov 28, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 29, 2022

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 29, 2022

Update the kvproto

b3ae7a6

Signed-off-by: JmPotato <ghzpotato@gmail.com>

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Nov 29, 2022

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 29, 2022

Defined2014 mentioned this pull request Nov 29, 2022

*: improve flashback to timestamp, let it compatible with TiCDC pingcap/tidb#39442

Merged

12 tasks

Avoid writing DEFAULT_CF twice

9204260

Signed-off-by: JmPotato <ghzpotato@gmail.com>

Refine the prewrite test

001a819

Signed-off-by: JmPotato <ghzpotato@gmail.com>

JmPotato mentioned this pull request Nov 29, 2022

restart tiflash during flashback, tiflash fail with "Code: 49, e.displayText() = DB::Exception: Found existing key in hex: " pingcap/tiflash#6379

Closed

Make the clippy happy

6b08481

Signed-off-by: JmPotato <ghzpotato@gmail.com>

sticnarf reviewed Nov 29, 2022

View reviewed changes

Do not inline get_value for SnapshotReader

da39377

Signed-off-by: JmPotato <ghzpotato@gmail.com>

JmPotato requested a review from sticnarf November 29, 2022 13:48

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 29, 2022

ti-chi-bot merged commit c1aceb0 into tikv:master Nov 29, 2022

ti-chi-bot added this to the Pool milestone Nov 29, 2022

JmPotato deleted the flashback_resolved_ts branch November 29, 2022 14:01

This was referenced Nov 30, 2022

Flashback may prewrite and commit an illegal key #13861

Closed

Flashback state may not be visible immediately in PeerFSM #13870

Closed

Conversation

JmPotato commented Nov 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Nov 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JmPotato commented Nov 18, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sticnarf Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JmPotato Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

JmPotato Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

sticnarf Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

JmPotato Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sticnarf left a comment

Choose a reason for hiding this comment

Uh oh!

JmPotato commented Nov 22, 2022

Uh oh!

Uh oh!

Connor1996 commented Nov 29, 2022

Uh oh!

ti-chi-bot commented Nov 29, 2022

Uh oh!

ti-chi-bot commented Nov 29, 2022

Uh oh!

JmPotato commented Nov 29, 2022

Uh oh!

JmPotato commented Nov 29, 2022

Uh oh!

JmPotato commented Nov 29, 2022

Uh oh!

sticnarf Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

JmPotato Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

HuSharp Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

sticnarf commented Nov 29, 2022

Uh oh!

ti-chi-bot commented Nov 29, 2022

Uh oh!

ti-chi-bot commented Nov 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

JmPotato commented Nov 18, 2022 •

edited

Loading

ti-chi-bot commented Nov 18, 2022 •

edited

Loading

sticnarf Nov 22, 2022 •

edited

Loading