-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
release-23.1: storage: fix comparison of suffixes #129743
release-23.1: storage: fix comparison of suffixes #129743
Conversation
Thanks for opening a backport. Please check the backport criteria before merging:
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
Also, please add a brief release justification to the body of your PR to justify this |
88a0e97
to
5a94f04
Compare
Had to make a change here since |
5a94f04
to
6eba49c
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.
Reviewed 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nicktrav and @RaduBerinde)
pkg/storage/pebble.go
line 148 at r1 (raw file):
Previously, RaduBerinde wrote…
Had to make a change here since
cmp.Compare
is not available on this branch.
change lgtm
This is a partial backport of cockroachdb#128043. Release justification: this fixes an issue hit in a real cluster where a range key is never cleaned up. The EngineComparer has a bug when comparing bare suffixes - we call the normalization function on a slice without the sentinel byte; that function's logic is based on length so it will not function properly. We recently observed that this can cause problems when a RangeKeySet has a suffix with the (now obsolete) synthetic bit set, and the corresponding `RangeKeyUnset` is issued by a newer version and doesn't have the bit set. These suffixes are supposed to equal each other but this is not currently the case. We fix the comparator to account for this case correctly and add a more comprehensive test. On the old code, the updated test failed with: ``` Compare(0000000000000002000000010d, 000000000000000200000001010e) = 1, expected 0 ``` Fixes: None Release note: None
6eba49c
to
702d4e6
Compare
Backport 1/1 commits from #129739.
/cc @cockroachdb/release
Backport 1/1 commits from #129605 on behalf of @RaduBerinde.
/cc @cockroachdb/release
This is a partial backport of #128043. CC @cockroachdb/release
Release justification: this fixes an issue hit in a real cluster where
a range key is never cleaned up.
The EngineComparer has a bug when comparing bare suffixes - we call
the normalization function on a slice without the sentinel byte; that
function's logic is based on length so it will not function properly.
We recently observed that this can cause problems when a
RangeKeySet has a suffix with the (now obsolete) synthetic bit set,
and the corresponding
RangeKeyUnset
is issued by a newer version anddoesn't have the bit set. These suffixes are supposed to equal each
other but this is not currently the case.
We fix the comparator to account for this case correctly and add a
more comprehensive test. On the old code, the updated test failed
with:
Fixes: #129592
Release note: None
Release justification: