Fix a bug where compaction with range deletion can persist kTypeMaxValid in file metadata (facebook#14122)

Summary:
Range deletion start keys are considered during compaction for cutting output files. Due to some ordering requirement (see comment above InsertNextValidRangeTombstoneAtLevel()) between truncated range deletion start key and a file's point keys, there was logic in https://github.com/facebook/rocksdb/blob/f6c9c3bf1cf05096e8ff8c03ded60c1e199edbb7/db/range_del_aggregator.cc#L39 that changes the value type to be kTypeMaxValid. However, kTypeMaxValid is not supposed to be persisted per https://github.com/facebook/rocksdb/blob/f6c9c3bf1cf05096e8ff8c03ded60c1e199edbb7/db/dbformat.h#L75-L76. This can cause forward compatibility issues reported in facebook#14101. This PR fixes this issue by removing the logic that sets kTypeMaxValid and always skip truncated range deletion start key in CompactionMergingIterator.

For existing SST files, we want to avoid using this kTypeMaxValid, so this PR also introduces a new placeholder value type. This allows us to re-strengthen the relevant value type checks (IsExtendedValueType()) that was loosen for kTypeMaxValid.

Pull Request resolved: facebook#14122

Test Plan:
- a unit test that persists kTypeMaxValid before this fix
- crash test with frequent range deletion: `python3 ./tools/db_crashtest.py blackbox --delrangepercent=11 --readpercent=35`
- Generate SST files with 0x1A as value type (kTypeMaxValid before this change) in file metadata. Run ldb with the strengthened check in IsExtendedValueType() to dump the MANIFEST. It failed to parse MANIFEST as expected before this PR and succeeds after this PR.
```
Error in processing file /tmp/rocksdbtest-543376/db_range_del_test_2549357_6547198162080866792/MANIFEST-000005 Corruption: VersionEdit: new-file4 entry  The file /tmp/rocksdbtest-543376/db_range_del_test_2549357_6547198162080866792/MANIFEST-000005 may be corrupted.
```

Reviewed By: pdillinger

Differential Revision: D87016541

Pulled By: cbi42

fbshipit-source-id: 9957a095db2cd9947463b403f352bd9a1fd70a76

Author: cbi42

db/db_range_del_test.cc

@@ -3825,6 +3825,134 @@ TEST_F(DBRangeDelTest, RowCache) {
   // and should not turn db into read-only mdoe.
   ASSERT_OK(Put(Key(5), "foo"));
 }
+
+TEST_F(DBRangeDelTest, FileCutWithTruncatedRangeDelKey) {
+  // Test for a bug that used to generate files with meta.smallest
+  // containing kMaxValid.
+  //
+  // Setup:
+  // - Write Key(2), Key(3) and DeleteRange(Key(1), Key(4))
+  // - Flush to L0
+  // - Use SingleKeySstPartitioner to force each user key into its own file
+  // - Compact files from L0 to L1 will generate files
+  // File[0]:
+  //   smallest=[user_key=key000001, seq=4, type=15],
+  //   largest= [user_key=key000002, seq=72057594037927935, type=15]
+  // File[1]:
+  //   smallest=[user_key=key000002, seq=2, type=1],
+  //   largest= [user_key=key000003, seq=72057594037927935, type=15]
+  // File[2]:
+  //   smallest=[user_key=key000003, seq=3, type=1],
+  //   largest= [user_key=key000004, seq=72057594037927935, type=15]
+  // With range deletions truncated to each files key range.
+  //
+  // - Compact these files again into L2. RocksDB usede to set truncated
+  // range deletion start key to have value type kMaxValid. The range deletion
+  // start key is used in compaction file cutting decision.
+  // - Verify the file boundary keys after compaction have valid boundary keys
+  //
+  // Before the fix:
+  // File[0]:
+  //   smallest=[user_key=key000001, seq=4, type=15],
+  //   largest= [user_key=key000002, seq=72057594037927935, type=15]
+  // File[1]:
+  //   smallest=[user_key=key000002, seq=2, type=26],
+  //   largest= [user_key=key000003, seq=72057594037927935, type=15]
+  // File[2]:
+  //   smallest=[user_key=key000003, seq=3, type=26],
+  //   largest= [user_key=key000004, seq=72057594037927935, type=15]
+  //
+  // After the fix:
+  // File[0]:
+  //   smallest=[user_key=key000001, seq=4, type=15],
+  //   largest= [user_key=key000002, seq=72057594037927935, type=15]
+  // File[1]:
+  //   smallest=[user_key=key000002, seq=2, type=1],
+  //   largest= [user_key=key000003, seq=72057594037927935, type=15]
+  // File[2]:
+  //   smallest=[user_key=key000003, seq=3, type=1],
+  //   largest= [user_key=key000004, seq=72057594037927935, type=15]
+
+  Options options = CurrentOptions();
+  options.disable_auto_compactions = true;
+
+  // Use partitioner that cuts before every new user key.
+  // Key(x) generates keys of length 9.
+  auto factory = std::shared_ptr<SstPartitionerFactory>(
+      NewSstPartitionerFixedPrefixFactory(10));
+  options.sst_partitioner_factory = factory;
+
+  DestroyAndReopen(options);
+
+  Random rnd(301);
+
+  // Create a file in a lower level so the compactions below are not
+  // bottommost compactions. Range deletion start keys are not considered
+  // in bottommost compaction.
+  ASSERT_OK(Put(Key(3), rnd.RandomBinaryString(100)));
+  ASSERT_OK(Flush());
+  MoveFilesToLevel(6);
+  ASSERT_EQ(1, NumTableFilesAtLevel(6));
+
+  ASSERT_OK(Put(Key(2), rnd.RandomString(100)));
+  // Snapshots keep point keys alive.
+  ManagedSnapshot snapshot1(db_);
+  ASSERT_OK(Put(Key(3), rnd.RandomString(100)));
+  ManagedSnapshot snapshot2(db_);
+  ASSERT_OK(db_->DeleteRange(WriteOptions(), db_->DefaultColumnFamily(), Key(1),
+                             Key(4)));
+  ASSERT_OK(Flush());
+  ASSERT_EQ(1, NumTableFilesAtLevel(0));
+
+  ColumnFamilyMetaData cf_meta_l0;
+  db_->GetColumnFamilyMetaData(db_->DefaultColumnFamily(), &cf_meta_l0);
+  ASSERT_EQ(1, cf_meta_l0.levels[0].files.size());
+  std::vector<std::string> l0_filenames;
+  for (const auto& sst_file : cf_meta_l0.levels[0].files) {
+    l0_filenames.push_back(sst_file.name);
+  }
+
+  // Compact L0 files to L1
+  CompactionOptions compact_options_l0;
+  ASSERT_OK(db_->CompactFiles(compact_options_l0, l0_filenames, 1));
+  ASSERT_EQ(3, NumTableFilesAtLevel(1));
+
+  // Check L1 file metadata
+  std::vector<std::vector<FileMetaData>> files_l1;
+  dbfull()->TEST_GetFilesMetaData(db_->DefaultColumnFamily(), &files_l1);
+
+  for (const auto& file : files_l1[1]) {
+    ASSERT_LT(ExtractValueType(file.smallest.Encode()), kTypeMaxValid);
+    ASSERT_LT(ExtractValueType(file.largest.Encode()), kTypeMaxValid);
+  }
+
+  // Get file names from level 1
+  ColumnFamilyMetaData cf_meta;
+  db_->GetColumnFamilyMetaData(db_->DefaultColumnFamily(), &cf_meta);
+  std::vector<std::string> input_filenames;
+  for (const auto& sst_file : cf_meta.levels[1].files) {
+    input_filenames.push_back(sst_file.name);
+  }
+
+  // Compact files from L1 to L2
+  CompactionOptions compact_options;
+  ASSERT_OK(db_->CompactFiles(compact_options, input_filenames, 2));
+
+  // Check L2 file metadata
+  std::vector<std::vector<FileMetaData>> files;
+  dbfull()->TEST_GetFilesMetaData(db_->DefaultColumnFamily(), &files);
+
+  for (const auto& file : files[2]) {
+    ASSERT_LT(ExtractValueType(file.smallest.Encode()), kTypeMaxValid);
+    ASSERT_LT(ExtractValueType(file.largest.Encode()), kTypeMaxValid);
+  }
+
+  // // Verify iteration works correctly
+  std::unique_ptr<Iterator> iter{db_->NewIterator(ReadOptions())};
+  iter->SeekToFirst();
+  ASSERT_OK(iter->status());
+  ASSERT_FALSE(iter->Valid());
+}
 }  // namespace ROCKSDB_NAMESPACE
 
 int main(int argc, char** argv) {

db/dbformat.h

@@ -72,6 +72,12 @@ enum ValueType : unsigned char {
   kTypeColumnFamilyWideColumnEntity = 0x17,     // WAL only
   kTypeValuePreferredSeqno = 0x18,              // Value with a unix write time
   kTypeColumnFamilyValuePreferredSeqno = 0x19,  // WAL only
+  // Placeholder value type for legacy SST files with incorrectly persisted
+  // file boundaries. Prior to the fix, TruncatedRangeDelIterator assigned
+  // kTypeMaxValid to truncated range deletion keys, which was then
+  // incorrectly persisted to SST file metadata. This placeholder value allows
+  // reading such legacy files for without using kTypeMaxValid.
+  kTypeTruncatedRangeDeletionSentinel = 0x1A,
   kTypeMaxValid,    // Should be after the last valid type, only used for
                     // validation
   kMaxValue = 0x7F  // Not used for storing records.
@@ -118,10 +124,11 @@ inline bool IsValueType(ValueType t) {
 
 // Checks whether a type is from user operation
 // kTypeRangeDeletion is in meta block so this API is separated from above
-// kTypeMaxValid can be from keys generated by
-// TruncatedRangeDelIterator::start_key()
+// kTypeTruncatedRangeDeletionSentinel is for legacy files with incorrectly
+// persisted file boundaries.
 inline bool IsExtendedValueType(ValueType t) {
-  return IsValueType(t) || t == kTypeRangeDeletion || t == kTypeMaxValid;
+  return IsValueType(t) || t == kTypeRangeDeletion ||
+         t == kTypeTruncatedRangeDeletionSentinel;
 }
 
 // We leave eight bits empty at the bottom so a type and sequence#
@@ -180,8 +187,7 @@ inline size_t InternalKeyEncodingLength(const ParsedInternalKey& key) {
 // Pack a sequence number and a ValueType into a uint64_t
 inline uint64_t PackSequenceAndType(uint64_t seq, ValueType t) {
   assert(seq <= kMaxSequenceNumber);
-  // kTypeMaxValid is used in TruncatedRangeDelIterator, see its constructor.
-  assert(IsExtendedValueType(t) || t == kTypeMaxValid);
+  assert(IsExtendedValueType(t));
   return (seq << 8) | t;
 }
 

db/range_del_aggregator.cc

@@ -36,7 +36,6 @@ TruncatedRangeDelIterator::TruncatedRangeDelIterator(
     Status pik_status = ParseInternalKey(smallest->Encode(), &parsed_smallest,
                                          false /* log_err_key */);  // TODO
     pik_status.PermitUncheckedError();
-    parsed_smallest.type = kTypeMaxValid;
     assert(pik_status.ok());
     smallest_ = &parsed_smallest;
   }
@@ -71,9 +70,6 @@ TruncatedRangeDelIterator::TruncatedRangeDelIterator(
       // the truncated end key can cover the largest key in this sstable, reduce
       // its sequence number by 1.
       parsed_largest.sequence -= 1;
-      // This line is not needed for correctness, but it ensures that the
-      // truncated end key is not covering keys from the next SST file.
-      parsed_largest.type = kTypeMaxValid;
     }
     largest_ = &parsed_largest;
   }

db/range_del_aggregator_test.cc

@@ -89,7 +89,9 @@ void VerifyIterator(
   for (size_t i = 0; i < expected_range_dels.size(); i++, iter->Next()) {
     ASSERT_TRUE(iter->Valid());
     EXPECT_EQ(0, icmp.Compare(iter->start_key(), expected_range_dels[i].start));
-    EXPECT_EQ(0, icmp.Compare(iter->end_key(), expected_range_dels[i].end));
+    EXPECT_EQ(0, icmp.Compare(iter->end_key(), expected_range_dels[i].end))
+        << iter->end_key().DebugString(false, false) << " "
+        << expected_range_dels[i].end.DebugString(false, false);
     EXPECT_EQ(expected_range_dels[i].seq, iter->seq());
   }
   EXPECT_FALSE(iter->Valid());
@@ -305,28 +307,28 @@ TEST_F(RangeDelAggregatorTest, TruncatedIterPartiallyCutTombstones) {
 
   VerifyIterator(
       &iter, bytewise_icmp,
-      {{InternalValue("d", 7, kTypeMaxValid), UncutEndpoint("e"), 10},
+      {{InternalValue("d", 7, kTypeValue), UncutEndpoint("e"), 10},
        {InternalValue("e", 8, kTypeRangeDeletion), UncutEndpoint("g"), 8},
        {InternalValue("j", 4, kTypeRangeDeletion),
-        InternalValue("m", 8, kTypeMaxValid), 4}});
+        InternalValue("m", 8, kTypeValue), 4}});
 
   VerifySeek(
       &iter, bytewise_icmp,
-      {{"d", InternalValue("d", 7, kTypeMaxValid), UncutEndpoint("e"), 10},
+      {{"d", InternalValue("d", 7, kTypeValue), UncutEndpoint("e"), 10},
        {"e", InternalValue("e", 8, kTypeRangeDeletion), UncutEndpoint("g"), 8},
        {"ia", InternalValue("j", 4, kTypeRangeDeletion),
-        InternalValue("m", 8, kTypeMaxValid), 4, false /* invalid */},
+        InternalValue("m", 8, kTypeValue), 4, false /* invalid */},
        {"n", InternalValue("", 0, kTypeRangeDeletion), UncutEndpoint(""), 0,
         true /* invalid */},
-       {"", InternalValue("d", 7, kTypeMaxValid), UncutEndpoint("e"), 10}});
+       {"", InternalValue("d", 7, kTypeValue), UncutEndpoint("e"), 10}});
 
   VerifySeekForPrev(
       &iter, bytewise_icmp,
-      {{"d", InternalValue("d", 7, kTypeMaxValid), UncutEndpoint("e"), 10},
+      {{"d", InternalValue("d", 7, kTypeValue), UncutEndpoint("e"), 10},
        {"e", InternalValue("e", 8, kTypeRangeDeletion), UncutEndpoint("g"), 8},
        {"ia", InternalValue("e", 8, kTypeRangeDeletion), UncutEndpoint("g"), 8},
        {"n", InternalValue("j", 4, kTypeRangeDeletion),
-        InternalValue("m", 8, kTypeMaxValid), 4, false /* invalid */},
+        InternalValue("m", 8, kTypeValue), 4, false /* invalid */},
        {"", InternalValue("", 0, kTypeRangeDeletion), UncutEndpoint(""), 0,
         true /* invalid */}});
 }
@@ -345,23 +347,21 @@ TEST_F(RangeDelAggregatorTest, TruncatedIterFullyCutTombstones) {
   TruncatedRangeDelIterator iter(std::move(input_iter), &bytewise_icmp,
                                  &smallest, &largest);
 
-  VerifyIterator(
-      &iter, bytewise_icmp,
-      {{InternalValue("f", 7, kTypeMaxValid), UncutEndpoint("g"), 8}});
+  VerifyIterator(&iter, bytewise_icmp,
+                 {{InternalValue("f", 7, kTypeValue), UncutEndpoint("g"), 8}});
 
-  VerifySeek(
-      &iter, bytewise_icmp,
-      {{"d", InternalValue("f", 7, kTypeMaxValid), UncutEndpoint("g"), 8},
-       {"f", InternalValue("f", 7, kTypeMaxValid), UncutEndpoint("g"), 8},
-       {"j", InternalValue("", 0, kTypeRangeDeletion), UncutEndpoint(""), 0,
-        true /* invalid */}});
+  VerifySeek(&iter, bytewise_icmp,
+             {{"d", InternalValue("f", 7, kTypeValue), UncutEndpoint("g"), 8},
+              {"f", InternalValue("f", 7, kTypeValue), UncutEndpoint("g"), 8},
+              {"j", InternalValue("", 0, kTypeRangeDeletion), UncutEndpoint(""),
+               0, true /* invalid */}});
 
   VerifySeekForPrev(
       &iter, bytewise_icmp,
       {{"d", InternalValue("", 0, kTypeRangeDeletion), UncutEndpoint(""), 0,
         true /* invalid */},
-       {"f", InternalValue("f", 7, kTypeMaxValid), UncutEndpoint("g"), 8},
-       {"j", InternalValue("f", 7, kTypeMaxValid), UncutEndpoint("g"), 8}});
+       {"f", InternalValue("f", 7, kTypeValue), UncutEndpoint("g"), 8},
+       {"j", InternalValue("f", 7, kTypeValue), UncutEndpoint("g"), 8}});
 }
 
 TEST_F(RangeDelAggregatorTest, SingleIterInAggregator) {

db/version_edit.cc

@@ -30,6 +30,7 @@ uint64_t PackFileNumberAndPathId(uint64_t number, uint64_t path_id) {
 Status FileMetaData::UpdateBoundaries(const Slice& key, const Slice& value,
                                       SequenceNumber seqno,
                                       ValueType value_type) {
+  assert(value_type < kTypeMaxValid);
   if (value_type == kTypeBlobIndex) {
     BlobIndex blob_index;
     const Status s = blob_index.DecodeFrom(value);

db/version_edit.h

@@ -317,6 +317,9 @@ struct FileMetaData {
   void UpdateBoundariesForRange(const InternalKey& start,
                                 const InternalKey& end, SequenceNumber seqno,
                                 const InternalKeyComparator& icmp) {
+    assert(ExtractValueType(start.Encode()) < kTypeMaxValid);
+    assert(ExtractValueType(end.Encode()) < kTypeMaxValid);
+
     if (smallest.size() == 0 || icmp.Compare(start, smallest) < 0) {
       smallest = start;
     }

table/compaction_merging_iterator.cc

@@ -191,13 +191,6 @@ class CompactionMergingIterator : public InternalIterator {
 
     bool operator()(HeapItem* a, HeapItem* b) const {
       int r = comparator_->Compare(a->key(), b->key());
-      // For each file, we assume all range tombstone start keys come before
-      // its file boundary sentinel key (file's meta.largest key).
-      // In the case when meta.smallest = meta.largest and range tombstone start
-      // key is truncated at meta.smallest, the start key will have op_type =
-      // kMaxValid to make it smaller (see TruncatedRangeDelIterator
-      // constructor). The following assertion validates this assumption.
-      assert(a->type == b->type || r != 0);
       return r > 0;
     }
 
@@ -242,8 +235,24 @@ class CompactionMergingIterator : public InternalIterator {
     return !minHeap_.empty() ? minHeap_.top() : nullptr;
   }
 
-  void InsertRangeTombstoneAtLevel(size_t level) {
+  // For each file under a LevelIterator, the lifetime of range tombstone
+  // iterator is tied to the point key iterator. So we want scan through
+  // all range tombstone start keys before the file boundary sentinel key
+  // (file's meta.largest). When meta.smallest == meta.largest, the truncated
+  // range del start key may be ordered after meta.largest.
+  // Here we skip the first range deletion start key if it's truncated.
+  // This range deletion start key is redundant for compaction file cutting
+  // decision anyway, since the same point key will be considered for file
+  // cutting too.
+  void InsertNextValidRangeTombstoneAtLevel(size_t level) {
     if (range_tombstone_iters_[level]->Valid()) {
+      if (range_tombstone_iters_[level]->start_key().type !=
+          kTypeRangeDeletion) {
+        range_tombstone_iters_[level]->Next();
+        if (!range_tombstone_iters_[level]->Valid()) {
+          return;
+        }
+      }
       pinned_heap_item_[level].SetTombstoneForCompaction(
           range_tombstone_iters_[level]->start_key());
       minHeap_.push(&pinned_heap_item_[level]);
@@ -262,7 +271,7 @@ void CompactionMergingIterator::SeekToFirst() {
   for (size_t i = 0; i < range_tombstone_iters_.size(); ++i) {
     if (range_tombstone_iters_[i]) {
       range_tombstone_iters_[i]->SeekToFirst();
-      InsertRangeTombstoneAtLevel(i);
+      InsertNextValidRangeTombstoneAtLevel(i);
     }
   }
 
@@ -290,7 +299,7 @@ void CompactionMergingIterator::Seek(const Slice& target) {
                  0) {
         range_tombstone_iters_[i]->Next();
       }
-      InsertRangeTombstoneAtLevel(i);
+      InsertNextValidRangeTombstoneAtLevel(i);
     }
   }
 
@@ -357,7 +366,7 @@ void CompactionMergingIterator::FindNextVisibleKey() {
       minHeap_.pop();
     }
     if (range_tombstone_iters_[current->level]) {
-      InsertRangeTombstoneAtLevel(current->level);
+      InsertNextValidRangeTombstoneAtLevel(current->level);
     }
   }
 }

unreleased_history/bug_fixes/fix-range-del-boundary.md

@@ -0,0 +1 @@
+* Fix a bug where compaction with range deletion can persist kTypeMaxValid in MANIFEST as file metadata. kTypeMaxValid is not supposed to be persisted and can change as new value types are introduced. This can cause a forward compatibility issue where older versions of RocksDB don't recognize kTypeMaxValid from newer versions. A new placeholder value type kTypeTruncatedRangeDeletionSentinel is also introduced to replace kTypeMaxValid when reading existing SST files' metadata from MANIFEST. This allows us to strengthen some checks to avoid using kTypeMaxValid in the future.

utilities/debug.cc

@@ -41,6 +41,8 @@ static std::unordered_map<std::string, ValueType> value_type_string_map = {
     {"TypeValuePreferredSeqno", ValueType::kTypeValuePreferredSeqno},
     {"TypeColumnFamilyValuePreferredSeqno",
      ValueType::kTypeColumnFamilyValuePreferredSeqno},
+    {"kTypeTruncatedRangeDeletionSentinel",
+     ValueType::kTypeTruncatedRangeDeletionSentinel},
 };
 
 std::string KeyVersion::GetTypeName() const {