Backport facebook/rocksdb#4432 #17
Conversation
This makes future backports apply cleanly.
This makes future backports apply cleanly.
Summary: `CollapsedRangeDelMap` internally uses seqno zero as a sentinel value to denote a gap between range tombstones or the end of range tombstones. It therefore expects to never have consecutive sentinel tombstones. However, since `DeleteRange` is now supported in `SstFileWriter`, an ingested file may contain range tombstones, and that ingested file may be assigned global seqno zero. When such tombstones are added to the collapsed map, they resemble sentinel tombstones due to having seqno zero. Then, the invariant mentioned above about never having consecutive sentinel tombstones can be violated. The symptom of this violation was dereferencing the `end()` iterator (facebook#4204). The fix in this PR is to not add range tombstones with seqno zero to the collapsed map. They're not needed anyways since they can't possibly cover anything (in case of a key and a range tombstone with the same seqno, the key is visible). Pull Request resolved: facebook#4216 Differential Revision: D9121716 Pulled By: ajkr fbshipit-source-id: f5b78a70bea9527354603ea7ac8542a7e2b6a210
Summary: Add a unit test for range collapsing when non-default comparator is used. This exposes the bug fixed in facebook#4386. Pull Request resolved: facebook#4388 Differential Revision: D9918252 Pulled By: ajkr fbshipit-source-id: 99501b96b251eab41791a7e33b27055ee36c5c39
Summary: We add two subcommands `write_extern_sst` and `ingest_extern_sst` to ldb. This PR avoids changing existing code because we hope to cherry-pick to earlier releases to support compatibility check for external SST file ingestion. Pull Request resolved: facebook#4205 Differential Revision: D9112711 Pulled By: riversand963 fbshipit-source-id: 7cae88380d4de86da8440230e87eca66755648e4
Add some missing override keywords to make backporting easier. Adapts upstream commit 33ad906.
This is a partial backport of upstream commit ef7815b ("Support range deletion tombstones in IngestExternalFile SSTs") to make future backports cleaner.
) Summary: In C++ 11, the order of argument and move evaluation in a statement such as below is unspecified - foo(a.b).bar(std::move(a)) The compiler is free to evaluate std::move(a) first, and then a.b is unspecified. In C++ 17, this will be safe if a draft proposal around function chaining rules is accepted. Pull Request resolved: facebook#4348 Differential Revision: D9688810 Pulled By: anand1976 fbshipit-source-id: e4651d0ca03dcf007e50371a0fc72c0d1e710fb4
Summary: To more accurately truncate range tombstones at SST boundaries, we now represent them in RangeDelAggregator using InternalKeys, which are end-key-exclusive as they were before this change. During compaction, "atomic compaction unit boundaries" (the range of keys contained in neighbouring and overlaping SSTs) are propagated down to RangeDelAggregator to truncate range tombstones at those boundariies instead. See facebook#4432 (comment) and facebook#4432 (comment) for motivating examples. Pull Request resolved: facebook#4432 Differential Revision: D10263952 Pulled By: abhimadan fbshipit-source-id: 2fe85ff8a02b3a6a2de2edfe708012797a7bd579
|
@benesch Yep, I'll pick this up. Thanks for getting it this far. |
|
@benesch I added an additional commit which fixes up the problems with |
| AddTombstones(&range_del_agg, {{"a", "d", 20}}); | ||
| Slice b = "b", c = "c"; | ||
| ParsedInternalKey b = {"b", kMaxSequenceNumber, kMaxValue}; | ||
| ParsedInternalKey c {"c", kMaxSequenceNumber, kMaxValue}; | ||
| VerifyPartialTombstonesEq(PartialRangeTombstone(&b, &c, 10), tombstone); |
There was a problem hiding this comment.
I think these tests would benefit from cases where a tombstone has been split across multiple files. Perhaps that what you meant by more love.
There was a problem hiding this comment.
Yes, that's exactly what I was referring to. Didn't get around to doing this yet, but it is high on my TODO list.
There was a problem hiding this comment.
I've added a few more test cases that verify behavior in the presence of truncated tombstones. PTAL.
There was a problem hiding this comment.
Are you sure you pushed? I'm having trouble finding these new test cases.
There was a problem hiding this comment.
I pushed to the wrong branch. Sorry about that. Take another look.
There was a problem hiding this comment.
You're really doubling down on putting these reviews in hard mode, huh?
Change `RangeDelAggregator::GetTombstone,ShouldDeleteRange` to take internal keys instead of user keys.
| range_tombstone_.SetEndKey(nullptr); | ||
| if (range_tombstone_.end_key() != nullptr) { | ||
| ParsedInternalKey largest; | ||
| if (!ParseInternalKey(file_meta_->largest.Encode(), &largest) || |
There was a problem hiding this comment.
It is kinda odd that above failing to parse an internal key is an assertion failure and here's it just ignored. But I guess that inconsistency is everywhere in RocksDB, so, ¯_(ツ)_/¯
| AddTombstones(&range_del_agg, {{"a", "d", 20}}); | ||
| Slice b = "b", c = "c"; | ||
| ParsedInternalKey b = {"b", kMaxSequenceNumber, kMaxValue}; | ||
| ParsedInternalKey c {"c", kMaxSequenceNumber, kMaxValue}; | ||
| VerifyPartialTombstonesEq(PartialRangeTombstone(&b, &c, 10), tombstone); |
There was a problem hiding this comment.
Are you sure you pushed? I'm having trouble finding these new test cases.
| AddTombstones(&range_del_agg, {{"a", "d", 20}}); | ||
| Slice b = "b", c = "c"; | ||
| ParsedInternalKey b = {"b", kMaxSequenceNumber, kMaxValue}; | ||
| ParsedInternalKey c {"c", kMaxSequenceNumber, kMaxValue}; | ||
| VerifyPartialTombstonesEq(PartialRangeTombstone(&b, &c, 10), tombstone); |
There was a problem hiding this comment.
You're really doubling down on putting these reviews in hard mode, huh?
|
Not sure what is up with the travis failure. Something timed out ofter 10m. I restarted the job. FYI, I'm also going to backport facebook#4481 and facebook#4476. |
Picks up cockroachdb/rocksdb#17 and cockroachdb/rocksdb#18, which fix rare edge cases in which keys could go missing if a range deletion spanned SSTs in the wrong way. Release note: None
31700: c-deps: bump RocksDB to pick up more range deletion fixes r=petermattis a=benesch Picks up cockroachdb/rocksdb#17 and cockroachdb/rocksdb#18, which fix a rare edge case in which keys could go missing if a range deletion spanned SSTs in the wrong way. Release note: None Co-authored-by: Nikhil Benesch <[email protected]>
Picks up cockroachdb/rocksdb#17 and cockroachdb/rocksdb#18, which fix rare edge cases in which keys could go missing if a range deletion spanned SSTs in the wrong way. Release note: None
This backports facebook#4432. The first eight commits are small changes that make the backport apply more cleanly.
@petermattis unfortunately CollapsedRangeDelMap::{ShouldDeleteRange,GetTombstone} fail to compile because they expect user keys but the collapsed range del map now operates on internal keys. As discussed hoping you can take it from here.
This change is