Fix wrong smallest key of delete range tombstones #2799
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
@huachaohuang updated the pull request - view changes |
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| @@ -412,7 +412,6 @@ void RangeDelAggregator::AddToBuilder( | |||
|
|
|||
| // Note the order in which tombstones are stored is insignificant since we | |||
| // insert them into a std::map on the read path. | |||
| bool first_added = false; | |||
There was a problem hiding this comment.
could you have just moved this one line lower? Within each stripe the tombstones should be sorted. So we only need to consider the first tombstone in each stripe.
db/range_del_aggregator_test.cc
Outdated
| @@ -149,6 +149,38 @@ TEST_F(RangeDelAggregatorTest, AlternateMultipleAboveBelow) { | |||
| {"h", 0}}); | |||
| } | |||
|
|
|||
| TEST_F(RangeDelAggregatorTest, UnorderedTombstones) { | |||
There was a problem hiding this comment.
Also this belongs in db/db_range_del_test.cc
|
@huachaohuang updated the pull request - view changes - changes since last import |
|
Excellent, thanks everyone at PingCAP who helped find this bug :). |
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@huachaohuang updated the pull request - view changes - changes since last import |
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@zhangjinpeng1987 - sure, the 5.7.3 release is available now containing both fixes. |
Summary: Since tombstones are not stored in order, we may get a wrong smallest key if we only consider the first added tombstone. Check facebook#2752 for more details. Closes facebook#2799 Differential Revision: D5728217 Pulled By: ajkr fbshipit-source-id: 4a53edb0ca80d2a9fcf10749e52d47d57d6417d3
Summary: Since tombstones are not stored in order, we may get a wrong smallest key if we only consider the first added tombstone. Check facebook#2752 for more details. Closes facebook#2799 Differential Revision: D5728217 Pulled By: ajkr
Since tombstones are not stored in order, we may get a wrong smallest key if we only consider the first added tombstone.
Check #2752 for more details.