Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Storages: Make DMFile ready for new column indexes/types #8756
Storages: Make DMFile ready for new column indexes/types #8756
Changes from 10 commits
6e12532
3ea3c8a
e5a3b24
4b2d30b
2dd81f1
04dcf52
bdc1188
619e6b7
8d55a74
0c1ae2b
dceea42
d381e53
84b5ee0
c4a4b96
283ba53
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
How about we write the new format data at the end (and also write old format in the old place)? In this way it may be possible to keep downgrade compatibility as new format will be regarded as extra data and will be discarded.
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.
Good suggestion! The ColumnStat is small enough to ignore its impact on performance and data size. We can keep it until we actually need an incompatible storage format changed by other components.
I'll change it
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.
After some tests, I found the downgrade compatibility can not be satisfied because there is a default branch 😂
tiflash/dbms/src/Storages/DeltaMerge/File/DMFile.cpp
Lines 1055 to 1059 in d9dc96b
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.
Maybe we can just logging instread of throwing exception here.
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.
Good idea. What kind of downgrade compatibility do we need to have? How about we first land the code that will not throw error (and will not change the format at all) for a few major versions, and then land the compatible format? In this way we will have downgrade compatibility for 1 major version. I think Vector will not land pingcap/tiflash in 6 months, so that there could be enough time for us to make the change.
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.
There is only downgrade compatibility requirement between patch versions under the same minor version. And no requirement across minor or major versions now. But it is good to have.
I've removed the exception thrown branch in this PR because the correctness of bytes read is protected by checksum. I think ignoring unknown meta block types without exception or logging is an acceptable behavior.
@JinheLin @breezewish