Relax VersionStorageInfo::GetOverlappingInputs check #4050

petermattis · 2018-06-25T17:17:14Z

Do not consider the range tombstone sentinel key as causing 2 adjacent
sstables in a level to overlap. When a range tombstone's end key is the
largest key in an sstable, the sstable's end key is so to a "sentinel"
value that is the smallest key in the next sstable with a sequence
number of kMaxSequenceNumber. This "sentinel" is guaranteed to not
overlap in internal-key space with the next sstable. Unfortunately,
GetOverlappingFiles uses user-keys to determine overlap and was thus
considering 2 adjacent sstables in a level to overlap if they were
separated by this sentinel key. This in turn would cause compactions to
be larger than necessary.

Note that this conflicts with
#2769 and cases
DBRangeDelTest.CompactionTreatsSplitInputLevelDeletionAtomically to
fail.

petermattis · 2018-06-25T17:29:23Z

@ajkr In #2769 you said:

One of the core assumptions of DeleteRange is that files containing portions of the same range tombstone are treated as a single unit from the perspective of compaction picker. Need better tests for this. This PR adds the tests for manual compaction.

I'm not seeing why that invariant (treating all sstables which overlap a range tombstone as a single unit) is important. The only test which fails with this PR is the one you added in #2769, but that test is simply checking for the invariant you describe, not some problematic outcome of violating the invariant.

If we don't treat the sstables as a single unit, then a range tombstone can exist in multiple levels, though I'm not seeing why that is problematic as compaction already needs to deal with the same range tombstone appearing in multiple adjacent sstables. What am I missing?

petermattis · 2018-06-25T17:29:51Z

Cc @benesch who has been thinking about range tombstones a lot.

ajkr · 2018-06-25T19:16:28Z

Hm, looking at it more there might've been some circular reasoning behind that assumption :/. It was like:

For implementation convenience we assumed that, when a range tombstone spans multiple files, those files will be compacted as a single unit since that's how compaction picking already worked. Under this assumption, we don't need to break a range tombstone into smaller pieces before putting it in the meta-block. So the range tombstone meta-block actually contains range tombstones that extend outside the SST file's range.
Given what we decided to do in 1 (not shorten tombstones in meta-block), compaction picker must treat all files containing the same tombstone as a single unit. If it does not we run into this situation described by @zhangjinpeng1987 in bug with DeleteRange #2752 (copying relevant part below):

Write a delete range [a, d)

After compaction, this delete range may be added to multiple sst files: a.sst, b.sst, c.sst, but the range of these sst files are [a, b), [b, c), [c, d).

When a.sst and b.sst reach the bottommost level, the delete range of these two sst files will be dropped.

Write new keys in range [a, c).

When the new written keys [a, c) reach the bottommost level, their sequence number will be set to zero.

When the front c.sst compact with these keys [a, c) that sequence number are zero, these new written keys will be dropped wrongly.

ajkr · 2018-06-25T19:28:29Z

In summary I think a prerequisite to this PR would be shortening the range tombstones in the meta-block. Besides that I can't think of any problem.

petermattis · 2018-06-25T19:43:33Z

@ajkr Shortening the range tombstones in the meta-block was something I had a question about. Is there anything preventing that from happening? Feels strange to both @benesch and I that an sstable can contain a range tombstone that extends past the boundaries of the sstable.

ajkr · 2018-06-25T20:23:39Z

I think nothing prevents it. We just didn't need to do it before because the compaction picking always picked all files that a range tombstone spanned.

petermattis · 2018-06-26T16:32:23Z

I think nothing prevents it. We just didn't need to do it before because the compaction picking always picked all files that a range tombstone spanned.

Roger. I'll work on a PR to trim range tombstones to sstable boundaries and then revisit this PR. Also, thanks for the explanation of why the current invariant that all of the sstables which overlap a range tombstone must be treated as a single unit. That was not at all obvious.

When adding range tombstones to a RangeDelAggregator, truncate the tombstones to the sstable boundaries. This is done as an alternative to truncating the range tombstones within the sstables themselves as it properly handles existing data where range tombstones are not truncated in sstables. Truncating range tombstones to sstable boundaries avoids having two process all of the sstables with overlapping tombstones as a unit. See facebook#4050

petermattis · 2018-07-09T21:30:58Z

Depends on #4081.

When adding range tombstones to a RangeDelAggregator, truncate the tombstones to the sstable boundaries. This is done as an alternative to truncating the range tombstones within the sstables themselves as it properly handles existing data where range tombstones are not truncated in sstables. Truncating range tombstones to sstable boundaries avoids having two process all of the sstables with overlapping tombstones as a unit. See facebook#4050

petermattis · 2018-07-11T22:28:21Z

db/dbformat.h

  // be careful to use SerializeEndKey(), allocates new memory
  InternalKey SerializeEndKey() const {
-    return InternalKey(end_key_, seq_, kTypeRangeDeletion);
+    return InternalKey(end_key_, kMaxSequenceNumber, kTypeRangeDeletion);


@ajkr This was a fun bug to track down. The only usage of SerializeEndKey is from RangeDelAggregator::AddToBuilder. The last sstable output by a compaction could have it's largest key set to RangeTombstone::SerializeEndKey(), but the tombstone end-key is exclusive so that could cause the sstable to overlap with the next sstable. I need to think through whether this was a bug prior to this PR or not.

Oh got it... I suspect it wasn't a problem before this PR. In the compaction you described, the "next sstable" should've been pulled into a single compaction due to key-range overlap. The single compaction should be able to output files with non-overlapping key-ranges.

When adding range tombstones to a RangeDelAggregator, truncate the tombstones to the sstable boundaries. This is done as an alternative to truncating the range tombstones within the sstables themselves as it properly handles existing data where range tombstones are not truncated in sstables. Truncating range tombstones to sstable boundaries avoids having two process all of the sstables with overlapping tombstones as a unit. See facebook#4050

Do not consider the range tombstone sentinel key as causing 2 adjacent sstables in a level to overlap. When a range tombstone's end key is the largest key in an sstable, the sstable's end key is set to a "sentinel" value that is the smallest key in the next sstable with a sequence number of kMaxSequenceNumber. This "sentinel" is guaranteed to not overlap in internal-key space with the next sstable. Unfortunately, GetOverlappingFiles uses user-keys to determine overlap and was thus considering 2 adjacent sstables in a level to overlap if they were separated by this sentinel key. This in turn would cause compactions to be larger than necessary. This previous behavior of GetOverlappingInputs was necessary due to the following scenario: * Write a delete range [a, d). * After compaction, this deleted range may be added to multiple sst files: a.sst, b.sst, c.sst, but the boundaries of these sst files are [a, b), [b, c), [c, d). * When a.sst and b.sst reach the bottommost level, the delete range of the sst files will be dropped. * Write a new key in the range [a, c). * When the newly written key [a, c) reaches the bottommost level, its sequence number will be set to zero. * When the front c.sst compacts with the key in the range [a, c) the sequence number of that key is zero, and the key will be incorrectly dropped. That scenario no longer occurs because we are truncating range deletion tombstones to sstable boundaries when adding them to RangeDelAggregator.

ajkr

lgtm!

ajkr · 2018-07-13T20:18:24Z

db/db_range_del_test.cc

@@ -946,22 +949,111 @@ TEST_F(DBRangeDelTest, CompactionTreatsSplitInputLevelDeletionAtomically) {
    if (i == 0) {
      ASSERT_OK(db_->CompactFiles(
          CompactionOptions(), {meta.levels[1].files[0].name}, 2 /* level */));
+      ASSERT_EQ(0, NumTableFilesAtLevel(1));


I wonder why does CompactFiles pull in all the adjacent files?

Seems to. In the test, we tell CompactFiles to compact sstable 17. CompactionPicker::SanitizeCompactionInputFilesForAllLevels pulls in sstables 20, 19, 18, and 7 as well. Looks like that method has logic to expand the input file set similar to VersionSet::GetOverlappingInputs. Probably worthwhile to fix it.

Sure, we can follow up on that sometime, though I'll land this part now.

Great! Glad to see this get merged.

ajkr · 2018-07-13T21:10:16Z

db/dbformat.h

  // be careful to use SerializeEndKey(), allocates new memory
  InternalKey SerializeEndKey() const {
-    return InternalKey(end_key_, seq_, kTypeRangeDeletion);
+    return InternalKey(end_key_, kMaxSequenceNumber, kTypeRangeDeletion);


Oh got it... I suspect it wasn't a problem before this PR. In the compaction you described, the "next sstable" should've been pulled into a single compaction due to key-range overlap. The single compaction should be able to output files with non-overlapping key-ranges.

facebook-github-bot

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@ajkr is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Do not consider the range tombstone sentinel key as causing 2 adjacent sstables in a level to overlap. When a range tombstone's end key is the largest key in an sstable, the sstable's end key is so to a "sentinel" value that is the smallest key in the next sstable with a sequence number of kMaxSequenceNumber. This "sentinel" is guaranteed to not overlap in internal-key space with the next sstable. Unfortunately, GetOverlappingFiles uses user-keys to determine overlap and was thus considering 2 adjacent sstables in a level to overlap if they were separated by this sentinel key. This in turn would cause compactions to be larger than necessary. Note that this conflicts with facebook#2769 and cases `DBRangeDelTest.CompactionTreatsSplitInputLevelDeletionAtomically` to fail. Pull Request resolved: facebook#4050 Differential Revision: D8844423 Pulled By: ajkr fbshipit-source-id: df3f9f1db8f4cff2bff77376b98b83c2ae1d155b

facebook-github-bot added the CLA Signed label Jun 25, 2018

petermattis mentioned this pull request Jul 1, 2018

Truncate range tombstones from sstables #4081

Closed

ajkr self-requested a review July 9, 2018 19:54

petermattis force-pushed the pmattis/get-overlapping-inputs branch 2 times, most recently from 6af5b58 to a1ea122 Compare July 9, 2018 21:29

petermattis changed the title ~~[DO NOT MERGE] Relax VersionStorageInfo::GetOverlappingInputs check~~ Relax VersionStorageInfo::GetOverlappingInputs check Jul 9, 2018

petermattis force-pushed the pmattis/get-overlapping-inputs branch from a1ea122 to ac857ae Compare July 11, 2018 22:24

petermattis commented Jul 11, 2018

View reviewed changes

petermattis force-pushed the pmattis/get-overlapping-inputs branch from ac857ae to d1d5935 Compare July 12, 2018 13:55

petermattis force-pushed the pmattis/get-overlapping-inputs branch from d1d5935 to 2ae3dec Compare July 12, 2018 19:26

petermattis added 2 commits July 13, 2018 16:01

Removed the unused TableCache::NewRangeTombstoneIterator

008ff5b

petermattis force-pushed the pmattis/get-overlapping-inputs branch from 2ae3dec to fd766ca Compare July 13, 2018 20:23

petermattis force-pushed the pmattis/get-overlapping-inputs branch from fd766ca to 7363c90 Compare July 13, 2018 20:25

ajkr approved these changes Jul 13, 2018

View reviewed changes

facebook-github-bot reviewed Jul 13, 2018

View reviewed changes

facebook-github-bot closed this in 90fc406 Jul 14, 2018

petermattis deleted the pmattis/get-overlapping-inputs branch July 20, 2018 11:49

ajkr mentioned this pull request Jul 22, 2018

Range tombstones can create excessively large compactions #3977

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relax VersionStorageInfo::GetOverlappingInputs check #4050

Relax VersionStorageInfo::GetOverlappingInputs check #4050

petermattis commented Jun 25, 2018

petermattis commented Jun 25, 2018

petermattis commented Jun 25, 2018

ajkr commented Jun 25, 2018

ajkr commented Jun 25, 2018

petermattis commented Jun 25, 2018

ajkr commented Jun 25, 2018

petermattis commented Jun 26, 2018

petermattis commented Jul 9, 2018

petermattis Jul 11, 2018

ajkr Jul 13, 2018

ajkr left a comment

ajkr Jul 13, 2018

petermattis Jul 13, 2018

ajkr Jul 13, 2018

petermattis Jul 14, 2018

ajkr Jul 13, 2018

facebook-github-bot left a comment

facebook-github-bot left a comment

Relax VersionStorageInfo::GetOverlappingInputs check #4050

Relax VersionStorageInfo::GetOverlappingInputs check #4050

Conversation

petermattis commented Jun 25, 2018

petermattis commented Jun 25, 2018

petermattis commented Jun 25, 2018

ajkr commented Jun 25, 2018

ajkr commented Jun 25, 2018

petermattis commented Jun 25, 2018

ajkr commented Jun 25, 2018

petermattis commented Jun 26, 2018

petermattis commented Jul 9, 2018

petermattis Jul 11, 2018

Choose a reason for hiding this comment

ajkr Jul 13, 2018

Choose a reason for hiding this comment

ajkr left a comment

Choose a reason for hiding this comment

ajkr Jul 13, 2018

Choose a reason for hiding this comment

petermattis Jul 13, 2018

Choose a reason for hiding this comment

ajkr Jul 13, 2018

Choose a reason for hiding this comment

petermattis Jul 14, 2018

Choose a reason for hiding this comment

ajkr Jul 13, 2018

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment