Skip to content

Fix potential data loss for tombstoned table#1010

Merged
zanmato1984 merged 9 commits intomasterfrom
fix-1007
Mar 26, 2021
Merged

Fix potential data loss for tombstoned table#1010
zanmato1984 merged 9 commits intomasterfrom
fix-1007

Conversation

@zanmato1984
Copy link
Copy Markdown
Contributor

@zanmato1984 zanmato1984 commented Aug 13, 2020

What problem does this PR solve?

Issue Number: close #1007

Problem Summary:

What is changed and how it works?

What's Changed:

  • Do write the data into storage even if it is tombstoned.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn: none
  • Need to cherry-pick to the release branch: 4.0, 3.1.

Check List

Tests

  • Integration test

Side effects

  • None

Release note

  • Fix potential data loss when recovering a table that is previously dropped.

@zanmato1984 zanmato1984 added type/bugfix This PR fixes a bug. needs-cherry-pick-release-3.1 PR which needs to be cherry-picked to release-3.1 needs-cherry-pick-release-4.0 PR which needs to be cherry-picked to release-4.0 labels Aug 13, 2020
@zanmato1984
Copy link
Copy Markdown
Contributor Author

zanmato1984 commented Aug 13, 2020

/run-all-tests

@zanmato1984
Copy link
Copy Markdown
Contributor Author

zanmato1984 commented Aug 13, 2020

/run-all-tests

@zanmato1984
Copy link
Copy Markdown
Contributor Author

zanmato1984 commented Aug 13, 2020

/run-all-tests

@zanmato1984
Copy link
Copy Markdown
Contributor Author

zanmato1984 commented Mar 26, 2021

/run-all-tests

@zanmato1984
Copy link
Copy Markdown
Contributor Author

zanmato1984 commented Mar 26, 2021

/run-all-tests

JaySon-Huang
JaySon-Huang approved these changes Mar 26, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@JaySon-Huang JaySon-Huang left a comment

Choose a reason for hiding this comment

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

LGTM with minor comments

tests/fullstack-test/ddl/recover_table.test
Comment on lines +8 to +12
# Disable flushing.
>> DBGInvoke __set_flush_threshold(1000000, 1000000)

# Insert a record and it should stay in kvstore.
mysql> insert into test.t values (1);
Copy link
Copy Markdown
Contributor

@JaySon-Huang JaySon-Huang Mar 26, 2021

Choose a reason for hiding this comment

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

Actually, the record won't stay in kvstore when using DT as the storage engine.
I may file a PR to build the test case by using FAIL_POINT_PAUSE later.

Copy link
Copy Markdown
Contributor Author

@zanmato1984 zanmato1984 Mar 26, 2021

Choose a reason for hiding this comment

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

OK

Copy link
Copy Markdown
Contributor

@JaySon-Huang JaySon-Huang Mar 27, 2021

Choose a reason for hiding this comment

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

The case is added in this PR: #1666

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 26, 2021
@JaySon-Huang
Copy link
Copy Markdown
Contributor

JaySon-Huang commented Mar 26, 2021

I think we should also apply this change to the new logic in GenRegionPreDecodeBlockData:
https://github.com/pingcap/tics/blob/fd761ef4776679c2646065a430a94a6dfbcd17d9/dbms/src/Storages/Transaction/PartitionStreams.cpp#L383-L389

@zanmato1984
Copy link
Copy Markdown
Contributor Author

zanmato1984 commented Mar 26, 2021

I think we should also apply this change to the new logic in GenRegionPreDecodeBlockData:
https://github.com/pingcap/tics/blob/fd761ef4776679c2646065a430a94a6dfbcd17d9/dbms/src/Storages/Transaction/PartitionStreams.cpp#L383-L389

Thanks for reminding.

@zanmato1984
Copy link
Copy Markdown
Contributor Author

zanmato1984 commented Mar 26, 2021

I think we should also apply this change to the new logic in GenRegionPreDecodeBlockData:
https://github.com/pingcap/tics/blob/fd761ef4776679c2646065a430a94a6dfbcd17d9/dbms/src/Storages/Transaction/PartitionStreams.cpp#L383-L389

Addressed. I think we are OK if CI goes right.

@zanmato1984
Copy link
Copy Markdown
Contributor Author

zanmato1984 commented Mar 26, 2021

/run-all-tests

@zanmato1984 zanmato1984 merged commit 7cfe93d into master Mar 26, 2021
@zanmato1984 zanmato1984 deleted the fix-1007 branch March 26, 2021 14:43
@zanmato1984 zanmato1984 added the needs-cherry-pick-release-5.0 PR which needs to be cherry-picked to release-5.0 label Mar 26, 2021
@ti-srebot ti-srebot mentioned this pull request Mar 26, 2021
Closed
@ti-srebot
Copy link
Copy Markdown
Collaborator

ti-srebot commented Mar 26, 2021

cherry pick to release-3.1 in PR #1661

@ti-srebot ti-srebot mentioned this pull request Mar 26, 2021
Merged
@ti-srebot
Copy link
Copy Markdown
Collaborator

ti-srebot commented Mar 26, 2021

cherry pick to release-4.0 in PR #1662

@ti-srebot ti-srebot mentioned this pull request Mar 26, 2021
Merged
@ti-srebot
Copy link
Copy Markdown
Collaborator

ti-srebot commented Mar 26, 2021

cherry pick to release-5.0 in PR #1663

zanmato1984 added a commit that referenced this pull request Mar 26, 2021
@ti-srebot @zanmato1984
cherry pick #1010 to release-5.0 (#1663)
911724c 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

Co-authored-by: ruoxi <zanmato1984@gmail.com>
zanmato1984 added a commit that referenced this pull request Mar 26, 2021
@ti-srebot @zanmato1984
cherry pick #1010 to release-4.0 (#1662)
fb2d66c 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

Co-authored-by: ruoxi <zanmato1984@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JaySon-Huang JaySon-Huang JaySon-Huang approved these changes

Assignees

No one assigned

Labels

needs-cherry-pick-release-3.1 PR which needs to be cherry-picked to release-3.1 needs-cherry-pick-release-4.0 PR which needs to be cherry-picked to release-4.0 needs-cherry-pick-release-5.0 PR which needs to be cherry-picked to release-5.0 status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Some data may be lost if a table is flashed back

3 participants

@zanmato1984 @JaySon-Huang @ti-srebot