update HISTORY.md and version.h for 8.4.4

Summary:
This should only affect iterator when
- user uses DeleteRange(),
- An iterator from level L has a non-ok status (such non-ok status may not be caught before the bug fix in https://github.com/facebook/rocksdb/pull/11783), and
- A range tombstone covers a key from level > L and triggers a reseek sets the status_ to OK in SeekImpl()/SeekPrevImpl() e.g. https://github.com/facebook/rocksdb/blob/bd6a8340c3a2db764620e90b3ac5be173fc68a0c/table/merging_iterator.cc#L801

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

Differential Revision: D48908830

Pulled By: cbi42

fbshipit-source-id: eb564be375af4e33dc27542eff753260186e6d5d

CMakeLists.txt

@@ -1592,6 +1592,3 @@ option(WITH_BENCHMARK "build benchmark tests" OFF)
 if(WITH_BENCHMARK)
   add_subdirectory(${PROJECT_SOURCE_DIR}/microbench/)
 endif()
-
-target_include_directories(${PROJECT_NAME} PUBLIC
-  $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>)

HISTORY.md

@@ -1,40 +1,15 @@
 # Rocksdb Change Log
 > NOTE: Entries for next release do not go here. Follow instructions in `unreleased_history/README.txt`
 
-## 8.5.4 (09/26/2023)
-### Bug Fixes
-* Fixed a bug where compaction read under non direct IO still falls back to RocksDB internal prefetching after file system's prefetching returns non-OK status other than `Status::NotSupported()`
-
-### Behavior Changes
-* For non direct IO, eliminate the file system prefetching attempt for compaction read when `Options::compaction_readahead_size` is 0
-
-## 8.5.3 (09/01/2023)
+## 8.4.4 (09/01/2023)
 ### Bug Fixes
+* Fix a bug where if there is an error reading from offset 0 of a file from L1+ and that the file is not the first file in the sorted run, data can be lost in compaction and read/scan can return incorrect results.
+* Fix a bug where iterator may return incorrect result for DeleteRange() users if there was an error reading from a file.
 * Fixed a race condition in `GenericRateLimiter` that could cause it to stop granting requests
 
-## 8.5.2 (08/31/2023)
-### Bug fixes
-* Fix a bug where iterator may return incorrect result for DeleteRange() users if there was an error reading from a file.
-
-## 8.5.1 (08/31/2023)
-### Bug fixes
-* Fix a bug where if there is an error reading from offset 0 of a file from L1+ and that the file is not the first file in the sorted run, data can be lost in compaction and read/scan can return incorrect results.
-
-## 8.5.0 (07/21/2023)
-### Public API Changes
-* Removed recently added APIs `GeneralCache` and `MakeSharedGeneralCache()` as our plan changed to stop exposing a general-purpose cache interface. The old forms of these APIs, `Cache` and `NewLRUCache()`, are still available, although general-purpose caching support will be dropped eventually.
-
-### Behavior Changes
-* Option `periodic_compaction_seconds` no longer supports FIFO compaction: setting it has no effect on FIFO compactions. FIFO compaction users should only set option `ttl` instead.
-* Move prefetching responsibility to page cache for compaction read for non directIO use case
-
-### Performance Improvements
-* In case of direct_io, if buffer passed by callee is already aligned, RandomAccessFileRead::Read will avoid realloacting a new buffer, reducing memcpy and use already passed aligned buffer.
-* Small efficiency improvement to HyperClockCache by reducing chance of compiler-generated heap allocations
-
+## 8.4.3 (07/27/2023)
 ### Bug Fixes
-* Fix use_after_free bug in async_io MultiReads when underlying FS enabled kFSBuffer. kFSBuffer is when underlying FS pass their own buffer instead of using RocksDB scratch in FSReadRequest. Right now it's an experimental feature.
-* Fix a bug in FileTTLBooster that can cause users with a large number of levels (more than 65) to see errors like "runtime error: shift exponent .. is too large.." (#11673).
+* Fix use_after_free bug in async_io MultiReads when underlying FS enabled kFSBuffer. kFSBuffer is when underlying FS pass their own buffer instead of using RocksDB scratch in FSReadRequest.
 
 ## 8.4.0 (06/26/2023)
 ### New Features

cache/cache_bench_tool.cc

@@ -77,10 +77,6 @@ DEFINE_bool(lean, false,
             "If true, no additional computation is performed besides cache "
             "operations.");
 
-DEFINE_bool(early_exit, false,
-            "Exit before deallocating most memory. Good for malloc stats, e.g."
-            "MALLOC_CONF=\"stats_print:true\"");
-
 DEFINE_string(secondary_cache_uri, "",
               "Full URI for creating a custom secondary cache object");
 static class std::shared_ptr<ROCKSDB_NAMESPACE::SecondaryCache> secondary_cache;
@@ -597,10 +593,6 @@ class CacheBench {
       }
       thread->latency_ns_hist.Add(timer.ElapsedNanos());
     }
-    if (FLAGS_early_exit) {
-      MutexLock l(thread->shared->GetMutex());
-      exit(0);
-    }
     if (handle) {
       cache_->Release(handle);
       handle = nullptr;

cache/clock_cache.cc

@@ -9,7 +9,6 @@
 
 #include "cache/clock_cache.h"
 
-#include <cassert>
 #include <functional>
 #include <numeric>
 
@@ -119,6 +118,74 @@ inline bool ClockUpdate(ClockHandle& h) {
   }
 }
 
+}  // namespace
+
+void ClockHandleBasicData::FreeData(MemoryAllocator* allocator) const {
+  if (helper->del_cb) {
+    helper->del_cb(value, allocator);
+  }
+}
+
+HyperClockTable::HyperClockTable(
+    size_t capacity, bool /*strict_capacity_limit*/,
+    CacheMetadataChargePolicy metadata_charge_policy,
+    MemoryAllocator* allocator,
+    const Cache::EvictionCallback* eviction_callback, const uint32_t* hash_seed,
+    const Opts& opts)
+    : length_bits_(CalcHashBits(capacity, opts.estimated_value_size,
+                                metadata_charge_policy)),
+      length_bits_mask_((size_t{1} << length_bits_) - 1),
+      occupancy_limit_(static_cast<size_t>((uint64_t{1} << length_bits_) *
+                                           kStrictLoadFactor)),
+      array_(new HandleImpl[size_t{1} << length_bits_]),
+      allocator_(allocator),
+      eviction_callback_(*eviction_callback),
+      hash_seed_(*hash_seed) {
+  if (metadata_charge_policy ==
+      CacheMetadataChargePolicy::kFullChargeCacheMetadata) {
+    usage_ += size_t{GetTableSize()} * sizeof(HandleImpl);
+  }
+
+  static_assert(sizeof(HandleImpl) == 64U,
+                "Expecting size / alignment with common cache line size");
+}
+
+HyperClockTable::~HyperClockTable() {
+  // Assumes there are no references or active operations on any slot/element
+  // in the table.
+  for (size_t i = 0; i < GetTableSize(); i++) {
+    HandleImpl& h = array_[i];
+    switch (h.meta >> ClockHandle::kStateShift) {
+      case ClockHandle::kStateEmpty:
+        // noop
+        break;
+      case ClockHandle::kStateInvisible:  // rare but possible
+      case ClockHandle::kStateVisible:
+        assert(GetRefcount(h.meta) == 0);
+        h.FreeData(allocator_);
+#ifndef NDEBUG
+        Rollback(h.hashed_key, &h);
+        ReclaimEntryUsage(h.GetTotalCharge());
+#endif
+        break;
+      // otherwise
+      default:
+        assert(false);
+        break;
+    }
+  }
+
+#ifndef NDEBUG
+  for (size_t i = 0; i < GetTableSize(); i++) {
+    assert(array_[i].displacements.load() == 0);
+  }
+#endif
+
+  assert(usage_.load() == 0 ||
+         usage_.load() == size_t{GetTableSize()} * sizeof(HandleImpl));
+  assert(occupancy_ == 0);
+}
+
 // If an entry doesn't receive clock updates but is repeatedly referenced &
 // released, the acquire and release counters could overflow without some
 // intervention. This is that intervention, which should be inexpensive
@@ -192,209 +259,8 @@ inline void CorrectNearOverflow(uint64_t old_meta,
   }
 }
 
-inline bool BeginSlotInsert(const ClockHandleBasicData& proto, ClockHandle& h,
-                            uint64_t initial_countdown, bool* already_matches) {
-  assert(*already_matches == false);
-  // Optimistically transition the slot from "empty" to
-  // "under construction" (no effect on other states)
-  uint64_t old_meta = h.meta.fetch_or(
-      uint64_t{ClockHandle::kStateOccupiedBit} << ClockHandle::kStateShift,
-      std::memory_order_acq_rel);
-  uint64_t old_state = old_meta >> ClockHandle::kStateShift;
-
-  if (old_state == ClockHandle::kStateEmpty) {
-    // We've started inserting into an available slot, and taken
-    // ownership.
-    return true;
-  } else if (old_state != ClockHandle::kStateVisible) {
-    // Slot not usable / touchable now
-    return false;
-  }
-  // Existing, visible entry, which might be a match.
-  // But first, we need to acquire a ref to read it. In fact, number of
-  // refs for initial countdown, so that we boost the clock state if
-  // this is a match.
-  old_meta =
-      h.meta.fetch_add(ClockHandle::kAcquireIncrement * initial_countdown,
-                       std::memory_order_acq_rel);
-  // Like Lookup
-  if ((old_meta >> ClockHandle::kStateShift) == ClockHandle::kStateVisible) {
-    // Acquired a read reference
-    if (h.hashed_key == proto.hashed_key) {
-      // Match. Release in a way that boosts the clock state
-      old_meta =
-          h.meta.fetch_add(ClockHandle::kReleaseIncrement * initial_countdown,
-                           std::memory_order_acq_rel);
-      // Correct for possible (but rare) overflow
-      CorrectNearOverflow(old_meta, h.meta);
-      // Insert detached instead (only if return handle needed)
-      *already_matches = true;
-      return false;
-    } else {
-      // Mismatch. Pretend we never took the reference
-      old_meta =
-          h.meta.fetch_sub(ClockHandle::kAcquireIncrement * initial_countdown,
-                           std::memory_order_acq_rel);
-    }
-  } else if (UNLIKELY((old_meta >> ClockHandle::kStateShift) ==
-                      ClockHandle::kStateInvisible)) {
-    // Pretend we never took the reference
-    // WART/FIXME?: there's a tiny chance we release last ref to invisible
-    // entry here. If that happens, we let eviction take care of it.
-    old_meta =
-        h.meta.fetch_sub(ClockHandle::kAcquireIncrement * initial_countdown,
-                         std::memory_order_acq_rel);
-  } else {
-    // For other states, incrementing the acquire counter has no effect
-    // so we don't need to undo it.
-    // Slot not usable / touchable now.
-  }
-  (void)old_meta;
-  return false;
-}
-
-inline void FinishSlotInsert(const ClockHandleBasicData& proto, ClockHandle& h,
-                             uint64_t initial_countdown, bool keep_ref) {
-  // Save data fields
-  ClockHandleBasicData* h_alias = &h;
-  *h_alias = proto;
-
-  // Transition from "under construction" state to "visible" state
-  uint64_t new_meta = uint64_t{ClockHandle::kStateVisible}
-                      << ClockHandle::kStateShift;
-
-  // Maybe with an outstanding reference
-  new_meta |= initial_countdown << ClockHandle::kAcquireCounterShift;
-  new_meta |= (initial_countdown - keep_ref)
-              << ClockHandle::kReleaseCounterShift;
-
-#ifndef NDEBUG
-  // Save the state transition, with assertion
-  uint64_t old_meta = h.meta.exchange(new_meta, std::memory_order_release);
-  assert(old_meta >> ClockHandle::kStateShift ==
-         ClockHandle::kStateConstruction);
-#else
-  // Save the state transition
-  h.meta.store(new_meta, std::memory_order_release);
-#endif
-}
-
-bool TryInsert(const ClockHandleBasicData& proto, ClockHandle& h,
-               uint64_t initial_countdown, bool keep_ref,
-               bool* already_matches) {
-  bool b = BeginSlotInsert(proto, h, initial_countdown, already_matches);
-  if (b) {
-    FinishSlotInsert(proto, h, initial_countdown, keep_ref);
-  }
-  return b;
-}
-
-template <class HandleImpl, class Func>
-void ConstApplyToEntriesRange(Func /*const HandleImpl& -> void*/ func,
-                              const HandleImpl* begin, const HandleImpl* end,
-                              bool apply_if_will_be_deleted) {
-  uint64_t check_state_mask = ClockHandle::kStateShareableBit;
-  if (!apply_if_will_be_deleted) {
-    check_state_mask |= ClockHandle::kStateVisibleBit;
-  }
-
-  for (const HandleImpl* h = begin; h < end; ++h) {
-    // Note: to avoid using compare_exchange, we have to be extra careful.
-    uint64_t old_meta = h->meta.load(std::memory_order_relaxed);
-    // Check if it's an entry visible to lookups
-    if ((old_meta >> ClockHandle::kStateShift) & check_state_mask) {
-      // Increment acquire counter. Note: it's possible that the entry has
-      // completely changed since we loaded old_meta, but incrementing acquire
-      // count is always safe. (Similar to optimistic Lookup here.)
-      old_meta = h->meta.fetch_add(ClockHandle::kAcquireIncrement,
-                                   std::memory_order_acquire);
-      // Check whether we actually acquired a reference.
-      if ((old_meta >> ClockHandle::kStateShift) &
-          ClockHandle::kStateShareableBit) {
-        // Apply func if appropriate
-        if ((old_meta >> ClockHandle::kStateShift) & check_state_mask) {
-          func(*h);
-        }
-        // Pretend we never took the reference
-        h->meta.fetch_sub(ClockHandle::kAcquireIncrement,
-                          std::memory_order_release);
-        // No net change, so don't need to check for overflow
-      } else {
-        // For other states, incrementing the acquire counter has no effect
-        // so we don't need to undo it. Furthermore, we cannot safely undo
-        // it because we did not acquire a read reference to lock the
-        // entry in a Shareable state.
-      }
-    }
-  }
-}
-
-}  // namespace
-
-void ClockHandleBasicData::FreeData(MemoryAllocator* allocator) const {
-  if (helper->del_cb) {
-    helper->del_cb(value, allocator);
-  }
-}
-
-template <class HandleImpl>
-HandleImpl* BaseClockTable::StandaloneInsert(
-    const ClockHandleBasicData& proto) {
-  // Heap allocated separate from table
-  HandleImpl* h = new HandleImpl();
-  ClockHandleBasicData* h_alias = h;
-  *h_alias = proto;
-  h->SetStandalone();
-  // Single reference (standalone entries only created if returning a refed
-  // Handle back to user)
-  uint64_t meta = uint64_t{ClockHandle::kStateInvisible}
-                  << ClockHandle::kStateShift;
-  meta |= uint64_t{1} << ClockHandle::kAcquireCounterShift;
-  h->meta.store(meta, std::memory_order_release);
-  // Keep track of how much of usage is standalone
-  standalone_usage_.fetch_add(proto.GetTotalCharge(),
-                              std::memory_order_relaxed);
-  return h;
-}
-
-template <class Table>
-typename Table::HandleImpl* BaseClockTable::CreateStandalone(
-    ClockHandleBasicData& proto, size_t capacity, bool strict_capacity_limit,
-    bool allow_uncharged) {
-  Table& derived = static_cast<Table&>(*this);
-  typename Table::InsertState state;
-  derived.StartInsert(state);
-
-  const size_t total_charge = proto.GetTotalCharge();
-  if (strict_capacity_limit) {
-    Status s = ChargeUsageMaybeEvictStrict<Table>(
-        total_charge, capacity,
-        /*need_evict_for_occupancy=*/false, state);
-    if (!s.ok()) {
-      if (allow_uncharged) {
-        proto.total_charge = 0;
-      } else {
-        return nullptr;
-      }
-    }
-  } else {
-    // Case strict_capacity_limit == false
-    bool success = ChargeUsageMaybeEvictNonStrict<Table>(
-        total_charge, capacity,
-        /*need_evict_for_occupancy=*/false, state);
-    if (!success) {
-      // Force the issue
-      usage_.fetch_add(total_charge, std::memory_order_relaxed);
-    }
-  }
-
-  return StandaloneInsert<typename Table::HandleImpl>(proto);
-}
-
-template <class Table>
-Status BaseClockTable::ChargeUsageMaybeEvictStrict(
-    size_t total_charge, size_t capacity, bool need_evict_for_occupancy,
-    typename Table::InsertState& state) {
+inline Status HyperClockTable::ChargeUsageMaybeEvictStrict(
+    size_t total_charge, size_t capacity, bool need_evict_for_occupancy) {
   if (total_charge > capacity) {
     return Status::MemoryLimit(
         "Cache entry too large for a single cache shard: " +
@@ -421,8 +287,7 @@ Status BaseClockTable::ChargeUsageMaybeEvictStrict(
   if (request_evict_charge > 0) {
     size_t evicted_charge = 0;
     size_t evicted_count = 0;
-    static_cast<Table*>(this)->Evict(request_evict_charge, &evicted_charge,
-                                     &evicted_count, state);
+    Evict(request_evict_charge, &evicted_charge, &evicted_count);
     occupancy_.fetch_sub(evicted_count, std::memory_order_release);
     if (LIKELY(evicted_charge > need_evict_charge)) {
       assert(evicted_count > 0);
@@ -451,10 +316,8 @@ Status BaseClockTable::ChargeUsageMaybeEvictStrict(
   return Status::OK();
 }
 
-template <class Table>
-inline bool BaseClockTable::ChargeUsageMaybeEvictNonStrict(
-    size_t total_charge, size_t capacity, bool need_evict_for_occupancy,
-    typename Table::InsertState& state) {
+inline bool HyperClockTable::ChargeUsageMaybeEvictNonStrict(
+    size_t total_charge, size_t capacity, bool need_evict_for_occupancy) {
   // For simplicity, we consider that either the cache can accept the insert
   // with no evictions, or we must evict enough to make (at least) enough
   // space. It could lead to unnecessary failures or excessive evictions in
@@ -491,8 +354,7 @@ inline bool BaseClockTable::ChargeUsageMaybeEvictNonStrict(
   size_t evicted_charge = 0;
   size_t evicted_count = 0;
   if (need_evict_charge > 0) {
-    static_cast<Table*>(this)->Evict(need_evict_charge, &evicted_charge,
-                                     &evicted_count, state);
+    Evict(need_evict_charge, &evicted_charge, &evicted_count);
     // Deal with potential occupancy deficit
     if (UNLIKELY(need_evict_for_occupancy) && evicted_count == 0) {
       assert(evicted_charge == 0);
@@ -511,43 +373,54 @@ inline bool BaseClockTable::ChargeUsageMaybeEvictNonStrict(
   return true;
 }
 
-template <class Table>
-Status BaseClockTable::Insert(const ClockHandleBasicData& proto,
-                              typename Table::HandleImpl** handle,
-                              Cache::Priority priority, size_t capacity,
-                              bool strict_capacity_limit) {
-  using HandleImpl = typename Table::HandleImpl;
-  Table& derived = static_cast<Table&>(*this);
-
-  typename Table::InsertState state;
-  derived.StartInsert(state);
+inline HyperClockTable::HandleImpl* HyperClockTable::StandaloneInsert(
+    const ClockHandleBasicData& proto) {
+  // Heap allocated separate from table
+  HandleImpl* h = new HandleImpl();
+  ClockHandleBasicData* h_alias = h;
+  *h_alias = proto;
+  h->SetStandalone();
+  // Single reference (standalone entries only created if returning a refed
+  // Handle back to user)
+  uint64_t meta = uint64_t{ClockHandle::kStateInvisible}
+                  << ClockHandle::kStateShift;
+  meta |= uint64_t{1} << ClockHandle::kAcquireCounterShift;
+  h->meta.store(meta, std::memory_order_release);
+  // Keep track of how much of usage is standalone
+  standalone_usage_.fetch_add(proto.GetTotalCharge(),
+                              std::memory_order_relaxed);
+  return h;
+}
 
+Status HyperClockTable::Insert(const ClockHandleBasicData& proto,
+                               HandleImpl** handle, Cache::Priority priority,
+                               size_t capacity, bool strict_capacity_limit) {
   // Do we have the available occupancy? Optimistically assume we do
   // and deal with it if we don't.
   size_t old_occupancy = occupancy_.fetch_add(1, std::memory_order_acquire);
+  auto revert_occupancy_fn = [&]() {
+    occupancy_.fetch_sub(1, std::memory_order_relaxed);
+  };
   // Whether we over-committed and need an eviction to make up for it
-  bool need_evict_for_occupancy =
-      !derived.GrowIfNeeded(old_occupancy + 1, state);
+  bool need_evict_for_occupancy = old_occupancy >= occupancy_limit_;
 
   // Usage/capacity handling is somewhat different depending on
   // strict_capacity_limit, but mostly pessimistic.
   bool use_standalone_insert = false;
   const size_t total_charge = proto.GetTotalCharge();
   if (strict_capacity_limit) {
-    Status s = ChargeUsageMaybeEvictStrict<Table>(
-        total_charge, capacity, need_evict_for_occupancy, state);
+    Status s = ChargeUsageMaybeEvictStrict(total_charge, capacity,
+                                           need_evict_for_occupancy);
     if (!s.ok()) {
-      // Revert occupancy
-      occupancy_.fetch_sub(1, std::memory_order_relaxed);
+      revert_occupancy_fn();
       return s;
     }
   } else {
     // Case strict_capacity_limit == false
-    bool success = ChargeUsageMaybeEvictNonStrict<Table>(
-        total_charge, capacity, need_evict_for_occupancy, state);
+    bool success = ChargeUsageMaybeEvictNonStrict(total_charge, capacity,
+                                                  need_evict_for_occupancy);
     if (!success) {
-      // Revert occupancy
-      occupancy_.fetch_sub(1, std::memory_order_relaxed);
+      revert_occupancy_fn();
       if (handle == nullptr) {
         // Don't insert the entry but still return ok, as if the entry
         // inserted into cache and evicted immediately.
@@ -560,6 +433,11 @@ Status BaseClockTable::Insert(const ClockHandleBasicData& proto,
       }
     }
   }
+  auto revert_usage_fn = [&]() {
+    usage_.fetch_sub(total_charge, std::memory_order_relaxed);
+    // No underflow
+    assert(usage_.load(std::memory_order_relaxed) < SIZE_MAX / 2);
+  };
 
   if (!use_standalone_insert) {
     // Attempt a table insert, but abort if we find an existing entry for the
@@ -573,37 +451,129 @@ Status BaseClockTable::Insert(const ClockHandleBasicData& proto,
     uint64_t initial_countdown = GetInitialCountdown(priority);
     assert(initial_countdown > 0);
 
-    HandleImpl* e =
-        derived.DoInsert(proto, initial_countdown, handle != nullptr, state);
+    size_t probe = 0;
+    HandleImpl* e = FindSlot(
+        proto.hashed_key,
+        [&](HandleImpl* h) {
+          // Optimistically transition the slot from "empty" to
+          // "under construction" (no effect on other states)
+          uint64_t old_meta =
+              h->meta.fetch_or(uint64_t{ClockHandle::kStateOccupiedBit}
+                                   << ClockHandle::kStateShift,
+                               std::memory_order_acq_rel);
+          uint64_t old_state = old_meta >> ClockHandle::kStateShift;
 
-    if (e) {
+          if (old_state == ClockHandle::kStateEmpty) {
+            // We've started inserting into an available slot, and taken
+            // ownership Save data fields
+            ClockHandleBasicData* h_alias = h;
+            *h_alias = proto;
+
+            // Transition from "under construction" state to "visible" state
+            uint64_t new_meta = uint64_t{ClockHandle::kStateVisible}
+                                << ClockHandle::kStateShift;
+
+            // Maybe with an outstanding reference
+            new_meta |= initial_countdown << ClockHandle::kAcquireCounterShift;
+            new_meta |= (initial_countdown - (handle != nullptr))
+                        << ClockHandle::kReleaseCounterShift;
+
+#ifndef NDEBUG
+            // Save the state transition, with assertion
+            old_meta = h->meta.exchange(new_meta, std::memory_order_release);
+            assert(old_meta >> ClockHandle::kStateShift ==
+                   ClockHandle::kStateConstruction);
+#else
+            // Save the state transition
+            h->meta.store(new_meta, std::memory_order_release);
+#endif
+            return true;
+          } else if (old_state != ClockHandle::kStateVisible) {
+            // Slot not usable / touchable now
+            return false;
+          }
+          // Existing, visible entry, which might be a match.
+          // But first, we need to acquire a ref to read it. In fact, number of
+          // refs for initial countdown, so that we boost the clock state if
+          // this is a match.
+          old_meta = h->meta.fetch_add(
+              ClockHandle::kAcquireIncrement * initial_countdown,
+              std::memory_order_acq_rel);
+          // Like Lookup
+          if ((old_meta >> ClockHandle::kStateShift) ==
+              ClockHandle::kStateVisible) {
+            // Acquired a read reference
+            if (h->hashed_key == proto.hashed_key) {
+              // Match. Release in a way that boosts the clock state
+              old_meta = h->meta.fetch_add(
+                  ClockHandle::kReleaseIncrement * initial_countdown,
+                  std::memory_order_acq_rel);
+              // Correct for possible (but rare) overflow
+              CorrectNearOverflow(old_meta, h->meta);
+              // Insert standalone instead (only if return handle needed)
+              use_standalone_insert = true;
+              return true;
+            } else {
+              // Mismatch. Pretend we never took the reference
+              old_meta = h->meta.fetch_sub(
+                  ClockHandle::kAcquireIncrement * initial_countdown,
+                  std::memory_order_acq_rel);
+            }
+          } else if (UNLIKELY((old_meta >> ClockHandle::kStateShift) ==
+                              ClockHandle::kStateInvisible)) {
+            // Pretend we never took the reference
+            // WART: there's a tiny chance we release last ref to invisible
+            // entry here. If that happens, we let eviction take care of it.
+            old_meta = h->meta.fetch_sub(
+                ClockHandle::kAcquireIncrement * initial_countdown,
+                std::memory_order_acq_rel);
+          } else {
+            // For other states, incrementing the acquire counter has no effect
+            // so we don't need to undo it.
+            // Slot not usable / touchable now.
+          }
+          (void)old_meta;
+          return false;
+        },
+        [&](HandleImpl* /*h*/) { return false; },
+        [&](HandleImpl* h) {
+          h->displacements.fetch_add(1, std::memory_order_relaxed);
+        },
+        probe);
+    if (e == nullptr) {
+      // Occupancy check and never abort FindSlot above should generally
+      // prevent this, except it's theoretically possible for other threads
+      // to evict and replace entries in the right order to hit every slot
+      // when it is populated. Assuming random hashing, the chance of that
+      // should be no higher than pow(kStrictLoadFactor, n) for n slots.
+      // That should be infeasible for roughly n >= 256, so if this assertion
+      // fails, that suggests something is going wrong.
+      assert(GetTableSize() < 256);
+      use_standalone_insert = true;
+    }
+    if (!use_standalone_insert) {
       // Successfully inserted
       if (handle) {
         *handle = e;
       }
       return Status::OK();
     }
-    // Not inserted
-    // Revert occupancy
-    occupancy_.fetch_sub(1, std::memory_order_relaxed);
+    // Roll back table insertion
+    Rollback(proto.hashed_key, e);
+    revert_occupancy_fn();
     // Maybe fall back on standalone insert
     if (handle == nullptr) {
-      // Revert usage
-      usage_.fetch_sub(total_charge, std::memory_order_relaxed);
-      // No underflow
-      assert(usage_.load(std::memory_order_relaxed) < SIZE_MAX / 2);
+      revert_usage_fn();
       // As if unrefed entry immdiately evicted
       proto.FreeData(allocator_);
       return Status::OK();
     }
-
-    use_standalone_insert = true;
   }
 
   // Run standalone insert
   assert(use_standalone_insert);
 
-  *handle = StandaloneInsert<HandleImpl>(proto);
+  *handle = StandaloneInsert(proto);
 
   // The OkOverwritten status is used to count "redundant" insertions into
   // block cache. This implementation doesn't strictly check for redundant
@@ -613,156 +583,37 @@ Status BaseClockTable::Insert(const ClockHandleBasicData& proto,
   return Status::OkOverwritten();
 }
 
-void BaseClockTable::Ref(ClockHandle& h) {
-  // Increment acquire counter
-  uint64_t old_meta = h.meta.fetch_add(ClockHandle::kAcquireIncrement,
-                                       std::memory_order_acquire);
-
-  assert((old_meta >> ClockHandle::kStateShift) &
-         ClockHandle::kStateShareableBit);
-  // Must have already had a reference
-  assert(GetRefcount(old_meta) > 0);
-  (void)old_meta;
-}
-
-#ifndef NDEBUG
-void BaseClockTable::TEST_RefN(ClockHandle& h, size_t n) {
-  // Increment acquire counter
-  uint64_t old_meta = h.meta.fetch_add(n * ClockHandle::kAcquireIncrement,
-                                       std::memory_order_acquire);
-
-  assert((old_meta >> ClockHandle::kStateShift) &
-         ClockHandle::kStateShareableBit);
-  (void)old_meta;
-}
-
-void BaseClockTable::TEST_ReleaseNMinus1(ClockHandle* h, size_t n) {
-  assert(n > 0);
-
-  // Like n-1 Releases, but assumes one more will happen in the caller to take
-  // care of anything like erasing an unreferenced, invisible entry.
-  uint64_t old_meta = h->meta.fetch_add(
-      (n - 1) * ClockHandle::kReleaseIncrement, std::memory_order_acquire);
-  assert((old_meta >> ClockHandle::kStateShift) &
-         ClockHandle::kStateShareableBit);
-  (void)old_meta;
-}
-#endif
-
-HyperClockTable::HyperClockTable(
-    size_t capacity, bool /*strict_capacity_limit*/,
-    CacheMetadataChargePolicy metadata_charge_policy,
-    MemoryAllocator* allocator,
-    const Cache::EvictionCallback* eviction_callback, const uint32_t* hash_seed,
-    const Opts& opts)
-    : BaseClockTable(metadata_charge_policy, allocator, eviction_callback,
-                     hash_seed),
-      length_bits_(CalcHashBits(capacity, opts.estimated_value_size,
-                                metadata_charge_policy)),
-      length_bits_mask_((size_t{1} << length_bits_) - 1),
-      occupancy_limit_(static_cast<size_t>((uint64_t{1} << length_bits_) *
-                                           kStrictLoadFactor)),
-      array_(new HandleImpl[size_t{1} << length_bits_]) {
-  if (metadata_charge_policy ==
-      CacheMetadataChargePolicy::kFullChargeCacheMetadata) {
-    usage_ += size_t{GetTableSize()} * sizeof(HandleImpl);
-  }
-
-  static_assert(sizeof(HandleImpl) == 64U,
-                "Expecting size / alignment with common cache line size");
-}
-
-HyperClockTable::~HyperClockTable() {
-  // Assumes there are no references or active operations on any slot/element
-  // in the table.
-  for (size_t i = 0; i < GetTableSize(); i++) {
-    HandleImpl& h = array_[i];
-    switch (h.meta >> ClockHandle::kStateShift) {
-      case ClockHandle::kStateEmpty:
-        // noop
-        break;
-      case ClockHandle::kStateInvisible:  // rare but possible
-      case ClockHandle::kStateVisible:
-        assert(GetRefcount(h.meta) == 0);
-        h.FreeData(allocator_);
-#ifndef NDEBUG
-        Rollback(h.hashed_key, &h);
-        ReclaimEntryUsage(h.GetTotalCharge());
-#endif
-        break;
-      // otherwise
-      default:
-        assert(false);
-        break;
+HyperClockTable::HandleImpl* HyperClockTable::CreateStandalone(
+    ClockHandleBasicData& proto, size_t capacity, bool strict_capacity_limit,
+    bool allow_uncharged) {
+  const size_t total_charge = proto.GetTotalCharge();
+  if (strict_capacity_limit) {
+    Status s = ChargeUsageMaybeEvictStrict(total_charge, capacity,
+                                           /*need_evict_for_occupancy=*/false);
+    if (!s.ok()) {
+      if (allow_uncharged) {
+        proto.total_charge = 0;
+      } else {
+        return nullptr;
+      }
+    }
+  } else {
+    // Case strict_capacity_limit == false
+    bool success =
+        ChargeUsageMaybeEvictNonStrict(total_charge, capacity,
+                                       /*need_evict_for_occupancy=*/false);
+    if (!success) {
+      // Force the issue
+      usage_.fetch_add(total_charge, std::memory_order_relaxed);
     }
   }
 
-#ifndef NDEBUG
-  for (size_t i = 0; i < GetTableSize(); i++) {
-    assert(array_[i].displacements.load() == 0);
-  }
-#endif
-
-  assert(usage_.load() == 0 ||
-         usage_.load() == size_t{GetTableSize()} * sizeof(HandleImpl));
-  assert(occupancy_ == 0);
-}
-
-void HyperClockTable::StartInsert(InsertState&) {}
-
-bool HyperClockTable::GrowIfNeeded(size_t new_occupancy, InsertState&) {
-  return new_occupancy <= occupancy_limit_;
-}
-
-HyperClockTable::HandleImpl* HyperClockTable::DoInsert(
-    const ClockHandleBasicData& proto, uint64_t initial_countdown,
-    bool keep_ref, InsertState&) {
-  bool already_matches = false;
-  HandleImpl* e = FindSlot(
-      proto.hashed_key,
-      [&](HandleImpl* h) {
-        return TryInsert(proto, *h, initial_countdown, keep_ref,
-                         &already_matches);
-      },
-      [&](HandleImpl* h) {
-        if (already_matches) {
-          // Stop searching & roll back displacements
-          Rollback(proto.hashed_key, h);
-          return true;
-        } else {
-          // Keep going
-          return false;
-        }
-      },
-      [&](HandleImpl* h, bool is_last) {
-        if (is_last) {
-          // Search is ending. Roll back displacements
-          Rollback(proto.hashed_key, h);
-        } else {
-          h->displacements.fetch_add(1, std::memory_order_relaxed);
-        }
-      });
-  if (already_matches) {
-    // Insertion skipped
-    return nullptr;
-  }
-  if (e != nullptr) {
-    // Successfully inserted
-    return e;
-  }
-  // Else, no available slot found. Occupancy check should generally prevent
-  // this, except it's theoretically possible for other threads to evict and
-  // replace entries in the right order to hit every slot when it is populated.
-  // Assuming random hashing, the chance of that should be no higher than
-  // pow(kStrictLoadFactor, n) for n slots. That should be infeasible for
-  // roughly n >= 256, so if this assertion fails, that suggests something is
-  // going wrong.
-  assert(GetTableSize() < 256);
-  return nullptr;
+  return StandaloneInsert(proto);
 }
 
 HyperClockTable::HandleImpl* HyperClockTable::Lookup(
     const UniqueId64x2& hashed_key) {
+  size_t probe = 0;
   HandleImpl* e = FindSlot(
       hashed_key,
       [&](HandleImpl* h) {
@@ -822,7 +673,7 @@ HyperClockTable::HandleImpl* HyperClockTable::Lookup(
       [&](HandleImpl* h) {
         return h->displacements.load(std::memory_order_relaxed) == 0;
       },
-      [&](HandleImpl* /*h*/, bool /*is_last*/) {});
+      [&](HandleImpl* /*h*/) {}, probe);
 
   return e;
 }
@@ -902,19 +753,43 @@ bool HyperClockTable::Release(HandleImpl* h, bool useful,
   }
 }
 
-#ifndef NDEBUG
+void HyperClockTable::Ref(HandleImpl& h) {
+  // Increment acquire counter
+  uint64_t old_meta = h.meta.fetch_add(ClockHandle::kAcquireIncrement,
+                                       std::memory_order_acquire);
+
+  assert((old_meta >> ClockHandle::kStateShift) &
+         ClockHandle::kStateShareableBit);
+  // Must have already had a reference
+  assert(GetRefcount(old_meta) > 0);
+  (void)old_meta;
+}
+
+void HyperClockTable::TEST_RefN(HandleImpl& h, size_t n) {
+  // Increment acquire counter
+  uint64_t old_meta = h.meta.fetch_add(n * ClockHandle::kAcquireIncrement,
+                                       std::memory_order_acquire);
+
+  assert((old_meta >> ClockHandle::kStateShift) &
+         ClockHandle::kStateShareableBit);
+  (void)old_meta;
+}
+
 void HyperClockTable::TEST_ReleaseN(HandleImpl* h, size_t n) {
   if (n > 0) {
-    // Do n-1 simple releases first
-    TEST_ReleaseNMinus1(h, n);
+    // Split into n - 1 and 1 steps.
+    uint64_t old_meta = h->meta.fetch_add(
+        (n - 1) * ClockHandle::kReleaseIncrement, std::memory_order_acquire);
+    assert((old_meta >> ClockHandle::kStateShift) &
+           ClockHandle::kStateShareableBit);
+    (void)old_meta;
 
-    // Then the last release might be more involved
     Release(h, /*useful*/ true, /*erase_if_last_ref*/ false);
   }
 }
-#endif
 
 void HyperClockTable::Erase(const UniqueId64x2& hashed_key) {
+  size_t probe = 0;
   (void)FindSlot(
       hashed_key,
       [&](HandleImpl* h) {
@@ -981,7 +856,48 @@ void HyperClockTable::Erase(const UniqueId64x2& hashed_key) {
       [&](HandleImpl* h) {
         return h->displacements.load(std::memory_order_relaxed) == 0;
       },
-      [&](HandleImpl* /*h*/, bool /*is_last*/) {});
+      [&](HandleImpl* /*h*/) {}, probe);
+}
+
+void HyperClockTable::ConstApplyToEntriesRange(
+    std::function<void(const HandleImpl&)> func, size_t index_begin,
+    size_t index_end, bool apply_if_will_be_deleted) const {
+  uint64_t check_state_mask = ClockHandle::kStateShareableBit;
+  if (!apply_if_will_be_deleted) {
+    check_state_mask |= ClockHandle::kStateVisibleBit;
+  }
+
+  for (size_t i = index_begin; i < index_end; i++) {
+    HandleImpl& h = array_[i];
+
+    // Note: to avoid using compare_exchange, we have to be extra careful.
+    uint64_t old_meta = h.meta.load(std::memory_order_relaxed);
+    // Check if it's an entry visible to lookups
+    if ((old_meta >> ClockHandle::kStateShift) & check_state_mask) {
+      // Increment acquire counter. Note: it's possible that the entry has
+      // completely changed since we loaded old_meta, but incrementing acquire
+      // count is always safe. (Similar to optimistic Lookup here.)
+      old_meta = h.meta.fetch_add(ClockHandle::kAcquireIncrement,
+                                  std::memory_order_acquire);
+      // Check whether we actually acquired a reference.
+      if ((old_meta >> ClockHandle::kStateShift) &
+          ClockHandle::kStateShareableBit) {
+        // Apply func if appropriate
+        if ((old_meta >> ClockHandle::kStateShift) & check_state_mask) {
+          func(h);
+        }
+        // Pretend we never took the reference
+        h.meta.fetch_sub(ClockHandle::kAcquireIncrement,
+                         std::memory_order_release);
+        // No net change, so don't need to check for overflow
+      } else {
+        // For other states, incrementing the acquire counter has no effect
+        // so we don't need to undo it. Furthermore, we cannot safely undo
+        // it because we did not acquire a read reference to lock the
+        // entry in a Shareable state.
+      }
+    }
+  }
 }
 
 void HyperClockTable::EraseUnRefEntries() {
@@ -1005,10 +921,10 @@ void HyperClockTable::EraseUnRefEntries() {
   }
 }
 
-template <typename MatchFn, typename AbortFn, typename UpdateFn>
 inline HyperClockTable::HandleImpl* HyperClockTable::FindSlot(
-    const UniqueId64x2& hashed_key, MatchFn match_fn, AbortFn abort_fn,
-    UpdateFn update_fn) {
+    const UniqueId64x2& hashed_key, std::function<bool(HandleImpl*)> match_fn,
+    std::function<bool(HandleImpl*)> abort_fn,
+    std::function<void(HandleImpl*)> update_fn, size_t& probe) {
   // NOTE: upper 32 bits of hashed_key[0] is used for sharding
   //
   // We use double-hashing probing. Every probe in the sequence is a
@@ -1022,21 +938,20 @@ inline HyperClockTable::HandleImpl* HyperClockTable::FindSlot(
   // TODO: we could also reconsider linear probing, though locality benefits
   // are limited because each slot is a full cache line
   size_t increment = static_cast<size_t>(hashed_key[0]) | 1U;
-  size_t first = ModTableSize(base);
-  size_t current = first;
-  bool is_last;
-  do {
+  size_t current = ModTableSize(base + probe * increment);
+  while (probe <= length_bits_mask_) {
     HandleImpl* h = &array_[current];
     if (match_fn(h)) {
+      probe++;
       return h;
     }
     if (abort_fn(h)) {
       return nullptr;
     }
+    probe++;
+    update_fn(h);
     current = ModTableSize(current + increment);
-    is_last = current == first;
-    update_fn(h, is_last);
-  } while (!is_last);
+  }
   // We looped back.
   return nullptr;
 }
@@ -1063,8 +978,7 @@ inline void HyperClockTable::ReclaimEntryUsage(size_t total_charge) {
 }
 
 inline void HyperClockTable::Evict(size_t requested_charge,
-                                   size_t* freed_charge, size_t* freed_count,
-                                   InsertState&) {
+                                   size_t* freed_charge, size_t* freed_count) {
   // precondition
   assert(requested_charge > 0);
 
@@ -1149,32 +1063,35 @@ void ClockCacheShard<Table>::ApplyToSomeEntries(
                              size_t charge,
                              const Cache::CacheItemHelper* helper)>& callback,
     size_t average_entries_per_lock, size_t* state) {
-  // The state will be a simple index into the table. Even with a dynamic
-  // hyper clock cache, entries will generally stay in their existing
-  // slots, so we don't need to be aware of the high-level organization
-  // that makes lookup efficient.
+  // The state is essentially going to be the starting hash, which works
+  // nicely even if we resize between calls because we use upper-most
+  // hash bits for table indexes.
+  size_t length_bits = table_.GetLengthBits();
   size_t length = table_.GetTableSize();
 
   assert(average_entries_per_lock > 0);
+  // Assuming we are called with same average_entries_per_lock repeatedly,
+  // this simplifies some logic (index_end will not overflow).
+  assert(average_entries_per_lock < length || *state == 0);
 
-  size_t index_begin = *state;
+  size_t index_begin = *state >> (sizeof(size_t) * 8u - length_bits);
   size_t index_end = index_begin + average_entries_per_lock;
   if (index_end >= length) {
     // Going to end.
     index_end = length;
     *state = SIZE_MAX;
   } else {
-    *state = index_end;
+    *state = index_end << (sizeof(size_t) * 8u - length_bits);
   }
 
   auto hash_seed = table_.GetHashSeed();
-  ConstApplyToEntriesRange(
+  table_.ConstApplyToEntriesRange(
       [callback, hash_seed](const HandleImpl& h) {
         UniqueId64x2 unhashed;
         callback(ReverseHash(h.hashed_key, &unhashed, hash_seed), h.value,
                  h.GetTotalCharge(), h.helper);
       },
-      table_.HandlePtr(index_begin), table_.HandlePtr(index_end), false);
+      index_begin, index_end, false);
 }
 
 int HyperClockTable::CalcHashBits(
@@ -1229,15 +1146,18 @@ Status ClockCacheShard<Table>::Insert(const Slice& key,
   proto.value = value;
   proto.helper = helper;
   proto.total_charge = charge;
-  return table_.template Insert<Table>(
-      proto, handle, priority, capacity_.load(std::memory_order_relaxed),
-      strict_capacity_limit_.load(std::memory_order_relaxed));
+  return table_.Insert(proto, handle, priority,
+                       capacity_.load(std::memory_order_relaxed),
+                       strict_capacity_limit_.load(std::memory_order_relaxed));
 }
 
 template <class Table>
-typename Table::HandleImpl* ClockCacheShard<Table>::CreateStandalone(
-    const Slice& key, const UniqueId64x2& hashed_key, Cache::ObjectPtr obj,
-    const Cache::CacheItemHelper* helper, size_t charge, bool allow_uncharged) {
+typename ClockCacheShard<Table>::HandleImpl*
+ClockCacheShard<Table>::CreateStandalone(const Slice& key,
+                                         const UniqueId64x2& hashed_key,
+                                         Cache::ObjectPtr obj,
+                                         const Cache::CacheItemHelper* helper,
+                                         size_t charge, bool allow_uncharged) {
   if (UNLIKELY(key.size() != kCacheKeySize)) {
     return nullptr;
   }
@@ -1246,7 +1166,7 @@ typename Table::HandleImpl* ClockCacheShard<Table>::CreateStandalone(
   proto.value = obj;
   proto.helper = helper;
   proto.total_charge = charge;
-  return table_.template CreateStandalone<Table>(
+  return table_.CreateStandalone(
       proto, capacity_.load(std::memory_order_relaxed),
       strict_capacity_limit_.load(std::memory_order_relaxed), allow_uncharged);
 }
@@ -1278,7 +1198,6 @@ bool ClockCacheShard<Table>::Release(HandleImpl* handle, bool useful,
   return table_.Release(handle, useful, erase_if_last_ref);
 }
 
-#ifndef NDEBUG
 template <class Table>
 void ClockCacheShard<Table>::TEST_RefN(HandleImpl* h, size_t n) {
   table_.TEST_RefN(*h, n);
@@ -1288,7 +1207,6 @@ template <class Table>
 void ClockCacheShard<Table>::TEST_ReleaseN(HandleImpl* h, size_t n) {
   table_.TEST_ReleaseN(h, n);
 }
-#endif
 
 template <class Table>
 bool ClockCacheShard<Table>::Release(HandleImpl* handle,
@@ -1331,7 +1249,7 @@ size_t ClockCacheShard<Table>::GetPinnedUsage() const {
   size_t table_pinned_usage = 0;
   const bool charge_metadata =
       metadata_charge_policy_ == kFullChargeCacheMetadata;
-  ConstApplyToEntriesRange(
+  table_.ConstApplyToEntriesRange(
       [&table_pinned_usage, charge_metadata](const HandleImpl& h) {
         uint64_t meta = h.meta.load(std::memory_order_relaxed);
         uint64_t refcount = GetRefcount(meta);
@@ -1344,7 +1262,7 @@ size_t ClockCacheShard<Table>::GetPinnedUsage() const {
           }
         }
       },
-      table_.HandlePtr(0), table_.HandlePtr(table_.GetTableSize()), true);
+      0, table_.GetTableSize(), true);
 
   return table_pinned_usage + table_.GetStandaloneUsage();
 }

cache/clock_cache.h

@@ -371,127 +371,14 @@ struct ClockHandle : public ClockHandleBasicData {
   static constexpr uint8_t kMaxCountdown = kHighCountdown;
   // TODO: make these coundown values tuning parameters for eviction?
 
-  // See above. Mutable for read reference counting.
-  mutable std::atomic<uint64_t> meta{};
+  // See above
+  std::atomic<uint64_t> meta{};
 
-  // Whether this is a "deteched" handle that is independently allocated
-  // with `new` (so must be deleted with `delete`).
-  // TODO: ideally this would be packed into some other data field, such
-  // as upper bits of total_charge, but that incurs a measurable performance
-  // regression.
-  bool standalone = false;
-
-  inline bool IsStandalone() const { return standalone; }
-
-  inline void SetStandalone() { standalone = true; }
+  // Anticipating use for SecondaryCache support
+  void* reserved_for_future_use = nullptr;
 };  // struct ClockHandle
 
-class BaseClockTable {
- public:
-  BaseClockTable(CacheMetadataChargePolicy metadata_charge_policy,
-                 MemoryAllocator* allocator,
-                 const Cache::EvictionCallback* eviction_callback,
-                 const uint32_t* hash_seed)
-      : metadata_charge_policy_(metadata_charge_policy),
-        allocator_(allocator),
-        eviction_callback_(*eviction_callback),
-        hash_seed_(*hash_seed) {}
-
-  template <class Table>
-  typename Table::HandleImpl* CreateStandalone(ClockHandleBasicData& proto,
-                                               size_t capacity,
-                                               bool strict_capacity_limit,
-                                               bool allow_uncharged);
-
-  template <class Table>
-  Status Insert(const ClockHandleBasicData& proto,
-                typename Table::HandleImpl** handle, Cache::Priority priority,
-                size_t capacity, bool strict_capacity_limit);
-
-  void Ref(ClockHandle& handle);
-
-  size_t GetOccupancy() const {
-    return occupancy_.load(std::memory_order_relaxed);
-  }
-
-  size_t GetUsage() const { return usage_.load(std::memory_order_relaxed); }
-
-  size_t GetStandaloneUsage() const {
-    return standalone_usage_.load(std::memory_order_relaxed);
-  }
-
-  uint32_t GetHashSeed() const { return hash_seed_; }
-
-#ifndef NDEBUG
-  // Acquire N references
-  void TEST_RefN(ClockHandle& handle, size_t n);
-  // Helper for TEST_ReleaseN
-  void TEST_ReleaseNMinus1(ClockHandle* handle, size_t n);
-#endif
-
- private:  // fns
-  // Creates a "standalone" handle for returning from an Insert operation that
-  // cannot be completed by actually inserting into the table.
-  // Updates `standalone_usage_` but not `usage_` nor `occupancy_`.
-  template <class HandleImpl>
-  HandleImpl* StandaloneInsert(const ClockHandleBasicData& proto);
-
-  // Helper for updating `usage_` for new entry with given `total_charge`
-  // and evicting if needed under strict_capacity_limit=true rules. This
-  // means the operation might fail with Status::MemoryLimit. If
-  // `need_evict_for_occupancy`, then eviction of at least one entry is
-  // required, and the operation should fail if not possible.
-  // NOTE: Otherwise, occupancy_ is not managed in this function
-  template <class Table>
-  Status ChargeUsageMaybeEvictStrict(size_t total_charge, size_t capacity,
-                                     bool need_evict_for_occupancy,
-                                     typename Table::InsertState& state);
-
-  // Helper for updating `usage_` for new entry with given `total_charge`
-  // and evicting if needed under strict_capacity_limit=false rules. This
-  // means that updating `usage_` always succeeds even if forced to exceed
-  // capacity. If `need_evict_for_occupancy`, then eviction of at least one
-  // entry is required, and the operation should return false if such eviction
-  // is not possible. `usage_` is not updated in that case. Otherwise, returns
-  // true, indicating success.
-  // NOTE: occupancy_ is not managed in this function
-  template <class Table>
-  bool ChargeUsageMaybeEvictNonStrict(size_t total_charge, size_t capacity,
-                                      bool need_evict_for_occupancy,
-                                      typename Table::InsertState& state);
-
- protected:  // data
-  // We partition the following members into different cache lines
-  // to avoid false sharing among Lookup, Release, Erase and Insert
-  // operations in ClockCacheShard.
-
-  // Clock algorithm sweep pointer.
-  std::atomic<uint64_t> clock_pointer_{};
-
-  ALIGN_AS(CACHE_LINE_SIZE)
-  // Number of elements in the table.
-  std::atomic<size_t> occupancy_{};
-
-  // Memory usage by entries tracked by the cache (including standalone)
-  std::atomic<size_t> usage_{};
-
-  // Part of usage by standalone entries (not in table)
-  std::atomic<size_t> standalone_usage_{};
-
-  ALIGN_AS(CACHE_LINE_SIZE)
-  const CacheMetadataChargePolicy metadata_charge_policy_;
-
-  // From Cache, for deleter
-  MemoryAllocator* const allocator_;
-
-  // A reference to Cache::eviction_callback_
-  const Cache::EvictionCallback& eviction_callback_;
-
-  // A reference to ShardedCacheBase::hash_seed_
-  const uint32_t& hash_seed_;
-};
-
-class HyperClockTable : public BaseClockTable {
+class HyperClockTable {
  public:
   // Target size to be exactly a common cache line size (see static_assert in
   // clock_cache.cc)
@@ -500,6 +387,16 @@ class HyperClockTable : public BaseClockTable {
     // up in this slot or a higher one.
     std::atomic<uint32_t> displacements{};
 
+    // Whether this is a "deteched" handle that is independently allocated
+    // with `new` (so must be deleted with `delete`).
+    // TODO: ideally this would be packed into some other data field, such
+    // as upper bits of total_charge, but that incurs a measurable performance
+    // regression.
+    bool standalone = false;
+
+    inline bool IsStandalone() const { return standalone; }
+
+    inline void SetStandalone() { standalone = true; }
   };  // struct HandleImpl
 
   struct Opts {
@@ -513,46 +410,49 @@ class HyperClockTable : public BaseClockTable {
                   const uint32_t* hash_seed, const Opts& opts);
   ~HyperClockTable();
 
-  // For BaseClockTable::Insert
-  struct InsertState {};
+  Status Insert(const ClockHandleBasicData& proto, HandleImpl** handle,
+                Cache::Priority priority, size_t capacity,
+                bool strict_capacity_limit);
 
-  void StartInsert(InsertState& state);
-
-  // Returns true iff there is room for the proposed number of entries.
-  bool GrowIfNeeded(size_t new_occupancy, InsertState& state);
-
-  HandleImpl* DoInsert(const ClockHandleBasicData& proto,
-                       uint64_t initial_countdown, bool take_ref,
-                       InsertState& state);
-
-  // Runs the clock eviction algorithm trying to reclaim at least
-  // requested_charge. Returns how much is evicted, which could be less
-  // if it appears impossible to evict the requested amount without blocking.
-  void Evict(size_t requested_charge, size_t* freed_charge, size_t* freed_count,
-             InsertState& state);
+  HandleImpl* CreateStandalone(ClockHandleBasicData& proto, size_t capacity,
+                               bool strict_capacity_limit,
+                               bool allow_uncharged);
 
   HandleImpl* Lookup(const UniqueId64x2& hashed_key);
 
   bool Release(HandleImpl* handle, bool useful, bool erase_if_last_ref);
 
+  void Ref(HandleImpl& handle);
+
   void Erase(const UniqueId64x2& hashed_key);
 
+  void ConstApplyToEntriesRange(std::function<void(const HandleImpl&)> func,
+                                size_t index_begin, size_t index_end,
+                                bool apply_if_will_be_deleted) const;
+
   void EraseUnRefEntries();
 
   size_t GetTableSize() const { return size_t{1} << length_bits_; }
 
-  size_t GetOccupancyLimit() const { return occupancy_limit_; }
+  int GetLengthBits() const { return length_bits_; }
 
-  const HandleImpl* HandlePtr(size_t idx) const { return &array_[idx]; }
-
-#ifndef NDEBUG
-  size_t& TEST_MutableOccupancyLimit() const {
-    return const_cast<size_t&>(occupancy_limit_);
+  size_t GetOccupancy() const {
+    return occupancy_.load(std::memory_order_relaxed);
   }
 
-  // Release N references
+  size_t GetOccupancyLimit() const { return occupancy_limit_; }
+
+  size_t GetUsage() const { return usage_.load(std::memory_order_relaxed); }
+
+  size_t GetStandaloneUsage() const {
+    return standalone_usage_.load(std::memory_order_relaxed);
+  }
+
+  uint32_t GetHashSeed() const { return hash_seed_; }
+
+  // Acquire/release N references
+  void TEST_RefN(HandleImpl& handle, size_t n);
   void TEST_ReleaseN(HandleImpl* handle, size_t n);
-#endif
 
  private:  // functions
   // Returns x mod 2^{length_bits_}.
@@ -560,18 +460,28 @@ class HyperClockTable : public BaseClockTable {
     return static_cast<size_t>(x) & length_bits_mask_;
   }
 
-  // Returns the first slot in the probe sequence with a handle e such that
-  // match_fn(e) is true. At every step, the function first tests whether
-  // match_fn(e) holds. If this is false, it evaluates abort_fn(e) to decide
-  // whether the search should be aborted, and if so, FindSlot immediately
-  // returns nullptr. For every handle e that is not a match and not aborted,
-  // FindSlot runs update_fn(e, is_last) where is_last is set to true iff that
-  // slot will be the last probed because the next would cycle back to the first
-  // slot probed. This function uses templates instead of std::function to
-  // minimize the risk of heap-allocated closures being created.
-  template <typename MatchFn, typename AbortFn, typename UpdateFn>
-  inline HandleImpl* FindSlot(const UniqueId64x2& hashed_key, MatchFn match_fn,
-                              AbortFn abort_fn, UpdateFn update_fn);
+  // Runs the clock eviction algorithm trying to reclaim at least
+  // requested_charge. Returns how much is evicted, which could be less
+  // if it appears impossible to evict the requested amount without blocking.
+  inline void Evict(size_t requested_charge, size_t* freed_charge,
+                    size_t* freed_count);
+
+  // Returns the first slot in the probe sequence, starting from the given
+  // probe number, with a handle e such that match(e) is true. At every
+  // step, the function first tests whether match(e) holds. If this is false,
+  // it evaluates abort(e) to decide whether the search should be aborted,
+  // and in the affirmative returns -1. For every handle e probed except
+  // the last one, the function runs update(e).
+  // The probe parameter is modified as follows. We say a probe to a handle
+  // e is aborting if match(e) is false and abort(e) is true. Then the final
+  // value of probe is one more than the last non-aborting probe during the
+  // call. This is so that that the variable can be used to keep track of
+  // progress across consecutive calls to FindSlot.
+  inline HandleImpl* FindSlot(const UniqueId64x2& hashed_key,
+                              std::function<bool(HandleImpl*)> match,
+                              std::function<bool(HandleImpl*)> stop,
+                              std::function<void(HandleImpl*)> update,
+                              size_t& probe);
 
   // Re-decrement all displacements in probe path starting from beginning
   // until (not including) the given handle
@@ -584,6 +494,33 @@ class HyperClockTable : public BaseClockTable {
   // before releasing it so that it can be provided to this function.
   inline void ReclaimEntryUsage(size_t total_charge);
 
+  // Helper for updating `usage_` for new entry with given `total_charge`
+  // and evicting if needed under strict_capacity_limit=true rules. This
+  // means the operation might fail with Status::MemoryLimit. If
+  // `need_evict_for_occupancy`, then eviction of at least one entry is
+  // required, and the operation should fail if not possible.
+  // NOTE: Otherwise, occupancy_ is not managed in this function
+  inline Status ChargeUsageMaybeEvictStrict(size_t total_charge,
+                                            size_t capacity,
+                                            bool need_evict_for_occupancy);
+
+  // Helper for updating `usage_` for new entry with given `total_charge`
+  // and evicting if needed under strict_capacity_limit=false rules. This
+  // means that updating `usage_` always succeeds even if forced to exceed
+  // capacity. If `need_evict_for_occupancy`, then eviction of at least one
+  // entry is required, and the operation should return false if such eviction
+  // is not possible. `usage_` is not updated in that case. Otherwise, returns
+  // true, indicating success.
+  // NOTE: occupancy_ is not managed in this function
+  inline bool ChargeUsageMaybeEvictNonStrict(size_t total_charge,
+                                             size_t capacity,
+                                             bool need_evict_for_occupancy);
+
+  // Creates a "standalone" handle for returning from an Insert operation that
+  // cannot be completed by actually inserting into the table.
+  // Updates `standalone_usage_` but not `usage_` nor `occupancy_`.
+  inline HandleImpl* StandaloneInsert(const ClockHandleBasicData& proto);
+
   MemoryAllocator* GetAllocator() const { return allocator_; }
 
   // Returns the number of bits used to hash an element in the hash
@@ -604,6 +541,33 @@ class HyperClockTable : public BaseClockTable {
 
   // Array of slots comprising the hash table.
   const std::unique_ptr<HandleImpl[]> array_;
+
+  // From Cache, for deleter
+  MemoryAllocator* const allocator_;
+
+  // A reference to Cache::eviction_callback_
+  const Cache::EvictionCallback& eviction_callback_;
+
+  // A reference to ShardedCacheBase::hash_seed_
+  const uint32_t& hash_seed_;
+
+  // We partition the following members into different cache lines
+  // to avoid false sharing among Lookup, Release, Erase and Insert
+  // operations in ClockCacheShard.
+
+  ALIGN_AS(CACHE_LINE_SIZE)
+  // Clock algorithm sweep pointer.
+  std::atomic<uint64_t> clock_pointer_{};
+
+  ALIGN_AS(CACHE_LINE_SIZE)
+  // Number of elements in the table.
+  std::atomic<size_t> occupancy_{};
+
+  // Memory usage by entries tracked by the cache (including standalone)
+  std::atomic<size_t> usage_{};
+
+  // Part of usage by standalone entries (not in table)
+  std::atomic<size_t> standalone_usage_{};
 };  // class HyperClockTable
 
 // A single shard of sharded cache.
@@ -702,14 +666,9 @@ class ALIGN_AS(CACHE_LINE_SIZE) ClockCacheShard final : public CacheShardBase {
     return Lookup(key, hashed_key);
   }
 
-#ifndef NDEBUG
-  size_t& TEST_MutableOccupancyLimit() const {
-    return table_.TEST_MutableOccupancyLimit();
-  }
   // Acquire/release N references
   void TEST_RefN(HandleImpl* handle, size_t n);
   void TEST_ReleaseN(HandleImpl* handle, size_t n);
-#endif
 
  private:  // data
   Table table_;

cache/lru_cache.cc

@@ -712,12 +712,12 @@ std::shared_ptr<Cache> LRUCacheOptions::MakeSharedCache() const {
   return cache;
 }
 
-std::shared_ptr<RowCache> LRUCacheOptions::MakeSharedRowCache() const {
+std::shared_ptr<GeneralCache> LRUCacheOptions::MakeSharedGeneralCache() const {
   if (secondary_cache) {
-    // Not allowed for a RowCache
+    // Not allowed for a GeneralCache
     return nullptr;
   }
-  // Works while RowCache is an alias for Cache
+  // Works while GeneralCache is an alias for Cache
   return MakeSharedCache();
 }
 }  // namespace ROCKSDB_NAMESPACE

cache/lru_cache_test.cc

@@ -715,62 +715,6 @@ TEST_F(ClockCacheTest, ClockCounterOverflowTest) {
   ASSERT_EQ(val.deleted, 1);
 }
 
-TEST_F(ClockCacheTest, ClockTableFull) {
-  // Force clock cache table to fill up (not usually allowed) in order
-  // to test full probe sequence that is theoretically possible due to
-  // parallel operations
-  NewShard(6, /*strict_capacity_limit*/ false);
-  size_t size = shard_->GetTableAddressCount();
-  ASSERT_LE(size + 3, 256);  // for using char keys
-  // Modify occupancy and capacity limits to attempt insert on full
-  shard_->TEST_MutableOccupancyLimit() = size + 100;
-  shard_->SetCapacity(size + 100);
-
-  DeleteCounter val;
-  std::vector<HandleImpl*> handles;
-  // NOTE: the three extra insertions should create standalone entries
-  for (size_t i = 0; i < size + 3; ++i) {
-    UniqueId64x2 hkey = TestHashedKey(static_cast<char>(i));
-    ASSERT_OK(shard_->Insert(TestKey(hkey), hkey, &val, &kDeleteCounterHelper,
-                             1, &handles.emplace_back(),
-                             Cache::Priority::HIGH));
-  }
-
-  for (size_t i = 0; i < size + 3; ++i) {
-    UniqueId64x2 hkey = TestHashedKey(static_cast<char>(i));
-    HandleImpl* h = shard_->Lookup(TestKey(hkey), hkey);
-    if (i < size) {
-      ASSERT_NE(h, nullptr);
-      shard_->Release(h);
-    } else {
-      // Standalone entries not visible by lookup
-      ASSERT_EQ(h, nullptr);
-    }
-  }
-
-  for (size_t i = 0; i < size + 3; ++i) {
-    ASSERT_NE(handles[i], nullptr);
-    shard_->Release(handles[i]);
-    if (i < size) {
-      // Everything still in cache
-      ASSERT_EQ(val.deleted, 0);
-    } else {
-      // Standalone entries freed on release
-      ASSERT_EQ(val.deleted, i + 1 - size);
-    }
-  }
-
-  for (size_t i = size + 3; i > 0; --i) {
-    UniqueId64x2 hkey = TestHashedKey(static_cast<char>(i - 1));
-    shard_->Erase(TestKey(hkey), hkey);
-    if (i - 1 > size) {
-      ASSERT_EQ(val.deleted, 3);
-    } else {
-      ASSERT_EQ(val.deleted, 3 + size - (i - 1));
-    }
-  }
-}
-
 // This test is mostly to exercise some corner case logic, by forcing two
 // keys to have the same hash, and more
 TEST_F(ClockCacheTest, CollidingInsertEraseTest) {

cache/secondary_cache_adapter.cc

@@ -376,7 +376,7 @@ void CacheWithSecondaryAdapter::WaitAll(AsyncLookupHandle* async_handles,
     for (AsyncLookupHandle* cur : my_pending) {
       my_secondary_handles.push_back(cur->pending_handle);
     }
-    secondary_cache_->WaitAll(std::move(my_secondary_handles));
+    secondary_cache_->WaitAll(my_secondary_handles);
   }
 
   // Process results

db/builder.cc

@@ -15,7 +15,6 @@
 
 #include "db/blob/blob_file_builder.h"
 #include "db/compaction/compaction_iterator.h"
-#include "db/dbformat.h"
 #include "db/event_helpers.h"
 #include "db/internal_stats.h"
 #include "db/merge_helper.h"
@@ -206,38 +205,21 @@ Status BuildTable(
         /*compaction=*/nullptr, compaction_filter.get(),
         /*shutting_down=*/nullptr, db_options.info_log, full_history_ts_low);
 
-    const size_t ts_sz = ucmp->timestamp_size();
-    const bool strip_timestamp =
-        ts_sz > 0 && !ioptions.persist_user_defined_timestamps;
-
-    std::string key_after_flush_buf;
     c_iter.SeekToFirst();
     for (; c_iter.Valid(); c_iter.Next()) {
       const Slice& key = c_iter.key();
       const Slice& value = c_iter.value();
       const ParsedInternalKey& ikey = c_iter.ikey();
-      Slice key_after_flush = key;
-      // If user defined timestamps will be stripped from user key after flush,
-      // the in memory version of the key act logically the same as one with a
-      // minimum timestamp. We update the timestamp here so file boundary and
-      // output validator, block builder all see the effect of the stripping.
-      if (strip_timestamp) {
-        key_after_flush_buf.clear();
-        ReplaceInternalKeyWithMinTimestamp(&key_after_flush_buf, key, ts_sz);
-        key_after_flush = key_after_flush_buf;
-      }
-
-      //  Generate a rolling 64-bit hash of the key and values
-      //  Note :
-      //  Here "key" integrates 'sequence_number'+'kType'+'user key'.
-      s = output_validator.Add(key_after_flush, value);
+      // Generate a rolling 64-bit hash of the key and values
+      // Note :
+      // Here "key" integrates 'sequence_number'+'kType'+'user key'.
+      s = output_validator.Add(key, value);
       if (!s.ok()) {
         break;
       }
-      builder->Add(key_after_flush, value);
+      builder->Add(key, value);
 
-      s = meta->UpdateBoundaries(key_after_flush, value, ikey.sequence,
-                                 ikey.type);
+      s = meta->UpdateBoundaries(key, value, ikey.sequence, ikey.type);
       if (!s.ok()) {
         break;
       }
@@ -262,7 +244,6 @@ Status BuildTable(
            range_del_it->Next()) {
         auto tombstone = range_del_it->Tombstone();
         auto kv = tombstone.Serialize();
-        // TODO(yuzhangyu): handle range deletion for UDT in memtables only.
         builder->Add(kv.first.Encode(), kv.second);
         InternalKey tombstone_end = tombstone.SerializeEndKey();
         meta->UpdateBoundariesForRange(kv.first, tombstone_end, tombstone.seq_,
@@ -373,8 +354,6 @@ Status BuildTable(
       s = *io_status;
     }
 
-    // TODO(yuzhangyu): handle the key copy in the blob when ts should be
-    // stripped.
     if (blob_file_builder) {
       if (s.ok()) {
         s = blob_file_builder->Finish();

db/c.cc

@@ -1057,36 +1057,6 @@ rocksdb_column_family_handle_t* rocksdb_create_column_family(
   return handle;
 }
 
-rocksdb_column_family_handle_t** rocksdb_create_column_families(
-    rocksdb_t* db, const rocksdb_options_t* column_family_options,
-    int num_column_families, const char* const* column_family_names,
-    size_t* lencfs, char** errptr) {
-  std::vector<ColumnFamilyHandle*> handles;
-  std::vector<std::string> names;
-  for (int i = 0; i != num_column_families; ++i) {
-    names.push_back(std::string(column_family_names[i]));
-  }
-  SaveError(errptr, db->rep->CreateColumnFamilies(
-                        ColumnFamilyOptions(column_family_options->rep), names,
-                        &handles));
-
-  *lencfs = handles.size();
-  rocksdb_column_family_handle_t** c_handles =
-      static_cast<rocksdb_column_family_handle_t**>(
-          malloc(sizeof(rocksdb_column_family_handle_t*) * handles.size()));
-  for (size_t i = 0; i != handles.size(); ++i) {
-    c_handles[i] = new rocksdb_column_family_handle_t;
-    c_handles[i]->rep = handles[i];
-  }
-
-  return c_handles;
-}
-
-void rocksdb_create_column_families_destroy(
-    rocksdb_column_family_handle_t** list) {
-  free(list);
-}
-
 rocksdb_column_family_handle_t* rocksdb_create_column_family_with_ttl(
     rocksdb_t* db, const rocksdb_options_t* column_family_options,
     const char* column_family_name, int ttl, char** errptr) {
@@ -3770,21 +3740,16 @@ void rocksdb_options_set_hash_link_list_rep(rocksdb_options_t* opt,
       ROCKSDB_NAMESPACE::NewHashLinkListRepFactory(bucket_count));
 }
 
-void rocksdb_options_set_plain_table_factory(
-    rocksdb_options_t* opt, uint32_t user_key_len, int bloom_bits_per_key,
-    double hash_table_ratio, size_t index_sparseness, size_t huge_page_tlb_size,
-    char encoding_type, unsigned char full_scan_mode,
-    unsigned char store_index_in_file) {
+void rocksdb_options_set_plain_table_factory(rocksdb_options_t* opt,
+                                             uint32_t user_key_len,
+                                             int bloom_bits_per_key,
+                                             double hash_table_ratio,
+                                             size_t index_sparseness) {
   ROCKSDB_NAMESPACE::PlainTableOptions options;
   options.user_key_len = user_key_len;
   options.bloom_bits_per_key = bloom_bits_per_key;
   options.hash_table_ratio = hash_table_ratio;
   options.index_sparseness = index_sparseness;
-  options.huge_page_tlb_size = huge_page_tlb_size;
-  options.encoding_type =
-      static_cast<ROCKSDB_NAMESPACE::EncodingType>(encoding_type);
-  options.full_scan_mode = full_scan_mode;
-  options.store_index_in_file = store_index_in_file;
 
   ROCKSDB_NAMESPACE::TableFactory* factory =
       ROCKSDB_NAMESPACE::NewPlainTableFactory(options);
@@ -5623,23 +5588,6 @@ int rocksdb_transactiondb_property_int(rocksdb_transactiondb_t* db,
   }
 }
 
-rocksdb_t* rocksdb_transactiondb_get_base_db(
-    rocksdb_transactiondb_t* txn_db) {
-  DB* base_db = txn_db->rep->GetBaseDB();
-
-  if (base_db != nullptr) {
-    rocksdb_t* result = new rocksdb_t;
-    result->rep = base_db;
-    return result;
-  }
-
-  return nullptr;
-}
-
-void rocksdb_transactiondb_close_base_db(rocksdb_t* base_db) {
-  delete base_db;
-}
-
 rocksdb_transaction_t* rocksdb_transaction_begin(
     rocksdb_transactiondb_t* txn_db,
     const rocksdb_writeoptions_t* write_options,

db/c_test.c

@@ -1674,8 +1674,7 @@ int main(int argc, char** argv) {
     rocksdb_options_set_prefix_extractor(
         options, rocksdb_slicetransform_create_fixed_prefix(3));
     rocksdb_options_set_hash_skip_list_rep(options, 5000, 4, 4);
-    rocksdb_options_set_plain_table_factory(options, 4, 10, 0.75, 16, 0, 0, 0,
-                                            0);
+    rocksdb_options_set_plain_table_factory(options, 4, 10, 0.75, 16);
     rocksdb_options_set_allow_concurrent_memtable_write(options, 0);
 
     db = rocksdb_open(options, dbname, &err);
@@ -3122,17 +3121,6 @@ int main(int argc, char** argv) {
     CheckTxnDBGetCF(txn_db, roptions, cfh, "cf_foo", NULL);
     CheckTxnDBPinGetCF(txn_db, roptions, cfh, "cf_foo", NULL);
 
-
-    //memory usage
-    rocksdb_t* base_db = rocksdb_transactiondb_get_base_db(txn_db);
-    rocksdb_memory_consumers_t* consumers = rocksdb_memory_consumers_create();
-    rocksdb_memory_consumers_add_db(consumers, base_db);
-    rocksdb_memory_usage_t* usage = rocksdb_approximate_memory_usage_create(consumers, &err);
-    CheckNoError(err);
-    rocksdb_approximate_memory_usage_destroy(usage);
-    rocksdb_memory_consumers_destroy(consumers);
-    rocksdb_transactiondb_close_base_db(base_db);
-
     // flush
     rocksdb_flushoptions_t* flush_options = rocksdb_flushoptions_create();
     rocksdb_flushoptions_set_wait(flush_options, 1);
@@ -3388,19 +3376,8 @@ int main(int argc, char** argv) {
     rocksdb_put(db, woptions, "key", 3, "value", 5, &err);
     CheckNoError(err);
     rocksdb_column_family_handle_t *cfh1, *cfh2;
-    char** list_const_cf_names = (char**)malloc(2 * sizeof(char*));
-    list_const_cf_names[0] = "txn_db_cf1";
-    list_const_cf_names[1] = "txn_db_cf2";
-    size_t cflen;
-    rocksdb_column_family_handle_t** list_cfh = rocksdb_create_column_families(
-        db, db_options, 2, (const char* const*)list_const_cf_names, &cflen,
-        &err);
-    free(list_const_cf_names);
-    CheckNoError(err);
-    assert(cflen == 2);
-    cfh1 = list_cfh[0];
-    cfh2 = list_cfh[1];
-    rocksdb_create_column_families_destroy(list_cfh);
+    cfh1 = rocksdb_create_column_family(db, db_options, "txn_db_cf1", &err);
+    cfh2 = rocksdb_create_column_family(db, db_options, "txn_db_cf2", &err);
     txn = rocksdb_optimistictransaction_begin(otxn_db, woptions, otxn_options,
                                               NULL);
     rocksdb_transaction_put_cf(txn, cfh1, "key_cf1", 7, "val_cf1", 7, &err);

db/column_family.cc

@@ -382,8 +382,8 @@ ColumnFamilyOptions SanitizeOptions(const ImmutableDBOptions& db_options,
 
   const uint64_t kAdjustedTtl = 30 * 24 * 60 * 60;
   if (result.ttl == kDefaultTtl) {
-    if (is_block_based_table) {
-      // For FIFO, max_open_files is checked in ValidateOptions().
+    if (is_block_based_table &&
+        result.compaction_style != kCompactionStyleFIFO) {
       result.ttl = kAdjustedTtl;
     } else {
       result.ttl = 0;
@@ -403,12 +403,16 @@ ColumnFamilyOptions SanitizeOptions(const ImmutableDBOptions& db_options,
       result.periodic_compaction_seconds = kAdjustedPeriodicCompSecs;
     }
   } else {
-    if (result.periodic_compaction_seconds != kDefaultPeriodicCompSecs &&
-        result.periodic_compaction_seconds > 0) {
-      ROCKS_LOG_WARN(
-          db_options.info_log.get(),
-          "periodic_compaction_seconds does not support FIFO compaction. You"
-          "may want to set option TTL instead.");
+    // result.compaction_style == kCompactionStyleFIFO
+    if (result.ttl == 0) {
+      if (is_block_based_table) {
+        if (result.periodic_compaction_seconds == kDefaultPeriodicCompSecs) {
+          result.periodic_compaction_seconds = kAdjustedPeriodicCompSecs;
+        }
+        result.ttl = result.periodic_compaction_seconds;
+      }
+    } else if (result.periodic_compaction_seconds != 0) {
+      result.ttl = std::min(result.ttl, result.periodic_compaction_seconds);
     }
   }
 

db/compaction/compaction.cc

@@ -23,13 +23,14 @@ namespace ROCKSDB_NAMESPACE {
 const uint64_t kRangeTombstoneSentinel =
     PackSequenceAndType(kMaxSequenceNumber, kTypeRangeDeletion);
 
-int sstableKeyCompare(const Comparator* uc, const Slice& a, const Slice& b) {
-  auto c = uc->CompareWithoutTimestamp(ExtractUserKey(a), ExtractUserKey(b));
+int sstableKeyCompare(const Comparator* user_cmp, const InternalKey& a,
+                      const InternalKey& b) {
+  auto c = user_cmp->CompareWithoutTimestamp(a.user_key(), b.user_key());
   if (c != 0) {
     return c;
   }
-  auto a_footer = ExtractInternalKeyFooter(a);
-  auto b_footer = ExtractInternalKeyFooter(b);
+  auto a_footer = ExtractInternalKeyFooter(a.Encode());
+  auto b_footer = ExtractInternalKeyFooter(b.Encode());
   if (a_footer == kRangeTombstoneSentinel) {
     if (b_footer != kRangeTombstoneSentinel) {
       return -1;

db/compaction/compaction.h

@@ -31,19 +31,8 @@ namespace ROCKSDB_NAMESPACE {
 // that key never appears in the database. We don't want adjacent sstables to
 // be considered overlapping if they are separated by the range tombstone
 // sentinel.
-int sstableKeyCompare(const Comparator* user_cmp, const Slice&, const Slice&);
-inline int sstableKeyCompare(const Comparator* user_cmp, const Slice& a,
-                             const InternalKey& b) {
-  return sstableKeyCompare(user_cmp, a, b.Encode());
-}
-inline int sstableKeyCompare(const Comparator* user_cmp, const InternalKey& a,
-                             const Slice& b) {
-  return sstableKeyCompare(user_cmp, a.Encode(), b);
-}
-inline int sstableKeyCompare(const Comparator* user_cmp, const InternalKey& a,
-                             const InternalKey& b) {
-  return sstableKeyCompare(user_cmp, a.Encode(), b.Encode());
-}
+int sstableKeyCompare(const Comparator* user_cmp, const InternalKey& a,
+                      const InternalKey& b);
 int sstableKeyCompare(const Comparator* user_cmp, const InternalKey* a,
                       const InternalKey& b);
 int sstableKeyCompare(const Comparator* user_cmp, const InternalKey& a,

db/compaction/compaction_job.cc

@@ -1096,17 +1096,17 @@ void CompactionJob::ProcessKeyValueCompaction(SubcompactionState* sub_compact) {
   // GenSubcompactionBoundaries doesn't strip away the timestamp.
   size_t ts_sz = cfd->user_comparator()->timestamp_size();
   if (start.has_value()) {
-    read_options.iterate_lower_bound = &(*start);
+    read_options.iterate_lower_bound = &start.value();
     if (ts_sz > 0) {
-      start_without_ts = StripTimestampFromUserKey(*start, ts_sz);
-      read_options.iterate_lower_bound = &(*start_without_ts);
+      start_without_ts = StripTimestampFromUserKey(start.value(), ts_sz);
+      read_options.iterate_lower_bound = &start_without_ts.value();
     }
   }
   if (end.has_value()) {
-    read_options.iterate_upper_bound = &(*end);
+    read_options.iterate_upper_bound = &end.value();
     if (ts_sz > 0) {
-      end_without_ts = StripTimestampFromUserKey(*end, ts_sz);
-      read_options.iterate_upper_bound = &(*end_without_ts);
+      end_without_ts = StripTimestampFromUserKey(end.value(), ts_sz);
+      read_options.iterate_upper_bound = &end_without_ts.value();
     }
   }
 
@@ -1138,7 +1138,8 @@ void CompactionJob::ProcessKeyValueCompaction(SubcompactionState* sub_compact) {
   }
 
   if (start.has_value()) {
-    start_ikey.SetInternalKey(*start, kMaxSequenceNumber, kValueTypeForSeek);
+    start_ikey.SetInternalKey(start.value(), kMaxSequenceNumber,
+                              kValueTypeForSeek);
     if (ts_sz > 0) {
       start_ikey.UpdateInternalKey(kMaxSequenceNumber, kValueTypeForSeek,
                                    &ts_slice);
@@ -1147,7 +1148,7 @@ void CompactionJob::ProcessKeyValueCompaction(SubcompactionState* sub_compact) {
     start_user_key = start_ikey.GetUserKey();
   }
   if (end.has_value()) {
-    end_ikey.SetInternalKey(*end, kMaxSequenceNumber, kValueTypeForSeek);
+    end_ikey.SetInternalKey(end.value(), kMaxSequenceNumber, kValueTypeForSeek);
     if (ts_sz > 0) {
       end_ikey.UpdateInternalKey(kMaxSequenceNumber, kValueTypeForSeek,
                                  &ts_slice);
@@ -1288,8 +1289,8 @@ void CompactionJob::ProcessKeyValueCompaction(SubcompactionState* sub_compact) {
   while (status.ok() && !cfd->IsDropped() && c_iter->Valid()) {
     // Invariant: c_iter.status() is guaranteed to be OK if c_iter->Valid()
     // returns true.
-    assert(!end.has_value() ||
-           cfd->user_comparator()->Compare(c_iter->user_key(), *end) < 0);
+    assert(!end.has_value() || cfd->user_comparator()->Compare(
+                                   c_iter->user_key(), end.value()) < 0);
 
     if (c_iter_stats.num_input_records % kRecordStatsEvery ==
         kRecordStatsEvery - 1) {
@@ -1817,10 +1818,10 @@ Status CompactionJob::OpenCompactionOutputFile(SubcompactionState* sub_compact,
   uint64_t current_time = static_cast<uint64_t>(temp_current_time);
   InternalKey tmp_start, tmp_end;
   if (sub_compact->start.has_value()) {
-    tmp_start.SetMinPossibleForUserKey(*(sub_compact->start));
+    tmp_start.SetMinPossibleForUserKey(sub_compact->start.value());
   }
   if (sub_compact->end.has_value()) {
-    tmp_end.SetMinPossibleForUserKey(*(sub_compact->end));
+    tmp_end.SetMinPossibleForUserKey(sub_compact->end.value());
   }
   uint64_t oldest_ancester_time =
       sub_compact->compaction->MinInputFileOldestAncesterTime(

db/compaction/compaction_outputs.cc

@@ -127,6 +127,11 @@ size_t CompactionOutputs::UpdateGrandparentBoundaryInfo(
   if (grandparents.empty()) {
     return curr_key_boundary_switched_num;
   }
+  assert(!internal_key.empty());
+  InternalKey ikey;
+  ikey.DecodeFrom(internal_key);
+  assert(ikey.Valid());
+
   const Comparator* ucmp = compaction_->column_family_data()->user_comparator();
 
   // Move the grandparent_index_ to the file containing the current user_key.
@@ -134,7 +139,7 @@ size_t CompactionOutputs::UpdateGrandparentBoundaryInfo(
   // index points to the last file containing the key.
   while (grandparent_index_ < grandparents.size()) {
     if (being_grandparent_gap_) {
-      if (sstableKeyCompare(ucmp, internal_key,
+      if (sstableKeyCompare(ucmp, ikey,
                             grandparents[grandparent_index_]->smallest) < 0) {
         break;
       }
@@ -147,13 +152,13 @@ size_t CompactionOutputs::UpdateGrandparentBoundaryInfo(
       being_grandparent_gap_ = false;
     } else {
       int cmp_result = sstableKeyCompare(
-          ucmp, internal_key, grandparents[grandparent_index_]->largest);
+          ucmp, ikey, grandparents[grandparent_index_]->largest);
       // If it's same key, make sure grandparent_index_ is pointing to the last
       // one.
       if (cmp_result < 0 ||
           (cmp_result == 0 &&
            (grandparent_index_ == grandparents.size() - 1 ||
-            sstableKeyCompare(ucmp, internal_key,
+            sstableKeyCompare(ucmp, ikey,
                               grandparents[grandparent_index_ + 1]->smallest) <
                 0))) {
         break;

db/compaction/compaction_outputs.h

@@ -206,10 +206,10 @@ class CompactionOutputs {
       // We may only split the output when the cursor is in the range. Split
       if ((!end.has_value() ||
            icmp->user_comparator()->Compare(
-               ExtractUserKey(output_split_key->Encode()), *end) < 0) &&
-          (!start.has_value() ||
-           icmp->user_comparator()->Compare(
-               ExtractUserKey(output_split_key->Encode()), *start) > 0)) {
+               ExtractUserKey(output_split_key->Encode()), end.value()) < 0) &&
+          (!start.has_value() || icmp->user_comparator()->Compare(
+                                     ExtractUserKey(output_split_key->Encode()),
+                                     start.value()) > 0)) {
         local_output_split_key_ = output_split_key;
       }
     }

db/compaction/compaction_picker_test.cc

@@ -1968,15 +1968,6 @@ TEST_F(CompactionPickerTest, OverlappingUserKeys11) {
   ASSERT_EQ(7U, compaction->input(1, 0)->fd.GetNumber());
 }
 
-TEST_F(CompactionPickerTest, FileTtlBoosterLargeNumLevels) {
-  const uint64_t kCurrentTime = 1000000;
-  FileTtlBooster booster(kCurrentTime, /*ttl=*/2048,
-                         /*num_non_empty_levels=*/100, /*level=*/1);
-  FileMetaData meta;
-  meta.oldest_ancester_time = kCurrentTime - 1023;
-  ASSERT_EQ(1, booster.GetBoostScore(&meta));
-}
-
 TEST_F(CompactionPickerTest, FileTtlBooster) {
   // Set TTL to 2048
   // TTL boosting for all levels starts at 1024,

db/compaction/file_pri.h

@@ -53,10 +53,8 @@ class FileTtlBooster {
       enabled_ = true;
       uint64_t all_boost_start_age = ttl / 2;
       uint64_t all_boost_age_range = (ttl / 32) * 31 - all_boost_start_age;
-      // TODO(cbi): more reasonable algorithm that gives different values
-      //   when num_non_empty_levels - level - 1 > 63.
       uint64_t boost_age_range =
-          all_boost_age_range >> std::min(63, num_non_empty_levels - level - 1);
+          all_boost_age_range >> (num_non_empty_levels - level - 1);
       boost_age_start_ = all_boost_start_age + boost_age_range;
       const uint64_t kBoostRatio = 16;
       // prevent 0 value to avoid divide 0 error.

db/db_compaction_test.cc

@@ -4600,7 +4600,6 @@ TEST_F(DBCompactionTest, LevelTtlCascadingCompactions) {
               PutVarint64(encoded_fieled, 0);
             }
           });
-      ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing();
 
       options.env = env_;
 
@@ -4617,6 +4616,7 @@ TEST_F(DBCompactionTest, LevelTtlCascadingCompactions) {
               ttl_compactions++;
             }
           });
+      ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing();
 
       // Add two L6 files with key ranges: [1 .. 100], [101 .. 200].
       Random rnd(301);
@@ -4748,7 +4748,6 @@ TEST_F(DBCompactionTest, LevelPeriodicCompaction) {
               PutVarint64(encoded_fieled, 0);
             }
           });
-      ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing();
 
       options.env = env_;
 
@@ -4765,6 +4764,7 @@ TEST_F(DBCompactionTest, LevelPeriodicCompaction) {
               periodic_compactions++;
             }
           });
+      ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing();
 
       Random rnd(301);
       for (int i = 0; i < kNumLevelFiles; ++i) {

db/db_impl/db_impl_compaction_flush.cc

@@ -852,8 +852,8 @@ void DBImpl::NotifyOnFlushBegin(ColumnFamilyData* cfd, FileMetaData* file_meta,
     }
   }
   mutex_.Lock();
-  // no need to signal bg_cv_ as it will be signaled at the end of the
-  // flush process.
+// no need to signal bg_cv_ as it will be signaled at the end of the
+// flush process.
 }
 
 void DBImpl::NotifyOnFlushCompleted(
@@ -2775,7 +2775,7 @@ ColumnFamilyData* DBImpl::PopFirstFromCompactionQueue() {
 
 DBImpl::FlushRequest DBImpl::PopFirstFromFlushQueue() {
   assert(!flush_queue_.empty());
-  FlushRequest flush_req = std::move(flush_queue_.front());
+  FlushRequest flush_req = flush_queue_.front();
   flush_queue_.pop_front();
   if (!immutable_db_options_.atomic_flush) {
     assert(flush_req.cfd_to_max_mem_id_to_persist.size() == 1);
@@ -2970,12 +2970,14 @@ Status DBImpl::BackgroundFlush(bool* made_progress, JobContext* job_context,
   autovector<ColumnFamilyData*> column_families_not_to_flush;
   while (!flush_queue_.empty()) {
     // This cfd is already referenced
-    auto [flush_reason, cfd_to_max_mem_id_to_persist] =
-        PopFirstFromFlushQueue();
+    const FlushRequest& flush_req = PopFirstFromFlushQueue();
+    FlushReason flush_reason = flush_req.flush_reason;
     superversion_contexts.clear();
-    superversion_contexts.reserve(cfd_to_max_mem_id_to_persist.size());
+    superversion_contexts.reserve(
+        flush_req.cfd_to_max_mem_id_to_persist.size());
 
-    for (const auto& [cfd, max_memtable_id] : cfd_to_max_mem_id_to_persist) {
+    for (const auto& iter : flush_req.cfd_to_max_mem_id_to_persist) {
+      ColumnFamilyData* cfd = iter.first;
       if (cfd->GetMempurgeUsed()) {
         // If imm() contains silent memtables (e.g.: because
         // MemPurge was activated), requesting a flush will
@@ -2989,7 +2991,7 @@ Status DBImpl::BackgroundFlush(bool* made_progress, JobContext* job_context,
         continue;
       }
       superversion_contexts.emplace_back(SuperVersionContext(true));
-      bg_flush_args.emplace_back(cfd, max_memtable_id,
+      bg_flush_args.emplace_back(cfd, iter.second,
                                  &(superversion_contexts.back()), flush_reason);
     }
     if (!bg_flush_args.empty()) {

db/db_impl/db_impl_open.cc

@@ -1655,7 +1655,6 @@ Status DBImpl::WriteLevel0TableForRecovery(int job_id, ColumnFamilyData* cfd,
       Version* version = cfd->current();
       version->Ref();
       const ReadOptions read_option(Env::IOActivity::kDBOpen);
-      uint64_t num_input_entries = 0;
       s = BuildTable(
           dbname_, versions_.get(), immutable_db_options_, tboptions,
           file_options_for_compaction_, read_option, cfd->table_cache(),
@@ -1665,8 +1664,7 @@ Status DBImpl::WriteLevel0TableForRecovery(int job_id, ColumnFamilyData* cfd,
           io_tracer_, BlobFileCreationReason::kRecovery,
           empty_seqno_time_mapping, &event_logger_, job_id, Env::IO_HIGH,
           nullptr /* table_properties */, write_hint,
-          nullptr /*full_history_ts_low*/, &blob_callback_, version,
-          &num_input_entries);
+          nullptr /*full_history_ts_low*/, &blob_callback_, version);
       version->Unref();
       LogFlush(immutable_db_options_.info_log);
       ROCKS_LOG_DEBUG(immutable_db_options_.info_log,
@@ -1680,19 +1678,6 @@ Status DBImpl::WriteLevel0TableForRecovery(int job_id, ColumnFamilyData* cfd,
       if (!io_s.ok() && s.ok()) {
         s = io_s;
       }
-
-      uint64_t total_num_entries = mem->num_entries();
-      if (s.ok() && total_num_entries != num_input_entries) {
-        std::string msg = "Expected " + std::to_string(total_num_entries) +
-                          " entries in memtable, but read " +
-                          std::to_string(num_input_entries);
-        ROCKS_LOG_WARN(immutable_db_options_.info_log,
-                       "[%s] [JOB %d] Level-0 flush during recover: %s",
-                       cfd->GetName().c_str(), job_id, msg.c_str());
-        if (immutable_db_options_.flush_verify_memtable_count) {
-          s = Status::Corruption(msg);
-        }
-      }
     }
   }
   ReleaseFileNumberFromPendingOutputs(pending_outputs_inserted_elem);

db/db_options_test.cc

@@ -880,13 +880,9 @@ TEST_F(DBOptionsTest, SanitizeFIFOPeriodicCompaction) {
   Options options;
   options.compaction_style = kCompactionStyleFIFO;
   options.env = CurrentOptions().env;
-  // Default value allows RocksDB to set ttl to 30 days.
-  ASSERT_EQ(30 * 24 * 60 * 60, dbfull()->GetOptions().ttl);
-
-  // Disable
   options.ttl = 0;
   Reopen(options);
-  ASSERT_EQ(0, dbfull()->GetOptions().ttl);
+  ASSERT_EQ(30 * 24 * 60 * 60, dbfull()->GetOptions().ttl);
 
   options.ttl = 100;
   Reopen(options);
@@ -896,12 +892,15 @@ TEST_F(DBOptionsTest, SanitizeFIFOPeriodicCompaction) {
   Reopen(options);
   ASSERT_EQ(100 * 24 * 60 * 60, dbfull()->GetOptions().ttl);
 
-  // periodic_compaction_seconds should have no effect
-  // on FIFO compaction.
+  options.ttl = 200;
+  options.periodic_compaction_seconds = 300;
+  Reopen(options);
+  ASSERT_EQ(200, dbfull()->GetOptions().ttl);
+
   options.ttl = 500;
   options.periodic_compaction_seconds = 300;
   Reopen(options);
-  ASSERT_EQ(500, dbfull()->GetOptions().ttl);
+  ASSERT_EQ(300, dbfull()->GetOptions().ttl);
 }
 
 TEST_F(DBOptionsTest, SetFIFOCompactionOptions) {

db/db_test.cc

@@ -6902,16 +6902,16 @@ TEST_F(DBTest, RowCache) {
   options.statistics = ROCKSDB_NAMESPACE::CreateDBStatistics();
   LRUCacheOptions cache_options;
   cache_options.capacity = 8192;
-  options.row_cache = cache_options.MakeSharedRowCache();
+  options.row_cache = cache_options.MakeSharedGeneralCache();
   // BEGIN check that Cache classes as aliases of each other.
-  // Currently, RowCache and BlockCache are aliases for Cache.
+  // Currently, GeneralCache and BlockCache are aliases for Cache.
   // This is expected to change (carefully, intentionally)
-  std::shared_ptr<RowCache> row_cache = options.row_cache;
-  std::shared_ptr<Cache> cache = row_cache;
-  std::shared_ptr<BlockCache> block_cache = row_cache;
-  row_cache = cache;
+  std::shared_ptr<GeneralCache> general_cache = options.row_cache;
+  std::shared_ptr<Cache> cache = general_cache;
+  std::shared_ptr<BlockCache> block_cache = general_cache;
+  general_cache = cache;
   block_cache = cache;
-  row_cache = block_cache;
+  general_cache = block_cache;
   cache = block_cache;
   // END check that Cache classes as aliases of each other.
   DestroyAndReopen(options);

db/db_wal_test.cc

@@ -311,32 +311,21 @@ TEST_F(DBWALTest, Recover) {
   } while (ChangeWalOptions());
 }
 
-class DBWALTestWithTimestamp
-    : public DBBasicTestWithTimestampBase,
-      public testing::WithParamInterface<test::UserDefinedTimestampTestMode> {
+class DBWALTestWithTimestamp : public DBBasicTestWithTimestampBase {
  public:
   DBWALTestWithTimestamp()
       : DBBasicTestWithTimestampBase("db_wal_test_with_timestamp") {}
 
-  void SetUp() override {
-    persist_udt_ = test::ShouldPersistUDT(GetParam());
-    DBBasicTestWithTimestampBase::SetUp();
-  }
-
   Status CreateAndReopenWithCFWithTs(const std::vector<std::string>& cfs,
-                                     const Options& options,
-                                     bool avoid_flush_during_recovery = false) {
+                                     const Options& options) {
     CreateColumnFamilies(cfs, options);
-    return ReopenColumnFamiliesWithTs(cfs, options,
-                                      avoid_flush_during_recovery);
+    return ReopenColumnFamiliesWithTs(cfs, options);
   }
 
   Status ReopenColumnFamiliesWithTs(const std::vector<std::string>& cfs,
-                                    Options ts_options,
-                                    bool avoid_flush_during_recovery = false) {
+                                    Options ts_options) {
     Options default_options = CurrentOptions();
     default_options.create_if_missing = false;
-    default_options.avoid_flush_during_recovery = avoid_flush_during_recovery;
     ts_options.create_if_missing = false;
 
     std::vector<Options> cf_options(cfs.size(), ts_options);
@@ -354,88 +343,46 @@ class DBWALTestWithTimestamp
   }
 
   void CheckGet(const ReadOptions& read_opts, uint32_t cf, const Slice& key,
-                const std::string& expected_value,
-                const std::string& expected_ts) {
+                const std::string& expected_value) {
     std::string actual_value;
-    std::string actual_ts;
-    ASSERT_OK(
-        db_->Get(read_opts, handles_[cf], key, &actual_value, &actual_ts));
+    ASSERT_OK(db_->Get(read_opts, handles_[cf], key, &actual_value));
     ASSERT_EQ(expected_value, actual_value);
-    ASSERT_EQ(expected_ts, actual_ts);
   }
-
- protected:
-  bool persist_udt_;
 };
 
-TEST_P(DBWALTestWithTimestamp, RecoverAndNoFlush) {
-  // Set up the option that enables user defined timestmp size.
-  std::string ts1 = Timestamp(1, 0);
-  const size_t kTimestampSize = ts1.size();
-  TestComparator test_cmp(kTimestampSize);
-  Options ts_options;
-  ts_options.create_if_missing = true;
-  ts_options.comparator = &test_cmp;
-  // Test that user-defined timestamps are recovered from WAL regardless of
-  // the value of this flag because UDTs are saved in WAL nonetheless.
-  // We however need to explicitly disable flush during recovery by setting
-  // `avoid_flush_during_recovery=true` so that we can avoid timestamps getting
-  // stripped when the `persist_user_defined_timestamps` flag is false, so that
-  // all written timestamps are available for testing user-defined time travel
-  // read.
-  ts_options.persist_user_defined_timestamps = persist_udt_;
-  bool avoid_flush_during_recovery = true;
-
-  ReadOptions read_opts;
-  do {
-    Slice ts_slice = ts1;
-    read_opts.timestamp = &ts_slice;
-    ASSERT_OK(CreateAndReopenWithCFWithTs({"pikachu"}, ts_options,
-                                          avoid_flush_during_recovery));
-    ASSERT_EQ(GetNumberOfSstFilesForColumnFamily(db_, "pikachu"), 0U);
-    ASSERT_OK(Put(1, "foo", ts1, "v1"));
-    ASSERT_OK(Put(1, "baz", ts1, "v5"));
-
-    ASSERT_OK(ReopenColumnFamiliesWithTs({"pikachu"}, ts_options,
-                                         avoid_flush_during_recovery));
-    ASSERT_EQ(GetNumberOfSstFilesForColumnFamily(db_, "pikachu"), 0U);
-    // Do a timestamped read with ts1 after second reopen.
-    CheckGet(read_opts, 1, "foo", "v1", ts1);
-    CheckGet(read_opts, 1, "baz", "v5", ts1);
-
-    // Write more value versions for key "foo" and "bar" before and after second
-    // reopen.
-    std::string ts2 = Timestamp(2, 0);
-    ASSERT_OK(Put(1, "bar", ts2, "v2"));
-    ASSERT_OK(Put(1, "foo", ts2, "v3"));
-
-    ASSERT_OK(ReopenColumnFamiliesWithTs({"pikachu"}, ts_options,
-                                         avoid_flush_during_recovery));
-    ASSERT_EQ(GetNumberOfSstFilesForColumnFamily(db_, "pikachu"), 0U);
-    std::string ts3 = Timestamp(3, 0);
-    ASSERT_OK(Put(1, "foo", ts3, "v4"));
-
-    // Do a timestamped read with ts1 after third reopen.
-    CheckGet(read_opts, 1, "foo", "v1", ts1);
-    std::string value;
-    ASSERT_TRUE(db_->Get(read_opts, handles_[1], "bar", &value).IsNotFound());
-    CheckGet(read_opts, 1, "baz", "v5", ts1);
-
-    // Do a timestamped read with ts2 after third reopen.
-    ts_slice = ts2;
-    CheckGet(read_opts, 1, "foo", "v3", ts2);
-    CheckGet(read_opts, 1, "bar", "v2", ts2);
-    CheckGet(read_opts, 1, "baz", "v5", ts1);
-
-    // Do a timestamped read with ts3 after third reopen.
-    ts_slice = ts3;
-    CheckGet(read_opts, 1, "foo", "v4", ts3);
-    CheckGet(read_opts, 1, "bar", "v2", ts2);
-    CheckGet(read_opts, 1, "baz", "v5", ts1);
-  } while (ChangeWalOptions());
-}
-
-TEST_P(DBWALTestWithTimestamp, RecoverInconsistentTimestamp) {
+TEST_F(DBWALTestWithTimestamp, Recover) {
+  // Set up the option that enables user defined timestmp size.
+  std::string ts = Timestamp(1, 0);
+  const size_t kTimestampSize = ts.size();
+  TestComparator test_cmp(kTimestampSize);
+  Options ts_options;
+  ts_options.create_if_missing = true;
+  ts_options.comparator = &test_cmp;
+
+  ReadOptions read_opts;
+  Slice ts_slice = ts;
+  read_opts.timestamp = &ts_slice;
+  do {
+    ASSERT_OK(CreateAndReopenWithCFWithTs({"pikachu"}, ts_options));
+    ASSERT_OK(Put(1, "foo", ts, "v1"));
+    ASSERT_OK(Put(1, "baz", ts, "v5"));
+
+    ASSERT_OK(ReopenColumnFamiliesWithTs({"pikachu"}, ts_options));
+    CheckGet(read_opts, 1, "foo", "v1");
+    CheckGet(read_opts, 1, "baz", "v5");
+    ASSERT_OK(Put(1, "bar", ts, "v2"));
+    ASSERT_OK(Put(1, "foo", ts, "v3"));
+
+    ASSERT_OK(ReopenColumnFamiliesWithTs({"pikachu"}, ts_options));
+    CheckGet(read_opts, 1, "foo", "v3");
+    ASSERT_OK(Put(1, "foo", ts, "v4"));
+    CheckGet(read_opts, 1, "foo", "v4");
+    CheckGet(read_opts, 1, "bar", "v2");
+    CheckGet(read_opts, 1, "baz", "v5");
+  } while (ChangeWalOptions());
+}
+
+TEST_F(DBWALTestWithTimestamp, RecoverInconsistentTimestamp) {
   // Set up the option that enables user defined timestmp size.
   std::string ts = Timestamp(1, 0);
   const size_t kTimestampSize = ts.size();
@@ -443,72 +390,17 @@ TEST_P(DBWALTestWithTimestamp, RecoverInconsistentTimestamp) {
   Options ts_options;
   ts_options.create_if_missing = true;
   ts_options.comparator = &test_cmp;
-  ts_options.persist_user_defined_timestamps = persist_udt_;
 
   ASSERT_OK(CreateAndReopenWithCFWithTs({"pikachu"}, ts_options));
   ASSERT_OK(Put(1, "foo", ts, "v1"));
   ASSERT_OK(Put(1, "baz", ts, "v5"));
 
-  // In real use cases, switching to a different user comparator is prohibited
-  // by a sanity check during DB open that does a user comparator name
-  // comparison. This test mocked and bypassed that sanity check because the
-  // before and after user comparator are both named "TestComparator". This is
-  // to test the user-defined timestamp recovery logic for WAL files have
-  // the intended consistency check.
-  // `HandleWriteBatchTimestampSizeDifference` in udt_util.h has more details.
   TestComparator diff_test_cmp(kTimestampSize + 1);
   ts_options.comparator = &diff_test_cmp;
   ASSERT_TRUE(
       ReopenColumnFamiliesWithTs({"pikachu"}, ts_options).IsInvalidArgument());
 }
 
-TEST_P(DBWALTestWithTimestamp, RecoverAndFlush) {
-  // Set up the option that enables user defined timestamp size.
-  std::string min_ts = Timestamp(0, 0);
-  std::string write_ts = Timestamp(1, 0);
-  const size_t kTimestampSize = write_ts.size();
-  TestComparator test_cmp(kTimestampSize);
-  Options ts_options;
-  ts_options.create_if_missing = true;
-  ts_options.comparator = &test_cmp;
-  ts_options.persist_user_defined_timestamps = persist_udt_;
-
-  std::string smallest_ukey_without_ts = "baz";
-  std::string largest_ukey_without_ts = "foo";
-
-  ASSERT_OK(CreateAndReopenWithCFWithTs({"pikachu"}, ts_options));
-  // No flush, no sst files, because of no data.
-  ASSERT_EQ(GetNumberOfSstFilesForColumnFamily(db_, "pikachu"), 0U);
-  ASSERT_OK(Put(1, largest_ukey_without_ts, write_ts, "v1"));
-  ASSERT_OK(Put(1, smallest_ukey_without_ts, write_ts, "v5"));
-
-  ASSERT_OK(ReopenColumnFamiliesWithTs({"pikachu"}, ts_options));
-  // Memtable recovered from WAL flushed because `avoid_flush_during_recovery`
-  // defaults to false, created one L0 file.
-  ASSERT_EQ(GetNumberOfSstFilesForColumnFamily(db_, "pikachu"), 1U);
-
-  std::vector<std::vector<FileMetaData>> level_to_files;
-  dbfull()->TEST_GetFilesMetaData(handles_[1], &level_to_files);
-  ASSERT_GT(level_to_files.size(), 1);
-  // L0 only has one SST file.
-  ASSERT_EQ(level_to_files[0].size(), 1);
-  auto meta = level_to_files[0][0];
-  if (persist_udt_) {
-    ASSERT_EQ(smallest_ukey_without_ts + write_ts, meta.smallest.user_key());
-    ASSERT_EQ(largest_ukey_without_ts + write_ts, meta.largest.user_key());
-  } else {
-    ASSERT_EQ(smallest_ukey_without_ts + min_ts, meta.smallest.user_key());
-    ASSERT_EQ(largest_ukey_without_ts + min_ts, meta.largest.user_key());
-  }
-}
-
-// Param 0: test mode for the user-defined timestamp feature
-INSTANTIATE_TEST_CASE_P(
-    DBWALTestWithTimestamp, DBWALTestWithTimestamp,
-    ::testing::Values(
-        test::UserDefinedTimestampTestMode::kStripUserDefinedTimestamp,
-        test::UserDefinedTimestampTestMode::kNormal));
-
 TEST_F(DBWALTest, RecoverWithTableHandle) {
   do {
     Options options = CurrentOptions();

db/db_with_timestamp_basic_test.cc

@@ -3273,76 +3273,6 @@ TEST_F(UpdateFullHistoryTsLowTest, ConcurrentUpdate) {
   Close();
 }
 
-// Tests the effect of flag `persist_user_defined_timestamps` on the file
-// boundaries contained in the Manifest, a.k.a FileMetaData.smallest,
-// FileMetaData.largest.
-class HandleFileBoundariesTest
-    : public DBBasicTestWithTimestampBase,
-      public testing::WithParamInterface<test::UserDefinedTimestampTestMode> {
- public:
-  HandleFileBoundariesTest()
-      : DBBasicTestWithTimestampBase("/handle_file_boundaries") {}
-};
-
-TEST_P(HandleFileBoundariesTest, ConfigurePersistUdt) {
-  Options options = CurrentOptions();
-  options.env = env_;
-  // Write a timestamp that is not the min timestamp to help test the behavior
-  // of flag `persist_user_defined_timestamps`.
-  std::string write_ts = Timestamp(1, 0);
-  std::string min_ts = Timestamp(0, 0);
-  std::string smallest_ukey_without_ts = "bar";
-  std::string largest_ukey_without_ts = "foo";
-  const size_t kTimestampSize = write_ts.size();
-  TestComparator test_cmp(kTimestampSize);
-  options.comparator = &test_cmp;
-  bool persist_udt = test::ShouldPersistUDT(GetParam());
-  options.persist_user_defined_timestamps = persist_udt;
-  DestroyAndReopen(options);
-
-  ASSERT_OK(
-      db_->Put(WriteOptions(), smallest_ukey_without_ts, write_ts, "val1"));
-  ASSERT_OK(
-      db_->Put(WriteOptions(), largest_ukey_without_ts, write_ts, "val2"));
-
-  // Create a L0 SST file and its record is added to the Manfiest.
-  ASSERT_OK(Flush());
-  Close();
-
-  options.create_if_missing = false;
-  // Reopen the DB and process manifest file.
-  Reopen(options);
-
-  std::vector<std::vector<FileMetaData>> level_to_files;
-  dbfull()->TEST_GetFilesMetaData(dbfull()->DefaultColumnFamily(),
-                                  &level_to_files);
-  ASSERT_GT(level_to_files.size(), 1);
-  // L0 only has one SST file.
-  ASSERT_EQ(level_to_files[0].size(), 1);
-  auto file_meta = level_to_files[0][0];
-  if (persist_udt) {
-    ASSERT_EQ(smallest_ukey_without_ts + write_ts,
-              file_meta.smallest.user_key());
-    ASSERT_EQ(largest_ukey_without_ts + write_ts, file_meta.largest.user_key());
-  } else {
-    // If `persist_user_defined_timestamps` is false, the file boundaries should
-    // have the min timestamp. Behind the scenes, when file boundaries in
-    // FileMetaData is persisted to Manifest, the original user-defined
-    // timestamps in user key are stripped. When manifest is read and processed
-    // during DB open, a min timestamp is padded to the file boundaries. This
-    // test's writes contain non min timestamp to verify this logic end-to-end.
-    ASSERT_EQ(smallest_ukey_without_ts + min_ts, file_meta.smallest.user_key());
-    ASSERT_EQ(largest_ukey_without_ts + min_ts, file_meta.largest.user_key());
-  }
-  Close();
-}
-
-INSTANTIATE_TEST_CASE_P(
-    ConfigurePersistUdt, HandleFileBoundariesTest,
-    ::testing::Values(
-        test::UserDefinedTimestampTestMode::kStripUserDefinedTimestamp,
-        test::UserDefinedTimestampTestMode::kNormal));
-
 TEST_F(DBBasicTestWithTimestamp,
        GCPreserveRangeTombstoneWhenNoOrSmallFullHistoryLow) {
   Options options = CurrentOptions();

db/dbformat.cc

@@ -120,16 +120,6 @@ void StripTimestampFromInternalKey(std::string* result, const Slice& key,
                  kNumInternalBytes);
 }
 
-void ReplaceInternalKeyWithMinTimestamp(std::string* result, const Slice& key,
-                                        size_t ts_sz) {
-  const size_t key_sz = key.size();
-  assert(key_sz >= ts_sz + kNumInternalBytes);
-  result->reserve(key_sz);
-  result->append(key.data(), key_sz - kNumInternalBytes - ts_sz);
-  result->append(ts_sz, static_cast<unsigned char>(0));
-  result->append(key.data() + key_sz - kNumInternalBytes, kNumInternalBytes);
-}
-
 std::string ParsedInternalKey::DebugString(bool log_err_key, bool hex) const {
   std::string result = "'";
   if (log_err_key) {

db/dbformat.h

@@ -168,57 +168,51 @@ inline void UnPackSequenceAndType(uint64_t packed, uint64_t* seq,
 EntryType GetEntryType(ValueType value_type);
 
 // Append the serialization of "key" to *result.
-void AppendInternalKey(std::string* result, const ParsedInternalKey& key);
+extern void AppendInternalKey(std::string* result,
+                              const ParsedInternalKey& key);
 
 // Append the serialization of "key" to *result, replacing the original
 // timestamp with argument ts.
-void AppendInternalKeyWithDifferentTimestamp(std::string* result,
-                                             const ParsedInternalKey& key,
-                                             const Slice& ts);
+extern void AppendInternalKeyWithDifferentTimestamp(
+    std::string* result, const ParsedInternalKey& key, const Slice& ts);
 
 // Serialized internal key consists of user key followed by footer.
 // This function appends the footer to *result, assuming that *result already
 // contains the user key at the end.
-void AppendInternalKeyFooter(std::string* result, SequenceNumber s,
-                             ValueType t);
+extern void AppendInternalKeyFooter(std::string* result, SequenceNumber s,
+                                    ValueType t);
 
 // Append the key and a minimal timestamp to *result
-void AppendKeyWithMinTimestamp(std::string* result, const Slice& key,
-                               size_t ts_sz);
+extern void AppendKeyWithMinTimestamp(std::string* result, const Slice& key,
+                                      size_t ts_sz);
 
 // Append the key and a maximal timestamp to *result
-void AppendKeyWithMaxTimestamp(std::string* result, const Slice& key,
-                               size_t ts_sz);
+extern void AppendKeyWithMaxTimestamp(std::string* result, const Slice& key,
+                                      size_t ts_sz);
 
 // `key` is a user key with timestamp. Append the user key without timestamp
 // and the maximal timestamp to *result.
-void AppendUserKeyWithMaxTimestamp(std::string* result, const Slice& key,
-                                   size_t ts_sz);
+extern void AppendUserKeyWithMaxTimestamp(std::string* result, const Slice& key,
+                                          size_t ts_sz);
 
 // `key` is an internal key containing a user key without timestamp. Create a
 // new key in *result by padding a min timestamp of size `ts_sz` to the user key
 // and copying the remaining internal key bytes.
-void PadInternalKeyWithMinTimestamp(std::string* result, const Slice& key,
-                                    size_t ts_sz);
+extern void PadInternalKeyWithMinTimestamp(std::string* result,
+                                           const Slice& key, size_t ts_sz);
 
 // `key` is an internal key containing a user key with timestamp of size
 // `ts_sz`. Create a new internal key in *result by stripping the timestamp from
 // the user key and copying the remaining internal key bytes.
-void StripTimestampFromInternalKey(std::string* result, const Slice& key,
-                                   size_t ts_sz);
-
-// `key` is an internal key containing a user key with timestamp of size
-// `ts_sz`. Create a new internal key in *result while replace the original
-// timestamp with min timestamp.
-void ReplaceInternalKeyWithMinTimestamp(std::string* result, const Slice& key,
-                                        size_t ts_sz);
+extern void StripTimestampFromInternalKey(std::string* result, const Slice& key,
+                                          size_t ts_sz);
 
 // Attempt to parse an internal key from "internal_key".  On success,
 // stores the parsed data in "*result", and returns true.
 //
 // On error, returns false, leaves "*result" in an undefined state.
-Status ParseInternalKey(const Slice& internal_key, ParsedInternalKey* result,
-                        bool log_err_key);
+extern Status ParseInternalKey(const Slice& internal_key,
+                               ParsedInternalKey* result, bool log_err_key);
 
 // Returns the user key portion of an internal key.
 inline Slice ExtractUserKey(const Slice& internal_key) {
@@ -789,7 +783,8 @@ class InternalKeySliceTransform : public SliceTransform {
 // Read the key of a record from a write batch.
 // if this record represent the default column family then cf_record
 // must be passed as false, otherwise it must be passed as true.
-bool ReadKeyFromWriteBatchEntry(Slice* input, Slice* key, bool cf_record);
+extern bool ReadKeyFromWriteBatchEntry(Slice* input, Slice* key,
+                                       bool cf_record);
 
 // Read record from a write batch piece from input.
 // tag, column_family, key, value and blob are return values. Callers own the
@@ -798,9 +793,9 @@ bool ReadKeyFromWriteBatchEntry(Slice* input, Slice* key, bool cf_record);
 // input will be advanced to after the record.
 // If user-defined timestamp is enabled for a column family, then the `key`
 // resulting from this call will include timestamp.
-Status ReadRecordFromWriteBatch(Slice* input, char* tag,
-                                uint32_t* column_family, Slice* key,
-                                Slice* value, Slice* blob, Slice* xid);
+extern Status ReadRecordFromWriteBatch(Slice* input, char* tag,
+                                       uint32_t* column_family, Slice* key,
+                                       Slice* value, Slice* blob, Slice* xid);
 
 // When user call DeleteRange() to delete a range of keys,
 // we will store a serialized RangeTombstone in MemTable and SST.

db/dbformat_test.cc

@@ -312,27 +312,6 @@ TEST_F(FormatTest, StripTimestampFromInternalKey) {
   ASSERT_EQ(kTypeValue, key_without_timestamp.type);
 }
 
-TEST_F(FormatTest, ReplaceInternalKeyWithMinTimestamp) {
-  std::string orig_user_key = "foo";
-  size_t ts_sz = 8;
-  orig_user_key.append(ts_sz, static_cast<unsigned char>(1));
-  std::string orig_internal_key = IKey(orig_user_key, 100, kTypeValue);
-
-  std::string key_buf;
-  ReplaceInternalKeyWithMinTimestamp(&key_buf, orig_internal_key, ts_sz);
-  ParsedInternalKey new_key;
-  Slice in(key_buf);
-  ASSERT_OK(ParseInternalKey(in, &new_key, true /*log_err_key*/));
-
-  std::string min_timestamp(ts_sz, static_cast<unsigned char>(0));
-  size_t ukey_diff_offset = new_key.user_key.difference_offset(orig_user_key);
-  ASSERT_EQ(min_timestamp,
-            Slice(new_key.user_key.data() + ukey_diff_offset, ts_sz));
-  ASSERT_EQ(orig_user_key.size(), new_key.user_key.size());
-  ASSERT_EQ(100, new_key.sequence);
-  ASSERT_EQ(kTypeValue, new_key.type);
-}
-
 }  // namespace ROCKSDB_NAMESPACE
 
 int main(int argc, char** argv) {

db/file_indexer.cc

@@ -56,15 +56,17 @@ void FileIndexer::GetNextLevelIndex(const size_t level, const size_t file_index,
   } else if (cmp_smallest == 0) {
     *left_bound = index.smallest_lb;
     *right_bound = index.smallest_rb;
-  } else if (cmp_largest < 0) {
+  } else if (cmp_smallest > 0 && cmp_largest < 0) {
     *left_bound = index.smallest_lb;
     *right_bound = index.largest_rb;
   } else if (cmp_largest == 0) {
     *left_bound = index.largest_lb;
     *right_bound = index.largest_rb;
-  } else {
+  } else if (cmp_largest > 0) {
     *left_bound = index.largest_lb;
     *right_bound = level_rb_[level + 1];
+  } else {
+    assert(false);
   }
 
   assert(*left_bound >= 0);

db/flush_job_test.cc

@@ -117,8 +117,6 @@ class FlushJobTestBase : public testing::Test {
     db_options_.statistics = CreateDBStatistics();
 
     cf_options_.comparator = ucmp_;
-    cf_options_.persist_user_defined_timestamps = persist_udt_;
-    cf_options_.paranoid_file_checks = paranoid_file_checks_;
 
     std::vector<ColumnFamilyDescriptor> column_families;
     cf_options_.table_factory = mock_table_factory_;
@@ -151,9 +149,6 @@ class FlushJobTestBase : public testing::Test {
   std::atomic<bool> shutting_down_;
   std::shared_ptr<mock::MockTableFactory> mock_table_factory_;
 
-  bool persist_udt_ = true;
-  bool paranoid_file_checks_ = false;
-
   SeqnoToTimeMapping empty_seqno_to_time_mapping_;
 };
 
@@ -605,13 +600,7 @@ TEST_F(FlushJobTest, GetRateLimiterPriorityForWrite) {
   }
 }
 
-// Test parameters:
-// param 0): paranoid file check
-// param 1): user-defined timestamp test mode
-class FlushJobTimestampTest
-    : public FlushJobTestBase,
-      public testing::WithParamInterface<
-          std::tuple<bool, test::UserDefinedTimestampTestMode>> {
+class FlushJobTimestampTest : public FlushJobTestBase {
  public:
   FlushJobTimestampTest()
       : FlushJobTestBase(test::PerThreadDBPath("flush_job_ts_gc_test"),
@@ -627,36 +616,13 @@ class FlushJobTimestampTest
   }
 
  protected:
-  void SetUp() override {
-    paranoid_file_checks_ = std::get<0>(GetParam());
-    auto udt_test_mode = std::get<1>(GetParam());
-    persist_udt_ = test::ShouldPersistUDT(udt_test_mode);
-    FlushJobTestBase::SetUp();
-  }
   static constexpr uint64_t kStartTs = 10;
   static constexpr SequenceNumber kStartSeq = 0;
   SequenceNumber curr_seq_{kStartSeq};
   std::atomic<uint64_t> curr_ts_{kStartTs};
-
-  void CheckFileMetaData(ColumnFamilyData* cfd,
-                         const InternalKey& expected_smallest,
-                         const InternalKey& expected_largest,
-                         const FileMetaData* meta_from_flush) const {
-    ASSERT_EQ(expected_smallest.Encode(), meta_from_flush->smallest.Encode());
-    ASSERT_EQ(expected_largest.Encode(), meta_from_flush->largest.Encode());
-
-    const VersionStorageInfo* storage_info = cfd->current()->storage_info();
-    const std::vector<FileMetaData*>& l0_files = storage_info->LevelFiles(0);
-
-    ASSERT_EQ(l0_files.size(), 1);
-    auto installed_file_meta = l0_files[0];
-    ASSERT_EQ(expected_smallest.Encode(),
-              installed_file_meta->smallest.Encode());
-    ASSERT_EQ(expected_largest.Encode(), installed_file_meta->largest.Encode());
-  }
 };
 
-TEST_P(FlushJobTimestampTest, AllKeysExpired) {
+TEST_F(FlushJobTimestampTest, AllKeysExpired) {
   ColumnFamilyData* cfd = versions_->GetColumnFamilySet()->GetDefault();
   autovector<MemTable*> to_delete;
 
@@ -703,24 +669,17 @@ TEST_P(FlushJobTimestampTest, AllKeysExpired) {
 
   {
     std::string key = test::EncodeInt(0);
-    if (!persist_udt_) {
-      // When `AdvancedColumnFamilyOptions.persist_user_defined_timestamps` flag
-      // is set to false. The user-defined timestamp is stripped from user key
-      // during flush, making the user key logically containing the minimum
-      // timestamp.
-      key.append(test::EncodeInt(0));
-    } else {
-      key.append(test::EncodeInt(curr_ts_.load(std::memory_order_relaxed) - 1));
-    }
+    key.append(test::EncodeInt(curr_ts_.load(std::memory_order_relaxed) - 1));
     InternalKey ikey(key, curr_seq_ - 1, ValueType::kTypeDeletionWithTimestamp);
-    CheckFileMetaData(cfd, ikey, ikey, &fmeta);
+    ASSERT_EQ(ikey.Encode(), fmeta.smallest.Encode());
+    ASSERT_EQ(ikey.Encode(), fmeta.largest.Encode());
   }
 
   job_context.Clean();
   ASSERT_TRUE(to_delete.empty());
 }
 
-TEST_P(FlushJobTimestampTest, NoKeyExpired) {
+TEST_F(FlushJobTimestampTest, NoKeyExpired) {
   ColumnFamilyData* cfd = versions_->GetColumnFamilySet()->GetDefault();
   autovector<MemTable*> to_delete;
 
@@ -763,38 +722,18 @@ TEST_P(FlushJobTimestampTest, NoKeyExpired) {
 
   {
     std::string ukey = test::EncodeInt(0);
-    std::string smallest_key;
-    std::string largest_key;
-    if (!persist_udt_) {
-      // When `AdvancedColumnFamilyOptions.persist_user_defined_timestamps` flag
-      // is set to false. The user-defined timestamp is stripped from user key
-      // during flush, making the user key logically containing the minimum
-      // timestamp, which is hardcoded to be all zeros for now.
-      smallest_key = ukey + test::EncodeInt(0);
-      largest_key = ukey + test::EncodeInt(0);
-    } else {
-      smallest_key =
-          ukey + test::EncodeInt(curr_ts_.load(std::memory_order_relaxed) - 1);
-      largest_key = ukey + test::EncodeInt(kStartTs);
-    }
+    std::string smallest_key =
+        ukey + test::EncodeInt(curr_ts_.load(std::memory_order_relaxed) - 1);
+    std::string largest_key = ukey + test::EncodeInt(kStartTs);
     InternalKey smallest(smallest_key, curr_seq_ - 1, ValueType::kTypeValue);
     InternalKey largest(largest_key, kStartSeq, ValueType::kTypeValue);
-    CheckFileMetaData(cfd, smallest, largest, &fmeta);
+    ASSERT_EQ(smallest.Encode(), fmeta.smallest.Encode());
+    ASSERT_EQ(largest.Encode(), fmeta.largest.Encode());
   }
   job_context.Clean();
   ASSERT_TRUE(to_delete.empty());
 }
 
-// Param 0: paranoid file check
-// Param 1: test mode for the user-defined timestamp feature
-INSTANTIATE_TEST_CASE_P(
-    FlushJobTimestampTest, FlushJobTimestampTest,
-    ::testing::Combine(
-        ::testing::Bool(),
-        ::testing::Values(
-            test::UserDefinedTimestampTestMode::kStripUserDefinedTimestamp,
-            test::UserDefinedTimestampTestMode::kNormal)));
-
 }  // namespace ROCKSDB_NAMESPACE
 
 int main(int argc, char** argv) {

db/memtable.cc

@@ -810,7 +810,7 @@ Status MemTable::Add(SequenceNumber s, ValueType type,
         earliest_seqno_.load(std::memory_order_relaxed);
     while (
         (cur_earliest_seqno == kMaxSequenceNumber || s < cur_earliest_seqno) &&
-        !earliest_seqno_.compare_exchange_weak(cur_earliest_seqno, s)) {
+        !first_seqno_.compare_exchange_weak(cur_earliest_seqno, s)) {
     }
   }
   if (type == kTypeRangeDeletion) {

db/repair_test.cc

@@ -315,13 +315,7 @@ TEST_F(RepairTest, UnflushedSst) {
   ASSERT_EQ(Get("key"), "val");
 }
 
-// Test parameters:
-// param 0): paranoid file check
-// param 1): user-defined timestamp test mode
-class RepairTestWithTimestamp
-    : public DBBasicTestWithTimestampBase,
-      public testing::WithParamInterface<
-          std::tuple<bool, test::UserDefinedTimestampTestMode>> {
+class RepairTestWithTimestamp : public DBBasicTestWithTimestampBase {
  public:
   RepairTestWithTimestamp()
       : DBBasicTestWithTimestampBase("repair_test_with_timestamp") {}
@@ -332,46 +326,23 @@ class RepairTestWithTimestamp
   }
 
   void CheckGet(const ReadOptions& read_opts, const Slice& key,
-                const std::string& expected_value,
-                const std::string& expected_ts) {
+                const std::string& expected_value) {
     std::string actual_value;
-    std::string actual_ts;
-    ASSERT_OK(db_->Get(read_opts, handles_[0], key, &actual_value, &actual_ts));
+    ASSERT_OK(db_->Get(read_opts, handles_[0], key, &actual_value));
     ASSERT_EQ(expected_value, actual_value);
-    ASSERT_EQ(expected_ts, actual_ts);
-  }
-
-  void CheckFileBoundaries(const Slice& smallest_user_key,
-                           const Slice& largest_user_key) {
-    std::vector<std::vector<FileMetaData>> level_to_files;
-    dbfull()->TEST_GetFilesMetaData(dbfull()->DefaultColumnFamily(),
-                                    &level_to_files);
-    ASSERT_GT(level_to_files.size(), 1);
-    // L0 only has one SST file.
-    ASSERT_EQ(level_to_files[0].size(), 1);
-    auto file_meta = level_to_files[0][0];
-    ASSERT_EQ(smallest_user_key, file_meta.smallest.user_key());
-    ASSERT_EQ(largest_user_key, file_meta.largest.user_key());
   }
 };
 
-TEST_P(RepairTestWithTimestamp, UnflushedSst) {
+TEST_F(RepairTestWithTimestamp, UnflushedSst) {
   Destroy(last_options_);
 
-  bool paranoid_file_checks = std::get<0>(GetParam());
-  bool persist_udt = test::ShouldPersistUDT(std::get<1>(GetParam()));
-  std::string smallest_ukey_without_ts = "bar";
-  std::string largest_ukey_without_ts = "foo";
   Options options = CurrentOptions();
   options.env = env_;
   options.create_if_missing = true;
-  std::string min_ts = Timestamp(0, 0);
-  std::string write_ts = Timestamp(1, 0);
-  const size_t kTimestampSize = write_ts.size();
+  std::string ts = Timestamp(0, 0);
+  const size_t kTimestampSize = ts.size();
   TestComparator test_cmp(kTimestampSize);
   options.comparator = &test_cmp;
-  options.persist_user_defined_timestamps = persist_udt;
-  options.paranoid_file_checks = paranoid_file_checks;
 
   ColumnFamilyOptions cf_options(options);
   std::vector<ColumnFamilyDescriptor> column_families;
@@ -380,10 +351,7 @@ TEST_P(RepairTestWithTimestamp, UnflushedSst) {
 
   ASSERT_OK(DB::Open(options, dbname_, column_families, &handles_, &db_));
 
-  ASSERT_OK(Put(smallest_ukey_without_ts, write_ts,
-                smallest_ukey_without_ts + ":val"));
-  ASSERT_OK(
-      Put(largest_ukey_without_ts, write_ts, largest_ukey_without_ts + ":val"));
+  ASSERT_OK(Put("key", ts, "val"));
   VectorLogPtr wal_files;
   ASSERT_OK(dbfull()->GetSortedWalFiles(wal_files));
   ASSERT_EQ(wal_files.size(), 1);
@@ -413,46 +381,12 @@ TEST_P(RepairTestWithTimestamp, UnflushedSst) {
     ASSERT_GT(total_ssts_size, 0);
   }
 
-  // Check file boundaries are correct for different
-  // `persist_user_defined_timestamps` option values.
-  if (persist_udt) {
-    CheckFileBoundaries(smallest_ukey_without_ts + write_ts,
-                        largest_ukey_without_ts + write_ts);
-  } else {
-    CheckFileBoundaries(smallest_ukey_without_ts + min_ts,
-                        largest_ukey_without_ts + min_ts);
-  }
-
   ReadOptions read_opts;
-  Slice read_ts_slice = write_ts;
+  Slice read_ts_slice = ts;
   read_opts.timestamp = &read_ts_slice;
-  if (persist_udt) {
-    CheckGet(read_opts, smallest_ukey_without_ts,
-             smallest_ukey_without_ts + ":val", write_ts);
-    CheckGet(read_opts, largest_ukey_without_ts,
-             largest_ukey_without_ts + ":val", write_ts);
-  } else {
-    // TODO(yuzhangyu): currently when `persist_user_defined_timestamps` is
-    //  false, ts is unconditionally stripped during flush.
-    //  When `full_history_ts_low` is set and respected during flush.
-    //  We should prohibit reading below `full_history_ts_low` all together.
-    CheckGet(read_opts, smallest_ukey_without_ts,
-             smallest_ukey_without_ts + ":val", min_ts);
-    CheckGet(read_opts, largest_ukey_without_ts,
-             largest_ukey_without_ts + ":val", min_ts);
-  }
+  CheckGet(read_opts, "key", "val");
 }
 
-// Param 0: paranoid file check
-// Param 1: test mode for the user-defined timestamp feature
-INSTANTIATE_TEST_CASE_P(
-    UnflushedSst, RepairTestWithTimestamp,
-    ::testing::Combine(
-        ::testing::Bool(),
-        ::testing::Values(
-            test::UserDefinedTimestampTestMode::kStripUserDefinedTimestamp,
-            test::UserDefinedTimestampTestMode::kNormal)));
-
 TEST_F(RepairTest, SeparateWalDir) {
   do {
     Options options = CurrentOptions();

db/table_cache.cc

@@ -111,17 +111,13 @@ Status TableCache::GetTableReader(
     RecordTick(ioptions_.stats, NO_FILE_OPENS);
   } else if (s.IsPathNotFound()) {
     fname = Rocks2LevelTableFileName(fname);
-    // If this file is also not found, we want to use the error message
-    // that contains the table file name which is less confusing.
-    Status temp_s =
-        PrepareIOFromReadOptions(ro, ioptions_.clock, fopts.io_options);
-    if (temp_s.ok()) {
-      temp_s = ioptions_.fs->NewRandomAccessFile(fname, file_options, &file,
-                                                 nullptr);
+    s = PrepareIOFromReadOptions(ro, ioptions_.clock, fopts.io_options);
+    if (s.ok()) {
+      s = ioptions_.fs->NewRandomAccessFile(fname, file_options, &file,
+                                            nullptr);
     }
-    if (temp_s.ok()) {
+    if (s.ok()) {
       RecordTick(ioptions_.stats, NO_FILE_OPENS);
-      s = temp_s;
     }
   }
 

db/version_edit.cc

@@ -93,8 +93,7 @@ void VersionEdit::Clear() {
   full_history_ts_low_.clear();
 }
 
-bool VersionEdit::EncodeTo(std::string* dst,
-                           std::optional<size_t> ts_sz) const {
+bool VersionEdit::EncodeTo(std::string* dst) const {
   if (has_db_id_) {
     PutVarint32(dst, kDbId);
     PutLengthPrefixedSlice(dst, db_id_);
@@ -134,8 +133,6 @@ bool VersionEdit::EncodeTo(std::string* dst,
   }
 
   bool min_log_num_written = false;
-
-  assert(new_files_.empty() || ts_sz.has_value());
   for (size_t i = 0; i < new_files_.size(); i++) {
     const FileMetaData& f = new_files_[i].second;
     if (!f.smallest.Valid() || !f.largest.Valid() ||
@@ -145,7 +142,8 @@ bool VersionEdit::EncodeTo(std::string* dst,
     PutVarint32(dst, kNewFile4);
     PutVarint32Varint64(dst, new_files_[i].first /* level */, f.fd.GetNumber());
     PutVarint64(dst, f.fd.GetFileSize());
-    EncodeFileBoundaries(dst, f, ts_sz.value());
+    PutLengthPrefixedSlice(dst, f.smallest.Encode());
+    PutLengthPrefixedSlice(dst, f.largest.Encode());
     PutVarint64Varint64(dst, f.fd.smallest_seqno, f.fd.largest_seqno);
     // Customized fields' format:
     // +-----------------------------+
@@ -460,23 +458,6 @@ const char* VersionEdit::DecodeNewFile4From(Slice* input) {
   return nullptr;
 }
 
-void VersionEdit::EncodeFileBoundaries(std::string* dst,
-                                       const FileMetaData& meta,
-                                       size_t ts_sz) const {
-  if (ts_sz == 0 || meta.user_defined_timestamps_persisted) {
-    PutLengthPrefixedSlice(dst, meta.smallest.Encode());
-    PutLengthPrefixedSlice(dst, meta.largest.Encode());
-    return;
-  }
-  std::string smallest_buf;
-  std::string largest_buf;
-  StripTimestampFromInternalKey(&smallest_buf, meta.smallest.Encode(), ts_sz);
-  StripTimestampFromInternalKey(&largest_buf, meta.largest.Encode(), ts_sz);
-  PutLengthPrefixedSlice(dst, smallest_buf);
-  PutLengthPrefixedSlice(dst, largest_buf);
-  return;
-};
-
 Status VersionEdit::DecodeFrom(const Slice& src) {
   Clear();
 #ifndef NDEBUG

db/version_edit.h

@@ -9,7 +9,6 @@
 
 #pragma once
 #include <algorithm>
-#include <optional>
 #include <set>
 #include <string>
 #include <utility>
@@ -490,8 +489,6 @@ class VersionEdit {
   using NewFiles = std::vector<std::pair<int, FileMetaData>>;
   const NewFiles& GetNewFiles() const { return new_files_; }
 
-  NewFiles& GetMutableNewFiles() { return new_files_; }
-
   // Retrieve all the compact cursors
   using CompactCursors = std::vector<std::pair<int, InternalKey>>;
   const CompactCursors& GetCompactCursors() const { return compact_cursors_; }
@@ -642,17 +639,7 @@ class VersionEdit {
   }
 
   // return true on success.
-  // `ts_sz` is the size in bytes for the user-defined timestamp contained in
-  // a user key. This argument is optional because it's only required for
-  // encoding a `VersionEdit` with new SST files to add. It's used to handle the
-  // file boundaries: `smallest`, `largest` when
-  // `FileMetaData.user_defined_timestamps_persisted` is false. When reading
-  // the Manifest file, a mirroring change needed to handle
-  // file boundaries are not added to the `VersionEdit.DecodeFrom` function
-  // because timestamp size is not available at `VersionEdit` decoding time,
-  // it's instead added to `VersionEditHandler::OnNonCfOperation`.
-  bool EncodeTo(std::string* dst,
-                std::optional<size_t> ts_sz = std::nullopt) const;
+  bool EncodeTo(std::string* dst) const;
   Status DecodeFrom(const Slice& src);
 
   std::string DebugString(bool hex_key = false) const;
@@ -673,12 +660,6 @@ class VersionEdit {
 
   const char* DecodeNewFile4From(Slice* input);
 
-  // Encode file boundaries `FileMetaData.smallest` and `FileMetaData.largest`.
-  // User-defined timestamps in the user key will be stripped if they shouldn't
-  // be persisted.
-  void EncodeFileBoundaries(std::string* dst, const FileMetaData& meta,
-                            size_t ts_sz) const;
-
   int max_level_ = 0;
   std::string db_id_;
   std::string comparator_;

db/version_edit_handler.cc

@@ -86,8 +86,7 @@ void VersionEditHandlerBase::Iterate(log::Reader& reader,
         message << ' ';
       }
       // append the filename to the corruption message
-      message << " The file " << reader.file()->file_name()
-              << " may be corrupted.";
+      message << "in file " << reader.file()->file_name();
       // overwrite the status with the extended status
       s = Status(s.code(), s.subcode(), s.severity(), message.str());
     }
@@ -309,17 +308,6 @@ Status VersionEditHandler::OnNonCfOperation(VersionEdit& edit,
       tmp_cfd = version_set_->GetColumnFamilySet()->GetColumnFamily(
           edit.column_family_);
       assert(tmp_cfd != nullptr);
-      // It's important to handle file boundaries before `MaybeCreateVersion`
-      // because `VersionEditHandlerPointInTime::MaybeCreateVersion` does
-      // `FileMetaData` verification that involves the file boundaries.
-      // All `VersionEditHandlerBase` subclasses that need to deal with
-      // `FileMetaData` for new files are also subclasses of
-      // `VersionEditHandler`, so it's sufficient to do the file boundaries
-      // handling in this method.
-      s = MaybeHandleFileBoundariesForNewFiles(edit, tmp_cfd);
-      if (!s.ok()) {
-        return s;
-      }
       s = MaybeCreateVersion(edit, tmp_cfd, /*force_create_version=*/false);
       if (s.ok()) {
         s = builder_iter->second->version_builder()->Apply(&edit);
@@ -659,47 +647,6 @@ Status VersionEditHandler::ExtractInfoFromVersionEdit(ColumnFamilyData* cfd,
   return s;
 }
 
-Status VersionEditHandler::MaybeHandleFileBoundariesForNewFiles(
-    VersionEdit& edit, const ColumnFamilyData* cfd) {
-  if (edit.GetNewFiles().empty()) {
-    return Status::OK();
-  }
-  auto ucmp = cfd->user_comparator();
-  assert(ucmp);
-  size_t ts_sz = ucmp->timestamp_size();
-  if (ts_sz == 0) {
-    return Status::OK();
-  }
-
-  VersionEdit::NewFiles& new_files = edit.GetMutableNewFiles();
-  assert(!new_files.empty());
-  bool file_boundaries_need_handling = false;
-  for (auto& new_file : new_files) {
-    FileMetaData& meta = new_file.second;
-    if (meta.user_defined_timestamps_persisted) {
-      // `FileMetaData.user_defined_timestamps_persisted` field is the value of
-      // the flag `AdvancedColumnFamilyOptions.persist_user_defined_timestamps`
-      // at the time when the SST file was created. As a result, all added SST
-      // files in one `VersionEdit` should have the same value for it.
-      if (file_boundaries_need_handling) {
-        return Status::Corruption(
-            "New files in one VersionEdit has different "
-            "user_defined_timestamps_persisted value.");
-      }
-      break;
-    }
-    file_boundaries_need_handling = true;
-    std::string smallest_buf;
-    std::string largest_buf;
-    PadInternalKeyWithMinTimestamp(&smallest_buf, meta.smallest.Encode(),
-                                   ts_sz);
-    PadInternalKeyWithMinTimestamp(&largest_buf, meta.largest.Encode(), ts_sz);
-    meta.smallest.DecodeFrom(smallest_buf);
-    meta.largest.DecodeFrom(largest_buf);
-  }
-  return Status::OK();
-}
-
 VersionEditHandlerPointInTime::VersionEditHandlerPointInTime(
     bool read_only, std::vector<ColumnFamilyDescriptor> column_families,
     VersionSet* version_set, const std::shared_ptr<IOTracer>& io_tracer,

db/version_edit_handler.h

@@ -206,17 +206,6 @@ class VersionEditHandler : public VersionEditHandlerBase {
  private:
   Status ExtractInfoFromVersionEdit(ColumnFamilyData* cfd,
                                     const VersionEdit& edit);
-
-  // When `FileMetaData.user_defined_timestamps_persisted` is false and
-  // user-defined timestamp size is non-zero. User-defined timestamps are
-  // stripped from file boundaries: `smallest`, `largest` in
-  // `VersionEdit.DecodeFrom` before they were written to Manifest.
-  // This is the mirroring change to handle file boundaries on the Manifest read
-  // path for this scenario: to pad a minimum timestamp to the user key in
-  // `smallest` and `largest` so their format are consistent with the running
-  // user comparator.
-  Status MaybeHandleFileBoundariesForNewFiles(VersionEdit& edit,
-                                              const ColumnFamilyData* cfd);
 };
 
 // A class similar to its base class, i.e. VersionEditHandler.

db/version_edit_test.cc

@@ -21,20 +21,12 @@
 namespace ROCKSDB_NAMESPACE {
 
 static void TestEncodeDecode(const VersionEdit& edit) {
-  // Encoding one `VersionEdit` and decoding it again should result in the
-  // exact same `VersionEdit`. However, a special handling is applied to file
-  // boundaries: `FileMetaData.smallest`, `FileMetaData.largest` when
-  // user-defined timestamps should not be persisted. In that scenario, this
-  // invariant does not hold. We disable this scenario in this util method to
-  // enable all other test cases continue to verify this invariant, while the
-  // special case is separately covered in test
-  // `EncodeDecodeNewFile4HandleFileBoundary`.
   std::string encoded, encoded2;
-  edit.EncodeTo(&encoded, 0 /* ts_sz */);
+  edit.EncodeTo(&encoded);
   VersionEdit parsed;
   Status s = parsed.DecodeFrom(encoded);
   ASSERT_TRUE(s.ok()) << s.ToString();
-  parsed.EncodeTo(&encoded2, 0 /* ts_sz */);
+  parsed.EncodeTo(&encoded2);
   ASSERT_EQ(encoded, encoded2);
 }
 
@@ -101,7 +93,7 @@ TEST_F(VersionEditTest, EncodeDecodeNewFile4) {
   TestEncodeDecode(edit);
 
   std::string encoded, encoded2;
-  edit.EncodeTo(&encoded, 0 /* ts_sz */);
+  edit.EncodeTo(&encoded);
   VersionEdit parsed;
   Status s = parsed.DecodeFrom(encoded);
   ASSERT_TRUE(s.ok()) << s.ToString();
@@ -127,57 +119,6 @@ TEST_F(VersionEditTest, EncodeDecodeNewFile4) {
   ASSERT_TRUE(new_files[3].second.user_defined_timestamps_persisted);
 }
 
-TEST_F(VersionEditTest, EncodeDecodeNewFile4HandleFileBoundary) {
-  static const uint64_t kBig = 1ull << 50;
-  size_t ts_sz = 16;
-  static std::string min_ts(ts_sz, static_cast<unsigned char>(0));
-  VersionEdit edit;
-  std::string smallest = "foo";
-  std::string largest = "zoo";
-  // In real manifest writing scenarios, one `VersionEdit` should not contain
-  // files with different `user_defined_timestamps_persisted` flag value.
-  // This is just for testing file boundaries handling w.r.t persisting user
-  // defined timestamps during `VersionEdit` encoding.
-  edit.AddFile(
-      3, 300, 3, 100, InternalKey(smallest + min_ts, kBig + 500, kTypeValue),
-      InternalKey(largest + min_ts, kBig + 600, kTypeDeletion), kBig + 500,
-      kBig + 600, true, Temperature::kUnknown, kInvalidBlobFileNumber,
-      kUnknownOldestAncesterTime, kUnknownFileCreationTime,
-      300 /* epoch_number */, kUnknownFileChecksum,
-      kUnknownFileChecksumFuncName, kNullUniqueId64x2,
-      0 /* compensated_range_deletion_size */, 0 /* tail_size */,
-      false /* user_defined_timestamps_persisted */);
-  edit.AddFile(3, 300, 3, 100,
-               InternalKey(smallest + min_ts, kBig + 500, kTypeValue),
-               InternalKey(largest + min_ts, kBig + 600, kTypeDeletion),
-               kBig + 500, kBig + 600, true, Temperature::kUnknown,
-               kInvalidBlobFileNumber, kUnknownOldestAncesterTime,
-               kUnknownFileCreationTime, 300 /* epoch_number */,
-               kUnknownFileChecksum, kUnknownFileChecksumFuncName,
-               kNullUniqueId64x2, 0 /* compensated_range_deletion_size */,
-               0 /* tail_size */, true /* user_defined_timestamps_persisted */);
-
-  std::string encoded;
-  edit.EncodeTo(&encoded, ts_sz);
-  VersionEdit parsed;
-  Status s = parsed.DecodeFrom(encoded);
-  ASSERT_TRUE(s.ok()) << s.ToString();
-  auto& new_files = parsed.GetNewFiles();
-  ASSERT_TRUE(new_files.size() == 2);
-  ASSERT_FALSE(new_files[0].second.user_defined_timestamps_persisted);
-  // First file's boundaries do not contain user-defined timestamps.
-  ASSERT_EQ(InternalKey(smallest, kBig + 500, kTypeValue).Encode(),
-            new_files[0].second.smallest.Encode());
-  ASSERT_EQ(InternalKey(largest, kBig + 600, kTypeDeletion).Encode(),
-            new_files[0].second.largest.Encode());
-  ASSERT_TRUE(new_files[1].second.user_defined_timestamps_persisted);
-  // Second file's boundaries contain user-defined timestamps.
-  ASSERT_EQ(InternalKey(smallest + min_ts, kBig + 500, kTypeValue).Encode(),
-            new_files[1].second.smallest.Encode());
-  ASSERT_EQ(InternalKey(largest + min_ts, kBig + 600, kTypeDeletion).Encode(),
-            new_files[1].second.largest.Encode());
-}
-
 TEST_F(VersionEditTest, ForwardCompatibleNewFile4) {
   static const uint64_t kBig = 1ull << 50;
   VersionEdit edit;
@@ -217,7 +158,7 @@ TEST_F(VersionEditTest, ForwardCompatibleNewFile4) {
         }
       });
   ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing();
-  edit.EncodeTo(&encoded, 0 /* ts_sz */);
+  edit.EncodeTo(&encoded);
   ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing();
 
   VersionEdit parsed;
@@ -257,7 +198,7 @@ TEST_F(VersionEditTest, NewFile4NotSupportedField) {
         PutLengthPrefixedSlice(str, str1);
       });
   ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing();
-  edit.EncodeTo(&encoded, 0 /* ts_sz */);
+  edit.EncodeTo(&encoded);
   ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing();
 
   VersionEdit parsed;
@@ -273,7 +214,7 @@ TEST_F(VersionEditTest, EncodeEmptyFile) {
                1 /*epoch_number*/, kUnknownFileChecksum,
                kUnknownFileChecksumFuncName, kNullUniqueId64x2, 0, 0, true);
   std::string buffer;
-  ASSERT_TRUE(!edit.EncodeTo(&buffer, 0 /* ts_sz */));
+  ASSERT_TRUE(!edit.EncodeTo(&buffer));
 }
 
 TEST_F(VersionEditTest, ColumnFamilyTest) {
@@ -642,7 +583,7 @@ TEST_F(VersionEditTest, IgnorableTags) {
   edit.SetColumnFamily(kColumnFamilyId);
 
   std::string encoded;
-  ASSERT_TRUE(edit.EncodeTo(&encoded, 0 /* ts_sz */));
+  ASSERT_TRUE(edit.EncodeTo(&encoded));
 
   VersionEdit decoded;
   ASSERT_OK(decoded.DecodeFrom(encoded));

db/version_set.cc

@@ -5118,12 +5118,6 @@ Status VersionSet::ProcessManifestWrites(
   assert(manifest_writers_.front() == &first_writer);
 
   autovector<VersionEdit*> batch_edits;
-  // This vector keeps track of the corresponding user-defined timestamp size
-  // for `batch_edits` side by side, which is only needed for encoding a
-  // `VersionEdit` that adds new SST files.
-  // Note that anytime `batch_edits` has new element added or get existing
-  // element removed, `batch_edits_ts_sz` should be updated too.
-  autovector<std::optional<size_t>> batch_edits_ts_sz;
   autovector<Version*> versions;
   autovector<const MutableCFOptions*> mutable_cf_options_ptrs;
   std::vector<std::unique_ptr<BaseReferencedVersionBuilder>> builder_guards;
@@ -5139,7 +5133,6 @@ Status VersionSet::ProcessManifestWrites(
     // No group commits for column family add or drop
     LogAndApplyCFHelper(first_writer.edit_list.front(), &max_last_sequence);
     batch_edits.push_back(first_writer.edit_list.front());
-    batch_edits_ts_sz.push_back(std::nullopt);
   } else {
     auto it = manifest_writers_.cbegin();
     size_t group_start = std::numeric_limits<size_t>::max();
@@ -5216,9 +5209,6 @@ Status VersionSet::ProcessManifestWrites(
         TEST_SYNC_POINT_CALLBACK("VersionSet::ProcessManifestWrites:NewVersion",
                                  version);
       }
-      const Comparator* ucmp = last_writer->cfd->user_comparator();
-      assert(ucmp);
-      std::optional<size_t> edit_ts_sz = ucmp->timestamp_size();
       for (const auto& e : last_writer->edit_list) {
         if (e->is_in_atomic_group_) {
           if (batch_edits.empty() || !batch_edits.back()->is_in_atomic_group_ ||
@@ -5239,7 +5229,6 @@ Status VersionSet::ProcessManifestWrites(
           return s;
         }
         batch_edits.push_back(e);
-        batch_edits_ts_sz.push_back(edit_ts_sz);
       }
     }
     for (int i = 0; i < static_cast<int>(versions.size()); ++i) {
@@ -5405,11 +5394,9 @@ Status VersionSet::ProcessManifestWrites(
 #ifndef NDEBUG
       size_t idx = 0;
 #endif
-      assert(batch_edits.size() == batch_edits_ts_sz.size());
-      for (size_t bidx = 0; bidx < batch_edits.size(); bidx++) {
-        auto& e = batch_edits[bidx];
+      for (auto& e : batch_edits) {
         std::string record;
-        if (!e->EncodeTo(&record, batch_edits_ts_sz[bidx])) {
+        if (!e->EncodeTo(&record)) {
           s = Status::Corruption("Unable to encode VersionEdit:" +
                                  e->DebugString(true));
           break;
@@ -6302,9 +6289,8 @@ Status VersionSet::GetLiveFilesChecksumInfo(FileChecksumList* checksum_list) {
   return s;
 }
 
-Status VersionSet::DumpManifest(
-    Options& options, std::string& dscname, bool verbose, bool hex, bool json,
-    const std::vector<ColumnFamilyDescriptor>& cf_descs) {
+Status VersionSet::DumpManifest(Options& options, std::string& dscname,
+                                bool verbose, bool hex, bool json) {
   assert(options.env);
   // TODO: plumb Env::IOActivity
   const ReadOptions read_options;
@@ -6330,22 +6316,13 @@ Status VersionSet::DumpManifest(
         std::move(file), dscname, db_options_->log_readahead_size, io_tracer_);
   }
 
-  std::map<std::string, const ColumnFamilyDescriptor*> cf_name_to_desc;
-  for (const auto& cf_desc : cf_descs) {
-    cf_name_to_desc[cf_desc.name] = &cf_desc;
-  }
-  std::vector<ColumnFamilyDescriptor> final_cf_descs;
+  std::vector<ColumnFamilyDescriptor> cf_descs;
   for (const auto& cf : column_families) {
-    const auto iter = cf_name_to_desc.find(cf);
-    if (iter != cf_name_to_desc.cend()) {
-      final_cf_descs.push_back(*iter->second);
-    } else {
-      final_cf_descs.emplace_back(cf, options);
-    }
+    cf_descs.emplace_back(cf, options);
   }
 
-  DumpManifestHandler handler(final_cf_descs, this, io_tracer_, read_options,
-                              verbose, hex, json);
+  DumpManifestHandler handler(cf_descs, this, io_tracer_, read_options, verbose,
+                              hex, json);
   {
     VersionSet::LogReporter reporter;
     reporter.status = &s;
@@ -6528,10 +6505,8 @@ Status VersionSet::WriteCurrentStateToManifest(
 
       edit.SetLastSequence(descriptor_last_sequence_);
 
-      const Comparator* ucmp = cfd->user_comparator();
-      assert(ucmp);
       std::string record;
-      if (!edit.EncodeTo(&record, ucmp->timestamp_size())) {
+      if (!edit.EncodeTo(&record)) {
         return Status::Corruption("Unable to Encode VersionEdit:" +
                                   edit.DebugString(true));
       }
@@ -6883,7 +6858,7 @@ InternalIterator* VersionSet::MakeInputIterator(
           const FileMetaData& fmd = *flevel->files[i].file_metadata;
           if (start.has_value() &&
               cfd->user_comparator()->CompareWithoutTimestamp(
-                  *start, fmd.largest.user_key()) > 0) {
+                  start.value(), fmd.largest.user_key()) > 0) {
             continue;
           }
           // We should be able to filter out the case where the end key
@@ -6891,7 +6866,7 @@ InternalIterator* VersionSet::MakeInputIterator(
           // We try to be extra safe here.
           if (end.has_value() &&
               cfd->user_comparator()->CompareWithoutTimestamp(
-                  *end, fmd.smallest.user_key()) < 0) {
+                  end.value(), fmd.smallest.user_key()) < 0) {
             continue;
           }
           TruncatedRangeDelIterator* range_tombstone_iter = nullptr;

db/version_set.h

@@ -1273,8 +1273,8 @@ class VersionSet {
 
   // printf contents (for debugging)
   Status DumpManifest(Options& options, std::string& manifestFileName,
-                      bool verbose, bool hex = false, bool json = false,
-                      const std::vector<ColumnFamilyDescriptor>& cf_descs = {});
+                      bool verbose, bool hex = false, bool json = false);
+
 
   const std::string& DbSessionId() const { return db_session_id_; }
 

db/version_set_test.cc

@@ -3320,7 +3320,7 @@ class VersionSetTestMissingFiles : public VersionSetTestBase,
     ++last_seqno_;
     assert(log_writer_.get() != nullptr);
     std::string record;
-    ASSERT_TRUE(edit.EncodeTo(&record, 0 /* ts_sz */));
+    ASSERT_TRUE(edit.EncodeTo(&record));
     Status s = log_writer_->AddRecord(record);
     ASSERT_OK(s);
   }

env/env_encryption.cc

@@ -30,6 +30,14 @@ std::shared_ptr<EncryptionProvider> EncryptionProvider::NewCTRProvider(
   return std::make_shared<CTREncryptionProvider>(cipher);
 }
 
+// Read up to "n" bytes from the file.  "scratch[0..n-1]" may be
+// written by this routine.  Sets "*result" to the data that was
+// read (including if fewer than "n" bytes were successfully read).
+// May set "*result" to point at data in "scratch[0..n-1]", so
+// "scratch[0..n-1]" must be live when "*result" is used.
+// If an error was encountered, returns a non-OK status.
+//
+// REQUIRES: External synchronization
 IOStatus EncryptedSequentialFile::Read(size_t n, const IOOptions& options,
                                        Slice* result, char* scratch,
                                        IODebugContext* dbg) {
@@ -44,12 +52,19 @@ IOStatus EncryptedSequentialFile::Read(size_t n, const IOOptions& options,
         stream_->Decrypt(offset_, (char*)result->data(), result->size()));
   }
   if (io_s.ok()) {
-    offset_ += result->size();  // We've already read data from disk, so update
+    offset_ += result->size();  // We've already ready data from disk, so update
                                 // offset_ even if decryption fails.
   }
   return io_s;
 }
 
+// Skip "n" bytes from the file. This is guaranteed to be no
+// slower that reading the same data, but may be faster.
+//
+// If end of file is reached, skipping will stop at the end of the
+// file, and Skip will return OK.
+//
+// REQUIRES: External synchronization
 IOStatus EncryptedSequentialFile::Skip(uint64_t n) {
   auto status = file_->Skip(n);
   if (!status.ok()) {
@@ -59,19 +74,28 @@ IOStatus EncryptedSequentialFile::Skip(uint64_t n) {
   return status;
 }
 
+// Indicates the upper layers if the current SequentialFile implementation
+// uses direct IO.
 bool EncryptedSequentialFile::use_direct_io() const {
   return file_->use_direct_io();
 }
 
+// Use the returned alignment value to allocate
+// aligned buffer for Direct I/O
 size_t EncryptedSequentialFile::GetRequiredBufferAlignment() const {
   return file_->GetRequiredBufferAlignment();
 }
 
+// Remove any kind of caching of data from the offset to offset+length
+// of this file. If the length is 0, then it refers to the end of file.
+// If the system is not caching the file contents, then this is a noop.
 IOStatus EncryptedSequentialFile::InvalidateCache(size_t offset,
                                                   size_t length) {
   return file_->InvalidateCache(offset + prefixLength_, length);
 }
 
+// Positioned Read for direct I/O
+// If Direct I/O enabled, offset, n, and scratch should be properly aligned
 IOStatus EncryptedSequentialFile::PositionedRead(uint64_t offset, size_t n,
                                                  const IOOptions& options,
                                                  Slice* result, char* scratch,
@@ -91,6 +115,16 @@ IOStatus EncryptedSequentialFile::PositionedRead(uint64_t offset, size_t n,
   return io_s;
 }
 
+// Read up to "n" bytes from the file starting at "offset".
+// "scratch[0..n-1]" may be written by this routine.  Sets "*result"
+// to the data that was read (including if fewer than "n" bytes were
+// successfully read).  May set "*result" to point at data in
+// "scratch[0..n-1]", so "scratch[0..n-1]" must be live when
+// "*result" is used.  If an error was encountered, returns a non-OK
+// status.
+//
+// Safe for concurrent use by multiple threads.
+// If Direct I/O enabled, offset, n, and scratch should be aligned properly.
 IOStatus EncryptedRandomAccessFile::Read(uint64_t offset, size_t n,
                                          const IOOptions& options,
                                          Slice* result, char* scratch,
@@ -109,12 +143,29 @@ IOStatus EncryptedRandomAccessFile::Read(uint64_t offset, size_t n,
   return io_s;
 }
 
+// Readahead the file starting from offset by n bytes for caching.
 IOStatus EncryptedRandomAccessFile::Prefetch(uint64_t offset, size_t n,
                                              const IOOptions& options,
                                              IODebugContext* dbg) {
+  // return Status::OK();
   return file_->Prefetch(offset + prefixLength_, n, options, dbg);
 }
 
+// Tries to get an unique ID for this file that will be the same each time
+// the file is opened (and will stay the same while the file is open).
+// Furthermore, it tries to make this ID at most "max_size" bytes. If such an
+// ID can be created this function returns the length of the ID and places it
+// in "id"; otherwise, this function returns 0, in which case "id"
+// may not have been modified.
+//
+// This function guarantees, for IDs from a given environment, two unique ids
+// cannot be made equal to each other by adding arbitrary bytes to one of
+// them. That is, no unique ID is the prefix of another.
+//
+// This function guarantees that the returned ID will not be interpretable as
+// a single varint.
+//
+// Note: these IDs are only valid for the duration of the process.
 size_t EncryptedRandomAccessFile::GetUniqueId(char* id, size_t max_size) const {
   return file_->GetUniqueId(id, max_size);
 };
@@ -123,19 +174,29 @@ void EncryptedRandomAccessFile::Hint(AccessPattern pattern) {
   file_->Hint(pattern);
 }
 
+// Indicates the upper layers if the current RandomAccessFile implementation
+// uses direct IO.
 bool EncryptedRandomAccessFile::use_direct_io() const {
   return file_->use_direct_io();
 }
 
+// Use the returned alignment value to allocate
+// aligned buffer for Direct I/O
 size_t EncryptedRandomAccessFile::GetRequiredBufferAlignment() const {
   return file_->GetRequiredBufferAlignment();
 }
 
+// Remove any kind of caching of data from the offset to offset+length
+// of this file. If the length is 0, then it refers to the end of file.
+// If the system is not caching the file contents, then this is a noop.
 IOStatus EncryptedRandomAccessFile::InvalidateCache(size_t offset,
                                                     size_t length) {
   return file_->InvalidateCache(offset + prefixLength_, length);
 }
 
+// A file abstraction for sequential writing.  The implementation
+// must provide buffering since callers may append small fragments
+// at a time to the file.
 IOStatus EncryptedWritableFile::Append(const Slice& data,
                                        const IOOptions& options,
                                        IODebugContext* dbg) {
@@ -191,39 +252,67 @@ IOStatus EncryptedWritableFile::PositionedAppend(const Slice& data,
   return file_->PositionedAppend(dataToAppend, offset, options, dbg);
 }
 
+// Indicates the upper layers if the current WritableFile implementation
+// uses direct IO.
 bool EncryptedWritableFile::use_direct_io() const {
   return file_->use_direct_io();
 }
 
+// true if Sync() and Fsync() are safe to call concurrently with Append()
+// and Flush().
 bool EncryptedWritableFile::IsSyncThreadSafe() const {
   return file_->IsSyncThreadSafe();
 }
 
+// Use the returned alignment value to allocate
+// aligned buffer for Direct I/O
 size_t EncryptedWritableFile::GetRequiredBufferAlignment() const {
   return file_->GetRequiredBufferAlignment();
 }
 
+/*
+ * Get the size of valid data in the file.
+ */
 uint64_t EncryptedWritableFile::GetFileSize(const IOOptions& options,
                                             IODebugContext* dbg) {
   return file_->GetFileSize(options, dbg) - prefixLength_;
 }
 
+// Truncate is necessary to trim the file to the correct size
+// before closing. It is not always possible to keep track of the file
+// size due to whole pages writes. The behavior is undefined if called
+// with other writes to follow.
 IOStatus EncryptedWritableFile::Truncate(uint64_t size,
                                          const IOOptions& options,
                                          IODebugContext* dbg) {
   return file_->Truncate(size + prefixLength_, options, dbg);
 }
 
+// Remove any kind of caching of data from the offset to offset+length
+// of this file. If the length is 0, then it refers to the end of file.
+// If the system is not caching the file contents, then this is a noop.
+// This call has no effect on dirty pages in the cache.
 IOStatus EncryptedWritableFile::InvalidateCache(size_t offset, size_t length) {
   return file_->InvalidateCache(offset + prefixLength_, length);
 }
 
+// Sync a file range with disk.
+// offset is the starting byte of the file range to be synchronized.
+// nbytes specifies the length of the range to be synchronized.
+// This asks the OS to initiate flushing the cached data to disk,
+// without waiting for completion.
+// Default implementation does nothing.
 IOStatus EncryptedWritableFile::RangeSync(uint64_t offset, uint64_t nbytes,
                                           const IOOptions& options,
                                           IODebugContext* dbg) {
   return file_->RangeSync(offset + prefixLength_, nbytes, options, dbg);
 }
 
+// PrepareWrite performs any necessary preparation for a write
+// before the write actually occurs.  This allows for pre-allocation
+// of space on devices where it can result in less file
+// fragmentation and/or less waste from over-zealous filesystem
+// pre-allocation.
 void EncryptedWritableFile::PrepareWrite(size_t offset, size_t len,
                                          const IOOptions& options,
                                          IODebugContext* dbg) {
@@ -241,6 +330,7 @@ void EncryptedWritableFile::GetPreallocationStatus(
   file_->GetPreallocationStatus(block_size, last_allocated_block);
 }
 
+// Pre-allocates space for a file.
 IOStatus EncryptedWritableFile::Allocate(uint64_t offset, uint64_t len,
                                          const IOOptions& options,
                                          IODebugContext* dbg) {
@@ -262,14 +352,22 @@ IOStatus EncryptedWritableFile::Close(const IOOptions& options,
   return file_->Close(options, dbg);
 }
 
+// A file abstraction for random reading and writing.
+
+// Indicates if the class makes use of direct I/O
+// If false you must pass aligned buffer to Write()
 bool EncryptedRandomRWFile::use_direct_io() const {
   return file_->use_direct_io();
 }
 
+// Use the returned alignment value to allocate
+// aligned buffer for Direct I/O
 size_t EncryptedRandomRWFile::GetRequiredBufferAlignment() const {
   return file_->GetRequiredBufferAlignment();
 }
 
+// Write bytes in `data` at  offset `offset`, Returns Status::OK() on success.
+// Pass aligned buffer when use_direct_io() returns true.
 IOStatus EncryptedRandomRWFile::Write(uint64_t offset, const Slice& data,
                                       const IOOptions& options,
                                       IODebugContext* dbg) {
@@ -296,6 +394,9 @@ IOStatus EncryptedRandomRWFile::Write(uint64_t offset, const Slice& data,
   return file_->Write(offset, dataToWrite, options, dbg);
 }
 
+// Read up to `n` bytes starting from offset `offset` and store them in
+// result, provided `scratch` size should be at least `n`.
+// Returns Status::OK() on success.
 IOStatus EncryptedRandomRWFile::Read(uint64_t offset, size_t n,
                                      const IOOptions& options, Slice* result,
                                      char* scratch, IODebugContext* dbg) const {
@@ -577,6 +678,7 @@ class EncryptedFileSystemImpl : public EncryptedFileSystem {
     return provider_->AddCipher(descriptor, cipher, len, for_write);
   }
 
+  // NewSequentialFile opens a file for sequential reading.
   IOStatus NewSequentialFile(const std::string& fname,
                              const FileOptions& options,
                              std::unique_ptr<FSSequentialFile>* result,
@@ -614,6 +716,7 @@ class EncryptedFileSystemImpl : public EncryptedFileSystem {
     return status;
   }
 
+  // NewRandomAccessFile opens a file for random read access.
   IOStatus NewRandomAccessFile(const std::string& fname,
                                const FileOptions& options,
                                std::unique_ptr<FSRandomAccessFile>* result,
@@ -644,6 +747,7 @@ class EncryptedFileSystemImpl : public EncryptedFileSystem {
     return status;
   }
 
+  // NewWritableFile opens a file for sequential writing.
   IOStatus NewWritableFile(const std::string& fname, const FileOptions& options,
                            std::unique_ptr<FSWritableFile>* result,
                            IODebugContext* dbg) override {
@@ -661,6 +765,13 @@ class EncryptedFileSystemImpl : public EncryptedFileSystem {
     return CreateWritableEncryptedFile(fname, underlying, options, result, dbg);
   }
 
+  // Create an object that writes to a new file with the specified
+  // name.  Deletes any existing file with the same name and creates a
+  // new file.  On success, stores a pointer to the new file in
+  // *result and returns OK.  On failure stores nullptr in *result and
+  // returns non-OK.
+  //
+  // The returned file will only be accessed by one thread at a time.
   IOStatus ReopenWritableFile(const std::string& fname,
                               const FileOptions& options,
                               std::unique_ptr<FSWritableFile>* result,
@@ -679,6 +790,7 @@ class EncryptedFileSystemImpl : public EncryptedFileSystem {
     return CreateWritableEncryptedFile(fname, underlying, options, result, dbg);
   }
 
+  // Reuse an existing file by renaming it and opening it as writable.
   IOStatus ReuseWritableFile(const std::string& fname,
                              const std::string& old_fname,
                              const FileOptions& options,
@@ -698,6 +810,11 @@ class EncryptedFileSystemImpl : public EncryptedFileSystem {
     return CreateWritableEncryptedFile(fname, underlying, options, result, dbg);
   }
 
+  // Open `fname` for random read and write, if file doesn't exist the file
+  // will be created.  On success, stores a pointer to the new file in
+  // *result and returns OK.  On failure returns non-OK.
+  //
+  // The returned file will only be accessed by one thread at a time.
   IOStatus NewRandomRWFile(const std::string& fname, const FileOptions& options,
                            std::unique_ptr<FSRandomRWFile>* result,
                            IODebugContext* dbg) override {
@@ -737,6 +854,20 @@ class EncryptedFileSystemImpl : public EncryptedFileSystem {
     return status;
   }
 
+  // Store in *result the attributes of the children of the specified
+  // directory.
+  // In case the implementation lists the directory prior to iterating the
+  // files
+  // and files are concurrently deleted, the deleted files will be omitted
+  // from
+  // result.
+  // The name attributes are relative to "dir".
+  // Original contents of *results are dropped.
+  // Returns OK if "dir" exists and "*result" contains its children.
+  //         NotFound if "dir" does not exist, the calling process does not
+  //         have
+  //                  permission to access "dir", or if "dir" is invalid.
+  //         IOError if an IO Error was encountered
   IOStatus GetChildrenFileAttributes(const std::string& dir,
                                      const IOOptions& options,
                                      std::vector<FileAttributes>* result,
@@ -763,6 +894,7 @@ class EncryptedFileSystemImpl : public EncryptedFileSystem {
     return IOStatus::OK();
   }
 
+  // Store the size of fname in *file_size.
   IOStatus GetFileSize(const std::string& fname, const IOOptions& options,
                        uint64_t* file_size, IODebugContext* dbg) override {
     auto status =
@@ -808,13 +940,16 @@ std::shared_ptr<FileSystem> NewEncryptedFS(
     return nullptr;
   }
 }
-
+// Returns an Env that encrypts data when stored on disk and decrypts data when
+// read from disk.
 Env* NewEncryptedEnv(Env* base_env,
                      const std::shared_ptr<EncryptionProvider>& provider) {
   return new CompositeEnvWrapper(
       base_env, NewEncryptedFS(base_env->GetFileSystem(), provider));
 }
 
+// Encrypt one or more (partial) blocks of data at the file offset.
+// Length of data is given in dataSize.
 Status BlockAccessCipherStream::Encrypt(uint64_t fileOffset, char* data,
                                         size_t dataSize) {
   // Calculate block index
@@ -833,7 +968,7 @@ Status BlockAccessCipherStream::Encrypt(uint64_t fileOffset, char* data,
     if (n != blockSize) {
       // We're not encrypting a full block.
       // Copy data to blockBuffer
-      if (!blockBuffer) {
+      if (!blockBuffer.get()) {
         // Allocate buffer
         blockBuffer = std::unique_ptr<char[]>(new char[blockSize]);
       }
@@ -859,6 +994,8 @@ Status BlockAccessCipherStream::Encrypt(uint64_t fileOffset, char* data,
   }
 }
 
+// Decrypt one or more (partial) blocks of data at the file offset.
+// Length of data is given in dataSize.
 Status BlockAccessCipherStream::Decrypt(uint64_t fileOffset, char* data,
                                         size_t dataSize) {
   // Calculate block index
@@ -877,7 +1014,7 @@ Status BlockAccessCipherStream::Decrypt(uint64_t fileOffset, char* data,
     if (n != blockSize) {
       // We're not decrypting a full block.
       // Copy data to blockBuffer
-      if (!blockBuffer) {
+      if (!blockBuffer.get()) {
         // Allocate buffer
         blockBuffer = std::unique_ptr<char[]>(new char[blockSize]);
       }
@@ -918,7 +1055,6 @@ static std::unordered_map<std::string, OptionTypeInfo>
          {0 /* No offset, whole struct*/, OptionType::kInt,
           OptionVerificationType::kNormal, OptionTypeFlags::kNone}},
 };
-
 // Implements a BlockCipher using ROT13.
 //
 // Note: This is a sample implementation of BlockCipher,
@@ -935,17 +1071,22 @@ class ROT13BlockCipher : public BlockCipher {
 
   static const char* kClassName() { return "ROT13"; }
   const char* Name() const override { return kClassName(); }
-
+  // BlockSize returns the size of each block supported by this cipher stream.
   size_t BlockSize() override { return blockSize_; }
+
+  // Encrypt a block of data.
+  // Length of data is equal to BlockSize().
   Status Encrypt(char* data) override {
     for (size_t i = 0; i < blockSize_; ++i) {
       data[i] += 13;
     }
     return Status::OK();
   }
+
+  // Decrypt a block of data.
+  // Length of data is equal to BlockSize().
   Status Decrypt(char* data) override { return Encrypt(data); }
 };
-
 static const std::unordered_map<std::string, OptionTypeInfo>
     ctr_encryption_provider_type_info = {
         {"cipher",
@@ -955,11 +1096,14 @@ static const std::unordered_map<std::string, OptionTypeInfo>
 };
 }  // anonymous namespace
 
+// Allocate scratch space which is passed to EncryptBlock/DecryptBlock.
 void CTRCipherStream::AllocateScratch(std::string& scratch) {
   auto blockSize = cipher_->BlockSize();
   scratch.reserve(blockSize);
 }
 
+// Encrypt a block of data at the given block index.
+// Length of data is equal to BlockSize();
 Status CTRCipherStream::EncryptBlock(uint64_t blockIndex, char* data,
                                      char* scratch) {
   // Create nonce + counter
@@ -967,7 +1111,7 @@ Status CTRCipherStream::EncryptBlock(uint64_t blockIndex, char* data,
   memmove(scratch, iv_.data(), blockSize);
   EncodeFixed64(scratch, blockIndex + initialCounter_);
 
-  // Encrypt nonce + counter
+  // Encrypt nonce+counter
   auto status = cipher_->Encrypt(scratch);
   if (!status.ok()) {
     return status;
@@ -980,6 +1124,8 @@ Status CTRCipherStream::EncryptBlock(uint64_t blockIndex, char* data,
   return Status::OK();
 }
 
+// Decrypt a block of data at the given block index.
+// Length of data is equal to BlockSize();
 Status CTRCipherStream::DecryptBlock(uint64_t blockIndex, char* data,
                                      char* scratch) {
   // For CTR decryption & encryption are the same
@@ -1001,6 +1147,10 @@ bool CTREncryptionProvider::IsInstanceOf(const std::string& name) const {
   }
 }
 
+// GetPrefixLength returns the length of the prefix that is added to every file
+// and used for storing encryption options.
+// For optimal performance, the prefix length should be a multiple of
+// the page size.
 size_t CTREncryptionProvider::GetPrefixLength() const {
   return defaultPrefixLength;
 }
@@ -1029,6 +1179,8 @@ static void decodeCTRParameters(const char* prefix, size_t blockSize,
   iv = Slice(prefix + blockSize, blockSize);
 }
 
+// CreateNewPrefix initialized an allocated block of prefix memory
+// for a new file.
 Status CTREncryptionProvider::CreateNewPrefix(const std::string& /*fname*/,
                                               char* prefix,
                                               size_t prefixLength) const {
@@ -1060,8 +1212,10 @@ Status CTREncryptionProvider::CreateNewPrefix(const std::string& /*fname*/,
     status = cipherStream.Encrypt(0, prefix + (2 * blockSize),
                                   prefixLength - (2 * blockSize));
   }
-
-  return status;
+  if (!status.ok()) {
+    return status;
+  }
+  return Status::OK();
 }
 
 // PopulateSecretPrefixPart initializes the data into a new prefix block
@@ -1113,7 +1267,7 @@ Status CTREncryptionProvider::CreateCipherStream(
 }
 
 // CreateCipherStreamFromPrefix creates a block access cipher stream for a file
-// given name and options. The given prefix is already decrypted.
+// given given name and options. The given prefix is already decrypted.
 Status CTREncryptionProvider::CreateCipherStreamFromPrefix(
     const std::string& /*fname*/, const EnvOptions& /*options*/,
     uint64_t initialCounter, const Slice& iv, const Slice& /*prefix*/,

env/env_encryption_ctr.h

@@ -27,13 +27,19 @@ class CTRCipherStream final : public BlockAccessCipherStream {
       : cipher_(c), iv_(iv, c->BlockSize()), initialCounter_(initialCounter){};
   virtual ~CTRCipherStream(){};
 
+  // BlockSize returns the size of each block supported by this cipher stream.
   size_t BlockSize() override { return cipher_->BlockSize(); }
 
  protected:
+  // Allocate scratch space which is passed to EncryptBlock/DecryptBlock.
   void AllocateScratch(std::string&) override;
 
+  // Encrypt a block of data at the given block index.
+  // Length of data is equal to BlockSize();
   Status EncryptBlock(uint64_t blockIndex, char* data, char* scratch) override;
 
+  // Decrypt a block of data at the given block index.
+  // Length of data is equal to BlockSize();
   Status DecryptBlock(uint64_t blockIndex, char* data, char* scratch) override;
 };
 
@@ -60,9 +66,20 @@ class CTREncryptionProvider : public EncryptionProvider {
   static const char* kClassName() { return "CTR"; }
   const char* Name() const override { return kClassName(); }
   bool IsInstanceOf(const std::string& name) const override;
+  // GetPrefixLength returns the length of the prefix that is added to every
+  // file
+  // and used for storing encryption options.
+  // For optimal performance when using direct IO, the prefix length should be a
+  // multiple of the page size.
   size_t GetPrefixLength() const override;
+
+  // CreateNewPrefix initialized an allocated block of prefix memory
+  // for a new file.
   Status CreateNewPrefix(const std::string& fname, char* prefix,
                          size_t prefixLength) const override;
+
+  // CreateCipherStream creates a block access cipher stream for a file given
+  // given name and options.
   Status CreateCipherStream(
       const std::string& fname, const EnvOptions& options, Slice& prefix,
       std::unique_ptr<BlockAccessCipherStream>* result) override;

env/unique_id_gen.cc

@@ -21,7 +21,6 @@
 #ifdef __SSE4_2__
 #ifdef _WIN32
 #include <intrin.h>
-#define _rdtsc() __rdtsc()
 #else
 #include <x86intrin.h>
 #endif

file/prefetch_test.cc

@@ -128,14 +128,7 @@ std::string BuildKey(int num, std::string postfix = "") {
   return "my_key_" + std::to_string(num) + postfix;
 }
 
-// This test verifies the following basic functionalities of prefetching:
-// (1) If underline file system supports prefetch, and directIO is not enabled
-// make sure prefetch() is called and FilePrefetchBuffer is not used.
-// (2) If underline file system doesn't support prefetch, or directIO is
-// enabled, make sure prefetch() is not called and FilePrefetchBuffer is
-// used.
-// (3) Measure read bytes, hit and miss of SST's tail prefetching during table
-// open.
+// This test verifies the basic functionality of prefetching.
 TEST_P(PrefetchTest, Basic) {
   // First param is if the mockFS support_prefetch or not
   bool support_prefetch =
@@ -151,7 +144,6 @@ TEST_P(PrefetchTest, Basic) {
   Options options;
   SetGenericOptions(env.get(), use_direct_io, options);
   options.statistics = CreateDBStatistics();
-  options.compaction_readahead_size = 2 * 1024 * 1024;
 
   const int kNumKeys = 1100;
   int buff_prefetch_count = 0;
@@ -173,7 +165,6 @@ TEST_P(PrefetchTest, Basic) {
     ASSERT_OK(batch.Put(BuildKey(i), "value for range 1 key"));
   }
   ASSERT_OK(db_->Write(WriteOptions(), &batch));
-  ASSERT_OK(dbfull()->TEST_WaitForFlushMemTable());
 
   // create second key range
   batch.Clear();
@@ -181,7 +172,6 @@ TEST_P(PrefetchTest, Basic) {
     ASSERT_OK(batch.Put(BuildKey(i, "key2"), "value for range 2 key"));
   }
   ASSERT_OK(db_->Write(WriteOptions(), &batch));
-  ASSERT_OK(dbfull()->TEST_WaitForFlushMemTable());
 
   // delete second key range
   batch.Clear();
@@ -189,20 +179,6 @@ TEST_P(PrefetchTest, Basic) {
     ASSERT_OK(batch.Delete(BuildKey(i, "key2")));
   }
   ASSERT_OK(db_->Write(WriteOptions(), &batch));
-  ASSERT_OK(db_->Flush(FlushOptions()));
-
-  // To verify SST file tail prefetch (once per file) during flush output
-  // verification
-  if (support_prefetch && !use_direct_io) {
-    ASSERT_TRUE(fs->IsPrefetchCalled());
-    ASSERT_EQ(3, fs->GetPrefetchCount());
-    ASSERT_EQ(0, buff_prefetch_count);
-    fs->ClearPrefetchCount();
-  } else {
-    ASSERT_FALSE(fs->IsPrefetchCalled());
-    ASSERT_EQ(buff_prefetch_count, 3);
-    buff_prefetch_count = 0;
-  }
 
   // compact database
   std::string start_key = BuildKey(0);
@@ -229,27 +205,25 @@ TEST_P(PrefetchTest, Basic) {
   const uint64_t cur_table_open_prefetch_tail_hit =
       options.statistics->getTickerCount(TABLE_OPEN_PREFETCH_TAIL_HIT);
 
-  // To verify prefetch during compaction input read
   if (support_prefetch && !use_direct_io) {
+    // If underline file system supports prefetch, and directIO is not enabled
+    // make sure prefetch() is called and FilePrefetchBuffer is not used.
     ASSERT_TRUE(fs->IsPrefetchCalled());
-    // To rule out false positive by the SST file tail prefetch during
-    // compaction output verification
-    ASSERT_GT(fs->GetPrefetchCount(), 1);
-    ASSERT_EQ(0, buff_prefetch_count);
     fs->ClearPrefetchCount();
+    ASSERT_EQ(0, buff_prefetch_count);
   } else {
+    // If underline file system doesn't support prefetch, or directIO is
+    // enabled, make sure prefetch() is not called and FilePrefetchBuffer is
+    // used.
     ASSERT_FALSE(fs->IsPrefetchCalled());
-    // To rule out false positive by the SST file tail prefetch during
-    // compaction output verification
-    ASSERT_GT(buff_prefetch_count, 1);
-    buff_prefetch_count = 0;
-
+    ASSERT_GT(buff_prefetch_count, 0);
     ASSERT_GT(cur_table_open_prefetch_tail_read.count,
               prev_table_open_prefetch_tail_read.count);
     ASSERT_GT(cur_table_open_prefetch_tail_hit,
               prev_table_open_prefetch_tail_hit);
     ASSERT_GE(cur_table_open_prefetch_tail_miss,
               prev_table_open_prefetch_tail_miss);
+    buff_prefetch_count = 0;
   }
 
   // count the keys
@@ -262,7 +236,7 @@ TEST_P(PrefetchTest, Basic) {
     (void)num_keys;
   }
 
-  // To verify prefetch during user scan
+  // Make sure prefetch is called only if file system support prefetch.
   if (support_prefetch && !use_direct_io) {
     ASSERT_TRUE(fs->IsPrefetchCalled());
     fs->ClearPrefetchCount();

file/random_access_file_reader.cc

@@ -97,15 +97,6 @@ IOStatus RandomAccessFileReader::Read(
 
   IOStatus io_s;
   uint64_t elapsed = 0;
-  size_t alignment = file_->GetRequiredBufferAlignment();
-  bool is_aligned = false;
-  if (scratch != nullptr) {
-    // Check if offset, length and buffer are aligned.
-    is_aligned = (offset & (alignment - 1)) == 0 &&
-                 (n & (alignment - 1)) == 0 &&
-                 (uintptr_t(scratch) & (alignment - 1)) == 0;
-  }
-
   {
     StopWatch sw(clock_, stats_, hist_type_,
                  (opts.io_activity != Env::IOActivity::kUnknown)
@@ -115,7 +106,8 @@ IOStatus RandomAccessFileReader::Read(
                  true /*delay_enabled*/);
     auto prev_perf_level = GetPerfLevel();
     IOSTATS_TIMER_GUARD(read_nanos);
-    if (use_direct_io() && is_aligned == false) {
+    if (use_direct_io()) {
+      size_t alignment = file_->GetRequiredBufferAlignment();
       size_t aligned_offset =
           TruncateToPageBoundary(alignment, static_cast<size_t>(offset));
       size_t offset_advance = static_cast<size_t>(offset) - aligned_offset;
@@ -190,9 +182,9 @@ IOStatus RandomAccessFileReader::Read(
           if (rate_limiter_->IsRateLimited(RateLimiter::OpType::kRead)) {
             sw.DelayStart();
           }
-          allowed = rate_limiter_->RequestToken(
-              n - pos, (use_direct_io() ? alignment : 0), rate_limiter_priority,
-              stats_, RateLimiter::OpType::kRead);
+          allowed = rate_limiter_->RequestToken(n - pos, 0 /* alignment */,
+                                                rate_limiter_priority, stats_,
+                                                RateLimiter::OpType::kRead);
           if (rate_limiter_->IsRateLimited(RateLimiter::OpType::kRead)) {
             sw.DelayStop();
           }

include/rocksdb/advanced_options.h

@@ -858,23 +858,24 @@ struct AdvancedColumnFamilyOptions {
   // Dynamically changeable through SetOptions() API
   bool report_bg_io_stats = false;
 
-  // This option has different meanings for different compaction styles:
-  //
-  // Leveled: Non-bottom-level files with all keys older than TTL will go
-  //    through the compaction process. This usually happens in a cascading
-  //    way so that those entries will be compacted to bottommost level/file.
-  //    The feature is used to remove stale entries that have been deleted or
-  //    updated from the file system.
-  //
-  // FIFO: Files with all keys older than TTL will be deleted. TTL is only
-  //    supported if option max_open_files is set to -1.
-  //
+  // Files containing updates older than TTL will go through the compaction
+  // process. This usually happens in a cascading way so that those entries
+  // will be compacted to bottommost level/file.
+  // The feature is used to remove stale entries that have been deleted or
+  // updated from the file system.
+  // Pre-req: This needs max_open_files to be set to -1.
+  // In Level: Non-bottom-level files older than TTL will go through the
+  //           compaction process.
+  // In FIFO: Files older than TTL will be deleted.
   // unit: seconds. Ex: 1 day = 1 * 24 * 60 * 60
+  // In FIFO, this option will have the same meaning as
+  // periodic_compaction_seconds. Whichever stricter will be used.
   // 0 means disabling.
   // UINT64_MAX - 1 (0xfffffffffffffffe) is special flag to allow RocksDB to
   // pick default.
   //
-  // Default: 30 days if using block based table. 0 (disable) otherwise.
+  // Default: 30 days for leveled compaction + block based table. disable
+  //          otherwise.
   //
   // Dynamically changeable through SetOptions() API
   uint64_t ttl = 0xfffffffffffffffe;
@@ -890,9 +891,12 @@ struct AdvancedColumnFamilyOptions {
   // age is based on the file's last modified time (given by the underlying
   // Env).
   //
-  // Supported in leveled and universal compaction.
+  // Supported in all compaction styles.
   // In Universal compaction, rocksdb will try to do a full compaction when
   // possible, see more in UniversalCompactionBuilder::PickPeriodicCompaction().
+  // In FIFO compaction, this option has the same meaning as TTL and whichever
+  // stricter will be used.
+  // Pre-req: max_open_file == -1.
   // unit: seconds. Ex: 7 days = 7 * 24 * 60 * 60
   //
   // Values:
@@ -901,6 +905,9 @@ struct AdvancedColumnFamilyOptions {
   //     as needed. For now, RocksDB will change this value to 30 days
   //     (i.e 30 * 24 * 60 * 60) so that every file goes through the compaction
   //     process at least once every 30 days if not compacted sooner.
+  //     In FIFO compaction, since the option has the same meaning as ttl,
+  //     when this value is left default, and ttl is left to 0, 30 days will be
+  //     used. Otherwise, min(ttl, periodic_compaction_seconds) will be used.
   //
   // Default: UINT64_MAX - 1 (allow RocksDB to auto-tune)
   //
@@ -1113,30 +1120,11 @@ struct AdvancedColumnFamilyOptions {
   //
   // When it's false, the user-defined timestamps will be removed from the user
   // keys when data is flushed from memtables to SST files. Other places that
-  // user keys can be persisted like file boundaries in file metadata and blob
-  // files go through a similar process. There are two major motivations
-  // for this flag:
-  // 1) backward compatibility: if the user later decides to
-  // disable the user-defined timestamp feature for the column family, these SST
-  // files can be handled by a user comparator that is not aware of user-defined
-  // timestamps.
-  // 2) enable user-defined timestamp feature for an existing column family
-  // while set this flag to be `false`: user keys in the newly generated SST
-  // files are of the same format as the existing SST files.
-  //
-  // When setting this flag to `false`, users should also call
-  // `DB::IncreaseFullHistoryTsLow` to set a cutoff timestamp for flush. RocksDB
-  // refrains from flushing a memtable with data still above
-  // the cutoff timestamp with best effort. Users can do user-defined
-  // multi-versioned read above the cutoff timestamp. When users try to read
-  // below the cutoff timestamp, an error will be returned.
-  //
-  // Note that if WAL is enabled, unlike SST files, user-defined timestamps are
-  // persisted to WAL even if this flag is set to `false`. The benefit of this
-  // is that user-defined timestamps can be recovered with the caveat that users
-  // should flush all memtables so there is no active WAL files before doing a
-  // downgrade or toggling on / off the user-defined timestamp feature on a
-  // column family.
+  // user keys can be persisted like WAL and blob files go through a similar
+  // process. Users should call `DB::IncreaseFullHistoryTsLow` to set a cutoff
+  // timestamp. RocksDB refrains from flushing a memtable with data still above
+  // the cutoff timestamp with best effort. When users try to read below the
+  // cutoff timestamp, an error will be returned.
   //
   // Default: true (user-defined timestamps are persisted)
   // Not dynamically changeable, change it requires db restart and

include/rocksdb/c.h

@@ -411,16 +411,6 @@ rocksdb_create_column_family(rocksdb_t* db,
                              const rocksdb_options_t* column_family_options,
                              const char* column_family_name, char** errptr);
 
-extern ROCKSDB_LIBRARY_API rocksdb_column_family_handle_t**
-rocksdb_create_column_families(rocksdb_t* db,
-                               const rocksdb_options_t* column_family_options,
-                               int num_column_families,
-                               const char* const* column_family_names,
-                               size_t* lencfs, char** errptr);
-
-extern ROCKSDB_LIBRARY_API void rocksdb_create_column_families_destroy(
-    rocksdb_column_family_handle_t** list);
-
 extern ROCKSDB_LIBRARY_API rocksdb_column_family_handle_t*
 rocksdb_create_column_family_with_ttl(
     rocksdb_t* db, const rocksdb_options_t* column_family_options,
@@ -1528,8 +1518,7 @@ extern ROCKSDB_LIBRARY_API void rocksdb_options_set_hash_skip_list_rep(
 extern ROCKSDB_LIBRARY_API void rocksdb_options_set_hash_link_list_rep(
     rocksdb_options_t*, size_t);
 extern ROCKSDB_LIBRARY_API void rocksdb_options_set_plain_table_factory(
-    rocksdb_options_t*, uint32_t, int, double, size_t, size_t, char,
-    unsigned char, unsigned char);
+    rocksdb_options_t*, uint32_t, int, double, size_t);
 
 extern ROCKSDB_LIBRARY_API void rocksdb_options_set_min_level_to_compress(
     rocksdb_options_t* opt, int level);
@@ -2457,12 +2446,6 @@ extern ROCKSDB_LIBRARY_API char* rocksdb_transactiondb_property_value(
 extern ROCKSDB_LIBRARY_API int rocksdb_transactiondb_property_int(
     rocksdb_transactiondb_t* db, const char* propname, uint64_t* out_val);
 
-extern ROCKSDB_LIBRARY_API rocksdb_t* rocksdb_transactiondb_get_base_db(
-    rocksdb_transactiondb_t* txn_db);
-
-extern ROCKSDB_LIBRARY_API void rocksdb_transactiondb_close_base_db(
-    rocksdb_t* base_db);
-
 extern ROCKSDB_LIBRARY_API rocksdb_transaction_t* rocksdb_transaction_begin(
     rocksdb_transactiondb_t* txn_db,
     const rocksdb_writeoptions_t* write_options,

include/rocksdb/cache.h

@@ -32,18 +32,18 @@ class SecondaryCache;
 // example, HyperClockCache is not usable as a general cache because it expects
 // only fixed-size block cache keys, but this limitation is not yet reflected
 // in the API function signatures.
-// * Phase 1 (done) - Make both BlockCache and RowCache aliases for Cache,
-// and make a factory function for row caches. Encourage users of row_cache
-// (not common) to switch to the factory function for row caches.
-// * Phase 2 - Split off RowCache as its own class, removing secondary
+// * Phase 1 (done) - Make both BlockCache and GeneralCache aliases for Cache,
+// and make a factory function for general caches. Encourage users of row_cache
+// (not common) to switch to the factory function for general caches.
+// * Phase 2 - Split off GenericCache as its own class, removing secondary
 // cache support features and more from the API to simplify it. Between Phase 1
 // and Phase 2 users of row_cache will need to update their code. Any time
-// after Phase 2, the block cache and row cache APIs can become more specialized
-// in ways incompatible with general caches.
+// after Phase 2, the block cache API can become more specialized in ways
+// incompatible with general caches.
 // * Phase 3 - Move existing RocksDB uses of Cache to BlockCache, and deprecate
 // (but not yet remove) Cache as an alias for BlockCache.
 using BlockCache = Cache;
-using RowCache = Cache;
+using GeneralCache = Cache;
 
 // Classifications of block cache entries.
 //
@@ -155,7 +155,7 @@ struct ShardedCacheOptions {
   CacheMetadataChargePolicy metadata_charge_policy =
       kDefaultCacheMetadataChargePolicy;
 
-  // A SecondaryCache instance to use the non-volatile tier. For a RowCache
+  // A SecondaryCache instance to use the non-volatile tier. For a GeneralCache
   // this option must be kept as default empty.
   std::shared_ptr<SecondaryCache> secondary_cache;
 
@@ -260,9 +260,9 @@ struct LRUCacheOptions : public ShardedCacheOptions {
   // Construct an instance of LRUCache using these options
   std::shared_ptr<Cache> MakeSharedCache() const;
 
-  // Construct an instance of LRUCache for use as a row cache, typically for
-  // `DBOptions::row_cache`. Some options are not relevant to row caches.
-  std::shared_ptr<RowCache> MakeSharedRowCache() const;
+  // Construct an instance of LRUCache for use as a general cache (e.g. for
+  // row_cache). Some options are not relevant to general caches.
+  std::shared_ptr<GeneralCache> MakeSharedGeneralCache() const;
 };
 
 // DEPRECATED wrapper function

include/rocksdb/env_encryption.h

@@ -67,7 +67,7 @@ class BlockAccessCipherStream {
 // including data loss, unreported corruption, deadlocks, and more.
 class BlockCipher : public Customizable {
  public:
-  virtual ~BlockCipher() {}
+  virtual ~BlockCipher(){};
 
   // Creates a new BlockCipher from the input config_options and value
   // The value describes the type of provider (and potentially optional
@@ -114,13 +114,13 @@ class BlockCipher : public Customizable {
 // including data loss, unreported corruption, deadlocks, and more.
 class EncryptionProvider : public Customizable {
  public:
-  virtual ~EncryptionProvider() {}
+  virtual ~EncryptionProvider(){};
 
-  // Creates a new EncryptionProvider from the input config_options and value.
+  // Creates a new EncryptionProvider from the input config_options and value
   // The value describes the type of provider (and potentially optional
   // configuration parameters) used to create this provider.
   // For example, if the value is "CTR", a CTREncryptionProvider will be
-  // created. If the value is end with "://test" (e.g CTR://test"), the
+  // created. If the value is ends with "://test" (e.g CTR://test"), the
   // provider will be initialized in "TEST" mode prior to being returned.
   //
   // @param config_options  Options to control how this provider is created
@@ -153,7 +153,7 @@ class EncryptionProvider : public Customizable {
                                  size_t prefixLength) const = 0;
 
   // Method to add a new cipher key for use by the EncryptionProvider.
-  // @param descriptor   Descriptor for this key
+  // @param description  Descriptor for this key.
   // @param cipher       The cryptographic key to use
   // @param len          The length of the cipher key
   // @param for_write If true, this cipher should be used for writing files.
@@ -165,15 +165,15 @@ class EncryptionProvider : public Customizable {
                            size_t len, bool for_write) = 0;
 
   // CreateCipherStream creates a block access cipher stream for a file given
-  // name and options.
+  // given name and options.
   virtual Status CreateCipherStream(
       const std::string& fname, const EnvOptions& options, Slice& prefix,
       std::unique_ptr<BlockAccessCipherStream>* result) = 0;
 
   // Returns a string representing an encryption marker prefix for this
   // provider. If a marker is provided, this marker can be used to tell whether
-  // a file is encrypted by this provider.  The marker will also be part of any
-  // encryption prefix for this provider.
+  // or not a file is encrypted by this provider.  The maker will also be part
+  // of any encryption prefix for this provider.
   virtual std::string GetMarker() const { return ""; }
 };
 
@@ -182,7 +182,7 @@ class EncryptedSequentialFile : public FSSequentialFile {
   std::unique_ptr<FSSequentialFile> file_;
   std::unique_ptr<BlockAccessCipherStream> stream_;
   uint64_t offset_;
-  const size_t prefixLength_;
+  size_t prefixLength_;
 
  public:
   // Default ctor. Given underlying sequential file is supposed to be at
@@ -195,22 +195,47 @@ class EncryptedSequentialFile : public FSSequentialFile {
         offset_(prefixLength),
         prefixLength_(prefixLength) {}
 
+  // Read up to "n" bytes from the file.  "scratch[0..n-1]" may be
+  // written by this routine.  Sets "*result" to the data that was
+  // read (including if fewer than "n" bytes were successfully read).
+  // May set "*result" to point at data in "scratch[0..n-1]", so
+  // "scratch[0..n-1]" must be live when "*result" is used.
+  // If an error was encountered, returns a non-OK status.
+  //
+  // REQUIRES: External synchronization
   IOStatus Read(size_t n, const IOOptions& options, Slice* result,
                 char* scratch, IODebugContext* dbg) override;
 
+  // Skip "n" bytes from the file. This is guaranteed to be no
+  // slower that reading the same data, but may be faster.
+  //
+  // If end of file is reached, skipping will stop at the end of the
+  // file, and Skip will return OK.
+  //
+  // REQUIRES: External synchronization
   IOStatus Skip(uint64_t n) override;
 
+  // Indicates the upper layers if the current SequentialFile implementation
+  // uses direct IO.
   bool use_direct_io() const override;
 
+  // Use the returned alignment value to allocate
+  // aligned buffer for Direct I/O
   size_t GetRequiredBufferAlignment() const override;
 
+  // Remove any kind of caching of data from the offset to offset+length
+  // of this file. If the length is 0, then it refers to the end of file.
+  // If the system is not caching the file contents, then this is a noop.
   IOStatus InvalidateCache(size_t offset, size_t length) override;
 
+  // Positioned Read for direct I/O
+  // If Direct I/O enabled, offset, n, and scratch should be properly aligned
   IOStatus PositionedRead(uint64_t offset, size_t n, const IOOptions& options,
                           Slice* result, char* scratch,
                           IODebugContext* dbg) override;
 };
 
+// A file abstraction for randomly reading the contents of a file.
 class EncryptedRandomAccessFile : public FSRandomAccessFile {
  protected:
   std::unique_ptr<FSRandomAccessFile> file_;
@@ -225,24 +250,60 @@ class EncryptedRandomAccessFile : public FSRandomAccessFile {
         stream_(std::move(s)),
         prefixLength_(prefixLength) {}
 
+  // Read up to "n" bytes from the file starting at "offset".
+  // "scratch[0..n-1]" may be written by this routine.  Sets "*result"
+  // to the data that was read (including if fewer than "n" bytes were
+  // successfully read).  May set "*result" to point at data in
+  // "scratch[0..n-1]", so "scratch[0..n-1]" must be live when
+  // "*result" is used.  If an error was encountered, returns a non-OK
+  // status.
+  //
+  // Safe for concurrent use by multiple threads.
+  // If Direct I/O enabled, offset, n, and scratch should be aligned properly.
   IOStatus Read(uint64_t offset, size_t n, const IOOptions& options,
                 Slice* result, char* scratch,
                 IODebugContext* dbg) const override;
 
+  // Readahead the file starting from offset by n bytes for caching.
   IOStatus Prefetch(uint64_t offset, size_t n, const IOOptions& options,
                     IODebugContext* dbg) override;
 
+  // Tries to get an unique ID for this file that will be the same each time
+  // the file is opened (and will stay the same while the file is open).
+  // Furthermore, it tries to make this ID at most "max_size" bytes. If such an
+  // ID can be created this function returns the length of the ID and places it
+  // in "id"; otherwise, this function returns 0, in which case "id"
+  // may not have been modified.
+  //
+  // This function guarantees, for IDs from a given environment, two unique ids
+  // cannot be made equal to each other by adding arbitrary bytes to one of
+  // them. That is, no unique ID is the prefix of another.
+  //
+  // This function guarantees that the returned ID will not be interpretable as
+  // a single varint.
+  //
+  // Note: these IDs are only valid for the duration of the process.
   size_t GetUniqueId(char* id, size_t max_size) const override;
 
   void Hint(AccessPattern pattern) override;
 
+  // Indicates the upper layers if the current RandomAccessFile implementation
+  // uses direct IO.
   bool use_direct_io() const override;
 
+  // Use the returned alignment value to allocate
+  // aligned buffer for Direct I/O
   size_t GetRequiredBufferAlignment() const override;
 
+  // Remove any kind of caching of data from the offset to offset+length
+  // of this file. If the length is 0, then it refers to the end of file.
+  // If the system is not caching the file contents, then this is a noop.
   IOStatus InvalidateCache(size_t offset, size_t length) override;
 };
 
+// A file abstraction for sequential writing.  The implementation
+// must provide buffering since callers may append small fragments
+// at a time to the file.
 class EncryptedWritableFile : public FSWritableFile {
  protected:
   std::unique_ptr<FSWritableFile> file_;
@@ -267,22 +328,50 @@ class EncryptedWritableFile : public FSWritableFile {
                             const IOOptions& options,
                             IODebugContext* dbg) override;
 
+  // true if Sync() and Fsync() are safe to call concurrently with Append()
+  // and Flush().
   bool IsSyncThreadSafe() const override;
 
+  // Indicates the upper layers if the current WritableFile implementation
+  // uses direct IO.
   bool use_direct_io() const override;
 
+  // Use the returned alignment value to allocate
+  // aligned buffer for Direct I/O
   size_t GetRequiredBufferAlignment() const override;
 
+  /*
+   * Get the size of valid data in the file.
+   */
   uint64_t GetFileSize(const IOOptions& options, IODebugContext* dbg) override;
 
+  // Truncate is necessary to trim the file to the correct size
+  // before closing. It is not always possible to keep track of the file
+  // size due to whole pages writes. The behavior is undefined if called
+  // with other writes to follow.
   IOStatus Truncate(uint64_t size, const IOOptions& options,
                     IODebugContext* dbg) override;
 
+  // Remove any kind of caching of data from the offset to offset+length
+  // of this file. If the length is 0, then it refers to the end of file.
+  // If the system is not caching the file contents, then this is a noop.
+  // This call has no effect on dirty pages in the cache.
   IOStatus InvalidateCache(size_t offset, size_t length) override;
 
+  // Sync a file range with disk.
+  // offset is the starting byte of the file range to be synchronized.
+  // nbytes specifies the length of the range to be synchronized.
+  // This asks the OS to initiate flushing the cached data to disk,
+  // without waiting for completion.
+  // Default implementation does nothing.
   IOStatus RangeSync(uint64_t offset, uint64_t nbytes, const IOOptions& options,
                      IODebugContext* dbg) override;
 
+  // PrepareWrite performs any necessary preparation for a write
+  // before the write actually occurs.  This allows for pre-allocation
+  // of space on devices where it can result in less file
+  // fragmentation and/or less waste from over-zealous filesystem
+  // pre-allocation.
   void PrepareWrite(size_t offset, size_t len, const IOOptions& options,
                     IODebugContext* dbg) override;
 
@@ -291,6 +380,7 @@ class EncryptedWritableFile : public FSWritableFile {
   void GetPreallocationStatus(size_t* block_size,
                               size_t* last_allocated_block) override;
 
+  // Pre-allocates space for a file.
   IOStatus Allocate(uint64_t offset, uint64_t len, const IOOptions& options,
                     IODebugContext* dbg) override;
 
@@ -301,6 +391,7 @@ class EncryptedWritableFile : public FSWritableFile {
   IOStatus Close(const IOOptions& options, IODebugContext* dbg) override;
 };
 
+// A file abstraction for random reading and writing.
 class EncryptedRandomRWFile : public FSRandomRWFile {
  protected:
   std::unique_ptr<FSRandomRWFile> file_;
@@ -315,13 +406,22 @@ class EncryptedRandomRWFile : public FSRandomRWFile {
         stream_(std::move(s)),
         prefixLength_(prefixLength) {}
 
+  // Indicates if the class makes use of direct I/O
+  // If false you must pass aligned buffer to Write()
   bool use_direct_io() const override;
 
+  // Use the returned alignment value to allocate
+  // aligned buffer for Direct I/O
   size_t GetRequiredBufferAlignment() const override;
 
+  // Write bytes in `data` at  offset `offset`, Returns Status::OK() on success.
+  // Pass aligned buffer when use_direct_io() returns true.
   IOStatus Write(uint64_t offset, const Slice& data, const IOOptions& options,
                  IODebugContext* dbg) override;
 
+  // Read up to `n` bytes starting from offset `offset` and store them in
+  // result, provided `scratch` size should be at least `n`.
+  // Returns Status::OK() on success.
   IOStatus Read(uint64_t offset, size_t n, const IOOptions& options,
                 Slice* result, char* scratch,
                 IODebugContext* dbg) const override;

include/rocksdb/options.h

@@ -1071,7 +1071,7 @@ struct DBOptions {
   //
   // By default, i.e., when it is false, rocksdb does not advance the sequence
   // number for new snapshots unless all the writes with lower sequence numbers
-  // are already finished. This provides the immutability that we expect from
+  // are already finished. This provides the immutability that we except from
   // snapshots. Moreover, since Iterator and MultiGet internally depend on
   // snapshots, the snapshot immutability results into Iterator and MultiGet
   // offering consistent-point-in-time view. If set to true, although
@@ -1157,7 +1157,7 @@ struct DBOptions {
 
   // A global cache for table-level rows.
   // Default: nullptr (disabled)
-  std::shared_ptr<RowCache> row_cache = nullptr;
+  std::shared_ptr<GeneralCache> row_cache = nullptr;
 
   // A filter object supplied to be invoked while processing write-ahead-logs
   // (WALs) during recovery. The filter provides a way to inspect log

include/rocksdb/version.h

@@ -12,7 +12,7 @@
 // NOTE: in 'main' development branch, this should be the *next*
 // minor or major version number planned for release.
 #define ROCKSDB_MAJOR 8
-#define ROCKSDB_MINOR 5
+#define ROCKSDB_MINOR 4
 #define ROCKSDB_PATCH 4
 
 // Do not use these. We made the mistake of declaring macros starting with

port/stack_trace.cc

@@ -31,10 +31,6 @@ void* SaveStack(int* /*num_frames*/, int /*first_frames_to_skip*/) {
 #include <string.h>
 #include <unistd.h>
 
-#ifdef OS_OPENBSD
-#include <sys/wait.h>
-#include <sys/sysctl.h>
-#endif  // OS_OPENBSD
 #ifdef OS_FREEBSD
 #include <sys/sysctl.h>
 #endif  // OS_FREEBSD
@@ -55,32 +51,11 @@ namespace port {
 
 namespace {
 
-#if defined(OS_LINUX) || defined(OS_FREEBSD) || defined(OS_OPENBSD) || defined(OS_GNU_KFREEBSD)
+#if defined(OS_LINUX) || defined(OS_FREEBSD) || defined(OS_GNU_KFREEBSD)
 const char* GetExecutableName() {
   static char name[1024];
 
-#if defined(OS_FREEBSD)
-  int mib[4] = {CTL_KERN, KERN_PROC, KERN_PROC_PATHNAME, -1};
-  size_t namesz = sizeof(name);
-
-  auto ret = sysctl(mib, 4, name, &namesz, nullptr, 0);
-  if (-1 == ret) {
-    return nullptr;
-  } else {
-    return name;
-  }
-#elif defined(OS_OPENBSD)
-  int mib[4] = {CTL_KERN, KERN_PROC_ARGS, getpid(), KERN_PROC_ARGV};
-  size_t namesz = sizeof(name);
-  char* bin[namesz];
-
-  auto ret = sysctl(mib, 4, bin, &namesz, nullptr, 0);
-  if (-1 == ret) {
-    return nullptr;
-  } else {
-    return bin[0];
-  }
-#else
+#if !defined(OS_FREEBSD)
   char link[1024];
   snprintf(link, sizeof(link), "/proc/%d/exe", getpid());
   auto read = readlink(link, name, sizeof(name) - 1);
@@ -90,6 +65,16 @@ const char* GetExecutableName() {
     name[read] = 0;
     return name;
   }
+#else
+  int mib[4] = {CTL_KERN, KERN_PROC, KERN_PROC_PATHNAME, -1};
+  size_t namesz = sizeof(name);
+
+  auto ret = sysctl(mib, 4, name, &namesz, nullptr, 0);
+  if (-1 == ret) {
+    return nullptr;
+  } else {
+    return name;
+  }
 #endif
 }
 
@@ -286,7 +271,7 @@ void PrintStack(int first_frames_to_skip) {
   const int kMaxFrames = 100;
   void* frames[kMaxFrames];
 
-  int num_frames = (int) backtrace(frames, kMaxFrames);
+  auto num_frames = backtrace(frames, kMaxFrames);
   PrintStack(&frames[first_frames_to_skip], num_frames - first_frames_to_skip);
 }
 
@@ -299,7 +284,7 @@ void* SaveStack(int* num_frames, int first_frames_to_skip) {
   const int kMaxFrames = 100;
   void* frames[kMaxFrames];
 
-  int count = (int) backtrace(frames, kMaxFrames);
+  auto count = backtrace(frames, kMaxFrames);
   *num_frames = count - first_frames_to_skip;
   void* callstack = malloc(sizeof(void*) * *num_frames);
   memcpy(callstack, &frames[first_frames_to_skip], sizeof(void*) * *num_frames);

table/block_based/block_based_table_factory.cc

@@ -83,7 +83,7 @@ size_t TailPrefetchStats::GetSuggestedPrefetchSize() {
   //
   // and we use every of the value as a candidate, and estimate how much we
   // wasted, compared to read. For example, when we use the 3rd record
-  // as candidate. This area is what we read:
+  // as candiate. This area is what we read:
   //                                     +---+
   //                             +---+   |   |
   //                             |   |   |   |
@@ -123,7 +123,7 @@ size_t TailPrefetchStats::GetSuggestedPrefetchSize() {
   //  +---+    +---+    +---+    +---+   +---+
   //
   // Which can be calculated iteratively.
-  // The difference between wasted using 4th and 3rd record, will
+  // The difference between wasted using 4st and 3rd record, will
   // be following area:
   //                                     +---+
   //  +--+  +-+   ++  +-+  +-+   +---+   |   |
@@ -143,8 +143,8 @@ size_t TailPrefetchStats::GetSuggestedPrefetchSize() {
   //  |   |    |   |    |   |    |   |   |   |
   //  +---+    +---+    +---+    +---+   +---+
   //
-  // which will be the size difference between 4th and 3rd record,
-  // times 3, which is number of records before the 4th.
+  // which will be the size difference between 4st and 3rd record,
+  // times 3, which is number of records before the 4st.
   // Here we assume that all data within the prefetch range will be useful. In
   // reality, it may not be the case when a partial block is inside the range,
   // or there are data in the middle that is not read. We ignore those cases

table/block_based/block_builder.cc

@@ -53,7 +53,8 @@ BlockBuilder::BlockBuilder(
     : block_restart_interval_(block_restart_interval),
       use_delta_encoding_(use_delta_encoding),
       use_value_delta_encoding_(use_value_delta_encoding),
-      strip_ts_sz_(persist_user_defined_timestamps ? 0 : ts_sz),
+      ts_sz_(ts_sz),
+      persist_user_defined_timestamps_(persist_user_defined_timestamps),
       is_user_key_(is_user_key),
       restarts_(1, 0),  // First restart point is at offset 0
       counter_(0),
@@ -99,8 +100,8 @@ size_t BlockBuilder::EstimateSizeAfterKV(const Slice& key,
   // Note: this is an imprecise estimate as it accounts for the whole key size
   // instead of non-shared key size.
   estimate += key.size();
-  if (strip_ts_sz_ > 0) {
-    estimate -= strip_ts_sz_;
+  if (ts_sz_ > 0 && !persist_user_defined_timestamps_) {
+    estimate -= ts_sz_;
   }
   // In value delta encoding we estimate the value delta size as half the full
   // value size since only the size field of block handle is encoded.
@@ -174,13 +175,13 @@ void BlockBuilder::AddWithLastKey(const Slice& key, const Slice& value,
   // or Reset. This is more convenient for the caller and we can be more
   // clever inside BlockBuilder. On this hot code path, we want to avoid
   // conditional jumps like `buffer_.empty() ? ... : ...` so we can use a
-  // fast arithmetic operation instead, with an assertion to be sure our logic
-  // is sound.
+  // fast min operation instead, with an assertion to be sure our logic is
+  // sound.
   size_t buffer_size = buffer_.size();
   size_t last_key_size = last_key_param.size();
-  assert(buffer_size == 0 || buffer_size >= last_key_size - strip_ts_sz_);
+  assert(buffer_size == 0 || buffer_size >= last_key_size);
 
-  Slice last_key(last_key_param.data(), last_key_size * (buffer_size > 0));
+  Slice last_key(last_key_param.data(), std::min(buffer_size, last_key_size));
 
   AddWithLastKeyImpl(key, value, last_key, delta_value, buffer_size);
 }
@@ -254,11 +255,11 @@ inline void BlockBuilder::AddWithLastKeyImpl(const Slice& key,
 const Slice BlockBuilder::MaybeStripTimestampFromKey(std::string* key_buf,
                                                      const Slice& key) {
   Slice stripped_key = key;
-  if (strip_ts_sz_ > 0) {
+  if (ts_sz_ > 0 && !persist_user_defined_timestamps_) {
     if (is_user_key_) {
-      stripped_key.remove_suffix(strip_ts_sz_);
+      stripped_key.remove_suffix(ts_sz_);
     } else {
-      StripTimestampFromInternalKey(key_buf, key, strip_ts_sz_);
+      StripTimestampFromInternalKey(key_buf, key, ts_sz_);
       stripped_key = *key_buf;
     }
   }

table/block_based/block_builder.h

@@ -94,10 +94,11 @@ class BlockBuilder {
   const bool use_delta_encoding_;
   // Refer to BlockIter::DecodeCurrentValue for format of delta encoded values
   const bool use_value_delta_encoding_;
-  // Size in bytes for the user-defined timestamp to strip in a user key.
-  // This is non-zero if there is user-defined timestamp in the user key and it
-  // should not be persisted.
-  const size_t strip_ts_sz_;
+  // Size in bytes for the user-defined timestamp in a user key.
+  const size_t ts_sz_;
+  // Whether the user-defined timestamp part in user keys should be persisted.
+  // If false, it will be stripped from the key before it's encoded.
+  const bool persist_user_defined_timestamps_;
   // Whether the keys provided to build this block are user keys. If not,
   // the keys are internal keys. This will affect how timestamp stripping is
   // done for the key if `persisted_user_defined_timestamps_` is false and

table/block_based/block_prefetcher.cc

@@ -17,30 +17,9 @@ void BlockPrefetcher::PrefetchIfNeeded(
     const size_t readahead_size, bool is_for_compaction,
     const bool no_sequential_checking,
     const Env::IOPriority rate_limiter_priority) {
-  const size_t len = BlockBasedTable::BlockSizeWithTrailer(handle);
-  const size_t offset = handle.offset();
-
+  // num_file_reads is used  by FilePrefetchBuffer only when
+  // implicit_auto_readahead is set.
   if (is_for_compaction) {
-    if (!rep->file->use_direct_io() && compaction_readahead_size_ > 0) {
-      // If FS supports prefetching (readahead_limit_ will be non zero in that
-      // case) and current block exists in prefetch buffer then return.
-      if (offset + len <= readahead_limit_) {
-        return;
-      }
-      Status s = rep->file->Prefetch(offset, len + compaction_readahead_size_,
-                                     rate_limiter_priority);
-      if (s.ok()) {
-        readahead_limit_ = offset + len + compaction_readahead_size_;
-        return;
-      } else if (!s.IsNotSupported()) {
-        return;
-      }
-    }
-    // If FS prefetch is not supported, fall back to use internal prefetch
-    // buffer.
-    //
-    // num_file_reads is used  by FilePrefetchBuffer only when
-    // implicit_auto_readahead is set.
     rep->CreateFilePrefetchBufferIfNotExists(
         compaction_readahead_size_, compaction_readahead_size_,
         &prefetch_buffer_, /*implicit_auto_readahead=*/false,
@@ -81,6 +60,9 @@ void BlockPrefetcher::PrefetchIfNeeded(
     return;
   }
 
+  size_t len = BlockBasedTable::BlockSizeWithTrailer(handle);
+  size_t offset = handle.offset();
+
   // If FS supports prefetching (readahead_limit_ will be non zero in that case)
   // and current block exists in prefetch buffer then return.
   if (offset + len <= readahead_limit_) {
@@ -116,6 +98,8 @@ void BlockPrefetcher::PrefetchIfNeeded(
   }
 
   // If prefetch is not supported, fall back to use internal prefetch buffer.
+  // Discarding other return status of Prefetch calls intentionally, as
+  // we can fallback to reading from disk if Prefetch fails.
   Status s = rep->file->Prefetch(
       handle.offset(),
       BlockBasedTable::BlockSizeWithTrailer(handle) + readahead_size_,

table/block_based/block_prefetcher.h

@@ -58,12 +58,11 @@ class BlockPrefetcher {
   // lookup_context_.caller = kCompaction.
   size_t compaction_readahead_size_;
 
-  // readahead_size_ is used in non-compaction read if underlying FS supports
-  // prefetching.
+  // readahead_size_ is used if underlying FS supports prefetching.
   size_t readahead_size_;
   size_t readahead_limit_ = 0;
-  // initial_auto_readahead_size_ is used in non-compaction read if RocksDB uses
-  // internal prefetch buffer.
+  // initial_auto_readahead_size_ is used if RocksDB uses internal prefetch
+  // buffer.
   uint64_t initial_auto_readahead_size_;
   uint64_t num_file_reads_ = 0;
   uint64_t prev_offset_ = 0;

table/block_based/index_builder.h

@@ -9,6 +9,8 @@
 
 #pragma once
 
+#include <assert.h>
+
 #include <cinttypes>
 #include <list>
 #include <string>
@@ -37,8 +39,9 @@ class IndexBuilder {
       BlockBasedTableOptions::IndexType index_type,
       const ROCKSDB_NAMESPACE::InternalKeyComparator* comparator,
       const InternalKeySliceTransform* int_key_slice_transform,
-      bool use_value_delta_encoding, const BlockBasedTableOptions& table_opt,
-      size_t ts_sz, bool persist_user_defined_timestamps);
+      const bool use_value_delta_encoding,
+      const BlockBasedTableOptions& table_opt, size_t ts_sz,
+      const bool persist_user_defined_timestamps);
 
   // Index builder will construct a set of blocks which contain:
   //  1. One primary index block.
@@ -48,13 +51,13 @@ class IndexBuilder {
     Slice index_block_contents;
     std::unordered_map<std::string, Slice> meta_blocks;
   };
-  IndexBuilder(const InternalKeyComparator* comparator, size_t ts_sz,
-               bool persist_user_defined_timestamps)
+  explicit IndexBuilder(const InternalKeyComparator* comparator, size_t ts_sz,
+                        bool persist_user_defined_timestamps)
       : comparator_(comparator),
         ts_sz_(ts_sz),
         persist_user_defined_timestamps_(persist_user_defined_timestamps) {}
 
-  virtual ~IndexBuilder() = default;
+  virtual ~IndexBuilder() {}
 
   // Add a new index entry to index block.
   // To allow further optimization, we provide `last_key_in_current_block` and
@@ -129,7 +132,7 @@ class IndexBuilder {
 //     substitute key that serves the same function.
 class ShortenedIndexBuilder : public IndexBuilder {
  public:
-  ShortenedIndexBuilder(
+  explicit ShortenedIndexBuilder(
       const InternalKeyComparator* comparator,
       const int index_block_restart_interval, const uint32_t format_version,
       const bool use_value_delta_encoding,
@@ -156,15 +159,15 @@ class ShortenedIndexBuilder : public IndexBuilder {
     seperator_is_key_plus_seq_ = (format_version <= 2);
   }
 
-  void OnKeyAdded(const Slice& key) override {
+  virtual void OnKeyAdded(const Slice& key) override {
     if (include_first_key_ && current_block_first_internal_key_.empty()) {
       current_block_first_internal_key_.assign(key.data(), key.size());
     }
   }
 
-  void AddIndexEntry(std::string* last_key_in_current_block,
-                     const Slice* first_key_in_next_block,
-                     const BlockHandle& block_handle) override {
+  virtual void AddIndexEntry(std::string* last_key_in_current_block,
+                             const Slice* first_key_in_next_block,
+                             const BlockHandle& block_handle) override {
     if (first_key_in_next_block != nullptr) {
       if (shortening_mode_ !=
           BlockBasedTableOptions::IndexShorteningMode::kNoShortening) {
@@ -233,8 +236,9 @@ class ShortenedIndexBuilder : public IndexBuilder {
   }
 
   using IndexBuilder::Finish;
-  Status Finish(IndexBlocks* index_blocks,
-                const BlockHandle& /*last_partition_block_handle*/) override {
+  virtual Status Finish(
+      IndexBlocks* index_blocks,
+      const BlockHandle& /*last_partition_block_handle*/) override {
     if (seperator_is_key_plus_seq_) {
       index_blocks->index_block_contents = index_block_builder_.Finish();
     } else {
@@ -245,9 +249,9 @@ class ShortenedIndexBuilder : public IndexBuilder {
     return Status::OK();
   }
 
-  size_t IndexSize() const override { return index_size_; }
+  virtual size_t IndexSize() const override { return index_size_; }
 
-  bool seperator_is_key_plus_seq() override {
+  virtual bool seperator_is_key_plus_seq() override {
     return seperator_is_key_plus_seq_;
   }
 
@@ -302,12 +306,13 @@ class ShortenedIndexBuilder : public IndexBuilder {
 // data copy or small heap allocations for prefixes.
 class HashIndexBuilder : public IndexBuilder {
  public:
-  HashIndexBuilder(const InternalKeyComparator* comparator,
-                   const SliceTransform* hash_key_extractor,
-                   int index_block_restart_interval, int format_version,
-                   bool use_value_delta_encoding,
-                   BlockBasedTableOptions::IndexShorteningMode shortening_mode,
-                   size_t ts_sz, const bool persist_user_defined_timestamps)
+  explicit HashIndexBuilder(
+      const InternalKeyComparator* comparator,
+      const SliceTransform* hash_key_extractor,
+      int index_block_restart_interval, int format_version,
+      bool use_value_delta_encoding,
+      BlockBasedTableOptions::IndexShorteningMode shortening_mode, size_t ts_sz,
+      const bool persist_user_defined_timestamps)
       : IndexBuilder(comparator, ts_sz, persist_user_defined_timestamps),
         primary_index_builder_(comparator, index_block_restart_interval,
                                format_version, use_value_delta_encoding,
@@ -315,15 +320,15 @@ class HashIndexBuilder : public IndexBuilder {
                                ts_sz, persist_user_defined_timestamps),
         hash_key_extractor_(hash_key_extractor) {}
 
-  void AddIndexEntry(std::string* last_key_in_current_block,
-                     const Slice* first_key_in_next_block,
-                     const BlockHandle& block_handle) override {
+  virtual void AddIndexEntry(std::string* last_key_in_current_block,
+                             const Slice* first_key_in_next_block,
+                             const BlockHandle& block_handle) override {
     ++current_restart_index_;
     primary_index_builder_.AddIndexEntry(last_key_in_current_block,
                                          first_key_in_next_block, block_handle);
   }
 
-  void OnKeyAdded(const Slice& key) override {
+  virtual void OnKeyAdded(const Slice& key) override {
     auto key_prefix = hash_key_extractor_->Transform(key);
     bool is_first_entry = pending_block_num_ == 0;
 
@@ -350,8 +355,9 @@ class HashIndexBuilder : public IndexBuilder {
     }
   }
 
-  Status Finish(IndexBlocks* index_blocks,
-                const BlockHandle& last_partition_block_handle) override {
+  virtual Status Finish(
+      IndexBlocks* index_blocks,
+      const BlockHandle& last_partition_block_handle) override {
     if (pending_block_num_ != 0) {
       FlushPendingPrefix();
     }
@@ -364,12 +370,12 @@ class HashIndexBuilder : public IndexBuilder {
     return s;
   }
 
-  size_t IndexSize() const override {
+  virtual size_t IndexSize() const override {
     return primary_index_builder_.IndexSize() + prefix_block_.size() +
            prefix_meta_block_.size();
   }
 
-  bool seperator_is_key_plus_seq() override {
+  virtual bool seperator_is_key_plus_seq() override {
     return primary_index_builder_.seperator_is_key_plus_seq();
   }
 
@@ -415,24 +421,27 @@ class PartitionedIndexBuilder : public IndexBuilder {
  public:
   static PartitionedIndexBuilder* CreateIndexBuilder(
       const ROCKSDB_NAMESPACE::InternalKeyComparator* comparator,
-      bool use_value_delta_encoding, const BlockBasedTableOptions& table_opt,
-      size_t ts_sz, bool persist_user_defined_timestamps);
+      const bool use_value_delta_encoding,
+      const BlockBasedTableOptions& table_opt, size_t ts_sz,
+      const bool persist_user_defined_timestamps);
 
-  PartitionedIndexBuilder(const InternalKeyComparator* comparator,
-                          const BlockBasedTableOptions& table_opt,
-                          bool use_value_delta_encoding, size_t ts_sz,
-                          bool persist_user_defined_timestamps);
+  explicit PartitionedIndexBuilder(const InternalKeyComparator* comparator,
+                                   const BlockBasedTableOptions& table_opt,
+                                   const bool use_value_delta_encoding,
+                                   size_t ts_sz,
+                                   const bool persist_user_defined_timestamps);
 
-  ~PartitionedIndexBuilder() override;
+  virtual ~PartitionedIndexBuilder();
 
-  void AddIndexEntry(std::string* last_key_in_current_block,
-                     const Slice* first_key_in_next_block,
-                     const BlockHandle& block_handle) override;
+  virtual void AddIndexEntry(std::string* last_key_in_current_block,
+                             const Slice* first_key_in_next_block,
+                             const BlockHandle& block_handle) override;
 
-  Status Finish(IndexBlocks* index_blocks,
-                const BlockHandle& last_partition_block_handle) override;
+  virtual Status Finish(
+      IndexBlocks* index_blocks,
+      const BlockHandle& last_partition_block_handle) override;
 
-  size_t IndexSize() const override { return index_size_; }
+  virtual size_t IndexSize() const override { return index_size_; }
   size_t TopLevelIndexSize(uint64_t) const { return top_level_index_size_; }
   size_t NumPartitions() const;
 
@@ -451,13 +460,11 @@ class PartitionedIndexBuilder : public IndexBuilder {
   // cutting the next partition
   void RequestPartitionCut();
 
-  bool seperator_is_key_plus_seq() override {
+  virtual bool seperator_is_key_plus_seq() override {
     return seperator_is_key_plus_seq_;
   }
 
-  bool get_use_value_delta_encoding() const {
-    return use_value_delta_encoding_;
-  }
+  bool get_use_value_delta_encoding() { return use_value_delta_encoding_; }
 
  private:
   // Set after ::Finish is called

table/plain/plain_table_builder.h

@@ -85,16 +85,7 @@ class PlainTableBuilder : public TableBuilder {
   // Finish() call, returns the size of the final generated file.
   uint64_t FileSize() const override;
 
-  TableProperties GetTableProperties() const override {
-    TableProperties ret = properties_;
-    for (const auto& collector : table_properties_collectors_) {
-      for (const auto& prop : collector->GetReadableProperties()) {
-        ret.readable_properties.insert(prop);
-      }
-      collector->Finish(&ret.user_collected_properties).PermitUncheckedError();
-    }
-    return ret;
-  }
+  TableProperties GetTableProperties() const override { return properties_; }
 
   bool SaveIndexInFile() const { return store_index_in_file_; }
 

table/sst_file_dumper.cc

@@ -196,7 +196,6 @@ Status SstFileDumper::NewTableReader(
 }
 
 Status SstFileDumper::VerifyChecksum() {
-  assert(read_options_.verify_checksums);
   // We could pass specific readahead setting into read options if needed.
   return table_reader_->VerifyChecksum(read_options_,
                                        TableReaderCaller::kSSTDumpTool);

tools/benchmark.sh

@@ -6,7 +6,6 @@
 EXIT_INVALID_ARGS=1
 EXIT_NOT_COMPACTION_TEST=2
 EXIT_UNKNOWN_JOB=3
-EXIT_INVALID_PATH=4
 
 # Size Constants
 K=1024
@@ -115,11 +114,6 @@ fi
 
 job_id=${JOB_ID}
 
-if [ ! -x ./db_bench ]; then
-  echo "./db_bench not found. Please make sure it exists in the current directory."
-  exit $EXIT_INVALID_PATH
-fi
-
 # Make it easier to run only the compaction test. Getting valid data requires
 # a number of iterations and having an ability to run the test separately from
 # rest of the benchmarks helps.

tools/check_format_compatible.sh

@@ -125,7 +125,7 @@ EOF
 
 # To check for DB forward compatibility with loading options (old version
 # reading data from new), as well as backward compatibility
-declare -a db_forward_with_options_refs=("6.27.fb" "6.28.fb" "6.29.fb" "7.0.fb" "7.1.fb" "7.2.fb" "7.3.fb" "7.4.fb" "7.5.fb" "7.6.fb" "7.7.fb" "7.8.fb" "7.9.fb" "7.10.fb" "8.0.fb" "8.1.fb" "8.2.fb" "8.3.fb" "8.4.fb")
+declare -a db_forward_with_options_refs=("6.27.fb" "6.28.fb" "6.29.fb" "7.0.fb" "7.1.fb" "7.2.fb" "7.3.fb" "7.4.fb" "7.5.fb" "7.6.fb" "7.7.fb" "7.8.fb" "7.9.fb" "7.10.fb" "8.0.fb" "8.1.fb" "8.2.fb" "8.3.fb")
 # To check for DB forward compatibility without loading options (in addition
 # to the "with loading options" set), as well as backward compatibility
 declare -a db_forward_no_options_refs=() # N/A at the moment

tools/db_crashtest.py

@@ -734,7 +734,6 @@ def gen_cmd(params, unknown_params):
                 "stress_cmd",
                 "test_tiered_storage",
                 "cleanup_cmd",
-                "skip_tmpdir_check"
             }
             and v is not None
         ]
@@ -1017,7 +1016,6 @@ def main():
     parser.add_argument("--stress_cmd")
     parser.add_argument("--test_tiered_storage", action="store_true")
     parser.add_argument("--cleanup_cmd")
-    parser.add_argument("--skip_tmpdir_check", action="store_true")
 
     all_params = dict(
         list(default_params.items())
@@ -1044,13 +1042,13 @@ def main():
     args, unknown_args = parser.parse_known_args()
 
     test_tmpdir = os.environ.get(_TEST_DIR_ENV_VAR)
-    if test_tmpdir is not None and not args.skip_tmpdir_check:
+    if test_tmpdir is not None:
         isdir = False
         try:
             isdir = os.path.isdir(test_tmpdir)
             if not isdir:
                 print(
-                    "ERROR: %s env var is set to a non-existent directory: %s. Update it to correct directory path."
+                    "%s env var is set to a non-existent directory: %s"
                     % (_TEST_DIR_ENV_VAR, test_tmpdir)
                 )
                 sys.exit(1)

tools/ldb_cmd.cc

@@ -1310,8 +1310,7 @@ void DBLoaderCommand::DoCommand() {
 namespace {
 
 void DumpManifestFile(Options options, std::string file, bool verbose, bool hex,
-                      bool json,
-                      const std::vector<ColumnFamilyDescriptor>& cf_descs) {
+                      bool json) {
   EnvOptions sopt;
   std::string dbname("dummy");
   std::shared_ptr<Cache> tc(NewLRUCache(options.max_open_files - 10,
@@ -1327,7 +1326,7 @@ void DumpManifestFile(Options options, std::string file, bool verbose, bool hex,
   VersionSet versions(dbname, &immutable_db_options, sopt, tc.get(), &wb, &wc,
                       /*block_cache_tracer=*/nullptr, /*io_tracer=*/nullptr,
                       /*db_id*/ "", /*db_session_id*/ "");
-  Status s = versions.DumpManifest(options, file, verbose, hex, json, cf_descs);
+  Status s = versions.DumpManifest(options, file, verbose, hex, json);
   if (!s.ok()) {
     fprintf(stderr, "Error in processing file %s %s\n", file.c_str(),
             s.ToString().c_str());
@@ -1439,8 +1438,7 @@ void ManifestDumpCommand::DoCommand() {
     fprintf(stdout, "Processing Manifest file %s\n", manifestfile.c_str());
   }
 
-  DumpManifestFile(options_, manifestfile, verbose_, is_key_hex_, json_,
-                   column_families_);
+  DumpManifestFile(options_, manifestfile, verbose_, is_key_hex_, json_);
 
   if (verbose_) {
     fprintf(stdout, "Processing Manifest file %s done\n", manifestfile.c_str());
@@ -2048,7 +2046,7 @@ void DBDumperCommand::DoCommand() {
         break;
       case kDescriptorFile:
         DumpManifestFile(options_, path_, /* verbose_ */ false, is_key_hex_,
-                         /*  json_ */ false, column_families_);
+                         /*  json_ */ false);
         break;
       case kBlobFile:
         DumpBlobFile(path_, is_key_hex_, is_value_hex_,
@@ -3708,8 +3706,7 @@ void DBFileDumperCommand::DoCommand() {
   manifest_filepath = NormalizePath(manifest_filepath);
 
   std::cout << manifest_filepath << std::endl;
-  DumpManifestFile(options_, manifest_filepath, false, false, false,
-                   column_families_);
+  DumpManifestFile(options_, manifest_filepath, false, false, false);
   std::cout << std::endl;
 
   std::vector<ColumnFamilyMetaData> column_families;

tools/run_blob_bench.sh

@@ -19,7 +19,6 @@
 
 # Exit Codes
 EXIT_INVALID_ARGS=1
-EXIT_INVALID_PATH=2
 
 # Size constants
 K=1024
@@ -75,11 +74,6 @@ if [ $# -ge 1 ]; then
   fi
 fi
 
-if [ ! -f tools/benchmark.sh ]; then
-  echo "tools/benchmark.sh not found"
-  exit $EXIT_INVALID_PATH
-fi
-
 # shellcheck disable=SC2153
 if [ -z "$DB_DIR" ]; then
   echo "DB_DIR is not defined"

tools/sst_dump_tool.cc

@@ -419,10 +419,6 @@ int SSTDumpTool::Run(int argc, char const* const* argv, Options options) {
       filename = std::string(dir_or_file) + "/" + filename;
     }
 
-    if (command == "verify") {
-      verify_checksum = true;
-    }
-
     ROCKSDB_NAMESPACE::SstFileDumper dumper(
         options, filename, Temperature::kUnknown, readahead_size,
         verify_checksum, output_hex, decode_blob_index);

unreleased_history/release.sh

@@ -31,7 +31,7 @@ awk '/#define ROCKSDB_MAJOR/ { major = $3 }
      /#define ROCKSDB_MINOR/ { minor = $3 }
      /#define ROCKSDB_PATCH/ { patch = $3 }
      END { printf "## " major "." minor "." patch }' < include/rocksdb/version.h >> HISTORY.new
-echo " (`git log -n1 --date=format:"%m/%d/%Y" --format="%ad"`)" >> HISTORY.new
+echo " (`date +%x`)" >> HISTORY.new
 
 function process_file () {
   # use awk to correct extra or missing newlines, missing '* ' on first line

util/crc32c.cc

@@ -1117,13 +1117,7 @@ static inline Function Choose_Extend() {
   }
 #elif defined(__SSE4_2__) && defined(__PCLMUL__) && !defined NO_THREEWAY_CRC32C
   // NOTE: runtime detection no longer supported on x86
-#ifdef _MSC_VER
-#pragma warning(disable: 4551)
-#endif
-  (void)ExtendImpl<DefaultCRC32>; // suppress unused warning
-#ifdef _MSC_VER
-#pragma warning(default: 4551)
-#endif
+  (void)ExtendImpl<DefaultCRC32>;  // suppress unused warning
   return crc32c_3way;
 #else
   return ExtendImpl<DefaultCRC32>;

util/filelock_test.cc

@@ -11,9 +11,6 @@
 #include <sys/types.h>
 #include <sys/wait.h>
 #endif
-#ifdef __OpenBSD__
-#include <sys/wait.h>
-#endif
 #include <vector>
 
 #include "test_util/testharness.h"

util/udt_util.cc

@@ -36,7 +36,7 @@ RecoveryType GetRecoveryType(const size_t running_ts_sz,
     return RecoveryType::kPadTimestamp;
   }
 
-  if (running_ts_sz != *recorded_ts_sz) {
+  if (running_ts_sz != recorded_ts_sz.value()) {
     return RecoveryType::kUnrecoverable;
   }
 
@@ -214,7 +214,7 @@ Status TimestampRecoveryHandler::ReconcileTimestampDiscrepancy(
       break;
     case RecoveryType::kStripTimestamp:
       assert(record_ts_sz.has_value());
-      *new_key = StripTimestampFromUserKey(key, *record_ts_sz);
+      *new_key = StripTimestampFromUserKey(key, record_ts_sz.value());
       new_batch_diff_from_orig_batch_ = true;
       break;
     case RecoveryType::kPadTimestamp: