Allow empty MultiScan result in BlockBasedTableIterator Prepare (facebook#14046)

Summary:
Currently in BlockBasedTableIterator's Prepare(), the index lookup for a MultiScan range is expected to return atleast 1 data block (unless UDI is in use). This is because there's an implicit assumption that only ranges intersecting with the keys in the file will be prepared. This assumption, however, doesn't hold if there are range deletions and the smallest and/or largest keys in the file extend beyond the keys in the file. The LevelIterator prunes the MultiScan ranges based on the smallest/largest key, so its possible for a range to only overlap the range deletion portion of the file and not overlap any of the data blocks. Furthermore, the BlockBasedTableIterator is now much more forgiving of Seek to targets outside of prepared ranges after facebook#14040 .

Keeping the above in mind, this PR removes the check in BlockBasedTableIterator for non-empty index result. It adds assertions in LevelIterator to verify that ranges are being properly pruned. Another side effect is we can no longer rely solely on a scan range having 0 data blocks (i.e cur_scan_start_idx >= cur_scan_end_idx) to decide if the iterator is out of bound. We can only do so for all but the last range prepared range.

Pull Request resolved: facebook#14046

Test Plan:
1. Add unit test in db_iterator_test
2. Run crash test

Reviewed By: xingbowang

Differential Revision: D84623871

Pulled By: anand1976

fbshipit-source-id: 2418e629f92b1c46c555ddea3761140f700819e4

Author: anand1976

db/db_iterator_test.cc

@@ -4464,72 +4464,138 @@ TEST_P(DBMultiScanIteratorTest, RangeBetweenFiles) {
   iter.reset();
 }
 
-TEST_P(DBMultiScanIteratorTest, OutOfL0FileRange) {
-  // Test that prepare does not fail scan when a scan range
-  // is outside of a L0 file's key range.
+// This test case tests multiscan in the presence of fragmented range
+// tombstones in the LSM.
+TEST_P(DBMultiScanIteratorTest, FragmentedRangeTombstones) {
   auto options = CurrentOptions();
+  // Compaction may create files 2x the target_file_size_base,
+  // so set this to 50KB so we atleast end up with 2 files of
+  // 100KB
+  options.target_file_size_base = 50 << 10;  // 50KB
+  options.compaction_style = kCompactionStyleUniversal;
+  options.num_levels = 50;
   options.compression = kNoCompression;
   DestroyAndReopen(options);
 
-  Random rnd(301);
-  // Create a Lmax file
-  // key01 ~ key99
-  for (int i = 0; i < 100; ++i) {
-    std::stringstream ss;
-    ss << std::setw(2) << std::setfill('0') << i;
-    ASSERT_OK(Put("k" + ss.str(), rnd.RandomString(1024)));
-  }
+  // Setup the LSM as follows -
+  // 1. Ingest a file with 100 keys
+  // 2. Ingest a file with one overlapping key
+  // 3. Do a Put and flush a file to L0 with one overlapping key
+  // 4. Ingest a standalone delete range file that covers the full key space
+  //    and a file with the same 100 keys with new values. This will ingest
+  //    into L0 due to the presence of an existing file in L0
+  // The final LSM will have an SST in Lmax with 100 keys, and 2 SST files
+  // in Lmax-1 with half the keys each and completely overlapping delete ranges
+  std::unordered_map<std::string, std::string> kvs;
+  auto rnd = Random::GetTLSInstance();
+  auto create_ingestion_data_file_and_update_key_value =
+      [&](const std::string& filename, int start_key, int end_key) {
+        std::unique_ptr<SstFileWriter> writer;
+        writer.reset(new SstFileWriter(EnvOptions(), options));
+        ASSERT_OK(writer->Open(filename));
+        for (int i = start_key; i < end_key; ++i) {
+          auto kiter = kvs.find(Key(i));
+          if (kiter != kvs.end()) {
+            kvs.erase(kiter);
+          }
+          auto res =
+              kvs.emplace(std::make_pair(Key(i), rnd->RandomString(2 << 10)));
+          ASSERT_OK(writer->Put(res.first->first, res.first->second));
+        }
+        ASSERT_OK(writer->Finish());
+        writer.reset();
+      };
+
+  CreateColumnFamilies({"new_cf"}, options);
+  std::string ingest_file = dbname_ + "test.sst";
+  // Write ~200KB data
+  create_ingestion_data_file_and_update_key_value(ingest_file + "_0", 0, 100);
+  create_ingestion_data_file_and_update_key_value(ingest_file + "_1", 50, 51);
+  ColumnFamilyHandle* cfh = handles_[0];
+  IngestExternalFileOptions ifo;
+  Status s = dbfull()->IngestExternalFile(
+      cfh, {ingest_file + "_0", ingest_file + "_1"}, ifo);
+  ASSERT_OK(s);
+
+  ASSERT_OK(Put(0, Key(50), rnd->RandomString(2 << 10)));
   ASSERT_OK(Flush());
-  CompactRangeOptions cro;
-  cro.bottommost_level_compaction = BottommostLevelCompaction::kForce;
-  ASSERT_OK(db_->CompactRange(cro, nullptr, nullptr));
 
-  // Create a L0 file
-  // key00 ~ key09
-  for (int i = 0; i < 10; ++i) {
-    std::stringstream ss;
-    ss << std::setw(2) << std::setfill('0') << i;
-    ASSERT_OK(Put("k" + ss.str(), rnd.RandomString(1024)));
+  {
+    std::unique_ptr<SstFileWriter> writer;
+    writer.reset(new SstFileWriter(EnvOptions(), options));
+    ASSERT_OK(writer->Open(ingest_file + "_2"));
+    ASSERT_OK(writer->DeleteRange("a", "z"));
+    ASSERT_OK(writer->Finish());
+    writer.reset();
   }
-  ASSERT_OK(Flush());
-  ASSERT_EQ(NumTableFilesAtLevel(0), 1);
+  create_ingestion_data_file_and_update_key_value(ingest_file + "_3", 0, 100);
+  s = dbfull()->IngestExternalFile(
+      cfh, {ingest_file + "_2", ingest_file + "_3"}, ifo);
+  ASSERT_OK(s);
 
-  // The second range is outside of L0 file's key range
-  std::vector<std::string> key_ranges({"k04", "k06", "k12", "k14"});
+  ASSERT_OK(dbfull()->TEST_WaitForCompact());
+
+  // The first scan range overlaps the DB key range, while the second extends
+  // beyond but overlaps the delete range
+  std::vector<std::string> key_ranges({"key000085", "key000090", "l", "n"});
   ReadOptions ro;
-  Slice ub;
-  ro.iterate_upper_bound = &ub;
   ro.fill_cache = GetParam();
   MultiScanArgs scan_options(BytewiseComparator());
   scan_options.insert(key_ranges[0], key_ranges[1]);
   scan_options.insert(key_ranges[2], key_ranges[3]);
-  ColumnFamilyHandle* cfh = dbfull()->DefaultColumnFamily();
-  std::unique_ptr<Iterator> iter(dbfull()->NewIterator(ro, cfh));
-  ASSERT_NE(iter, nullptr);
-  iter->Prepare(scan_options);
-  int count = 0;
-  ub = key_ranges[1];
-  iter->Seek(key_ranges[0]);
-  while (iter->status().ok() && iter->Valid()) {
-    ASSERT_GE(iter->key().compare(key_ranges[0]), 0);
-    ASSERT_LT(iter->key().compare(key_ranges[1]), 0);
-    count++;
-    iter->Next();
+  std::unique_ptr<MultiScan> iter =
+      dbfull()->NewMultiScan(ro, cfh, scan_options);
+  try {
+    int i = 0;
+    int count = 0;
+    for (auto range : *iter) {
+      for (auto it : range) {
+        ASSERT_GE(it.first.ToString(), key_ranges[i]);
+        ASSERT_LT(it.first.ToString(), key_ranges[i + 1]);
+        auto kiter = kvs.find(it.first.ToString());
+        ASSERT_NE(kiter, kvs.end());
+        ASSERT_EQ(kiter->second, it.second.ToString());
+        count++;
+      }
+      i += 2;
+    }
+    ASSERT_EQ(i, 4);
+    ASSERT_EQ(count, 5);
+  } catch (MultiScanException& ex) {
+    ASSERT_OK(ex.status());
   }
-  ASSERT_OK(iter->status()) << iter->status().ToString();
-  ASSERT_EQ(count, 2);
+  iter.reset();
 
-  ub = key_ranges[3];
-  count = 0;
-  iter->Seek(key_ranges[2]);
-  while (iter->status().ok() && iter->Valid()) {
-    ASSERT_GE(iter->key().compare(key_ranges[2]), 0);
-    ASSERT_LT(iter->key().compare(key_ranges[3]), 0);
-    count++;
-    iter->Next();
+  // The second scan range start overlaps the delete range in the first file
+  // in Lmax-1, while the end overlaps the keys in the second file
+  (*scan_options).clear();
+  key_ranges[0] = "key000010";
+  key_ranges[1] = "key000020";
+  key_ranges[2] = "key0000500";
+  key_ranges[3] = "key000060";
+  scan_options.insert(key_ranges[0], key_ranges[1]);
+  scan_options.insert(key_ranges[2], key_ranges[3]);
+  iter = dbfull()->NewMultiScan(ro, cfh, scan_options);
+  try {
+    int i = 0;
+    int count = 0;
+    for (auto range : *iter) {
+      for (auto it : range) {
+        ASSERT_GE(it.first.ToString(), key_ranges[i]);
+        ASSERT_LT(it.first.ToString(), key_ranges[i + 1]);
+        auto kiter = kvs.find(it.first.ToString());
+        ASSERT_NE(kiter, kvs.end());
+        ASSERT_EQ(kiter->second, it.second.ToString());
+        count++;
+      }
+      i += 2;
+    }
+    ASSERT_EQ(i, 4);
+    ASSERT_EQ(count, 19);
+  } catch (MultiScanException& ex) {
+    ASSERT_OK(ex.status());
   }
-  ASSERT_OK(iter->status()) << iter->status().ToString();
-  ASSERT_EQ(count, 2);
+  iter.reset();
 }
 
 }  // namespace ROCKSDB_NAMESPACE

db/version_set.cc

@@ -1200,7 +1200,6 @@ class LevelIterator final : public InternalIterator {
     // Propagate multiscan configs
     for (auto& file_to_arg : *file_to_scan_opts_) {
       file_to_arg.second.CopyConfigFrom(*so);
-      file_to_arg.second.SetRequireFileOverlap(true);
     }
   }
 
@@ -1276,6 +1275,10 @@ class LevelIterator final : public InternalIterator {
     }
   }
 
+#ifndef NDEBUG
+  bool OverlapRange(const ScanOptions& opts);
+#endif
+
   TableCache* table_cache_;
   const ReadOptions& read_options_;
   const FileOptions& file_options_;
@@ -1658,6 +1661,19 @@ void LevelIterator::SkipEmptyFileBackward() {
   }
 }
 
+#ifndef NDEBUG
+bool LevelIterator::OverlapRange(const ScanOptions& opts) {
+  return (user_comparator_.CompareWithoutTimestamp(
+              opts.range.start.value(), /*a_has_ts=*/false,
+              ExtractUserKey(flevel_->files[file_index_].largest_key),
+              /*b_has_ts=*/true) <= 0 &&
+          user_comparator_.CompareWithoutTimestamp(
+              opts.range.limit.value(), /*a_has_ts=*/false,
+              ExtractUserKey(flevel_->files[file_index_].smallest_key),
+              /*b_has_ts=*/true) > 0);
+}
+#endif
+
 void LevelIterator::SetFileIterator(InternalIterator* iter) {
   if (pinned_iters_mgr_ && iter) {
     iter->SetPinnedItersMgr(pinned_iters_mgr_);
@@ -1667,6 +1683,8 @@ void LevelIterator::SetFileIterator(InternalIterator* iter) {
   if (iter && scan_opts_) {
     if (FileHasMultiScanArg(file_index_)) {
       const MultiScanArgs& new_opts = GetMultiScanArgForFile(file_index_);
+      assert(OverlapRange(*new_opts.GetScanRanges().begin()) &&
+             OverlapRange(*new_opts.GetScanRanges().rbegin()));
       file_iter_.Prepare(&new_opts);
     }
   }

include/rocksdb/options.h

@@ -1857,12 +1857,6 @@ class MultiScanArgs {
 
   const Comparator* GetComparator() const { return comp_; }
 
-  void SetRequireFileOverlap(bool require_overlap) {
-    require_file_overlap_ = require_overlap;
-  }
-
-  bool RequireFileOverlap() const { return require_file_overlap_; }
-
   // Copies the configurations (excluding actual scan ranges) from another
   // MultiScanArgs.
   void CopyConfigFrom(const MultiScanArgs& other) {
@@ -1894,11 +1888,6 @@ class MultiScanArgs {
   // The comparator used for ordering ranges
   const Comparator* comp_;
   std::vector<ScanOptions> original_ranges_;
-
-  // Internal use only.
-  // Fail the Prepare() on a file if a scan range does not overlap
-  // with the file.
-  bool require_file_overlap_{false};
 };
 
 // Options that control read operations

table/block_based/block_based_table_iterator.cc

@@ -970,13 +970,12 @@ BlockBasedTableIterator::MultiScanState::~MultiScanState() {
 // scan opt. If we reach the end of the last scan opt, UpperBoundCheckResult()
 // will return kUnknown instead of kOutOfBound. This mechanism requires that
 // scan opts are properly pruned such that there is no scan opt that is after
-// this file's key range. This check can be enforeced by setting
-// MultiScanArgs::require_file_overlap to true.
+// this file's key range.
 // FIXME: DBIter and MergingIterator may
 // internally do Seek() on child iterators, e.g. due to
 // ReadOptions::max_skippable_internal_keys or reseeking into range deletion
-// end key. So these Seeks can cause iterator to fall back to normal
-// (non-prepared) iterator and ignore the optimizations done in Prepare().
+// end key. These Seeks will be handled properly, as long as the target is
+// moving forward.
 void BlockBasedTableIterator::Prepare(const MultiScanArgs* multiscan_opts) {
   assert(!multi_scan_);
   if (!index_iter_->status().ok()) {
@@ -995,9 +994,9 @@ void BlockBasedTableIterator::Prepare(const MultiScanArgs* multiscan_opts) {
   std::vector<std::string> data_block_separators;
   std::vector<std::tuple<size_t, size_t>> block_index_ranges_per_scan;
   const std::vector<ScanOptions>& scan_opts = multiscan_opts->GetScanRanges();
-  multi_scan_status_ = CollectBlockHandles(
-      scan_opts, multiscan_opts->RequireFileOverlap(), &scan_block_handles,
-      &block_index_ranges_per_scan, &data_block_separators);
+  multi_scan_status_ =
+      CollectBlockHandles(scan_opts, &scan_block_handles,
+                          &block_index_ranges_per_scan, &data_block_separators);
   if (!multi_scan_status_.ok()) {
     return;
   }
@@ -1168,7 +1167,13 @@ bool BlockBasedTableIterator::SeekMultiScanImpl(const Slice* seek_target) {
     // We should have the data block already loaded
     ++multi_scan_->next_scan_idx;
     if (cur_scan_start_idx >= cur_scan_end_idx) {
-      return out_of_bound;
+      if (multi_scan_->next_scan_idx <
+          multi_scan_->block_index_ranges_per_scan.size()) {
+        return out_of_bound;
+      } else {
+        ResetDataIter();
+        return false;
+      }
     } else {
       is_out_of_bound_ = false;
     }
@@ -1418,7 +1423,7 @@ Status BlockBasedTableIterator::CreateAndPinBlockFromBuffer(
 constexpr auto kVerbose = false;
 
 Status BlockBasedTableIterator::CollectBlockHandles(
-    const std::vector<ScanOptions>& scan_opts, bool require_file_overlap,
+    const std::vector<ScanOptions>& scan_opts,
     std::vector<BlockHandle>* scan_block_handles,
     std::vector<std::tuple<size_t, size_t>>* block_index_ranges_per_scan,
     std::vector<std::string>* data_block_separators) {
@@ -1481,16 +1486,6 @@ Status BlockBasedTableIterator::CollectBlockHandles(
         data_block_separators->push_back(index_iter_->user_key().ToString());
       }
       ++num_blocks;
-    } else if (num_blocks == 0 && index_iter_->UpperBoundCheckResult() !=
-                                      IterBoundCheck::kOutOfBound) {
-      // If require_file_overlap is set, then the scan ranges for this file
-      // must intersect with the file. Otherwise, allow empty intersection.
-      if (require_file_overlap) {
-        // This is important for FindBlockForwardInMultiScan() which only
-        // lets the upper layer (LevelIterator) advance to the next SST file
-        // when the last scan range is exhausted.
-        return Status::InvalidArgument("Scan does not intersect with file");
-      }
     }
     block_index_ranges_per_scan->emplace_back(
         scan_block_handles->size() - num_blocks, scan_block_handles->size());

table/block_based/block_based_table_iterator.h

@@ -698,7 +698,7 @@ class BlockBasedTableIterator : public InternalIteratorBase<Slice> {
                                      CachableEntry<Block>& pinned_block_entry);
 
   Status CollectBlockHandles(
-      const std::vector<ScanOptions>& scan_opts, bool require_file_overlap,
+      const std::vector<ScanOptions>& scan_opts,
       std::vector<BlockHandle>* scan_block_handles,
       std::vector<std::tuple<size_t, size_t>>* block_index_ranges_per_scan,
       std::vector<std::string>* data_block_boundary_keys);