From: Changyu Bi <changyubi@meta.com>
Date: Sat, 22 Jun 2024 18:31:16 +0000 (-0700)
Subject: Fix assertion failure in ConstructFragmentedRangeTombstones() (#12796)
X-Git-Tag: v9.4.0~5
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=b4a84efb4e842b782e976de5b22a4554c2f76edd;p=rocksdb.git

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 91080673f..2b6c39d6e 100644
--- 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 11ab6ea41..9b42d130f 100644
--- 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