Fix assertion failure in ConstructFragmentedRangeTombstones() (#12796)

Summary:
the assertion `assert(!IsFragmentedRangeTombstonesConstructed(false));` assumes ConstructFragmentedRangeTombstones() is called only once for a memtable. This is not true since SwitchMemtable() can be called multiple times on the same live memtable, if a previous attempt fails. So remove the assertion in this PR and simplify relevant code.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/12796

Test Plan: the exact condition to trigger manifest write in SwitchMemtable() is complicated. Will monitor crash test to see if there's no more failure.

Reviewed By: hx235

Differential Revision: D58913310

Pulled By: cbi42

fbshipit-source-id: 458bb9eebcf6743e9001186fcb757e4b50e8a5d2

diff --git a/db/memtable.cc b/db/memtable.cc
index 91080673ffa3ee7775d89bf87f07f6080bc08af8..2b6c39d6e7c808993a5a5c33cb448ffc52cfa835 100644 (file)
--- a/db/memtable.cc
+++ b/db/memtable.cc
@@ -618,8 +618,9 @@ FragmentedRangeTombstoneIterator* MemTable::NewRangeTombstoneIteratorInternal(
 }
 
 void MemTable::ConstructFragmentedRangeTombstones() {
-  assert(!IsFragmentedRangeTombstonesConstructed(false));
-  // There should be no concurrent Construction
+  // There should be no concurrent Construction.
+  // We could also check fragmented_range_tombstone_list_ to avoid repeate
+  // constructions. We just construct them here again to be safe.
   if (!is_range_del_table_empty_.load(std::memory_order_relaxed)) {
     // TODO: plumb Env::IOActivity, Env::IOPriority
     auto* unfragmented_iter = new MemTableIterator(

diff --git a/db/memtable.h b/db/memtable.h
index 11ab6ea41b85c8be43e461c2abc0aa36aecc18b1..9b42d130f0b3812fd1e3a975863c7907355136e2 100644 (file)
--- a/db/memtable.h
+++ b/db/memtable.h
@@ -534,21 +534,21 @@ class MemTable {
   // Returns a heuristic flush decision
   bool ShouldFlushNow();
 
+  // Updates `fragmented_range_tombstone_list_` that will be used to serve reads
+  // when this memtable becomes an immutable memtable (in some
+  // MemtableListVersion::memlist_). Should be called when this memtable is
+  // about to become immutable. May be called multiple times since
+  // SwitchMemtable() may fail.
   void ConstructFragmentedRangeTombstones();
 
   // Returns whether a fragmented range tombstone list is already constructed
   // for this memtable. It should be constructed right before a memtable is
   // added to an immutable memtable list. Note that if a memtable does not have
-  // any range tombstone, then no range tombstone list will ever be constructed.
-  // @param allow_empty Specifies whether a memtable with no range tombstone is
-  // considered to have its fragmented range tombstone list constructed.
-  bool IsFragmentedRangeTombstonesConstructed(bool allow_empty = true) const {
-    if (allow_empty) {
-      return fragmented_range_tombstone_list_.get() != nullptr ||
-             is_range_del_table_empty_;
-    } else {
-      return fragmented_range_tombstone_list_.get() != nullptr;
-    }
+  // any range tombstone, then no range tombstone list will ever be constructed
+  // and true is returned in that case.
+  bool IsFragmentedRangeTombstonesConstructed() const {
+    return fragmented_range_tombstone_list_.get() != nullptr ||
+           is_range_del_table_empty_;
   }
 
   // Get the newest user-defined timestamp contained in this MemTable. Check