-
Notifications
You must be signed in to change notification settings - Fork 411
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
Fix potential data inconsistency under heavy ddl operation #5046
Fix potential data inconsistency under heavy ddl operation #5046
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
443ba75
to
4d64111
Compare
4d64111
to
05643a5
Compare
/run-all-tests |
1ca99aa
to
bea8447
Compare
Coverage for changed files
Coverage summary
full coverage report (for internal network access only) |
bea8447
to
57d9908
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-all-tests |
Coverage for changed files
Coverage summary
full coverage report (for internal network access only) |
/merge |
@lidezhu: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. 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. |
This pull request has been accepted and is ready to merge. Commit hash: 57d9908
|
Coverage for changed files
Coverage summary
full coverage report (for internal network access only) |
What problem does this PR solve?
Issue Number: ref #5032
Problem Summary: Currently we cache decoding schema for decoding raft data if a table schema doesn't change. And we judge whether a table schema has changed based on the table schema version.
But the schema version is not strictly consistent with the actual table schema which can be seen at https://github.com/pingcap/tiflash/blob/master/dbms/src/TiDB/Schema/SchemaBuilder.cpp#L362. That is when applying different schema changes in a diff, the table schema version will be set to the latest schema version after the first schema change is applied.
More concretely, when a lossy ddl change occurs, it will trigger
drop column
andadd column
schema changes and also rewrite some data at the same time. The schema changes will be applied one by one, and because tiflash updates the table schema version ahead of time when applying schema diff, after applying thedrop column
schema change, it will update the schema version to the latest schema version.And if the decode thread tries to obtain the current schema for decoding data before the
add column
is applied, the current schema and the latest schema version will be cached. Then after the subsequentadd column
operation is applied, the table schema version will not be updated, so the cache of the decode thread will not be invalidated.Therefore, the decode thread will decode the new data with an older schema, considering that the new added column is a dropped column and discarding it.
In addition, there is a lower chance of triggering this problem in the case of frequent add column and insert data.
cherry pick of #5044
What is changed and how it works?
Add an internal schema version for DecodingStorageSchemaSnapshot.
Check List
Tests
Side effects
Documentation
Release note