Extend existing unit tests to run with WriteUnPrepared as well

Summary:
DBImpl::FindObsoleteFiles() may call GetChildren() multiple times if different CFs are on the same path. Fix it.
Closes https://github.com/facebook/rocksdb/pull/3885

Differential Revision: D8084634

Pulled By: siying

fbshipit-source-id: b471fbc251f6a05e9243304dc14c0831060cc0b0

CMakeLists.txt

@@ -657,6 +657,8 @@ set(SOURCES
         utilities/transactions/transaction_util.cc
         utilities/transactions/write_prepared_txn.cc
         utilities/transactions/write_prepared_txn_db.cc
+        utilities/transactions/write_unprepared_txn.cc
+        utilities/transactions/write_unprepared_txn_db.cc
         utilities/ttl/db_ttl_impl.cc
         utilities/write_batch_with_index/write_batch_with_index.cc
         utilities/write_batch_with_index/write_batch_with_index_internal.cc

HISTORY.md

@@ -1,38 +1,15 @@
 # Rocksdb Change Log
 ## Unreleased
 ### Public API Change
-* Application using PessimisticTransactionDB is expected to rollback/commit recovered transactions before starting new ones. This assumption is used to skip concurrency control during recovery.
-
-### Bug Fixes
-* Fix a bug in WritePrepared txns where if the number of old snapshots goes beyond the snapshot cache size (128 default) the rest will not be checked when evicting a commit entry from the commit cache.
-
-### New Features
-* TransactionOptions::skip_concurrency_control allows pessimistic transactions to skip the overhead of concurrency control. Could be used for optimizing certain transactions or during recovery.
-
-## 5.14.4 (8/31/2018)
-### Bug Fixes
-* Suppress leak warning for clang(LLVM) ASAN
-
-## 5.14.3 (7/30/2018)
-### New Features
-* reduce file tail prefetch when not reading index/filter blocks.
-
-## 5.14.2 (7/3/2018)
-### Bug Fixes
-* Change default value of `bytes_max_delete_chunk` to 0 in NewSstFileManager() as it doesn't work well with checkpoints.
-* Set DEBUG_LEVEL=0 for RocksJava Mac Release build.
-
-### New Features
-* pin_top_level_index_and_filter (default true) in BlockBasedTableOptions can be used in combination with cache_index_and_filter_blocks to prefetch and pin the top-level index of partitioned index and filter blocks in cache. It has no impact when cache_index_and_filter_blocks is false.
-
-## 5.14.1 (6/20/2018)
-### Bug Fixes
-* Fix block-based table reader pinning blocks throughout its lifetime, causing memory usage increase.
-* Fix bug with prefix search in partition filters where a shared prefix would be ignored from the later partitions. The bug could report an eixstent key as missing. The bug could be triggered if prefix_extractor is set and partition filters is enabled.
+* For users of `Statistics` objects created via `CreateDBStatistics()`, the format of the string returned by its `ToString()` method has changed.
+* With LRUCache, when high_pri_pool_ratio > 0, midpoint insertion strategy will be enabled to put low-pri items to the tail of low-pri list (the midpoint) when they first inserted into the cache. This is to make cache entries never get hit age out faster, improving cache efficiency when large background scan presents.
 
 ### New Features
 * Changes the format of index blocks by storing the key in their raw form rather than converting them to InternalKey. This saves 8 bytes per index key. The feature is backward compatbile but not forward compatible. It is disabled by default unless format_version 3 or above is used.
 
+### Bug Fixes
+* fix deadlock with enable_pipelined_write=true and max_successive_merges > 0
+
 ## 5.14.0 (5/16/2018)
 ### Public API Change
 * Add a BlockBasedTableOption to align uncompressed data blocks on the smaller of block size or page size boundary, to reduce flash reads by avoiding reads spanning 4K pages.
@@ -43,6 +20,7 @@
 * Now, `DBOptions::use_direct_io_for_flush_and_compaction` only applies to background writes, and `DBOptions::use_direct_reads` applies to both user reads and background reads. This conforms with Linux's `open(2)` manpage, which advises against simultaneously reading a file in buffered and direct modes, due to possibly undefined behavior and degraded performance.
 * Iterator::Valid() always returns false if !status().ok(). So, now when doing a Seek() followed by some Next()s, there's no need to check status() after every operation.
 * Iterator::Seek()/SeekForPrev()/SeekToFirst()/SeekToLast() always resets status().
+* Introduced `CompressionOptions::kDefaultCompressionLevel`, which is a generic way to tell RocksDB to use the compression library's default level. It is now the default value for `CompressionOptions::level`. Previously the level defaulted to -1, which gave poor compression ratios in ZSTD.
 
 ### New Features
 * Introduce TTL for level compaction so that all files older than ttl go through the compaction process to get rid of old data.
@@ -59,7 +37,6 @@
 * Fix `BackupableDBOptions::max_valid_backups_to_open` to not delete backup files when refcount cannot be accurately determined.
 * Fix memory leak when pin_l0_filter_and_index_blocks_in_cache is used with partitioned filters
 * Disable rollback of merge operands in WritePrepared transactions to work around an issue in MyRocks. It can be enabled back by setting TransactionDBOptions::rollback_merge_operands to true.
-* Fix bug with prefix search in partition filters where a shared prefix would be ignored from the later partitions. The bug could report an eixstent key as missing. The bug could be triggered if prefix_extractor is set and partition filters is enabled.
 * Fix wrong results by ReverseBytewiseComparator::FindShortSuccessor()
 
 ### Java API Changes
@@ -141,8 +118,7 @@
 * `BackupableDBOptions::max_valid_backups_to_open == 0` now means no backups will be opened during BackupEngine initialization. Previously this condition disabled limiting backups opened.
 * `DBOptions::preserve_deletes` is a new option that allows one to specify that DB should not drop tombstones for regular deletes if they have sequence number larger than what was set by the new API call `DB::SetPreserveDeletesSequenceNumber(SequenceNumber seqnum)`. Disabled by default.
 * API call `DB::SetPreserveDeletesSequenceNumber(SequenceNumber seqnum)` was added, users who wish to preserve deletes are expected to periodically call this function to advance the cutoff seqnum (all deletes made before this seqnum can be dropped by DB). It's user responsibility to figure out how to advance the seqnum in the way so the tombstones are kept for the desired period of time, yet are eventually processed in time and don't eat up too much space.
-* `ReadOptions::iter_start_seqnum` was added;
-if set to something > 0 user will see 2 changes in iterators behavior 1) only keys written with sequence larger than this parameter would be returned and 2) the `Slice` returned by iter->key() now points to the memory that keep User-oriented representation of the internal key, rather than user key. New struct `FullKey` was added to represent internal keys, along with a new helper function `ParseFullKey(const Slice& internal_key, FullKey* result);`.
+* `ReadOptions::iter_start_seqnum` was added; if set to something > 0 user will see 2 changes in iterators behavior 1) only keys written with sequence larger than this parameter would be returned and 2) the `Slice` returned by iter->key() now points to the memory that keep User-oriented representation of the internal key, rather than user key. New struct `FullKey` was added to represent internal keys, along with a new helper function `ParseFullKey(const Slice& internal_key, FullKey* result);`.
 * Deprecate trash_dir param in NewSstFileManager, right now we will rename deleted files to <name>.trash instead of moving them to trash directory
 * Allow setting a custom trash/DB size ratio limit in the SstFileManager, after which files that are to be scheduled for deletion are deleted immediately, regardless of any delete ratelimit.
 * Return an error on write if write_options.sync = true and write_options.disableWAL = true to warn user of inconsistent options. Previously we will not write to WAL and not respecting the sync options in this case.

Makefile

@@ -85,10 +85,6 @@ ifeq ($(MAKECMDGOALS),rocksdbjavastaticrelease)
 	DEBUG_LEVEL=0
 endif
 
-ifeq ($(MAKECMDGOALS),rocksdbjavastaticreleasedocker)
-        DEBUG_LEVEL=0
-endif
-
 ifeq ($(MAKECMDGOALS),rocksdbjavastaticpublish)
 	DEBUG_LEVEL=0
 endif

TARGETS

@@ -28,7 +28,7 @@ rocksdb_compiler_flags = [
     # Added missing flags from output of build_detect_platform
     "-DROCKSDB_PTHREAD_ADAPTIVE_MUTEX",
     "-DROCKSDB_BACKTRACE",
-    "-Wnarrowing",
+    "-Wshorten-64-to-32",
 ]
 
 rocksdb_external_deps = [
@@ -280,6 +280,8 @@ cpp_library(
         "utilities/transactions/transaction_util.cc",
         "utilities/transactions/write_prepared_txn.cc",
         "utilities/transactions/write_prepared_txn_db.cc",
+        "utilities/transactions/write_unprepared_txn.cc",
+        "utilities/transactions/write_unprepared_txn_db.cc",
         "utilities/ttl/db_ttl_impl.cc",
         "utilities/write_batch_with_index/write_batch_with_index.cc",
         "utilities/write_batch_with_index/write_batch_with_index_internal.cc",
@@ -556,13 +558,13 @@ ROCKS_TESTS = [
         "serial",
     ],
     [
-        "db_iter_stress_test",
-        "db/db_iter_stress_test.cc",
+        "db_iter_test",
+        "db/db_iter_test.cc",
         "serial",
     ],
     [
-        "db_iter_test",
-        "db/db_iter_test.cc",
+        "db_iter_stress_test",
+        "db/db_iter_stress_test.cc",
         "serial",
     ],
     [

buckifier/targets_cfg.py

@@ -32,7 +32,7 @@ rocksdb_compiler_flags = [
     # Added missing flags from output of build_detect_platform
     "-DROCKSDB_PTHREAD_ADAPTIVE_MUTEX",
     "-DROCKSDB_BACKTRACE",
-    "-Wnarrowing",
+    "-Wshorten-64-to-32",
 ]
 
 rocksdb_external_deps = [

cache/lru_cache.cc

@@ -99,14 +99,20 @@ void LRUHandleTable::Resize() {
   length_ = new_length;
 }
 
-LRUCacheShard::LRUCacheShard()
-    : capacity_(0), high_pri_pool_usage_(0), strict_capacity_limit_(false),
-      high_pri_pool_ratio_(0), high_pri_pool_capacity_(0), usage_(0),
+LRUCacheShard::LRUCacheShard(size_t capacity, bool strict_capacity_limit,
+                             double high_pri_pool_ratio)
+    : capacity_(0),
+      high_pri_pool_usage_(0),
+      strict_capacity_limit_(strict_capacity_limit),
+      high_pri_pool_ratio_(high_pri_pool_ratio),
+      high_pri_pool_capacity_(0),
+      usage_(0),
       lru_usage_(0) {
   // Make empty circular linked list
   lru_.next = &lru_;
   lru_.prev = &lru_;
   lru_low_pri_ = &lru_;
+  SetCapacity(capacity);
 }
 
 LRUCacheShard::~LRUCacheShard() {}
@@ -193,7 +199,7 @@ void LRUCacheShard::LRU_Remove(LRUHandle* e) {
 void LRUCacheShard::LRU_Insert(LRUHandle* e) {
   assert(e->next == nullptr);
   assert(e->prev == nullptr);
-  if (high_pri_pool_ratio_ > 0 && e->IsHighPri()) {
+  if (high_pri_pool_ratio_ > 0 && (e->IsHighPri() || e->HasHit())) {
     // Inset "e" to head of LRU list.
     e->next = &lru_;
     e->prev = lru_.prev;
@@ -240,22 +246,6 @@ void LRUCacheShard::EvictFromLRU(size_t charge,
   }
 }
 
-void* LRUCacheShard::operator new(size_t size) {
-  return port::cacheline_aligned_alloc(size);
-}
-
-void* LRUCacheShard::operator new[](size_t size) {
-  return port::cacheline_aligned_alloc(size);
-}
-
-void LRUCacheShard::operator delete(void *memblock) {
-  port::cacheline_aligned_free(memblock);
-}
-
-void LRUCacheShard::operator delete[](void* memblock) {
-  port::cacheline_aligned_free(memblock);
-}
-
 void LRUCacheShard::SetCapacity(size_t capacity) {
   autovector<LRUHandle*> last_reference_list;
   {
@@ -285,6 +275,7 @@ Cache::Handle* LRUCacheShard::Lookup(const Slice& key, uint32_t hash) {
       LRU_Remove(e);
     }
     e->refs++;
+    e->SetHit();
   }
   return reinterpret_cast<Cache::Handle*>(e);
 }
@@ -473,15 +464,24 @@ LRUCache::LRUCache(size_t capacity, int num_shard_bits,
                    bool strict_capacity_limit, double high_pri_pool_ratio)
     : ShardedCache(capacity, num_shard_bits, strict_capacity_limit) {
   num_shards_ = 1 << num_shard_bits;
-  shards_ = new LRUCacheShard[num_shards_];
-  SetCapacity(capacity);
-  SetStrictCapacityLimit(strict_capacity_limit);
+  shards_ = reinterpret_cast<LRUCacheShard*>(
+      port::cacheline_aligned_alloc(sizeof(LRUCacheShard) * num_shards_));
+  size_t per_shard = (capacity + (num_shards_ - 1)) / num_shards_;
   for (int i = 0; i < num_shards_; i++) {
-    shards_[i].SetHighPriorityPoolRatio(high_pri_pool_ratio);
+    new (&shards_[i])
+        LRUCacheShard(per_shard, strict_capacity_limit, high_pri_pool_ratio);
   }
 }
 
-LRUCache::~LRUCache() { delete[] shards_; }
+LRUCache::~LRUCache() {
+  if (shards_ != nullptr) {
+    assert(num_shards_ > 0);
+    for (int i = 0; i < num_shards_; i++) {
+      shards_[i].~LRUCacheShard();
+    }
+    port::cacheline_aligned_free(shards_);
+  }
+}
 
 CacheShard* LRUCache::GetShard(int shard) {
   return reinterpret_cast<CacheShard*>(&shards_[shard]);
@@ -505,16 +505,10 @@ uint32_t LRUCache::GetHash(Handle* handle) const {
 
 void LRUCache::DisownData() {
 // Do not drop data if compile with ASAN to suppress leak warning.
-#if defined(__clang__)
-#if !defined(__has_feature) || !__has_feature(address_sanitizer)
-  shards_ = nullptr;
-  num_shards_ = 0;
-#endif
-#else   // __clang__
 #ifndef __SANITIZE_ADDRESS__
   shards_ = nullptr;
+  num_shards_ = 0;
 #endif  // !__SANITIZE_ADDRESS__
-#endif  // __clang__
 }
 
 size_t LRUCache::TEST_GetLRUSize() {

cache/lru_cache.h

@@ -77,6 +77,7 @@ struct LRUHandle {
   bool InCache() { return flags & 1; }
   bool IsHighPri() { return flags & 2; }
   bool InHighPriPool() { return flags & 4; }
+  bool HasHit() { return flags & 8; }
 
   void SetInCache(bool in_cache) {
     if (in_cache) {
@@ -102,6 +103,8 @@ struct LRUHandle {
     }
   }
 
+  void SetHit() { flags |= 8; }
+
   void Free() {
     assert((refs == 1 && InCache()) || (refs == 0 && !InCache()));
     if (deleter) {
@@ -156,7 +159,8 @@ class LRUHandleTable {
 // A single shard of sharded cache.
 class ALIGN_AS(CACHE_LINE_SIZE) LRUCacheShard : public CacheShard {
  public:
-  LRUCacheShard();
+  LRUCacheShard(size_t capacity, bool strict_capacity_limit,
+                double high_pri_pool_ratio);
   virtual ~LRUCacheShard();
 
   // Separate from constructor so caller can easily make an array of LRUCache
@@ -205,15 +209,6 @@ class ALIGN_AS(CACHE_LINE_SIZE) LRUCacheShard : public CacheShard {
   //  Retrives high pri pool ratio
   double GetHighPriPoolRatio();
 
-  // Overloading to aligned it to cache line size
-  void* operator new(size_t);
-
-  void* operator new[](size_t);
-
-  void operator delete(void *);
-
-  void operator delete[](void*);
-
  private:
   void LRU_Remove(LRUHandle* e);
   void LRU_Insert(LRUHandle* e);
@@ -300,7 +295,7 @@ class LRUCache : public ShardedCache {
   double GetHighPriPoolRatio();
 
  private:
-  LRUCacheShard* shards_;
+  LRUCacheShard* shards_ = nullptr;
   int num_shards_ = 0;
 };
 

cache/lru_cache_test.cc

@@ -7,6 +7,7 @@
 
 #include <string>
 #include <vector>
+#include "port/port.h"
 #include "util/testharness.h"
 
 namespace rocksdb {
@@ -14,22 +15,22 @@ namespace rocksdb {
 class LRUCacheTest : public testing::Test {
  public:
   LRUCacheTest() {}
-  ~LRUCacheTest() {}
+  ~LRUCacheTest() { DeleteCache(); }
+
+  void DeleteCache() {
+    if (cache_ != nullptr) {
+      cache_->~LRUCacheShard();
+      port::cacheline_aligned_free(cache_);
+      cache_ = nullptr;
+    }
+  }
 
   void NewCache(size_t capacity, double high_pri_pool_ratio = 0.0) {
-    cache_.reset(
-#if defined(_MSC_VER)
-#pragma warning(push)
-#pragma warning(disable: 4316) // We've validated the alignment with the new operators
-#endif
-      new LRUCacheShard()
-#if defined(_MSC_VER)
-#pragma warning(pop)
-#endif
-    );
-    cache_->SetCapacity(capacity);
-    cache_->SetStrictCapacityLimit(false);
-    cache_->SetHighPriorityPoolRatio(high_pri_pool_ratio);
+    DeleteCache();
+    cache_ = reinterpret_cast<LRUCacheShard*>(
+        port::cacheline_aligned_alloc(sizeof(LRUCacheShard)));
+    new (cache_) LRUCacheShard(capacity, false /*strict_capcity_limit*/,
+                               high_pri_pool_ratio);
   }
 
   void Insert(const std::string& key,
@@ -85,7 +86,7 @@ class LRUCacheTest : public testing::Test {
   }
 
  private:
-  std::unique_ptr<LRUCacheShard> cache_;
+  LRUCacheShard* cache_ = nullptr;
 };
 
 TEST_F(LRUCacheTest, BasicLRU) {
@@ -114,7 +115,30 @@ TEST_F(LRUCacheTest, BasicLRU) {
   ValidateLRUList({"e", "z", "d", "u", "v"});
 }
 
-TEST_F(LRUCacheTest, MidPointInsertion) {
+TEST_F(LRUCacheTest, MidpointInsertion) {
+  // Allocate 2 cache entries to high-pri pool.
+  NewCache(5, 0.45);
+
+  Insert("a", Cache::Priority::LOW);
+  Insert("b", Cache::Priority::LOW);
+  Insert("c", Cache::Priority::LOW);
+  Insert("x", Cache::Priority::HIGH);
+  Insert("y", Cache::Priority::HIGH);
+  ValidateLRUList({"a", "b", "c", "x", "y"}, 2);
+
+  // Low-pri entries inserted to the tail of low-pri list (the midpoint).
+  // After lookup, it will move to the tail of the full list.
+  Insert("d", Cache::Priority::LOW);
+  ValidateLRUList({"b", "c", "d", "x", "y"}, 2);
+  ASSERT_TRUE(Lookup("d"));
+  ValidateLRUList({"b", "c", "x", "y", "d"}, 2);
+
+  // High-pri entries will be inserted to the tail of full list.
+  Insert("z", Cache::Priority::HIGH);
+  ValidateLRUList({"c", "x", "y", "d", "z"}, 2);
+}
+
+TEST_F(LRUCacheTest, EntriesWithPriority) {
   // Allocate 2 cache entries to high-pri pool.
   NewCache(5, 0.45);
 
@@ -140,15 +164,15 @@ TEST_F(LRUCacheTest, MidPointInsertion) {
   Insert("a", Cache::Priority::LOW);
   ValidateLRUList({"v", "X", "a", "Y", "Z"}, 2);
 
-  // Low-pri entries will be inserted to head of low-pri pool after lookup.
+  // Low-pri entries will be inserted to head of high-pri pool after lookup.
   ASSERT_TRUE(Lookup("v"));
-  ValidateLRUList({"X", "a", "v", "Y", "Z"}, 2);
+  ValidateLRUList({"X", "a", "Y", "Z", "v"}, 2);
 
   // High-pri entries will be inserted to the head of the list after lookup.
   ASSERT_TRUE(Lookup("X"));
-  ValidateLRUList({"a", "v", "Y", "Z", "X"}, 2);
+  ValidateLRUList({"a", "Y", "Z", "v", "X"}, 2);
   ASSERT_TRUE(Lookup("Z"));
-  ValidateLRUList({"a", "v", "Y", "X", "Z"}, 2);
+  ValidateLRUList({"a", "Y", "v", "X", "Z"}, 2);
 
   Erase("Y");
   ValidateLRUList({"a", "v", "X", "Z"}, 2);
@@ -161,7 +185,7 @@ TEST_F(LRUCacheTest, MidPointInsertion) {
   Insert("g", Cache::Priority::LOW);
   ValidateLRUList({"d", "e", "f", "g", "Z"}, 1);
   ASSERT_TRUE(Lookup("d"));
-  ValidateLRUList({"e", "f", "g", "d", "Z"}, 1);
+  ValidateLRUList({"e", "f", "g", "Z", "d"}, 2);
 }
 
 }  // namespace rocksdb

db/c.cc

@@ -90,6 +90,7 @@ using rocksdb::LiveFileMetaData;
 using rocksdb::BackupEngine;
 using rocksdb::BackupableDBOptions;
 using rocksdb::BackupInfo;
+using rocksdb::BackupID;
 using rocksdb::RestoreOptions;
 using rocksdb::CompactRangeOptions;
 using rocksdb::RateLimiter;
@@ -531,10 +532,18 @@ rocksdb_backup_engine_t* rocksdb_backup_engine_open(
 }
 
 void rocksdb_backup_engine_create_new_backup(rocksdb_backup_engine_t* be,
-                                             rocksdb_t* db, char** errptr) {
+                                             rocksdb_t* db,
+                                             char** errptr) {
   SaveError(errptr, be->rep->CreateNewBackup(db->rep));
 }
 
+void rocksdb_backup_engine_create_new_backup_flush(rocksdb_backup_engine_t* be,
+                                                   rocksdb_t* db,
+                                                   unsigned char flush_before_backup,
+                                                   char** errptr) {
+  SaveError(errptr, be->rep->CreateNewBackup(db->rep, flush_before_backup));
+}
+
 void rocksdb_backup_engine_purge_old_backups(rocksdb_backup_engine_t* be,
                                              uint32_t num_backups_to_keep,
                                              char** errptr) {
@@ -554,6 +563,12 @@ void rocksdb_restore_options_set_keep_log_files(rocksdb_restore_options_t* opt,
   opt->rep.keep_log_files = v;
 }
 
+
+void rocksdb_backup_engine_verify_backup(rocksdb_backup_engine_t* be,
+    uint32_t backup_id, char** errptr) {
+  SaveError(errptr, be->rep->VerifyBackup(static_cast<BackupID>(backup_id)));
+}
+
 void rocksdb_backup_engine_restore_db_from_latest_backup(
     rocksdb_backup_engine_t* be, const char* db_dir, const char* wal_dir,
     const rocksdb_restore_options_t* restore_options, char** errptr) {
@@ -1969,11 +1984,6 @@ void rocksdb_block_based_options_set_pin_l0_filter_and_index_blocks_in_cache(
   options->rep.pin_l0_filter_and_index_blocks_in_cache = v;
 }
 
-void rocksdb_block_based_options_set_pin_top_level_index_and_filter(
-    rocksdb_block_based_table_options_t* options, unsigned char v) {
-  options->rep.pin_top_level_index_and_filter = v;
-}
-
 void rocksdb_options_set_block_based_table_factory(
     rocksdb_options_t *opt,
     rocksdb_block_based_table_options_t* table_options) {

db/column_family_test.cc

@@ -57,9 +57,9 @@ class EnvCounter : public EnvWrapper {
   std::atomic<int> num_new_writable_file_;
 };
 
-class ColumnFamilyTestBase : public testing::Test {
+class ColumnFamilyTest : public testing::Test {
  public:
-  ColumnFamilyTestBase(uint32_t format) : rnd_(139), format_(format) {
+  ColumnFamilyTest() : rnd_(139) {
     env_ = new EnvCounter(Env::Default());
     dbname_ = test::TmpDir() + "/column_family_test";
     db_options_.create_if_missing = true;
@@ -68,7 +68,7 @@ class ColumnFamilyTestBase : public testing::Test {
     DestroyDB(dbname_, Options(db_options_, column_family_options_));
   }
 
-  virtual ~ColumnFamilyTestBase() {
+  ~ColumnFamilyTest() {
     std::vector<ColumnFamilyDescriptor> column_families;
     for (auto h : handles_) {
       ColumnFamilyDescriptor cfdescriptor;
@@ -81,12 +81,6 @@ class ColumnFamilyTestBase : public testing::Test {
     delete env_;
   }
 
-  BlockBasedTableOptions GetBlockBasedTableOptions() {
-    BlockBasedTableOptions options;
-    options.format_version = format_;
-    return options;
-  }
-
   // Return the value to associate with the specified key
   Slice Value(int k, std::string* storage) {
     if (k == 0) {
@@ -305,6 +299,9 @@ class ColumnFamilyTestBase : public testing::Test {
   }
 
   void PutRandomData(int cf, int num, int key_value_size, bool save = false) {
+    if (cf >= static_cast<int>(keys_.size())) {
+      keys_.resize(cf + 1);
+    }
     for (int i = 0; i < num; ++i) {
       // 10 bytes for key, rest is value
       if (!save) {
@@ -312,7 +309,7 @@ class ColumnFamilyTestBase : public testing::Test {
                       RandomString(&rnd_, key_value_size - 10)));
       } else {
         std::string key = test::RandomKey(&rnd_, 11);
-        keys_.insert(key);
+        keys_[cf].insert(key);
         ASSERT_OK(Put(cf, key, RandomString(&rnd_, key_value_size - 10)));
       }
     }
@@ -512,29 +509,16 @@ class ColumnFamilyTestBase : public testing::Test {
 
   std::vector<ColumnFamilyHandle*> handles_;
   std::vector<std::string> names_;
-  std::set<std::string> keys_;
+  std::vector<std::set<std::string>> keys_;
   ColumnFamilyOptions column_family_options_;
   DBOptions db_options_;
   std::string dbname_;
   DB* db_ = nullptr;
   EnvCounter* env_;
   Random rnd_;
-  uint32_t format_;
 };
 
-class ColumnFamilyTest
-    : public ColumnFamilyTestBase,
-      virtual public ::testing::WithParamInterface<uint32_t> {
- public:
-  ColumnFamilyTest() : ColumnFamilyTestBase(GetParam()) {}
-};
-
-INSTANTIATE_TEST_CASE_P(FormatDef, ColumnFamilyTest,
-                        testing::Values(test::kDefaultFormatVersion));
-INSTANTIATE_TEST_CASE_P(FormatLatest, ColumnFamilyTest,
-                        testing::Values(test::kLatestFormatVersion));
-
-TEST_P(ColumnFamilyTest, DontReuseColumnFamilyID) {
+TEST_F(ColumnFamilyTest, DontReuseColumnFamilyID) {
   for (int iter = 0; iter < 3; ++iter) {
     Open();
     CreateColumnFamilies({"one", "two", "three"});
@@ -553,8 +537,7 @@ TEST_P(ColumnFamilyTest, DontReuseColumnFamilyID) {
       Reopen();
     }
     CreateColumnFamilies({"three2"});
-    // ID 3 that was used for dropped column family "three" should not be
-    // reused
+    // ID 3 that was used for dropped column family "three" should not be reused
     auto cfh3 = reinterpret_cast<ColumnFamilyHandleImpl*>(handles_[3]);
     ASSERT_EQ(4U, cfh3->GetID());
     Close();
@@ -563,7 +546,7 @@ TEST_P(ColumnFamilyTest, DontReuseColumnFamilyID) {
 }
 
 #ifndef ROCKSDB_LITE
-TEST_P(ColumnFamilyTest, CreateCFRaceWithGetAggProperty) {
+TEST_F(ColumnFamilyTest, CreateCFRaceWithGetAggProperty) {
   Open();
 
   rocksdb::SyncPoint::GetInstance()->LoadDependency(
@@ -586,13 +569,10 @@ TEST_P(ColumnFamilyTest, CreateCFRaceWithGetAggProperty) {
 }
 #endif  // !ROCKSDB_LITE
 
-class FlushEmptyCFTestWithParam
-    : public ColumnFamilyTestBase,
-      virtual public testing::WithParamInterface<std::tuple<uint32_t, bool>> {
+class FlushEmptyCFTestWithParam : public ColumnFamilyTest,
+                                  public testing::WithParamInterface<bool> {
  public:
-  FlushEmptyCFTestWithParam()
-      : ColumnFamilyTestBase(std::get<0>(GetParam())),
-        allow_2pc_(std::get<1>(GetParam())) {}
+  FlushEmptyCFTestWithParam() { allow_2pc_ = GetParam(); }
 
   // Required if inheriting from testing::WithParamInterface<>
   static void SetUpTestCase() {}
@@ -717,16 +697,10 @@ TEST_P(FlushEmptyCFTestWithParam, FlushEmptyCFTest2) {
   db_options_.env = env_;
 }
 
-INSTANTIATE_TEST_CASE_P(
-    FormatDef, FlushEmptyCFTestWithParam,
-    testing::Values(std::make_tuple(test::kDefaultFormatVersion, true),
-                    std::make_tuple(test::kDefaultFormatVersion, false)));
-INSTANTIATE_TEST_CASE_P(
-    FormatLatest, FlushEmptyCFTestWithParam,
-    testing::Values(std::make_tuple(test::kLatestFormatVersion, true),
-                    std::make_tuple(test::kLatestFormatVersion, false)));
+INSTANTIATE_TEST_CASE_P(FlushEmptyCFTestWithParam, FlushEmptyCFTestWithParam,
+                        ::testing::Bool());
 
-TEST_P(ColumnFamilyTest, AddDrop) {
+TEST_F(ColumnFamilyTest, AddDrop) {
   Open();
   CreateColumnFamilies({"one", "two", "three"});
   ASSERT_EQ("NOT_FOUND", Get(1, "fodor"));
@@ -752,7 +726,7 @@ TEST_P(ColumnFamilyTest, AddDrop) {
               std::vector<std::string>({"default", "four", "three"}));
 }
 
-TEST_P(ColumnFamilyTest, BulkAddDrop) {
+TEST_F(ColumnFamilyTest, BulkAddDrop) {
   constexpr int kNumCF = 1000;
   ColumnFamilyOptions cf_options;
   WriteOptions write_options;
@@ -790,7 +764,7 @@ TEST_P(ColumnFamilyTest, BulkAddDrop) {
   ASSERT_TRUE(families == std::vector<std::string>({"default"}));
 }
 
-TEST_P(ColumnFamilyTest, DropTest) {
+TEST_F(ColumnFamilyTest, DropTest) {
   // first iteration - dont reopen DB before dropping
   // second iteration - reopen DB before dropping
   for (int iter = 0; iter < 2; ++iter) {
@@ -814,7 +788,7 @@ TEST_P(ColumnFamilyTest, DropTest) {
   }
 }
 
-TEST_P(ColumnFamilyTest, WriteBatchFailure) {
+TEST_F(ColumnFamilyTest, WriteBatchFailure) {
   Open();
   CreateColumnFamiliesAndReopen({"one", "two"});
   WriteBatch batch;
@@ -832,7 +806,7 @@ TEST_P(ColumnFamilyTest, WriteBatchFailure) {
   Close();
 }
 
-TEST_P(ColumnFamilyTest, ReadWrite) {
+TEST_F(ColumnFamilyTest, ReadWrite) {
   Open();
   CreateColumnFamiliesAndReopen({"one", "two"});
   ASSERT_OK(Put(0, "foo", "v1"));
@@ -856,7 +830,7 @@ TEST_P(ColumnFamilyTest, ReadWrite) {
   Close();
 }
 
-TEST_P(ColumnFamilyTest, IgnoreRecoveredLog) {
+TEST_F(ColumnFamilyTest, IgnoreRecoveredLog) {
   std::string backup_logs = dbname_ + "/backup_logs";
 
   // delete old files in backup_logs directory
@@ -932,7 +906,7 @@ TEST_P(ColumnFamilyTest, IgnoreRecoveredLog) {
 }
 
 #ifndef ROCKSDB_LITE  // TEST functions used are not supported
-TEST_P(ColumnFamilyTest, FlushTest) {
+TEST_F(ColumnFamilyTest, FlushTest) {
   Open();
   CreateColumnFamiliesAndReopen({"one", "two"});
   ASSERT_OK(Put(0, "foo", "v1"));
@@ -980,7 +954,7 @@ TEST_P(ColumnFamilyTest, FlushTest) {
 }
 
 // Makes sure that obsolete log files get deleted
-TEST_P(ColumnFamilyTest, LogDeletionTest) {
+TEST_F(ColumnFamilyTest, LogDeletionTest) {
   db_options_.max_total_wal_size = std::numeric_limits<uint64_t>::max();
   column_family_options_.arena_block_size = 4 * 1024;
   column_family_options_.write_buffer_size = 128000;  // 128KB
@@ -1048,7 +1022,7 @@ TEST_P(ColumnFamilyTest, LogDeletionTest) {
 }
 #endif  // !ROCKSDB_LITE
 
-TEST_P(ColumnFamilyTest, CrashAfterFlush) {
+TEST_F(ColumnFamilyTest, CrashAfterFlush) {
   std::unique_ptr<FaultInjectionTestEnv> fault_env(
       new FaultInjectionTestEnv(env_));
   db_options_.env = fault_env.get();
@@ -1080,7 +1054,7 @@ TEST_P(ColumnFamilyTest, CrashAfterFlush) {
   db_options_.env = env_;
 }
 
-TEST_P(ColumnFamilyTest, OpenNonexistentColumnFamily) {
+TEST_F(ColumnFamilyTest, OpenNonexistentColumnFamily) {
   ASSERT_OK(TryOpen({"default"}));
   Close();
   ASSERT_TRUE(TryOpen({"default", "dne"}).IsInvalidArgument());
@@ -1088,7 +1062,7 @@ TEST_P(ColumnFamilyTest, OpenNonexistentColumnFamily) {
 
 #ifndef ROCKSDB_LITE  // WaitForFlush() is not supported
 // Makes sure that obsolete log files get deleted
-TEST_P(ColumnFamilyTest, DifferentWriteBufferSizes) {
+TEST_F(ColumnFamilyTest, DifferentWriteBufferSizes) {
   // disable flushing stale column families
   db_options_.max_total_wal_size = std::numeric_limits<uint64_t>::max();
   Open();
@@ -1194,7 +1168,7 @@ TEST_P(ColumnFamilyTest, DifferentWriteBufferSizes) {
 #endif  // !ROCKSDB_LITE
 
 #ifndef ROCKSDB_LITE  // Cuckoo is not supported in lite
-TEST_P(ColumnFamilyTest, MemtableNotSupportSnapshot) {
+TEST_F(ColumnFamilyTest, MemtableNotSupportSnapshot) {
   db_options_.allow_concurrent_memtable_write = false;
   Open();
   auto* s1 = dbfull()->GetSnapshot();
@@ -1232,7 +1206,7 @@ static TestComparator third_comparator;
 static TestComparator fourth_comparator;
 
 // Test that we can retrieve the comparator from a created CF
-TEST_P(ColumnFamilyTest, GetComparator) {
+TEST_F(ColumnFamilyTest, GetComparator) {
   Open();
   // Add a column family with no comparator specified
   CreateColumnFamilies({"first"});
@@ -1251,7 +1225,7 @@ TEST_P(ColumnFamilyTest, GetComparator) {
   Close();
 }
 
-TEST_P(ColumnFamilyTest, DifferentMergeOperators) {
+TEST_F(ColumnFamilyTest, DifferentMergeOperators) {
   Open();
   CreateColumnFamilies({"first", "second"});
   ColumnFamilyOptions default_cf, first, second;
@@ -1282,7 +1256,7 @@ TEST_P(ColumnFamilyTest, DifferentMergeOperators) {
 }
 
 #ifndef ROCKSDB_LITE  // WaitForFlush() is not supported
-TEST_P(ColumnFamilyTest, DifferentCompactionStyles) {
+TEST_F(ColumnFamilyTest, DifferentCompactionStyles) {
   Open();
   CreateColumnFamilies({"one", "two"});
   ColumnFamilyOptions default_cf, one, two;
@@ -1294,7 +1268,7 @@ TEST_P(ColumnFamilyTest, DifferentCompactionStyles) {
   default_cf.target_file_size_base = 30 << 10;
   default_cf.max_compaction_bytes = static_cast<uint64_t>(1) << 60;
 
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+  BlockBasedTableOptions table_options;
   table_options.no_block_cache = true;
   default_cf.table_factory.reset(NewBlockBasedTableFactory(table_options));
 
@@ -1354,7 +1328,7 @@ TEST_P(ColumnFamilyTest, DifferentCompactionStyles) {
 #ifndef ROCKSDB_LITE
 // Sync points not supported in RocksDB Lite
 
-TEST_P(ColumnFamilyTest, MultipleManualCompactions) {
+TEST_F(ColumnFamilyTest, MultipleManualCompactions) {
   Open();
   CreateColumnFamilies({"one", "two"});
   ColumnFamilyOptions default_cf, one, two;
@@ -1366,7 +1340,7 @@ TEST_P(ColumnFamilyTest, MultipleManualCompactions) {
   default_cf.write_buffer_size = 64 << 10;  // 64KB
   default_cf.target_file_size_base = 30 << 10;
   default_cf.max_compaction_bytes = default_cf.target_file_size_base * 1100;
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+  BlockBasedTableOptions table_options;
   table_options.no_block_cache = true;
   default_cf.table_factory.reset(NewBlockBasedTableFactory(table_options));
 
@@ -1443,17 +1417,15 @@ TEST_P(ColumnFamilyTest, MultipleManualCompactions) {
   CompactAll(2);
   AssertFilesPerLevel("0,1", 2);
   // Compare against saved keys
-  std::set<std::string>::iterator key_iter = keys_.begin();
-  while (key_iter != keys_.end()) {
+  std::set<std::string>::iterator key_iter = keys_[1].begin();
+  while (key_iter != keys_[1].end()) {
     ASSERT_NE("NOT_FOUND", Get(1, *key_iter));
     key_iter++;
   }
-  rocksdb::SyncPoint::GetInstance()->DisableProcessing();
-  rocksdb::SyncPoint::GetInstance()->ClearAllCallBacks();
   Close();
 }
 
-TEST_P(ColumnFamilyTest, AutomaticAndManualCompactions) {
+TEST_F(ColumnFamilyTest, AutomaticAndManualCompactions) {
   Open();
   CreateColumnFamilies({"one", "two"});
   ColumnFamilyOptions default_cf, one, two;
@@ -1465,8 +1437,7 @@ TEST_P(ColumnFamilyTest, AutomaticAndManualCompactions) {
   default_cf.write_buffer_size = 64 << 10;  // 64KB
   default_cf.target_file_size_base = 30 << 10;
   default_cf.max_compaction_bytes = default_cf.target_file_size_base * 1100;
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
-  ;
+  BlockBasedTableOptions table_options;
   table_options.no_block_cache = true;
   default_cf.table_factory.reset(NewBlockBasedTableFactory(table_options));
 
@@ -1540,16 +1511,14 @@ TEST_P(ColumnFamilyTest, AutomaticAndManualCompactions) {
   CompactAll(2);
   AssertFilesPerLevel("0,1", 2);
   // Compare against saved keys
-  std::set<std::string>::iterator key_iter = keys_.begin();
-  while (key_iter != keys_.end()) {
+  std::set<std::string>::iterator key_iter = keys_[1].begin();
+  while (key_iter != keys_[1].end()) {
     ASSERT_NE("NOT_FOUND", Get(1, *key_iter));
     key_iter++;
   }
-  rocksdb::SyncPoint::GetInstance()->DisableProcessing();
-  rocksdb::SyncPoint::GetInstance()->ClearAllCallBacks();
 }
 
-TEST_P(ColumnFamilyTest, ManualAndAutomaticCompactions) {
+TEST_F(ColumnFamilyTest, ManualAndAutomaticCompactions) {
   Open();
   CreateColumnFamilies({"one", "two"});
   ColumnFamilyOptions default_cf, one, two;
@@ -1561,8 +1530,7 @@ TEST_P(ColumnFamilyTest, ManualAndAutomaticCompactions) {
   default_cf.write_buffer_size = 64 << 10;  // 64KB
   default_cf.target_file_size_base = 30 << 10;
   default_cf.max_compaction_bytes = default_cf.target_file_size_base * 1100;
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
-  ;
+  BlockBasedTableOptions table_options;
   table_options.no_block_cache = true;
   default_cf.table_factory.reset(NewBlockBasedTableFactory(table_options));
 
@@ -1639,16 +1607,14 @@ TEST_P(ColumnFamilyTest, ManualAndAutomaticCompactions) {
   CompactAll(2);
   AssertFilesPerLevel("0,1", 2);
   // Compare against saved keys
-  std::set<std::string>::iterator key_iter = keys_.begin();
-  while (key_iter != keys_.end()) {
+  std::set<std::string>::iterator key_iter = keys_[1].begin();
+  while (key_iter != keys_[1].end()) {
     ASSERT_NE("NOT_FOUND", Get(1, *key_iter));
     key_iter++;
   }
-  rocksdb::SyncPoint::GetInstance()->DisableProcessing();
-  rocksdb::SyncPoint::GetInstance()->ClearAllCallBacks();
 }
 
-TEST_P(ColumnFamilyTest, SameCFManualManualCompactions) {
+TEST_F(ColumnFamilyTest, SameCFManualManualCompactions) {
   Open();
   CreateColumnFamilies({"one"});
   ColumnFamilyOptions default_cf, one;
@@ -1660,8 +1626,7 @@ TEST_P(ColumnFamilyTest, SameCFManualManualCompactions) {
   default_cf.write_buffer_size = 64 << 10;  // 64KB
   default_cf.target_file_size_base = 30 << 10;
   default_cf.max_compaction_bytes = default_cf.target_file_size_base * 1100;
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
-  ;
+  BlockBasedTableOptions table_options;
   table_options.no_block_cache = true;
   default_cf.table_factory.reset(NewBlockBasedTableFactory(table_options));
 
@@ -1741,16 +1706,14 @@ TEST_P(ColumnFamilyTest, SameCFManualManualCompactions) {
   ASSERT_LE(NumTableFilesAtLevel(0, 1), 2);
 
   // Compare against saved keys
-  std::set<std::string>::iterator key_iter = keys_.begin();
-  while (key_iter != keys_.end()) {
+  std::set<std::string>::iterator key_iter = keys_[1].begin();
+  while (key_iter != keys_[1].end()) {
     ASSERT_NE("NOT_FOUND", Get(1, *key_iter));
     key_iter++;
   }
-  rocksdb::SyncPoint::GetInstance()->DisableProcessing();
-  rocksdb::SyncPoint::GetInstance()->ClearAllCallBacks();
 }
 
-TEST_P(ColumnFamilyTest, SameCFManualAutomaticCompactions) {
+TEST_F(ColumnFamilyTest, SameCFManualAutomaticCompactions) {
   Open();
   CreateColumnFamilies({"one"});
   ColumnFamilyOptions default_cf, one;
@@ -1762,8 +1725,7 @@ TEST_P(ColumnFamilyTest, SameCFManualAutomaticCompactions) {
   default_cf.write_buffer_size = 64 << 10;  // 64KB
   default_cf.target_file_size_base = 30 << 10;
   default_cf.max_compaction_bytes = default_cf.target_file_size_base * 1100;
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
-  ;
+  BlockBasedTableOptions table_options;
   table_options.no_block_cache = true;
   default_cf.table_factory.reset(NewBlockBasedTableFactory(table_options));
 
@@ -1834,16 +1796,14 @@ TEST_P(ColumnFamilyTest, SameCFManualAutomaticCompactions) {
   ASSERT_LE(NumTableFilesAtLevel(0, 1), 2);
 
   // Compare against saved keys
-  std::set<std::string>::iterator key_iter = keys_.begin();
-  while (key_iter != keys_.end()) {
+  std::set<std::string>::iterator key_iter = keys_[1].begin();
+  while (key_iter != keys_[1].end()) {
     ASSERT_NE("NOT_FOUND", Get(1, *key_iter));
     key_iter++;
   }
-  rocksdb::SyncPoint::GetInstance()->DisableProcessing();
-  rocksdb::SyncPoint::GetInstance()->ClearAllCallBacks();
 }
 
-TEST_P(ColumnFamilyTest, SameCFManualAutomaticCompactionsLevel) {
+TEST_F(ColumnFamilyTest, SameCFManualAutomaticCompactionsLevel) {
   Open();
   CreateColumnFamilies({"one"});
   ColumnFamilyOptions default_cf, one;
@@ -1855,8 +1815,7 @@ TEST_P(ColumnFamilyTest, SameCFManualAutomaticCompactionsLevel) {
   default_cf.write_buffer_size = 64 << 10;  // 64KB
   default_cf.target_file_size_base = 30 << 10;
   default_cf.max_compaction_bytes = default_cf.target_file_size_base * 1100;
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
-  ;
+  BlockBasedTableOptions table_options;
   table_options.no_block_cache = true;
   default_cf.table_factory.reset(NewBlockBasedTableFactory(table_options));
 
@@ -1927,13 +1886,11 @@ TEST_P(ColumnFamilyTest, SameCFManualAutomaticCompactionsLevel) {
   AssertFilesPerLevel("0,1", 1);
 
   // Compare against saved keys
-  std::set<std::string>::iterator key_iter = keys_.begin();
-  while (key_iter != keys_.end()) {
+  std::set<std::string>::iterator key_iter = keys_[1].begin();
+  while (key_iter != keys_[1].end()) {
     ASSERT_NE("NOT_FOUND", Get(1, *key_iter));
     key_iter++;
   }
-  rocksdb::SyncPoint::GetInstance()->DisableProcessing();
-  rocksdb::SyncPoint::GetInstance()->ClearAllCallBacks();
 }
 
 // In this test, we generate enough files to trigger automatic compactions.
@@ -1942,7 +1899,7 @@ TEST_P(ColumnFamilyTest, SameCFManualAutomaticCompactionsLevel) {
 // This will wait because the automatic compaction has files it needs.
 // Once the conflict is hit, the automatic compaction starts and ends
 // Then the manual will run and end.
-TEST_P(ColumnFamilyTest, SameCFAutomaticManualCompactions) {
+TEST_F(ColumnFamilyTest, SameCFAutomaticManualCompactions) {
   Open();
   CreateColumnFamilies({"one"});
   ColumnFamilyOptions default_cf, one;
@@ -1954,8 +1911,7 @@ TEST_P(ColumnFamilyTest, SameCFAutomaticManualCompactions) {
   default_cf.write_buffer_size = 64 << 10;  // 64KB
   default_cf.target_file_size_base = 30 << 10;
   default_cf.max_compaction_bytes = default_cf.target_file_size_base * 1100;
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
-  ;
+  BlockBasedTableOptions table_options;
   table_options.no_block_cache = true;
   default_cf.table_factory.reset(NewBlockBasedTableFactory(table_options));
 
@@ -2019,13 +1975,11 @@ TEST_P(ColumnFamilyTest, SameCFAutomaticManualCompactions) {
   // VERIFY compaction "one"
   AssertFilesPerLevel("1", 1);
   // Compare against saved keys
-  std::set<std::string>::iterator key_iter = keys_.begin();
-  while (key_iter != keys_.end()) {
+  std::set<std::string>::iterator key_iter = keys_[1].begin();
+  while (key_iter != keys_[1].end()) {
     ASSERT_NE("NOT_FOUND", Get(1, *key_iter));
     key_iter++;
   }
-  rocksdb::SyncPoint::GetInstance()->DisableProcessing();
-  rocksdb::SyncPoint::GetInstance()->ClearAllCallBacks();
 }
 #endif  // !ROCKSDB_LITE
 
@@ -2042,7 +1996,7 @@ std::string IterStatus(Iterator* iter) {
 }
 }  // anonymous namespace
 
-TEST_P(ColumnFamilyTest, NewIteratorsTest) {
+TEST_F(ColumnFamilyTest, NewIteratorsTest) {
   // iter == 0 -- no tailing
   // iter == 2 -- tailing
   for (int iter = 0; iter < 2; ++iter) {
@@ -2089,7 +2043,7 @@ TEST_P(ColumnFamilyTest, NewIteratorsTest) {
 #endif  // !ROCKSDB_LITE
 
 #ifndef ROCKSDB_LITE  // ReadOnlyDB is not supported
-TEST_P(ColumnFamilyTest, ReadOnlyDBTest) {
+TEST_F(ColumnFamilyTest, ReadOnlyDBTest) {
   Open();
   CreateColumnFamiliesAndReopen({"one", "two", "three", "four"});
   ASSERT_OK(Put(0, "a", "b"));
@@ -2141,7 +2095,7 @@ TEST_P(ColumnFamilyTest, ReadOnlyDBTest) {
 #endif  // !ROCKSDB_LITE
 
 #ifndef ROCKSDB_LITE  //  WaitForFlush() is not supported in lite
-TEST_P(ColumnFamilyTest, DontRollEmptyLogs) {
+TEST_F(ColumnFamilyTest, DontRollEmptyLogs) {
   Open();
   CreateColumnFamiliesAndReopen({"one", "two", "three", "four"});
 
@@ -2165,7 +2119,7 @@ TEST_P(ColumnFamilyTest, DontRollEmptyLogs) {
 #endif  // !ROCKSDB_LITE
 
 #ifndef ROCKSDB_LITE  //  WaitForCompaction() is not supported in lite
-TEST_P(ColumnFamilyTest, FlushStaleColumnFamilies) {
+TEST_F(ColumnFamilyTest, FlushStaleColumnFamilies) {
   Open();
   CreateColumnFamilies({"one", "two"});
   ColumnFamilyOptions default_cf, one, two;
@@ -2200,7 +2154,7 @@ TEST_P(ColumnFamilyTest, FlushStaleColumnFamilies) {
 }
 #endif  // !ROCKSDB_LITE
 
-TEST_P(ColumnFamilyTest, CreateMissingColumnFamilies) {
+TEST_F(ColumnFamilyTest, CreateMissingColumnFamilies) {
   Status s = TryOpen({"one", "two"});
   ASSERT_TRUE(!s.ok());
   db_options_.create_missing_column_families = true;
@@ -2209,7 +2163,7 @@ TEST_P(ColumnFamilyTest, CreateMissingColumnFamilies) {
   Close();
 }
 
-TEST_P(ColumnFamilyTest, SanitizeOptions) {
+TEST_F(ColumnFamilyTest, SanitizeOptions) {
   DBOptions db_options;
   for (int s = kCompactionStyleLevel; s <= kCompactionStyleUniversal; ++s) {
     for (int l = 0; l <= 2; l++) {
@@ -2258,7 +2212,7 @@ TEST_P(ColumnFamilyTest, SanitizeOptions) {
   }
 }
 
-TEST_P(ColumnFamilyTest, ReadDroppedColumnFamily) {
+TEST_F(ColumnFamilyTest, ReadDroppedColumnFamily) {
   // iter 0 -- drop CF, don't reopen
   // iter 1 -- delete CF, reopen
   for (int iter = 0; iter < 2; ++iter) {
@@ -2330,7 +2284,7 @@ TEST_P(ColumnFamilyTest, ReadDroppedColumnFamily) {
   }
 }
 
-TEST_P(ColumnFamilyTest, FlushAndDropRaceCondition) {
+TEST_F(ColumnFamilyTest, FlushAndDropRaceCondition) {
   db_options_.create_missing_column_families = true;
   Open({"default", "one"});
   ColumnFamilyOptions options;
@@ -2396,13 +2350,12 @@ TEST_P(ColumnFamilyTest, FlushAndDropRaceCondition) {
 // skipped as persisting options is not supported in ROCKSDB_LITE
 namespace {
 std::atomic<int> test_stage(0);
-std::atomic<bool> ordered_by_writethread(false);
 const int kMainThreadStartPersistingOptionsFile = 1;
 const int kChildThreadFinishDroppingColumnFamily = 2;
+const int kChildThreadWaitingMainThreadPersistOptions = 3;
 void DropSingleColumnFamily(ColumnFamilyTest* cf_test, int cf_id,
                             std::vector<Comparator*>* comparators) {
-  while (test_stage < kMainThreadStartPersistingOptionsFile &&
-         !ordered_by_writethread) {
+  while (test_stage < kMainThreadStartPersistingOptionsFile) {
     Env::Default()->SleepForMicroseconds(100);
   }
   cf_test->DropColumnFamilies({cf_id});
@@ -2414,7 +2367,7 @@ void DropSingleColumnFamily(ColumnFamilyTest* cf_test, int cf_id,
 }
 }  // namespace
 
-TEST_P(ColumnFamilyTest, CreateAndDropRace) {
+TEST_F(ColumnFamilyTest, CreateAndDropRace) {
   const int kCfCount = 5;
   std::vector<ColumnFamilyOptions> cf_opts;
   std::vector<Comparator*> comparators;
@@ -2435,8 +2388,7 @@ TEST_P(ColumnFamilyTest, CreateAndDropRace) {
     // will be blocked until some other thread update the test_stage.
     if (main_thread_id == current_thread_id) {
       test_stage = kMainThreadStartPersistingOptionsFile;
-      while (test_stage < kChildThreadFinishDroppingColumnFamily &&
-             !ordered_by_writethread) {
+      while (test_stage < kChildThreadFinishDroppingColumnFamily) {
         Env::Default()->SleepForMicroseconds(100);
       }
     }
@@ -2447,7 +2399,11 @@ TEST_P(ColumnFamilyTest, CreateAndDropRace) {
         // This means a thread doing DropColumnFamily() is waiting for
         // other thread to finish persisting options.
         // In such case, we update the test_stage to unblock the main thread.
-        ordered_by_writethread = true;
+        test_stage = kChildThreadWaitingMainThreadPersistOptions;
+
+        // Note that based on the test setting, this must not be the
+        // main thread.
+        ASSERT_NE(main_thread_id, std::this_thread::get_id());
       });
 
   // Create a database with four column families
@@ -2458,8 +2414,7 @@ TEST_P(ColumnFamilyTest, CreateAndDropRace) {
 
   // Start a thread that will drop the first column family
   // and its comparator
-  rocksdb::port::Thread drop_cf_thread(DropSingleColumnFamily, this, 1,
-                                       &comparators);
+  rocksdb::port::Thread drop_cf_thread(DropSingleColumnFamily, this, 1, &comparators);
 
   DropColumnFamilies({2});
 
@@ -2471,13 +2426,10 @@ TEST_P(ColumnFamilyTest, CreateAndDropRace) {
       delete comparator;
     }
   }
-
-  rocksdb::SyncPoint::GetInstance()->DisableProcessing();
-  rocksdb::SyncPoint::GetInstance()->ClearAllCallBacks();
 }
 #endif  // !ROCKSDB_LITE
 
-TEST_P(ColumnFamilyTest, WriteStallSingleColumnFamily) {
+TEST_F(ColumnFamilyTest, WriteStallSingleColumnFamily) {
   const uint64_t kBaseRate = 800000u;
   db_options_.delayed_write_rate = kBaseRate;
   db_options_.max_background_compactions = 6;
@@ -2659,7 +2611,7 @@ TEST_P(ColumnFamilyTest, WriteStallSingleColumnFamily) {
   ASSERT_EQ(kBaseRate / 1.25, GetDbDelayedWriteRate());
 }
 
-TEST_P(ColumnFamilyTest, CompactionSpeedupSingleColumnFamily) {
+TEST_F(ColumnFamilyTest, CompactionSpeedupSingleColumnFamily) {
   db_options_.max_background_compactions = 6;
   Open({"default"});
   ColumnFamilyData* cfd =
@@ -2723,7 +2675,7 @@ TEST_P(ColumnFamilyTest, CompactionSpeedupSingleColumnFamily) {
   ASSERT_EQ(1, dbfull()->TEST_BGCompactionsAllowed());
 }
 
-TEST_P(ColumnFamilyTest, WriteStallTwoColumnFamilies) {
+TEST_F(ColumnFamilyTest, WriteStallTwoColumnFamilies) {
   const uint64_t kBaseRate = 810000u;
   db_options_.delayed_write_rate = kBaseRate;
   Open();
@@ -2798,7 +2750,7 @@ TEST_P(ColumnFamilyTest, WriteStallTwoColumnFamilies) {
   ASSERT_EQ(kBaseRate / 1.25, GetDbDelayedWriteRate());
 }
 
-TEST_P(ColumnFamilyTest, CompactionSpeedupTwoColumnFamilies) {
+TEST_F(ColumnFamilyTest, CompactionSpeedupTwoColumnFamilies) {
   db_options_.max_background_compactions = 6;
   column_family_options_.soft_pending_compaction_bytes_limit = 200;
   column_family_options_.hard_pending_compaction_bytes_limit = 2000;
@@ -2863,7 +2815,7 @@ TEST_P(ColumnFamilyTest, CompactionSpeedupTwoColumnFamilies) {
   ASSERT_EQ(1, dbfull()->TEST_BGCompactionsAllowed());
 }
 
-TEST_P(ColumnFamilyTest, CreateAndDestoryOptions) {
+TEST_F(ColumnFamilyTest, CreateAndDestoryOptions) {
   std::unique_ptr<ColumnFamilyOptions> cfo(new ColumnFamilyOptions());
   ColumnFamilyHandle* cfh;
   Open();
@@ -2875,7 +2827,7 @@ TEST_P(ColumnFamilyTest, CreateAndDestoryOptions) {
   ASSERT_OK(db_->DestroyColumnFamilyHandle(cfh));
 }
 
-TEST_P(ColumnFamilyTest, CreateDropAndDestroy) {
+TEST_F(ColumnFamilyTest, CreateDropAndDestroy) {
   ColumnFamilyHandle* cfh;
   Open();
   ASSERT_OK(db_->CreateColumnFamily(ColumnFamilyOptions(), "yoyo", &cfh));
@@ -2886,7 +2838,7 @@ TEST_P(ColumnFamilyTest, CreateDropAndDestroy) {
 }
 
 #ifndef ROCKSDB_LITE
-TEST_P(ColumnFamilyTest, CreateDropAndDestroyWithoutFileDeletion) {
+TEST_F(ColumnFamilyTest, CreateDropAndDestroyWithoutFileDeletion) {
   ColumnFamilyHandle* cfh;
   Open();
   ASSERT_OK(db_->CreateColumnFamily(ColumnFamilyOptions(), "yoyo", &cfh));
@@ -2897,7 +2849,7 @@ TEST_P(ColumnFamilyTest, CreateDropAndDestroyWithoutFileDeletion) {
   ASSERT_OK(db_->DestroyColumnFamilyHandle(cfh));
 }
 
-TEST_P(ColumnFamilyTest, FlushCloseWALFiles) {
+TEST_F(ColumnFamilyTest, FlushCloseWALFiles) {
   SpecialEnv env(Env::Default());
   db_options_.env = &env;
   db_options_.max_background_flushes = 1;
@@ -2939,7 +2891,7 @@ TEST_P(ColumnFamilyTest, FlushCloseWALFiles) {
 #endif  // !ROCKSDB_LITE
 
 #ifndef ROCKSDB_LITE  // WaitForFlush() is not supported
-TEST_P(ColumnFamilyTest, IteratorCloseWALFile1) {
+TEST_F(ColumnFamilyTest, IteratorCloseWALFile1) {
   SpecialEnv env(Env::Default());
   db_options_.env = &env;
   db_options_.max_background_flushes = 1;
@@ -2984,7 +2936,7 @@ TEST_P(ColumnFamilyTest, IteratorCloseWALFile1) {
   Close();
 }
 
-TEST_P(ColumnFamilyTest, IteratorCloseWALFile2) {
+TEST_F(ColumnFamilyTest, IteratorCloseWALFile2) {
   SpecialEnv env(Env::Default());
   // Allow both of flush and purge job to schedule.
   env.SetBackgroundThreads(2, Env::HIGH);
@@ -3041,7 +2993,7 @@ TEST_P(ColumnFamilyTest, IteratorCloseWALFile2) {
 #endif  // !ROCKSDB_LITE
 
 #ifndef ROCKSDB_LITE  // TEST functions are not supported in lite
-TEST_P(ColumnFamilyTest, ForwardIteratorCloseWALFile) {
+TEST_F(ColumnFamilyTest, ForwardIteratorCloseWALFile) {
   SpecialEnv env(Env::Default());
   // Allow both of flush and purge job to schedule.
   env.SetBackgroundThreads(2, Env::HIGH);
@@ -3118,7 +3070,7 @@ TEST_P(ColumnFamilyTest, ForwardIteratorCloseWALFile) {
 // Disable on windows because SyncWAL requires env->IsSyncThreadSafe()
 // to return true which is not so in unbuffered mode.
 #ifndef OS_WIN
-TEST_P(ColumnFamilyTest, LogSyncConflictFlush) {
+TEST_F(ColumnFamilyTest, LogSyncConflictFlush) {
   Open();
   CreateColumnFamiliesAndReopen({"one", "two"});
 
@@ -3153,7 +3105,7 @@ TEST_P(ColumnFamilyTest, LogSyncConflictFlush) {
 // test is being used to ensure a roll of wal files.
 // Basic idea is to test that WAL truncation is being detected and not
 // ignored
-TEST_P(ColumnFamilyTest, DISABLED_LogTruncationTest) {
+TEST_F(ColumnFamilyTest, DISABLED_LogTruncationTest) {
   Open();
   CreateColumnFamiliesAndReopen({"one", "two"});
 
@@ -3223,7 +3175,7 @@ TEST_P(ColumnFamilyTest, DISABLED_LogTruncationTest) {
   env_->DeleteDir(backup_logs);
 }
 
-TEST_P(ColumnFamilyTest, DefaultCfPathsTest) {
+TEST_F(ColumnFamilyTest, DefaultCfPathsTest) {
   Open();
   // Leave cf_paths for one column families to be empty.
   // Files should be generated according to db_paths for that
@@ -3250,7 +3202,7 @@ TEST_P(ColumnFamilyTest, DefaultCfPathsTest) {
   ASSERT_EQ(1, GetSstFileCount(dbname_));
 }
 
-TEST_P(ColumnFamilyTest, MultipleCFPathsTest) {
+TEST_F(ColumnFamilyTest, MultipleCFPathsTest) {
   Open();
   // Configure Column family specific paths.
   ColumnFamilyOptions cf_opt1, cf_opt2;
@@ -3261,19 +3213,38 @@ TEST_P(ColumnFamilyTest, MultipleCFPathsTest) {
   CreateColumnFamilies({"one", "two"}, {cf_opt1, cf_opt2});
   Reopen({ColumnFamilyOptions(), cf_opt1, cf_opt2});
 
-  PutRandomData(1, 100, 100);
+  PutRandomData(1, 100, 100, true /* save */);
   Flush(1);
 
   // Check that files are generated in appropriate paths.
   ASSERT_EQ(1, GetSstFileCount(cf_opt1.cf_paths[0].path));
   ASSERT_EQ(0, GetSstFileCount(dbname_));
 
-  PutRandomData(2, 100, 100);
+  PutRandomData(2, 100, 100, true /* save */);
   Flush(2);
 
   ASSERT_EQ(1, GetSstFileCount(cf_opt2.cf_paths[0].path));
   ASSERT_EQ(0, GetSstFileCount(dbname_));
+
+  // Re-open and verify the keys.
+  Reopen({ColumnFamilyOptions(), cf_opt1, cf_opt2});
+  DBImpl* dbi = reinterpret_cast<DBImpl*>(db_);
+  for (int cf = 1; cf != 3; ++cf) {
+    ReadOptions read_options;
+    read_options.readahead_size = 0;
+    auto it = dbi->NewIterator(read_options, handles_[cf]);
+    for (it->SeekToFirst(); it->Valid(); it->Next()) {
+      Slice key(it->key());
+      ASSERT_NE(keys_[cf].end(), keys_[cf].find(key.ToString()));
+    }
+    delete it;
+
+    for (const auto& key : keys_[cf]) {
+      ASSERT_NE("NOT_FOUND", Get(cf, key));
+    }
+  }
 }
+
 }  // namespace rocksdb
 
 int main(int argc, char** argv) {

db/compaction_picker.cc

@@ -366,7 +366,7 @@ bool CompactionPicker::IsRangeInCompaction(VersionStorageInfo* vstorage,
   assert(level < NumberLevels());
 
   vstorage->GetOverlappingInputs(level, smallest, largest, &inputs,
-                                 *level_index, level_index);
+                                 level_index ? *level_index : 0, level_index);
   return AreFilesInCompaction(inputs);
 }
 
@@ -947,6 +947,78 @@ void CompactionPicker::UnregisterCompaction(Compaction* c) {
   compactions_in_progress_.erase(c);
 }
 
+void CompactionPicker::PickFilesMarkedForCompaction(
+    const std::string& cf_name, VersionStorageInfo* vstorage, int* start_level,
+    int* output_level, CompactionInputFiles* start_level_inputs) {
+  if (vstorage->FilesMarkedForCompaction().empty()) {
+    return;
+  }
+
+  auto continuation = [&, cf_name](std::pair<int, FileMetaData*> level_file) {
+    // If it's being compacted it has nothing to do here.
+    // If this assert() fails that means that some function marked some
+    // files as being_compacted, but didn't call ComputeCompactionScore()
+    assert(!level_file.second->being_compacted);
+    *start_level = level_file.first;
+    *output_level =
+        (*start_level == 0) ? vstorage->base_level() : *start_level + 1;
+
+    if (*start_level == 0 && !level0_compactions_in_progress()->empty()) {
+      return false;
+    }
+
+    start_level_inputs->files = {level_file.second};
+    start_level_inputs->level = *start_level;
+    return ExpandInputsToCleanCut(cf_name, vstorage, start_level_inputs);
+  };
+
+  // take a chance on a random file first
+  Random64 rnd(/* seed */ reinterpret_cast<uint64_t>(vstorage));
+  size_t random_file_index = static_cast<size_t>(rnd.Uniform(
+      static_cast<uint64_t>(vstorage->FilesMarkedForCompaction().size())));
+
+  if (continuation(vstorage->FilesMarkedForCompaction()[random_file_index])) {
+    // found the compaction!
+    return;
+  }
+
+  for (auto& level_file : vstorage->FilesMarkedForCompaction()) {
+    if (continuation(level_file)) {
+      // found the compaction!
+      return;
+    }
+  }
+  start_level_inputs->files.clear();
+}
+
+bool CompactionPicker::GetOverlappingL0Files(
+    VersionStorageInfo* vstorage, CompactionInputFiles* start_level_inputs,
+    int output_level, int* parent_index) {
+  // Two level 0 compaction won't run at the same time, so don't need to worry
+  // about files on level 0 being compacted.
+  assert(level0_compactions_in_progress()->empty());
+  InternalKey smallest, largest;
+  GetRange(*start_level_inputs, &smallest, &largest);
+  // Note that the next call will discard the file we placed in
+  // c->inputs_[0] earlier and replace it with an overlapping set
+  // which will include the picked file.
+  start_level_inputs->files.clear();
+  vstorage->GetOverlappingInputs(0, &smallest, &largest,
+                                 &(start_level_inputs->files));
+
+  // If we include more L0 files in the same compaction run it can
+  // cause the 'smallest' and 'largest' key to get extended to a
+  // larger range. So, re-invoke GetRange to get the new key range
+  GetRange(*start_level_inputs, &smallest, &largest);
+  if (IsRangeInCompaction(vstorage, &smallest, &largest, output_level,
+                          parent_index)) {
+    return false;
+  }
+  assert(!start_level_inputs->files.empty());
+
+  return true;
+}
+
 bool LevelCompactionPicker::NeedsCompaction(
     const VersionStorageInfo* vstorage) const {
   if (!vstorage->ExpiredTtlFiles().empty()) {
@@ -1018,9 +1090,6 @@ class LevelCompactionBuilder {
   // otherwise, returns false.
   bool PickIntraL0Compaction();
 
-  // If there is any file marked for compaction, put put it into inputs.
-  void PickFilesMarkedForCompaction();
-
   void PickExpiredTtlFiles();
 
   const std::string& cf_name_;
@@ -1049,50 +1118,6 @@ class LevelCompactionBuilder {
   static const int kMinFilesForIntraL0Compaction = 4;
 };
 
-void LevelCompactionBuilder::PickFilesMarkedForCompaction() {
-  if (vstorage_->FilesMarkedForCompaction().empty()) {
-    return;
-  }
-
-  auto continuation = [&](std::pair<int, FileMetaData*> level_file) {
-    // If it's being compacted it has nothing to do here.
-    // If this assert() fails that means that some function marked some
-    // files as being_compacted, but didn't call ComputeCompactionScore()
-    assert(!level_file.second->being_compacted);
-    start_level_ = level_file.first;
-    output_level_ =
-        (start_level_ == 0) ? vstorage_->base_level() : start_level_ + 1;
-
-    if (start_level_ == 0 &&
-        !compaction_picker_->level0_compactions_in_progress()->empty()) {
-      return false;
-    }
-
-    start_level_inputs_.files = {level_file.second};
-    start_level_inputs_.level = start_level_;
-    return compaction_picker_->ExpandInputsToCleanCut(cf_name_, vstorage_,
-                                                      &start_level_inputs_);
-  };
-
-  // take a chance on a random file first
-  Random64 rnd(/* seed */ reinterpret_cast<uint64_t>(vstorage_));
-  size_t random_file_index = static_cast<size_t>(rnd.Uniform(
-      static_cast<uint64_t>(vstorage_->FilesMarkedForCompaction().size())));
-
-  if (continuation(vstorage_->FilesMarkedForCompaction()[random_file_index])) {
-    // found the compaction!
-    return;
-  }
-
-  for (auto& level_file : vstorage_->FilesMarkedForCompaction()) {
-    if (continuation(level_file)) {
-      // found the compaction!
-      return;
-    }
-  }
-  start_level_inputs_.files.clear();
-}
-
 void LevelCompactionBuilder::PickExpiredTtlFiles() {
   if (vstorage_->ExpiredTtlFiles().empty()) {
     return;
@@ -1182,7 +1207,9 @@ void LevelCompactionBuilder::SetupInitialFiles() {
   if (start_level_inputs_.empty()) {
     parent_index_ = base_index_ = -1;
 
-    PickFilesMarkedForCompaction();
+    // PickFilesMarkedForCompaction();
+    compaction_picker_->PickFilesMarkedForCompaction(
+        cf_name_, vstorage_, &start_level_, &output_level_, &start_level_inputs_);
     if (!start_level_inputs_.empty()) {
       is_manual_ = true;
       compaction_reason_ = CompactionReason::kFilesMarkedForCompaction;
@@ -1220,29 +1247,9 @@ void LevelCompactionBuilder::SetupInitialFiles() {
 
 bool LevelCompactionBuilder::SetupOtherL0FilesIfNeeded() {
   if (start_level_ == 0 && output_level_ != 0) {
-    // Two level 0 compaction won't run at the same time, so don't need to worry
-    // about files on level 0 being compacted.
-    assert(compaction_picker_->level0_compactions_in_progress()->empty());
-    InternalKey smallest, largest;
-    compaction_picker_->GetRange(start_level_inputs_, &smallest, &largest);
-    // Note that the next call will discard the file we placed in
-    // c->inputs_[0] earlier and replace it with an overlapping set
-    // which will include the picked file.
-    start_level_inputs_.files.clear();
-    vstorage_->GetOverlappingInputs(0, &smallest, &largest,
-                                    &start_level_inputs_.files);
-
-    // If we include more L0 files in the same compaction run it can
-    // cause the 'smallest' and 'largest' key to get extended to a
-    // larger range. So, re-invoke GetRange to get the new key range
-    compaction_picker_->GetRange(start_level_inputs_, &smallest, &largest);
-    if (compaction_picker_->IsRangeInCompaction(
-            vstorage_, &smallest, &largest, output_level_, &parent_index_)) {
-      return false;
-    }
+    return compaction_picker_->GetOverlappingL0Files(
+        vstorage_, &start_level_inputs_, output_level_, &parent_index_);
   }
-  assert(!start_level_inputs_.files.empty());
-
   return true;
 }
 

db/compaction_picker.h

@@ -175,6 +175,15 @@ class CompactionPicker {
                        const CompactionInputFiles& output_level_inputs,
                        std::vector<FileMetaData*>* grandparents);
 
+  void PickFilesMarkedForCompaction(const std::string& cf_name,
+                                    VersionStorageInfo* vstorage,
+                                    int* start_level, int* output_level,
+                                    CompactionInputFiles* start_level_inputs);
+
+  bool GetOverlappingL0Files(VersionStorageInfo* vstorage,
+                             CompactionInputFiles* start_level_inputs,
+                             int output_level, int* parent_index);
+
   // Register this compaction in the set of running compactions
   void RegisterCompaction(Compaction* c);
 

db/compaction_picker_test.cc

@@ -391,10 +391,10 @@ TEST_F(CompactionPickerTest, NeedsCompactionUniversal) {
   NewVersionStorage(1, kCompactionStyleUniversal);
   UniversalCompactionPicker universal_compaction_picker(
       ioptions_, &icmp_);
+  UpdateVersionStorageInfo();
   // must return false when there's no files.
   ASSERT_EQ(universal_compaction_picker.NeedsCompaction(vstorage_.get()),
             false);
-  UpdateVersionStorageInfo();
 
   // verify the trigger given different number of L0 files.
   for (int i = 1;
@@ -415,6 +415,7 @@ TEST_F(CompactionPickerTest, CompactionUniversalIngestBehindReservedLevel) {
   ioptions_.allow_ingest_behind = true;
   ioptions_.num_levels = 3;
   UniversalCompactionPicker universal_compaction_picker(ioptions_, &icmp_);
+  UpdateVersionStorageInfo();
   // must return false when there's no files.
   ASSERT_EQ(universal_compaction_picker.NeedsCompaction(vstorage_.get()),
             false);
@@ -448,6 +449,7 @@ TEST_F(CompactionPickerTest, CannotTrivialMoveUniversal) {
   mutable_cf_options_.compaction_options_universal.allow_trivial_move = true;
   NewVersionStorage(1, kCompactionStyleUniversal);
   UniversalCompactionPicker universal_compaction_picker(ioptions_, &icmp_);
+  UpdateVersionStorageInfo();
   // must return false when there's no files.
   ASSERT_EQ(universal_compaction_picker.NeedsCompaction(vstorage_.get()),
             false);

db/compaction_picker_universal.cc

@@ -162,7 +162,13 @@ bool UniversalCompactionPicker::IsInputFilesNonOverlapping(Compaction* c) {
 bool UniversalCompactionPicker::NeedsCompaction(
     const VersionStorageInfo* vstorage) const {
   const int kLevel0 = 0;
-  return vstorage->CompactionScore(kLevel0) >= 1;
+  if (vstorage->CompactionScore(kLevel0) >= 1) {
+    return true;
+  }
+  if (!vstorage->FilesMarkedForCompaction().empty()) {
+    return true;
+  }
+  return false;
 }
 
 void UniversalCompactionPicker::SortedRun::Dump(char* out_buf,
@@ -257,8 +263,9 @@ Compaction* UniversalCompactionPicker::PickCompaction(
       CalculateSortedRuns(*vstorage, ioptions_, mutable_cf_options);
 
   if (sorted_runs.size() == 0 ||
-      sorted_runs.size() <
-          (unsigned int)mutable_cf_options.level0_file_num_compaction_trigger) {
+      (vstorage->FilesMarkedForCompaction().empty() &&
+       sorted_runs.size() < (unsigned int)mutable_cf_options
+                                .level0_file_num_compaction_trigger)) {
     ROCKS_LOG_BUFFER(log_buffer, "[%s] Universal: nothing to do\n",
                      cf_name.c_str());
     TEST_SYNC_POINT_CALLBACK("UniversalCompactionPicker::PickCompaction:Return",
@@ -272,58 +279,73 @@ Compaction* UniversalCompactionPicker::PickCompaction(
       cf_name.c_str(), sorted_runs.size(), vstorage->LevelSummary(&tmp));
 
   // Check for size amplification first.
-  Compaction* c;
-  if ((c = PickCompactionToReduceSizeAmp(cf_name, mutable_cf_options, vstorage,
-                                         score, sorted_runs, log_buffer)) !=
-      nullptr) {
-    ROCKS_LOG_BUFFER(log_buffer, "[%s] Universal: compacting for size amp\n",
-                     cf_name.c_str());
-  } else {
-    // Size amplification is within limits. Try reducing read
-    // amplification while maintaining file size ratios.
-    unsigned int ratio =
-        mutable_cf_options.compaction_options_universal.size_ratio;
-
-    if ((c = PickCompactionToReduceSortedRuns(
-             cf_name, mutable_cf_options, vstorage, score, ratio, UINT_MAX,
-             sorted_runs, log_buffer)) != nullptr) {
-      ROCKS_LOG_BUFFER(log_buffer,
-                       "[%s] Universal: compacting for size ratio\n",
+  Compaction* c = nullptr;
+  if (sorted_runs.size() >=
+      static_cast<size_t>(
+          mutable_cf_options.level0_file_num_compaction_trigger)) {
+    if ((c = PickCompactionToReduceSizeAmp(cf_name, mutable_cf_options,
+                                           vstorage, score, sorted_runs,
+                                           log_buffer)) != nullptr) {
+      ROCKS_LOG_BUFFER(log_buffer, "[%s] Universal: compacting for size amp\n",
                        cf_name.c_str());
     } else {
-      // Size amplification and file size ratios are within configured limits.
-      // If max read amplification is exceeding configured limits, then force
-      // compaction without looking at filesize ratios and try to reduce
-      // the number of files to fewer than level0_file_num_compaction_trigger.
-      // This is guaranteed by NeedsCompaction()
-      assert(sorted_runs.size() >=
-             static_cast<size_t>(
-                 mutable_cf_options.level0_file_num_compaction_trigger));
-      // Get the total number of sorted runs that are not being compacted
-      int num_sr_not_compacted = 0;
-      for (size_t i = 0; i < sorted_runs.size(); i++) {
-        if (sorted_runs[i].being_compacted == false) {
-          num_sr_not_compacted++;
-        }
-      }
+      // Size amplification is within limits. Try reducing read
+      // amplification while maintaining file size ratios.
+      unsigned int ratio =
+          mutable_cf_options.compaction_options_universal.size_ratio;
 
-      // The number of sorted runs that are not being compacted is greater than
-      // the maximum allowed number of sorted runs
-      if (num_sr_not_compacted >
-          mutable_cf_options.level0_file_num_compaction_trigger) {
-        unsigned int num_files =
-            num_sr_not_compacted -
-            mutable_cf_options.level0_file_num_compaction_trigger + 1;
-        if ((c = PickCompactionToReduceSortedRuns(
-                 cf_name, mutable_cf_options, vstorage, score, UINT_MAX,
-                 num_files, sorted_runs, log_buffer)) != nullptr) {
-          ROCKS_LOG_BUFFER(log_buffer,
-                           "[%s] Universal: compacting for file num -- %u\n",
-                           cf_name.c_str(), num_files);
+      if ((c = PickCompactionToReduceSortedRuns(
+               cf_name, mutable_cf_options, vstorage, score, ratio, UINT_MAX,
+               sorted_runs, log_buffer)) != nullptr) {
+        ROCKS_LOG_BUFFER(log_buffer,
+                         "[%s] Universal: compacting for size ratio\n",
+                         cf_name.c_str());
+      } else {
+        // Size amplification and file size ratios are within configured limits.
+        // If max read amplification is exceeding configured limits, then force
+        // compaction without looking at filesize ratios and try to reduce
+        // the number of files to fewer than level0_file_num_compaction_trigger.
+        // This is guaranteed by NeedsCompaction()
+        assert(sorted_runs.size() >=
+               static_cast<size_t>(
+                   mutable_cf_options.level0_file_num_compaction_trigger));
+        // Get the total number of sorted runs that are not being compacted
+        int num_sr_not_compacted = 0;
+        for (size_t i = 0; i < sorted_runs.size(); i++) {
+          if (sorted_runs[i].being_compacted == false) {
+            num_sr_not_compacted++;
+          }
+        }
+
+        // The number of sorted runs that are not being compacted is greater
+        // than the maximum allowed number of sorted runs
+        if (num_sr_not_compacted >
+            mutable_cf_options.level0_file_num_compaction_trigger) {
+          unsigned int num_files =
+              num_sr_not_compacted -
+              mutable_cf_options.level0_file_num_compaction_trigger + 1;
+          if ((c = PickCompactionToReduceSortedRuns(
+                   cf_name, mutable_cf_options, vstorage, score, UINT_MAX,
+                   num_files, sorted_runs, log_buffer)) != nullptr) {
+            ROCKS_LOG_BUFFER(log_buffer,
+                             "[%s] Universal: compacting for file num -- %u\n",
+                             cf_name.c_str(), num_files);
+          }
         }
       }
     }
   }
+
+  if (c == nullptr) {
+    if ((c = PickDeleteTriggeredCompaction(cf_name, mutable_cf_options,
+                                           vstorage, score, sorted_runs,
+                                           log_buffer)) != nullptr) {
+      ROCKS_LOG_BUFFER(log_buffer,
+                       "[%s] Universal: delete triggered compaction\n",
+                       cf_name.c_str());
+    }
+  }
+
   if (c == nullptr) {
     TEST_SYNC_POINT_CALLBACK("UniversalCompactionPicker::PickCompaction:Return",
                              nullptr);
@@ -753,6 +775,125 @@ Compaction* UniversalCompactionPicker::PickCompactionToReduceSizeAmp(
       score, false /* deletion_compaction */,
       CompactionReason::kUniversalSizeAmplification);
 }
+
+// Pick files marked for compaction. Typically, files are marked by
+// CompactOnDeleteCollector due to the presence of tombstones.
+Compaction* UniversalCompactionPicker::PickDeleteTriggeredCompaction(
+    const std::string& cf_name, const MutableCFOptions& mutable_cf_options,
+    VersionStorageInfo* vstorage, double score,
+    const std::vector<SortedRun>& /*sorted_runs*/, LogBuffer* /*log_buffer*/) {
+  CompactionInputFiles start_level_inputs;
+  int output_level;
+  std::vector<CompactionInputFiles> inputs;
+
+  if (vstorage->num_levels() == 1) {
+    // This is single level universal. Since we're basically trying to reclaim
+    // space by processing files marked for compaction due to high tombstone
+    // density, let's do the same thing as compaction to reduce size amp which
+    // has the same goals.
+    bool compact = false;
+
+    start_level_inputs.level = 0;
+    start_level_inputs.files.clear();
+    output_level = 0;
+    for (FileMetaData* f : vstorage->LevelFiles(0)) {
+      if (f->marked_for_compaction) {
+        compact = true;
+      }
+      if (compact) {
+        start_level_inputs.files.push_back(f);
+      }
+    }
+    if (start_level_inputs.size() <= 1) {
+      // If only the last file in L0 is marked for compaction, ignore it
+      return nullptr;
+    }
+    inputs.push_back(start_level_inputs);
+  } else {
+    int start_level;
+
+    // For multi-level universal, the strategy is to make this look more like
+    // leveled. We pick one of the files marked for compaction and compact with
+    // overlapping files in the adjacent level.
+    PickFilesMarkedForCompaction(cf_name, vstorage, &start_level, &output_level,
+                                 &start_level_inputs);
+    if (start_level_inputs.empty()) {
+      return nullptr;
+    }
+
+    // Pick the first non-empty level after the start_level
+    for (output_level = start_level + 1; output_level < vstorage->num_levels();
+         output_level++) {
+      if (vstorage->NumLevelFiles(output_level) != 0) {
+        break;
+      }
+    }
+
+    // If all higher levels are empty, pick the highest level as output level
+    if (output_level == vstorage->num_levels()) {
+      if (start_level == 0) {
+        output_level = vstorage->num_levels() - 1;
+      } else {
+        // If start level is non-zero and all higher levels are empty, this
+        // compaction will translate into a trivial move. Since the idea is
+        // to reclaim space and trivial move doesn't help with that, we
+        // skip compaction in this case and return nullptr
+        return nullptr;
+      }
+    }
+    if (ioptions_.allow_ingest_behind &&
+        output_level == vstorage->num_levels() - 1) {
+      assert(output_level > 1);
+      output_level--;
+    }
+
+    if (output_level != 0) {
+      if (start_level == 0) {
+        if (!GetOverlappingL0Files(vstorage, &start_level_inputs, output_level,
+                                   nullptr)) {
+          return nullptr;
+        }
+      }
+
+      CompactionInputFiles output_level_inputs;
+      int parent_index = -1;
+
+      output_level_inputs.level = output_level;
+      if (!SetupOtherInputs(cf_name, mutable_cf_options, vstorage,
+                            &start_level_inputs, &output_level_inputs,
+                            &parent_index, -1)) {
+        return nullptr;
+      }
+      inputs.push_back(start_level_inputs);
+      if (!output_level_inputs.empty()) {
+        inputs.push_back(output_level_inputs);
+      }
+      if (FilesRangeOverlapWithCompaction(inputs, output_level)) {
+        return nullptr;
+      }
+    } else {
+      inputs.push_back(start_level_inputs);
+    }
+  }
+
+  uint64_t estimated_total_size = 0;
+  // Use size of the output level as estimated file size
+  for (FileMetaData* f : vstorage->LevelFiles(output_level)) {
+    estimated_total_size += f->fd.GetFileSize();
+  }
+  uint32_t path_id =
+      GetPathId(ioptions_, mutable_cf_options, estimated_total_size);
+  return new Compaction(
+      vstorage, ioptions_, mutable_cf_options, std::move(inputs), output_level,
+      MaxFileSizeForLevel(mutable_cf_options, output_level,
+                          kCompactionStyleUniversal),
+      /* max_grandparent_overlap_bytes */ LLONG_MAX, path_id,
+      GetCompressionType(ioptions_, vstorage, mutable_cf_options, output_level,
+                         1),
+      /* max_subcompactions */ 0, /* grandparents */ {}, /* is manual */ true,
+      score, false /* deletion_compaction */,
+      CompactionReason::kFilesMarkedForCompaction);
+}
 }  // namespace rocksdb
 
 #endif  // !ROCKSDB_LITE

db/compaction_picker_universal.h

@@ -73,6 +73,11 @@ class UniversalCompactionPicker : public CompactionPicker {
       VersionStorageInfo* vstorage, double score,
       const std::vector<SortedRun>& sorted_runs, LogBuffer* log_buffer);
 
+  Compaction* PickDeleteTriggeredCompaction(
+      const std::string& cf_name, const MutableCFOptions& mutable_cf_options,
+      VersionStorageInfo* vstorage, double score,
+      const std::vector<SortedRun>& sorted_runs, LogBuffer* log_buffer);
+
   // Used in universal compaction when the enabled_trivial_move
   // option is set. Checks whether there are any overlapping files
   // in the input. Returns true if the input files are non

db/comparator_db_test.cc

@@ -251,25 +251,18 @@ class TwoStrComparator : public Comparator {
 };
 }  // namespace
 
-class ComparatorDBTest
-    : public testing::Test,
-      virtual public ::testing::WithParamInterface<uint32_t> {
+class ComparatorDBTest : public testing::Test {
  private:
   std::string dbname_;
   Env* env_;
   DB* db_;
   Options last_options_;
   std::unique_ptr<const Comparator> comparator_guard;
-  uint32_t format_;
 
  public:
   ComparatorDBTest() : env_(Env::Default()), db_(nullptr) {
     comparator = BytewiseComparator();
     dbname_ = test::TmpDir() + "/comparator_db_test";
-    BlockBasedTableOptions toptions;
-    toptions.format_version = GetParam();
-    last_options_.table_factory.reset(
-        rocksdb::NewBlockBasedTableFactory(toptions));
     EXPECT_OK(DestroyDB(dbname_, last_options_));
   }
 
@@ -281,12 +274,8 @@ class ComparatorDBTest
 
   DB* GetDB() { return db_; }
 
-  void SetOwnedComparator(const Comparator* cmp, bool owner = true) {
-    if (owner) {
-      comparator_guard.reset(cmp);
-    } else {
-      comparator_guard.reset();
-    }
+  void SetOwnedComparator(const Comparator* cmp) {
+    comparator_guard.reset(cmp);
     comparator = cmp;
     last_options_.comparator = cmp;
   }
@@ -315,12 +304,7 @@ class ComparatorDBTest
   }
 };
 
-INSTANTIATE_TEST_CASE_P(FormatDef, ComparatorDBTest,
-                        testing::Values(test::kDefaultFormatVersion));
-INSTANTIATE_TEST_CASE_P(FormatLatest, ComparatorDBTest,
-                        testing::Values(test::kLatestFormatVersion));
-
-TEST_P(ComparatorDBTest, Bytewise) {
+TEST_F(ComparatorDBTest, Bytewise) {
   for (int rand_seed = 301; rand_seed < 306; rand_seed++) {
     DestroyAndReopen();
     Random rnd(rand_seed);
@@ -330,7 +314,7 @@ TEST_P(ComparatorDBTest, Bytewise) {
   }
 }
 
-TEST_P(ComparatorDBTest, SimpleSuffixReverseComparator) {
+TEST_F(ComparatorDBTest, SimpleSuffixReverseComparator) {
   SetOwnedComparator(new test::SimpleSuffixReverseComparator());
 
   for (int rnd_seed = 301; rnd_seed < 316; rnd_seed++) {
@@ -356,8 +340,8 @@ TEST_P(ComparatorDBTest, SimpleSuffixReverseComparator) {
   }
 }
 
-TEST_P(ComparatorDBTest, Uint64Comparator) {
-  SetOwnedComparator(test::Uint64Comparator(), false /* owner */);
+TEST_F(ComparatorDBTest, Uint64Comparator) {
+  SetOwnedComparator(test::Uint64Comparator());
 
   for (int rnd_seed = 301; rnd_seed < 316; rnd_seed++) {
     Options* opt = GetOptions();
@@ -380,7 +364,7 @@ TEST_P(ComparatorDBTest, Uint64Comparator) {
   }
 }
 
-TEST_P(ComparatorDBTest, DoubleComparator) {
+TEST_F(ComparatorDBTest, DoubleComparator) {
   SetOwnedComparator(new DoubleComparator());
 
   for (int rnd_seed = 301; rnd_seed < 316; rnd_seed++) {
@@ -405,7 +389,7 @@ TEST_P(ComparatorDBTest, DoubleComparator) {
   }
 }
 
-TEST_P(ComparatorDBTest, HashComparator) {
+TEST_F(ComparatorDBTest, HashComparator) {
   SetOwnedComparator(new HashComparator());
 
   for (int rnd_seed = 301; rnd_seed < 316; rnd_seed++) {
@@ -424,7 +408,7 @@ TEST_P(ComparatorDBTest, HashComparator) {
   }
 }
 
-TEST_P(ComparatorDBTest, TwoStrComparator) {
+TEST_F(ComparatorDBTest, TwoStrComparator) {
   SetOwnedComparator(new TwoStrComparator());
 
   for (int rnd_seed = 301; rnd_seed < 316; rnd_seed++) {
@@ -450,7 +434,7 @@ TEST_P(ComparatorDBTest, TwoStrComparator) {
   }
 }
 
-TEST_P(ComparatorDBTest, FindShortestSeparator) {
+TEST_F(ComparatorDBTest, FindShortestSeparator) {
   std::string s1 = "abc1xyz";
   std::string s2 = "abc3xy";
 
@@ -484,7 +468,7 @@ TEST_P(ComparatorDBTest, FindShortestSeparator) {
   ASSERT_TRUE(s1 > s2);
 }
 
-TEST_P(ComparatorDBTest, SeparatorSuccessorRandomizeTest) {
+TEST_F(ComparatorDBTest, SeparatorSuccessorRandomizeTest) {
   // Char list for boundary cases.
   std::array<unsigned char, 6> char_list{{0, 1, 2, 253, 254, 255}};
   Random rnd(301);

db/db_block_cache_test.cc

@@ -390,7 +390,10 @@ class MockCache : public LRUCache {
   static uint32_t high_pri_insert_count;
   static uint32_t low_pri_insert_count;
 
-  MockCache() : LRUCache(1 << 25, 0, false, 0.0) {}
+  MockCache()
+      : LRUCache((size_t)1 << 25 /*capacity*/, 0 /*num_shard_bits*/,
+                 false /*strict_capacity_limit*/, 0.0 /*high_pri_pool_ratio*/) {
+  }
 
   virtual Status Insert(const Slice& key, void* value, size_t charge,
                         void (*deleter)(const Slice& key, void* value),

db/db_compaction_filter_test.cc

@@ -45,6 +45,7 @@ class DBTestCompactionFilterWithCompactParam
   }
 };
 
+#ifndef ROCKSDB_VALGRIND_RUN
 INSTANTIATE_TEST_CASE_P(
     DBTestCompactionFilterWithCompactOption,
     DBTestCompactionFilterWithCompactParam,
@@ -53,6 +54,12 @@ INSTANTIATE_TEST_CASE_P(
                       DBTestBase::OptionConfig::kUniversalCompactionMultiLevel,
                       DBTestBase::OptionConfig::kLevelSubcompactions,
                       DBTestBase::OptionConfig::kUniversalSubcompactions));
+#else
+// Run fewer cases in valgrind
+INSTANTIATE_TEST_CASE_P(DBTestCompactionFilterWithCompactOption,
+                        DBTestCompactionFilterWithCompactParam,
+                        ::testing::Values(DBTestBase::OptionConfig::kDefault));
+#endif  // ROCKSDB_VALGRIND_RUN
 
 class KeepFilter : public CompactionFilter {
  public:

db/db_compaction_test.cc

@@ -251,6 +251,7 @@ const SstFileMetaData* PickFileRandomly(
 }
 }  // anonymous namespace
 
+#ifndef ROCKSDB_VALGRIND_RUN
 // All the TEST_P tests run once with sub_compactions disabled (i.e.
 // options.max_subcompactions = 1) and once with it enabled
 TEST_P(DBCompactionTestWithParam, CompactionDeletionTrigger) {
@@ -293,6 +294,7 @@ TEST_P(DBCompactionTestWithParam, CompactionDeletionTrigger) {
     ASSERT_GT(db_size[0] / 3, db_size[1]);
   }
 }
+#endif  // ROCKSDB_VALGRIND_RUN
 
 TEST_P(DBCompactionTestWithParam, CompactionsPreserveDeletes) {
   //  For each options type we test following

db/db_impl_files.cc

@@ -12,6 +12,7 @@
 #define __STDC_FORMAT_MACROS
 #endif
 #include <inttypes.h>
+#include <set>
 #include <unordered_set>
 #include "db/event_helpers.h"
 #include "db/memtable_list.h"
@@ -99,10 +100,10 @@ void DBImpl::FindObsoleteFiles(JobContext* job_context, bool force,
   if (doing_the_full_scan) {
     InfoLogPrefix info_log_prefix(!immutable_db_options_.db_log_dir.empty(),
         dbname_);
-    std::vector<std::string> paths;
+    std::set<std::string> paths;
     for (size_t path_id = 0; path_id < immutable_db_options_.db_paths.size();
          path_id++) {
-      paths.emplace_back(immutable_db_options_.db_paths[path_id].path);
+      paths.insert(immutable_db_options_.db_paths[path_id].path);
     }
 
     // Note that if cf_paths is not specified in the ColumnFamilyOptions
@@ -113,7 +114,11 @@ void DBImpl::FindObsoleteFiles(JobContext* job_context, bool force,
     for (auto cfd : *versions_->GetColumnFamilySet()) {
       for (size_t path_id = 0; path_id < cfd->ioptions()->cf_paths.size();
            path_id++) {
-        paths.emplace_back(cfd->ioptions()->cf_paths[path_id].path);
+        auto& path = cfd->ioptions()->cf_paths[path_id].path;
+
+        if (paths.find(path) == paths.end()) {
+          paths.insert(path);
+        }
       }
     }
 

db/db_impl_write.cc

@@ -211,6 +211,7 @@ Status DBImpl::WriteImpl(const WriteOptions& write_options,
   // and protects against concurrent loggers and concurrent writes
   // into memtables
 
+  TEST_SYNC_POINT("DBImpl::WriteImpl:BeforeLeaderEnters");
   last_batch_group_size_ =
       write_thread_.EnterAsBatchGroupLeader(&w, &write_group);
 
@@ -1280,7 +1281,11 @@ Status DBImpl::SwitchMemtable(ColumnFamilyData* cfd, WriteContext* context,
   // In case of pipelined write is enabled, wait for all pending memtable
   // writers.
   if (immutable_db_options_.enable_pipelined_write) {
+    // Memtable writers may call DB::Get in case max_successive_merges > 0,
+    // which may lock mutex. Unlocking mutex here to avoid deadlock.
+    mutex_.Unlock();
     write_thread_.WaitForMemTableWriters();
+    mutex_.Lock();
   }
 
   // Attempt to switch to a new memtable and trigger flush of old.

db/db_iter_stress_test.cc

@@ -173,7 +173,7 @@ struct StressTestIterator : public InternalIterator {
     }
   }
 
-  void SeekToFirst() {
+  void SeekToFirst() override {
     if (MaybeFail()) return;
     MaybeMutate();
 
@@ -181,7 +181,7 @@ struct StressTestIterator : public InternalIterator {
     iter = 0;
     SkipForward();
   }
-  void SeekToLast() {
+  void SeekToLast() override {
     if (MaybeFail()) return;
     MaybeMutate();
 
@@ -190,7 +190,7 @@ struct StressTestIterator : public InternalIterator {
     SkipBackward();
   }
 
-  void Seek(const Slice& target) {
+  void Seek(const Slice& target) override {
     if (MaybeFail()) return;
     MaybeMutate();
 
@@ -202,7 +202,7 @@ struct StressTestIterator : public InternalIterator {
     iter = (int)(it - data->entries.begin());
     SkipForward();
   }
-  void SeekForPrev(const Slice& target) {
+  void SeekForPrev(const Slice& target) override {
     if (MaybeFail()) return;
     MaybeMutate();
 
@@ -216,14 +216,14 @@ struct StressTestIterator : public InternalIterator {
     SkipBackward();
   }
 
-  void Next() {
+  void Next() override {
     assert(Valid());
     if (MaybeFail()) return;
     MaybeMutate();
     ++iter;
     SkipForward();
   }
-  void Prev() {
+  void Prev() override {
     assert(Valid());
     if (MaybeFail()) return;
     MaybeMutate();
@@ -231,11 +231,11 @@ struct StressTestIterator : public InternalIterator {
     SkipBackward();
   }
 
-  Slice key() const {
+  Slice key() const override {
     assert(Valid());
     return data->entries[iter].ikey;
   }
-  Slice value() const {
+  Slice value() const override {
     assert(Valid());
     return data->entries[iter].value;
   }
@@ -404,13 +404,14 @@ TEST_F(DBIteratorStressTest, StressTest) {
   Random64 rnd(826909345792864532ll);
 
   auto gen_key = [&](int max_key) {
+    assert(max_key > 0);
     int len = 0;
     int a = max_key;
     while (a) {
       a /= 10;
       ++len;
     }
-    std::string s = ToString(rnd.Next() % (uint64_t)max_key);
+    std::string s = ToString(rnd.Next() % static_cast<uint64_t>(max_key));
     s.insert(0, len - (int)s.size(), '0');
     return s;
   };

db/db_iter_test.cc

@@ -36,9 +36,7 @@ class TestIterator : public InternalIterator {
         valid_(false),
         sequence_number_(0),
         iter_(0),
-        cmp(comparator) {
-    data_.reserve(16);
-  }
+        cmp(comparator) {}
 
   void AddPut(std::string argkey, std::string argvalue) {
     Add(argkey, kTypeValue, argvalue);
@@ -2662,7 +2660,7 @@ TEST_F(DBIterWithMergeIterTest, InnerMergeIteratorDataRace1) {
   // MergeIterator::Prev() realized the mem table iterator is at its end
   // and before an SeekToLast() is called.
   rocksdb::SyncPoint::GetInstance()->SetCallBack(
-      "MergeIterator::Prev:BeforePrev",
+      "MergeIterator::Prev:BeforeSeekToLast",
       [&](void* /*arg*/) { internal_iter2_->Add("z", kTypeValue, "7", 12u); });
   rocksdb::SyncPoint::GetInstance()->EnableProcessing();
 
@@ -2698,7 +2696,7 @@ TEST_F(DBIterWithMergeIterTest, InnerMergeIteratorDataRace2) {
   // mem table after MergeIterator::Prev() realized the mem tableiterator is at
   // its end and before an SeekToLast() is called.
   rocksdb::SyncPoint::GetInstance()->SetCallBack(
-      "MergeIterator::Prev:BeforePrev", [&](void* /*arg*/) {
+      "MergeIterator::Prev:BeforeSeekToLast", [&](void* /*arg*/) {
         internal_iter2_->Add("z", kTypeValue, "7", 12u);
         internal_iter2_->Add("z", kTypeValue, "7", 11u);
       });
@@ -2736,7 +2734,7 @@ TEST_F(DBIterWithMergeIterTest, InnerMergeIteratorDataRace3) {
   // mem table after MergeIterator::Prev() realized the mem table iterator is at
   // its end and before an SeekToLast() is called.
   rocksdb::SyncPoint::GetInstance()->SetCallBack(
-      "MergeIterator::Prev:BeforePrev", [&](void* /*arg*/) {
+      "MergeIterator::Prev:BeforeSeekToLast", [&](void* /*arg*/) {
         internal_iter2_->Add("z", kTypeValue, "7", 16u, true);
         internal_iter2_->Add("z", kTypeValue, "7", 15u, true);
         internal_iter2_->Add("z", kTypeValue, "7", 14u, true);

db/db_iterator_test.cc

@@ -2044,43 +2044,6 @@ TEST_P(DBIteratorTest, CreationFailure) {
   delete iter;
 }
 
-TEST_P(DBIteratorTest, UpperBoundWithChangeDirection) {
-  Options options = CurrentOptions();
-  options.max_sequential_skip_in_iterations = 3;
-  DestroyAndReopen(options);
-
-  // write a bunch of kvs to the database.
-  ASSERT_OK(Put("a", "1"));
-  ASSERT_OK(Put("y", "1"));
-  ASSERT_OK(Put("y1", "1"));
-  ASSERT_OK(Put("y2", "1"));
-  ASSERT_OK(Put("y3", "1"));
-  ASSERT_OK(Put("z", "1"));
-  ASSERT_OK(Flush());
-  ASSERT_OK(Put("a", "1"));
-  ASSERT_OK(Put("z", "1"));
-  ASSERT_OK(Put("bar", "1"));
-  ASSERT_OK(Put("foo", "1"));
-
-  std::string upper_bound = "x";
-  Slice ub_slice(upper_bound);
-  ReadOptions ro;
-  ro.iterate_upper_bound = &ub_slice;
-  ro.max_skippable_internal_keys = 1000;
-
-  Iterator* iter = NewIterator(ro);
-  iter->Seek("foo");
-  ASSERT_TRUE(iter->Valid());
-  ASSERT_EQ("foo", iter->key().ToString());
-
-  iter->Prev();
-  ASSERT_TRUE(iter->Valid());
-  ASSERT_OK(iter->status());
-  ASSERT_EQ("bar", iter->key().ToString());
-
-  delete iter;
-}
-
 TEST_P(DBIteratorTest, TableFilter) {
   ASSERT_OK(Put("a", "1"));
   dbfull()->Flush(FlushOptions());
@@ -2147,47 +2110,6 @@ TEST_P(DBIteratorTest, TableFilter) {
   }
 }
 
-TEST_P(DBIteratorTest, UpperBoundWithPrevReseek) {
-  Options options = CurrentOptions();
-  options.max_sequential_skip_in_iterations = 3;
-  DestroyAndReopen(options);
-
-  // write a bunch of kvs to the database.
-  ASSERT_OK(Put("a", "1"));
-  ASSERT_OK(Put("y", "1"));
-  ASSERT_OK(Put("z", "1"));
-  ASSERT_OK(Flush());
-  ASSERT_OK(Put("a", "1"));
-  ASSERT_OK(Put("z", "1"));
-  ASSERT_OK(Put("bar", "1"));
-  ASSERT_OK(Put("foo", "1"));
-  ASSERT_OK(Put("foo", "2"));
-
-  ASSERT_OK(Put("foo", "3"));
-  ASSERT_OK(Put("foo", "4"));
-  ASSERT_OK(Put("foo", "5"));
-  const Snapshot* snapshot = db_->GetSnapshot();
-  ASSERT_OK(Put("foo", "6"));
-
-  std::string upper_bound = "x";
-  Slice ub_slice(upper_bound);
-  ReadOptions ro;
-  ro.snapshot = snapshot;
-  ro.iterate_upper_bound = &ub_slice;
-
-  Iterator* iter = NewIterator(ro);
-  iter->SeekForPrev("goo");
-  ASSERT_TRUE(iter->Valid());
-  ASSERT_EQ("foo", iter->key().ToString());
-  iter->Prev();
-
-  ASSERT_TRUE(iter->Valid());
-  ASSERT_EQ("bar", iter->key().ToString());
-
-  delete iter;
-  db_->ReleaseSnapshot(snapshot);
-}
-
 TEST_P(DBIteratorTest, SkipStatistics) {
   Options options = CurrentOptions();
   options.statistics = rocksdb::CreateDBStatistics();

db/db_options_test.cc

@@ -242,11 +242,12 @@ TEST_F(DBOptionsTest, WritableFileMaxBufferSize) {
   ASSERT_EQ(unmatch_cnt, 0);
   ASSERT_GE(match_cnt, 11);
 
-  buffer_size = 512 * 1024;
-  match_cnt = 0;
-  unmatch_cnt = 0;
   ASSERT_OK(
       dbfull()->SetDBOptions({{"writable_file_max_buffer_size", "524288"}}));
+  buffer_size = 512 * 1024;
+  match_cnt = 0;
+  unmatch_cnt = 0;  // SetDBOptions() will create a WriteableFileWriter
+
   ASSERT_EQ(buffer_size,
             dbfull()->GetDBOptions().writable_file_max_buffer_size);
   i = 0;

db/db_test.cc

@@ -2153,10 +2153,14 @@ TEST_F(DBTest, GroupCommitTest) {
   do {
     Options options = CurrentOptions();
     options.env = env_;
-    env_->log_write_slowdown_.store(100);
     options.statistics = rocksdb::CreateDBStatistics();
     Reopen(options);
 
+    rocksdb::SyncPoint::GetInstance()->LoadDependency(
+        {{"WriteThread::JoinBatchGroup:BeganWaiting",
+          "DBImpl::WriteImpl:BeforeLeaderEnters"}});
+    rocksdb::SyncPoint::GetInstance()->EnableProcessing();
+
     // Start threads
     GCThread thread[kGCNumThreads];
     for (int id = 0; id < kGCNumThreads; id++) {
@@ -2165,13 +2169,7 @@ TEST_F(DBTest, GroupCommitTest) {
       thread[id].done = false;
       env_->StartThread(GCThreadBody, &thread[id]);
     }
-
-    for (int id = 0; id < kGCNumThreads; id++) {
-      while (thread[id].done == false) {
-        env_->SleepForMicroseconds(100000);
-      }
-    }
-    env_->log_write_slowdown_.store(0);
+    env_->WaitForJoin();
 
     ASSERT_GT(TestGetTickerCount(options, WRITE_DONE_BY_OTHER), 0);
 
@@ -2563,6 +2561,7 @@ class ModelDB : public DB {
   std::string name_ = "";
 };
 
+#ifndef ROCKSDB_VALGRIND_RUN
 static std::string RandomKey(Random* rnd, int minimum = 0) {
   int len;
   do {
@@ -2719,6 +2718,7 @@ TEST_P(DBTestRandomized, Randomized) {
   if (model_snap != nullptr) model.ReleaseSnapshot(model_snap);
   if (db_snap != nullptr) db_->ReleaseSnapshot(db_snap);
 }
+#endif  // ROCKSDB_VALGRIND_RUN
 
 TEST_F(DBTest, BlockBasedTablePrefixIndexTest) {
   // create a DB with block prefix index

db/db_test_util.cc

@@ -428,17 +428,6 @@ Options DBTestBase::GetOptions(
       options.prefix_extractor.reset(NewNoopTransform());
       break;
     }
-    case kBlockBasedTableWithPartitionedIndexFormat3: {
-      table_options.format_version = 3;
-      // Format 3 changes the binary index format. Since partitioned index is a
-      // super-set of simple indexes, we are also using kTwoLevelIndexSearch to
-      // test this format.
-      table_options.index_type = BlockBasedTableOptions::kTwoLevelIndexSearch;
-      // The top-level index in partition filters are also affected by format 3.
-      table_options.filter_policy.reset(NewBloomFilterPolicy(10, false));
-      table_options.partition_filters = true;
-      break;
-    }
     case kBlockBasedTableWithIndexRestartInterval: {
       table_options.index_block_restart_interval = 8;
       break;

db/db_test_util.h

@@ -694,20 +694,13 @@ class DBTestBase : public testing::Test {
     kConcurrentSkipList = 29,
     kPipelinedWrite = 30,
     kConcurrentWALWrites = 31,
-    kDirectIO,
-    kLevelSubcompactions,
-    kBlockBasedTableWithIndexRestartInterval,
-    kBlockBasedTableWithPartitionedIndex,
-    kBlockBasedTableWithPartitionedIndexFormat3,
-    kPartitionedFilterWithNewTableReaderForCompactions,
-
-    // This must be the last line
-    kEnd,
-
-    // TODO: This option although been there for a while was disable due to a
-    // mistake. Enabling it makes somem tests to fail. We should enable it and
-    // fix the unit tests.
-    kUniversalSubcompactions,
+    kEnd = 32,
+    kDirectIO = 33,
+    kLevelSubcompactions = 34,
+    kUniversalSubcompactions = 35,
+    kBlockBasedTableWithIndexRestartInterval = 36,
+    kBlockBasedTableWithPartitionedIndex = 37,
+    kPartitionedFilterWithNewTableReaderForCompactions = 38,
   };
 
  public:

db/db_universal_compaction_test.cc

@@ -10,6 +10,7 @@
 #include "db/db_test_util.h"
 #include "port/stack_trace.h"
 #if !defined(ROCKSDB_LITE)
+#include "rocksdb/utilities/table_properties_collectors.h"
 #include "util/sync_point.h"
 
 namespace rocksdb {
@@ -40,6 +41,12 @@ class DBTestUniversalCompaction : public DBTestUniversalCompactionBase {
       DBTestUniversalCompactionBase("/db_universal_compaction_test") {}
 };
 
+class DBTestUniversalDeleteTrigCompaction : public DBTestBase {
+ public:
+  DBTestUniversalDeleteTrigCompaction()
+      : DBTestBase("/db_universal_compaction_test") {}
+};
+
 namespace {
 void VerifyCompactionResult(
     const ColumnFamilyMetaData& cf_meta,
@@ -655,7 +662,7 @@ TEST_P(DBTestUniversalCompaction, UniversalCompactionTargetLevel) {
   ASSERT_EQ("0,0,0,0,1", FilesPerLevel(0));
 }
 
-
+#ifndef ROCKSDB_VALGRIND_RUN
 class DBTestUniversalCompactionMultiLevels
     : public DBTestUniversalCompactionBase {
  public:
@@ -691,6 +698,7 @@ TEST_P(DBTestUniversalCompactionMultiLevels, UniversalCompactionMultiLevels) {
     ASSERT_EQ(Get(1, Key(i % num_keys)), Key(i));
   }
 }
+
 // Tests universal compaction with trivial move enabled
 TEST_P(DBTestUniversalCompactionMultiLevels, UniversalCompactionTrivialMove) {
   int32_t trivial_move = 0;
@@ -933,6 +941,7 @@ INSTANTIATE_TEST_CASE_P(DBTestUniversalCompactionParallel,
                         DBTestUniversalCompactionParallel,
                         ::testing::Combine(::testing::Values(1, 10),
                                            ::testing::Values(false)));
+#endif  // ROCKSDB_VALGRIND_RUN
 
 TEST_P(DBTestUniversalCompaction, UniversalCompactionOptions) {
   Options options = CurrentOptions();
@@ -1148,6 +1157,7 @@ TEST_P(DBTestUniversalCompaction, UniversalCompactionCompressRatio2) {
   ASSERT_LT(TotalSize(), 120000U * 12 * 0.8 + 120000 * 2);
 }
 
+#ifndef ROCKSDB_VALGRIND_RUN
 // Test that checks trivial move in universal compaction
 TEST_P(DBTestUniversalCompaction, UniversalCompactionTrivialMoveTest1) {
   int32_t trivial_move = 0;
@@ -1240,6 +1250,7 @@ TEST_P(DBTestUniversalCompaction, UniversalCompactionTrivialMoveTest2) {
 
   rocksdb::SyncPoint::GetInstance()->DisableProcessing();
 }
+#endif  // ROCKSDB_VALGRIND_RUN
 
 TEST_P(DBTestUniversalCompaction, UniversalCompactionFourPaths) {
   Options options = CurrentOptions();
@@ -1845,6 +1856,241 @@ INSTANTIATE_TEST_CASE_P(DBTestUniversalManualCompactionOutputPathId,
                         ::testing::Combine(::testing::Values(1, 8),
                                            ::testing::Bool()));
 
+TEST_F(DBTestUniversalDeleteTrigCompaction, BasicL0toL1) {
+  const int kNumKeys = 3000;
+  const int kWindowSize = 100;
+  const int kNumDelsTrigger = 90;
+
+  Options opts = CurrentOptions();
+  opts.table_properties_collector_factories.emplace_back(
+      NewCompactOnDeletionCollectorFactory(kWindowSize, kNumDelsTrigger));
+  opts.compaction_style = kCompactionStyleUniversal;
+  opts.level0_file_num_compaction_trigger = 2;
+  opts.compression = kNoCompression;
+  opts.compaction_options_universal.size_ratio = 10;
+  opts.compaction_options_universal.min_merge_width = 2;
+  opts.compaction_options_universal.max_size_amplification_percent = 200;
+  Reopen(opts);
+
+  // add an L1 file to prevent tombstones from dropping due to obsolescence
+  // during flush
+  int i;
+  for (i = 0; i < 2000; ++i) {
+    Put(Key(i), "val");
+  }
+  Flush();
+  //  MoveFilesToLevel(6);
+  dbfull()->CompactRange(CompactRangeOptions(), nullptr, nullptr);
+
+  for (i = 1999; i < kNumKeys; ++i) {
+    if (i >= kNumKeys - kWindowSize &&
+        i < kNumKeys - kWindowSize + kNumDelsTrigger) {
+      Delete(Key(i));
+    } else {
+      Put(Key(i), "val");
+    }
+  }
+  Flush();
+
+  dbfull()->TEST_WaitForCompact();
+  ASSERT_EQ(0, NumTableFilesAtLevel(0));
+  ASSERT_GT(NumTableFilesAtLevel(6), 0);
+}
+
+TEST_F(DBTestUniversalDeleteTrigCompaction, SingleLevel) {
+  const int kNumKeys = 3000;
+  const int kWindowSize = 100;
+  const int kNumDelsTrigger = 90;
+
+  Options opts = CurrentOptions();
+  opts.table_properties_collector_factories.emplace_back(
+      NewCompactOnDeletionCollectorFactory(kWindowSize, kNumDelsTrigger));
+  opts.compaction_style = kCompactionStyleUniversal;
+  opts.level0_file_num_compaction_trigger = 2;
+  opts.compression = kNoCompression;
+  opts.num_levels = 1;
+  opts.compaction_options_universal.size_ratio = 10;
+  opts.compaction_options_universal.min_merge_width = 2;
+  opts.compaction_options_universal.max_size_amplification_percent = 200;
+  Reopen(opts);
+
+  // add an L1 file to prevent tombstones from dropping due to obsolescence
+  // during flush
+  int i;
+  for (i = 0; i < 2000; ++i) {
+    Put(Key(i), "val");
+  }
+  Flush();
+
+  for (i = 1999; i < kNumKeys; ++i) {
+    if (i >= kNumKeys - kWindowSize &&
+        i < kNumKeys - kWindowSize + kNumDelsTrigger) {
+      Delete(Key(i));
+    } else {
+      Put(Key(i), "val");
+    }
+  }
+  Flush();
+
+  dbfull()->TEST_WaitForCompact();
+  ASSERT_EQ(1, NumTableFilesAtLevel(0));
+}
+
+TEST_F(DBTestUniversalDeleteTrigCompaction, MultipleLevels) {
+  const int kWindowSize = 100;
+  const int kNumDelsTrigger = 90;
+
+  Options opts = CurrentOptions();
+  opts.table_properties_collector_factories.emplace_back(
+      NewCompactOnDeletionCollectorFactory(kWindowSize, kNumDelsTrigger));
+  opts.compaction_style = kCompactionStyleUniversal;
+  opts.level0_file_num_compaction_trigger = 4;
+  opts.compression = kNoCompression;
+  opts.compaction_options_universal.size_ratio = 10;
+  opts.compaction_options_universal.min_merge_width = 2;
+  opts.compaction_options_universal.max_size_amplification_percent = 200;
+  Reopen(opts);
+
+  // add an L1 file to prevent tombstones from dropping due to obsolescence
+  // during flush
+  int i;
+  for (i = 0; i < 500; ++i) {
+    Put(Key(i), "val");
+  }
+  Flush();
+  for (i = 500; i < 1000; ++i) {
+    Put(Key(i), "val");
+  }
+  Flush();
+  for (i = 1000; i < 1500; ++i) {
+    Put(Key(i), "val");
+  }
+  Flush();
+  for (i = 1500; i < 2000; ++i) {
+    Put(Key(i), "val");
+  }
+  Flush();
+
+  dbfull()->TEST_WaitForCompact();
+  ASSERT_EQ(0, NumTableFilesAtLevel(0));
+  ASSERT_GT(NumTableFilesAtLevel(6), 0);
+
+  for (i = 1999; i < 2333; ++i) {
+    Put(Key(i), "val");
+  }
+  Flush();
+  for (i = 2333; i < 2666; ++i) {
+    Put(Key(i), "val");
+  }
+  Flush();
+  for (i = 2666; i < 2999; ++i) {
+    Put(Key(i), "val");
+  }
+  Flush();
+
+  dbfull()->TEST_WaitForCompact();
+  ASSERT_EQ(0, NumTableFilesAtLevel(0));
+  ASSERT_GT(NumTableFilesAtLevel(6), 0);
+  ASSERT_GT(NumTableFilesAtLevel(5), 0);
+
+  for (i = 1900; i < 2100; ++i) {
+    Delete(Key(i));
+  }
+  Flush();
+
+  dbfull()->TEST_WaitForCompact();
+  ASSERT_EQ(0, NumTableFilesAtLevel(0));
+  ASSERT_EQ(0, NumTableFilesAtLevel(1));
+  ASSERT_EQ(0, NumTableFilesAtLevel(2));
+  ASSERT_EQ(0, NumTableFilesAtLevel(3));
+  ASSERT_EQ(0, NumTableFilesAtLevel(4));
+  ASSERT_EQ(0, NumTableFilesAtLevel(5));
+  ASSERT_GT(NumTableFilesAtLevel(6), 0);
+}
+
+TEST_F(DBTestUniversalDeleteTrigCompaction, OverlappingL0) {
+  const int kWindowSize = 100;
+  const int kNumDelsTrigger = 90;
+
+  Options opts = CurrentOptions();
+  opts.table_properties_collector_factories.emplace_back(
+      NewCompactOnDeletionCollectorFactory(kWindowSize, kNumDelsTrigger));
+  opts.compaction_style = kCompactionStyleUniversal;
+  opts.level0_file_num_compaction_trigger = 5;
+  opts.compression = kNoCompression;
+  opts.compaction_options_universal.size_ratio = 10;
+  opts.compaction_options_universal.min_merge_width = 2;
+  opts.compaction_options_universal.max_size_amplification_percent = 200;
+  Reopen(opts);
+
+  // add an L1 file to prevent tombstones from dropping due to obsolescence
+  // during flush
+  int i;
+  for (i = 0; i < 2000; ++i) {
+    Put(Key(i), "val");
+  }
+  Flush();
+  for (i = 2000; i < 3000; ++i) {
+    Put(Key(i), "val");
+  }
+  Flush();
+  for (i = 3500; i < 4000; ++i) {
+    Put(Key(i), "val");
+  }
+  Flush();
+  for (i = 2900; i < 3100; ++i) {
+    Delete(Key(i));
+  }
+  Flush();
+
+  dbfull()->TEST_WaitForCompact();
+  ASSERT_EQ(2, NumTableFilesAtLevel(0));
+  ASSERT_GT(NumTableFilesAtLevel(6), 0);
+}
+
+TEST_F(DBTestUniversalDeleteTrigCompaction, IngestBehind) {
+  const int kNumKeys = 3000;
+  const int kWindowSize = 100;
+  const int kNumDelsTrigger = 90;
+
+  Options opts = CurrentOptions();
+  opts.table_properties_collector_factories.emplace_back(
+      NewCompactOnDeletionCollectorFactory(kWindowSize, kNumDelsTrigger));
+  opts.compaction_style = kCompactionStyleUniversal;
+  opts.level0_file_num_compaction_trigger = 2;
+  opts.compression = kNoCompression;
+  opts.allow_ingest_behind = true;
+  opts.compaction_options_universal.size_ratio = 10;
+  opts.compaction_options_universal.min_merge_width = 2;
+  opts.compaction_options_universal.max_size_amplification_percent = 200;
+  Reopen(opts);
+
+  // add an L1 file to prevent tombstones from dropping due to obsolescence
+  // during flush
+  int i;
+  for (i = 0; i < 2000; ++i) {
+    Put(Key(i), "val");
+  }
+  Flush();
+  //  MoveFilesToLevel(6);
+  dbfull()->CompactRange(CompactRangeOptions(), nullptr, nullptr);
+
+  for (i = 1999; i < kNumKeys; ++i) {
+    if (i >= kNumKeys - kWindowSize &&
+        i < kNumKeys - kWindowSize + kNumDelsTrigger) {
+      Delete(Key(i));
+    } else {
+      Put(Key(i), "val");
+    }
+  }
+  Flush();
+
+  dbfull()->TEST_WaitForCompact();
+  ASSERT_EQ(0, NumTableFilesAtLevel(0));
+  ASSERT_EQ(0, NumTableFilesAtLevel(6));
+  ASSERT_GT(NumTableFilesAtLevel(5), 0);
+}
+
 }  // namespace rocksdb
 
 #endif  // !defined(ROCKSDB_LITE)

db/internal_stats.cc

@@ -788,8 +788,8 @@ bool InternalStats::HandleEstimateTableReadersMem(uint64_t* value,
 }
 
 bool InternalStats::HandleEstimateLiveDataSize(uint64_t* value, DBImpl* /*db*/,
-                                               Version* /*version*/) {
-  const auto* vstorage = cfd_->current()->storage_info();
+                                               Version* version) {
+  const auto* vstorage = version->storage_info();
   *value = vstorage->EstimateLiveDataSize();
   return true;
 }

db/range_del_aggregator.cc

@@ -195,6 +195,10 @@ Status RangeDelAggregator::AddTombstones(
   input->SeekToFirst();
   bool first_iter = true;
   while (input->Valid()) {
+    // The tombstone map holds slices into the iterator's memory. This assert
+    // ensures pinning the iterator also pins the keys/values.
+    assert(input->IsKeyPinned() && input->IsValuePinned());
+
     if (first_iter) {
       if (rep_ == nullptr) {
         InitRep({upper_bound_});

db/version_edit.cc

@@ -20,7 +20,7 @@ namespace rocksdb {
 
 // Tag numbers for serialized VersionEdit.  These numbers are written to
 // disk and should not be changed.
-enum Tag {
+enum Tag : uint32_t {
   kComparator = 1,
   kLogNumber = 2,
   kNextFileNumber = 3,
@@ -42,7 +42,7 @@ enum Tag {
   kMaxColumnFamily = 203,
 };
 
-enum CustomTag {
+enum CustomTag : uint32_t {
   kTerminate = 1,  // The end of customized fields
   kNeedCompaction = 2,
   // Since Manifest is not entirely currently forward-compatible, and the only

db/write_batch.cc

@@ -39,6 +39,7 @@
 #include <stdexcept>
 #include <type_traits>
 #include <vector>
+#include <csignal>
 
 #include "db/column_family.h"
 #include "db/db_impl.h"
@@ -513,6 +514,7 @@ Status WriteBatch::Iterate(Handler* handler) const {
         handler->MarkBeginPrepare();
         empty_batch = false;
         if (handler->WriteAfterCommit()) {
+          std::raise(SIGINT);
           s = Status::NotSupported(
               "WritePrepared txn tag when write_after_commit_ is enabled (in "
               "default WriteCommitted mode). If it is not due to corruption, "

db/write_thread.cc

@@ -318,6 +318,7 @@ void WriteThread::JoinBatchGroup(Writer* w) {
      * 3.2) an existing memtable writer group leader tell us to finish memtable
      *      writes in parallel.
      */
+    TEST_SYNC_POINT_CALLBACK("WriteThread::JoinBatchGroup:BeganWaiting", w);
     AwaitState(w, STATE_GROUP_LEADER | STATE_MEMTABLE_WRITER_LEADER |
                       STATE_PARALLEL_MEMTABLE_WRITER | STATE_COMPLETED,
                &jbg_ctx);

env/env_posix.cc

@@ -241,9 +241,9 @@ class PosixEnv : public Env {
                                                   size, options));
         } else {
           s = IOError("while mmap file for read", fname, errno);
+          close(fd);
         }
       }
-      close(fd);
     } else {
       if (options.use_direct_reads && !options.use_mmap_reads) {
 #ifdef OS_MACOSX

env/env_test.cc

@@ -232,10 +232,10 @@ TEST_F(EnvPosixTest, MemoryMappedFileBuffer) {
 
   ASSERT_OK(status);
   ASSERT_NE(nullptr, mmap_buffer.get());
-  ASSERT_NE(nullptr, mmap_buffer->base);
-  ASSERT_EQ(kFileBytes, mmap_buffer->length);
-  std::string actual_data(static_cast<char*>(mmap_buffer->base),
-                          mmap_buffer->length);
+  ASSERT_NE(nullptr, mmap_buffer->GetBase());
+  ASSERT_EQ(kFileBytes, mmap_buffer->GetLen());
+  std::string actual_data(reinterpret_cast<const char*>(mmap_buffer->GetBase()),
+                          mmap_buffer->GetLen());
   ASSERT_EQ(expected_data, actual_data);
 }
 
@@ -1437,10 +1437,8 @@ TEST_P(EnvPosixTestWithParam, PosixRandomRWFile) {
 
   std::unique_ptr<RandomRWFile> file;
 
-#ifdef OS_LINUX
   // Cannot open non-existing file.
   ASSERT_NOK(env_->NewRandomRWFile(path, &file, EnvOptions()));
-#endif
 
   // Create the file using WriteableFile
   {

env/io_posix.cc

@@ -456,6 +456,7 @@ PosixMmapReadableFile::~PosixMmapReadableFile() {
     fprintf(stdout, "failed to munmap %p length %" ROCKSDB_PRIszt " \n",
             mmapped_region_, length_);
   }
+  close(fd_);
 }
 
 Status PosixMmapReadableFile::Read(uint64_t offset, size_t n, Slice* result,
@@ -1054,7 +1055,7 @@ Status PosixRandomRWFile::Close() {
 
 PosixMemoryMappedFileBuffer::~PosixMemoryMappedFileBuffer() {
   // TODO should have error handling though not much we can do...
-  munmap(this->base, length);
+  munmap(this->base_, length_);
 }
 
 /*

examples/rocksdb_option_file_example.ini

@@ -138,7 +138,6 @@
   block_restart_interval=16
   cache_index_and_filter_blocks=false
   pin_l0_filter_and_index_blocks_in_cache=false
-  pin_top_level_index_and_filter=false
   index_type=kBinarySearch
   hash_index_allow_collision=true
   flush_block_policy_factory=FlushBlockBySizePolicyFactory

include/rocksdb/advanced_options.h

@@ -87,6 +87,14 @@ struct CompactionOptionsFIFO {
 
 // Compression options for different compression algorithms like Zlib
 struct CompressionOptions {
+  // RocksDB's generic default compression level. Internally it'll be translated
+  // to the default compression level specific to the library being used (see
+  // comment above `ColumnFamilyOptions::compression`).
+  //
+  // The default value is the max 16-bit int as it'll be written out in OPTIONS
+  // file, which should be portable.
+  const static int kDefaultCompressionLevel = 32767;
+
   int window_bits;
   int level;
   int strategy;
@@ -120,7 +128,7 @@ struct CompressionOptions {
 
   CompressionOptions()
       : window_bits(-14),
-        level(-1),
+        level(kDefaultCompressionLevel),
         strategy(0),
         max_dict_bytes(0),
         zstd_max_train_bytes(0) {}

include/rocksdb/c.h

@@ -145,6 +145,10 @@ extern ROCKSDB_LIBRARY_API rocksdb_backup_engine_t* rocksdb_backup_engine_open(
 extern ROCKSDB_LIBRARY_API void rocksdb_backup_engine_create_new_backup(
     rocksdb_backup_engine_t* be, rocksdb_t* db, char** errptr);
 
+extern ROCKSDB_LIBRARY_API void rocksdb_backup_engine_create_new_backup_flush(
+    rocksdb_backup_engine_t* be, rocksdb_t* db, unsigned char flush_before_backup,
+    char** errptr);
+
 extern ROCKSDB_LIBRARY_API void rocksdb_backup_engine_purge_old_backups(
     rocksdb_backup_engine_t* be, uint32_t num_backups_to_keep, char** errptr);
 
@@ -155,6 +159,10 @@ extern ROCKSDB_LIBRARY_API void rocksdb_restore_options_destroy(
 extern ROCKSDB_LIBRARY_API void rocksdb_restore_options_set_keep_log_files(
     rocksdb_restore_options_t* opt, int v);
 
+extern ROCKSDB_LIBRARY_API void
+rocksdb_backup_engine_verify_backup(rocksdb_backup_engine_t* be,
+    uint32_t backup_id, char** errptr);
+
 extern ROCKSDB_LIBRARY_API void
 rocksdb_backup_engine_restore_db_from_latest_backup(
     rocksdb_backup_engine_t* be, const char* db_dir, const char* wal_dir,
@@ -692,9 +700,6 @@ rocksdb_block_based_options_set_cache_index_and_filter_blocks_with_high_priority
 extern ROCKSDB_LIBRARY_API void
 rocksdb_block_based_options_set_pin_l0_filter_and_index_blocks_in_cache(
     rocksdb_block_based_table_options_t*, unsigned char);
-extern ROCKSDB_LIBRARY_API void
-rocksdb_block_based_options_set_pin_top_level_index_and_filter(
-    rocksdb_block_based_table_options_t*, unsigned char);
 extern ROCKSDB_LIBRARY_API void rocksdb_options_set_block_based_table_factory(
     rocksdb_options_t* opt, rocksdb_block_based_table_options_t* table_options);
 

include/rocksdb/cache.h

@@ -47,6 +47,15 @@ struct LRUCacheOptions {
   bool strict_capacity_limit = false;
 
   // Percentage of cache reserved for high priority entries.
+  // If greater than zero, the LRU list will be split into a high-pri
+  // list and a low-pri list. High-pri entries will be insert to the
+  // tail of high-pri list, while low-pri entries will be first inserted to
+  // the low-pri list (the midpoint). This is refered to as
+  // midpoint insertion strategy to make entries never get hit in cache
+  // age out faster.
+  //
+  // See also
+  // BlockBasedTableOptions::cache_index_and_filter_blocks_with_high_priority.
   double high_pri_pool_ratio = 0.0;
 
   LRUCacheOptions() {}

include/rocksdb/env.h

@@ -42,7 +42,7 @@ class SequentialFile;
 class Slice;
 class WritableFile;
 class RandomRWFile;
-struct MemoryMappedFileBuffer;
+class MemoryMappedFileBuffer;
 class Directory;
 struct DBOptions;
 struct ImmutableDBOptions;
@@ -822,13 +822,24 @@ class RandomRWFile {
 
 // MemoryMappedFileBuffer object represents a memory-mapped file's raw buffer.
 // Subclasses should release the mapping upon destruction.
-struct MemoryMappedFileBuffer {
+class MemoryMappedFileBuffer {
+public:
   MemoryMappedFileBuffer(void* _base, size_t _length)
-      : base(_base), length(_length) {}
+      : base_(_base), length_(_length) {}
+
   virtual ~MemoryMappedFileBuffer() = 0;
 
-  void* const base;
-  const size_t length;
+  // We do not want to unmap this twice. We can make this class
+  // movable if desired, however, since
+  MemoryMappedFileBuffer(const MemoryMappedFileBuffer&) = delete;
+  MemoryMappedFileBuffer& operator=(const MemoryMappedFileBuffer&) = delete;
+
+  void*       GetBase() const { return base_;   }
+  size_t      GetLen() const  { return length_; }
+
+protected:
+  void*        base_;
+  const size_t length_;
 };
 
 // Directory object represents collection of files and implements

include/rocksdb/options.h

@@ -198,11 +198,21 @@ struct ColumnFamilyOptions : public AdvancedColumnFamilyOptions {
   // Typical speeds of kSnappyCompression on an Intel(R) Core(TM)2 2.4GHz:
   //    ~200-500MB/s compression
   //    ~400-800MB/s decompression
+  //
   // Note that these speeds are significantly faster than most
   // persistent storage speeds, and therefore it is typically never
   // worth switching to kNoCompression.  Even if the input data is
   // incompressible, the kSnappyCompression implementation will
   // efficiently detect that and will switch to uncompressed mode.
+  //
+  // If you do not set `compression_opts.level`, or set it to
+  // `CompressionOptions::kDefaultCompressionLevel`, we will attempt to pick the
+  // default corresponding to `compression` as follows:
+  //
+  // - kZSTD: 3
+  // - kZlibCompression: Z_DEFAULT_COMPRESSION (currently -1)
+  // - kLZ4HCCompression: 0
+  // - For all others, we do not specify a compression level
   CompressionType compression;
 
   // Compression algorithm that will be used for the bottommost level that
@@ -271,7 +281,7 @@ struct ColumnFamilyOptions : public AdvancedColumnFamilyOptions {
   // later in the vector.
   // Note that, if a path is supplied to multiple column
   // families, it would have files and total size from all
-  // the column families combined. User should privision for the
+  // the column families combined. User should provision for the
   // total size(from all the column families) in such cases.
   //
   // If left empty, db_paths will be used.
@@ -558,8 +568,9 @@ struct DBOptions {
 
   // manifest file is rolled over on reaching this limit.
   // The older manifest file be deleted.
-  // The default value is MAX_INT so that roll-over does not take place.
-  uint64_t max_manifest_file_size = std::numeric_limits<uint64_t>::max();
+  // The default value is 1GB so that the manifest file can grow, but not
+  // reach the limit of storage capacity.
+  uint64_t max_manifest_file_size = 1024 * 1024 * 1024;
 
   // Number of shards used for table cache.
   int table_cache_numshardbits = 6;

include/rocksdb/slice.h

@@ -43,7 +43,9 @@ class Slice {
 
   // Create a slice that refers to s[0,strlen(s)-1]
   /* implicit */
-  Slice(const char* s) : data_(s), size_(strlen(s)) { }
+  Slice(const char* s) : data_(s) {
+    size_ = (s == nullptr) ? 0 : strlen(s);
+  }
 
   // Create a single slice from SliceParts using buf as storage.
   // buf must exist as long as the returned Slice exists.

include/rocksdb/sst_file_manager.h

@@ -98,13 +98,12 @@ class SstFileManager {
 //    DeleteScheduler immediately
 // @param bytes_max_delete_chunk: if a single file is larger than delete chunk,
 //    ftruncate the file by this size each time, rather than dropping the whole
-//    file. 0 means to always delete the whole file. NOTE this options may not
-//    work well with checkpoints, which relies on file system hard links.
+//    file. 0 means to always delete the whole file.
 extern SstFileManager* NewSstFileManager(
     Env* env, std::shared_ptr<Logger> info_log = nullptr,
     std::string trash_dir = "", int64_t rate_bytes_per_sec = 0,
     bool delete_existing_trash = true, Status* status = nullptr,
     double max_trash_db_ratio = 0.25,
-    uint64_t bytes_max_delete_chunk = 0);
+    uint64_t bytes_max_delete_chunk = 64 * 1024 * 1024);
 
 }  // namespace rocksdb

include/rocksdb/statistics.h

@@ -619,6 +619,8 @@ struct HistogramData {
   // zero-initialize new members since old Statistics::histogramData()
   // implementations won't write them.
   double max = 0.0;
+  uint64_t count = 0;
+  uint64_t sum = 0;
 };
 
 enum StatsLevel {

include/rocksdb/table.h

@@ -77,13 +77,6 @@ struct BlockBasedTableOptions {
   // evicted from cache when the table reader is freed.
   bool pin_l0_filter_and_index_blocks_in_cache = false;
 
-  // If cache_index_and_filter_blocks is true and the below is true, then
-  // the top-level index of partitioned filter and index blocks are stored in
-  // the cache, but a reference is held in the "table reader" object so the
-  // blocks are pinned and only evicted from cache when the table reader is
-  // freed. This is not limited to l0 in LSM tree.
-  bool pin_top_level_index_and_filter = true;
-
   // The index type that will be used for this table.
   enum IndexType : char {
     // A space efficient index block that is optimized for

include/rocksdb/utilities/transaction_db.h

@@ -137,15 +137,6 @@ struct TransactionOptions {
 
   // The maximum number of bytes used for the write batch. 0 means no limit.
   size_t max_write_batch_size = 0;
-
-  // Skip Concurrency Control. This could be as an optimization if the
-  // application knows that the transaction would not have any conflict with
-  // concurrent transactions. It could also be used during recovery if (i)
-  // application guarantees no conflict between prepared transactions in the WAL
-  // (ii) application guarantees that recovered transactions will be rolled
-  // back/commit before new transactions start.
-  // Default: false
-  bool skip_concurrency_control = false;
 };
 
 // The per-write optimizations that do not involve transactions. TransactionDB
@@ -178,15 +169,12 @@ struct DeadlockInfo {
 struct DeadlockPath {
   std::vector<DeadlockInfo> path;
   bool limit_exceeded;
-  int64_t deadlock_time;
 
-  explicit DeadlockPath(
-      std::vector<DeadlockInfo> path_entry, const int64_t& dl_time)
-      : path(path_entry), limit_exceeded(false), deadlock_time(dl_time) {}
+  explicit DeadlockPath(std::vector<DeadlockInfo> path_entry)
+      : path(path_entry), limit_exceeded(false) {}
 
   // empty path, limit exceeded constructor and default constructor
-  explicit DeadlockPath(const int64_t& dl_time = 0, bool limit = false)
-      : path(0), limit_exceeded(limit), deadlock_time(dl_time) {}
+  explicit DeadlockPath(bool limit = false) : path(0), limit_exceeded(limit) {}
 
   bool empty() { return path.empty() && !limit_exceeded; }
 };

include/rocksdb/version.h

@@ -6,7 +6,7 @@
 
 #define ROCKSDB_MAJOR 5
 #define ROCKSDB_MINOR 14
-#define ROCKSDB_PATCH 4
+#define ROCKSDB_PATCH 0
 
 // Do not use these. We made the mistake of declaring macros starting with
 // double underscore. Now we have to live with our choice. We'll deprecate these

java/rocksjni/options.cc

@@ -3230,7 +3230,7 @@ Java_org_rocksdb_ColumnFamilyOptions_setCompactionFilterFactoryHandle(
     JNIEnv* /*env*/, jobject /*jobj*/, jlong jopt_handle,
     jlong jcompactionfilterfactory_handle) {
   auto* cff_factory =
-      reinterpret_cast<std::shared_ptr<rocksdb::CompactionFilterFactoryJniCallback>*>(
+      reinterpret_cast<std::shared_ptr<rocksdb::CompactionFilterFactory>*>(
           jcompactionfilterfactory_handle);
   reinterpret_cast<rocksdb::ColumnFamilyOptions*>(jopt_handle)
       ->compaction_filter_factory = *cff_factory;

java/rocksjni/portal.h

@@ -4292,12 +4292,25 @@ class JniUtil {
      * @param bytes The bytes to copy
      *
      * @return the Java byte[] or nullptr if an exception occurs
-     * 
-     * @throws RocksDBException thrown 
-     *   if memory size to copy exceeds general java specific array size limitation.
      */
     static jbyteArray copyBytes(JNIEnv* env, std::string bytes) {
-      return createJavaByteArrayWithSizeCheck(env, bytes.c_str(), bytes.size());
+      const jsize jlen = static_cast<jsize>(bytes.size());
+
+      jbyteArray jbytes = env->NewByteArray(jlen);
+      if(jbytes == nullptr) {
+        // exception thrown: OutOfMemoryError
+        return nullptr;
+      }
+
+      env->SetByteArrayRegion(jbytes, 0, jlen,
+        const_cast<jbyte*>(reinterpret_cast<const jbyte*>(bytes.c_str())));
+      if(env->ExceptionCheck()) {
+        // exception thrown: ArrayIndexOutOfBoundsException
+        env->DeleteLocalRef(jbytes);
+        return nullptr;
+      }
+
+      return jbytes;
     }
 
     /**
@@ -4460,47 +4473,6 @@ class JniUtil {
 
       return jbyte_strings;
     }
-    
-    /**
-      * Copies bytes to a new jByteArray with the check of java array size limitation.
-      *
-      * @param bytes pointer to memory to copy to a new jByteArray
-      * @param size number of bytes to copy
-      *
-      * @return the Java byte[] or nullptr if an exception occurs
-      * 
-      * @throws RocksDBException thrown 
-      *   if memory size to copy exceeds general java array size limitation to avoid overflow.
-      */
-    static jbyteArray createJavaByteArrayWithSizeCheck(JNIEnv* env, const char* bytes, const size_t size) {
-      // Limitation for java array size is vm specific
-      // In general it cannot exceed Integer.MAX_VALUE (2^31 - 1)
-      // Current HotSpot VM limitation for array size is Integer.MAX_VALUE - 5 (2^31 - 1 - 5)
-      // It means that the next call to env->NewByteArray can still end with 
-      // OutOfMemoryError("Requested array size exceeds VM limit") coming from VM
-      static const size_t MAX_JARRAY_SIZE = (static_cast<size_t>(1)) << 31;
-      if(size > MAX_JARRAY_SIZE) {
-        rocksdb::RocksDBExceptionJni::ThrowNew(env, "Requested array size exceeds VM limit");
-        return nullptr;
-      }
-      
-      const jsize jlen = static_cast<jsize>(size);
-      jbyteArray jbytes = env->NewByteArray(jlen);
-      if(jbytes == nullptr) {
-        // exception thrown: OutOfMemoryError	
-        return nullptr;
-      }
-      
-      env->SetByteArrayRegion(jbytes, 0, jlen,
-        const_cast<jbyte*>(reinterpret_cast<const jbyte*>(bytes)));
-      if(env->ExceptionCheck()) {
-        // exception thrown: ArrayIndexOutOfBoundsException
-        env->DeleteLocalRef(jbytes);
-        return nullptr;
-      }
-
-      return jbytes;
-    }
 
     /**
      * Copies bytes from a rocksdb::Slice to a jByteArray
@@ -4509,12 +4481,25 @@ class JniUtil {
      * @param bytes The bytes to copy
      *
      * @return the Java byte[] or nullptr if an exception occurs
-     * 
-     * @throws RocksDBException thrown 
-     *   if memory size to copy exceeds general java specific array size limitation.
      */
     static jbyteArray copyBytes(JNIEnv* env, const Slice& bytes) {
-      return createJavaByteArrayWithSizeCheck(env, bytes.data(), bytes.size());
+      const jsize jlen = static_cast<jsize>(bytes.size());
+
+      jbyteArray jbytes = env->NewByteArray(jlen);
+      if(jbytes == nullptr) {
+        // exception thrown: OutOfMemoryError
+        return nullptr;
+      }
+
+      env->SetByteArrayRegion(jbytes, 0, jlen,
+        const_cast<jbyte*>(reinterpret_cast<const jbyte*>(bytes.data())));
+      if(env->ExceptionCheck()) {
+        // exception thrown: ArrayIndexOutOfBoundsException
+        env->DeleteLocalRef(jbytes);
+        return nullptr;
+      }
+
+      return jbytes;
     }
 
     /*

java/rocksjni/sst_file_manager.cc

@@ -138,7 +138,7 @@ jobject Java_org_rocksdb_SstFileManager_getTrackedFiles(JNIEnv* env,
 
   const rocksdb::HashMapJni::FnMapKV<const std::string, const uint64_t>
       fn_map_kv =
-          [env](
+          [env, &tracked_files](
               const std::pair<const std::string, const uint64_t>& pair) {
             const jstring jtracked_file_path =
                 env->NewStringUTF(pair.first.c_str());

java/rocksjni/transaction.cc

@@ -48,7 +48,7 @@ void Java_org_rocksdb_Transaction_setSnapshotOnNextOperation__JJ(
     jlong jtxn_notifier_handle) {
   auto* txn = reinterpret_cast<rocksdb::Transaction*>(jhandle);
   auto* txn_notifier =
-      reinterpret_cast<std::shared_ptr<rocksdb::TransactionNotifierJniCallback>*>(
+      reinterpret_cast<std::shared_ptr<rocksdb::TransactionNotifier>*>(
           jtxn_notifier_handle);
   txn->SetSnapshotOnNextOperation(*txn_notifier);
 }

java/src/main/java/org/rocksdb/ColumnFamilyHandle.java

@@ -28,10 +28,8 @@ public class ColumnFamilyHandle extends RocksObject {
    * Gets the name of the Column Family.
    *
    * @return The name of the Column Family.
-   *
-   * @throws RocksDBException if an error occurs whilst retrieving the name.
    */
-  public byte[] getName() throws RocksDBException {
+  public byte[] getName() {
     return getName(nativeHandle_);
   }
 
@@ -73,22 +71,14 @@ public class ColumnFamilyHandle extends RocksObject {
     }
 
     final ColumnFamilyHandle that = (ColumnFamilyHandle) o;
-    try {
-      return rocksDB_.nativeHandle_ == that.rocksDB_.nativeHandle_ &&
-          getID() == that.getID() &&
-          Arrays.equals(getName(), that.getName());
-    } catch (RocksDBException e) {
-      throw new RuntimeException("Cannot compare column family handles", e);
-    }
+    return rocksDB_.nativeHandle_ == that.rocksDB_.nativeHandle_ &&
+        getID() == that.getID() &&
+        Arrays.equals(getName(), that.getName());
   }
 
   @Override
   public int hashCode() {
-    try {
-      return Objects.hash(getName(), getID(), rocksDB_.nativeHandle_);
-    } catch (RocksDBException e) {
-      throw new RuntimeException("Cannot calculate hash code of column family handle", e);
-    }
+    return Objects.hash(getName(), getID(), rocksDB_.nativeHandle_);
   }
 
   /**
@@ -106,7 +96,7 @@ public class ColumnFamilyHandle extends RocksObject {
     }
   }
 
-  private native byte[] getName(final long handle) throws RocksDBException;
+  private native byte[] getName(final long handle);
   private native int getID(final long handle);
   private native ColumnFamilyDescriptor getDescriptor(final long handle) throws RocksDBException;
   @Override protected final native void disposeInternal(final long handle);

java/src/main/java/org/rocksdb/WriteBatch.java

@@ -65,11 +65,8 @@ public class WriteBatch extends AbstractWriteBatch {
    * Retrieve the serialized version of this batch.
    *
    * @return the serialized representation of this write batch.
-   *
-   * @throws RocksDBException if an error occurs whilst retrieving
-   *   the serialized batch data.
    */
-  public byte[] data() throws RocksDBException {
+  public byte[] data() {
     return data(nativeHandle_);
   }
 
@@ -256,7 +253,7 @@ public class WriteBatch extends AbstractWriteBatch {
       final int serializedLength);
   private native void iterate(final long handle, final long handlerHandle)
       throws RocksDBException;
-  private native byte[] data(final long nativeHandle) throws RocksDBException;
+  private native byte[] data(final long nativeHandle);
   private native long getDataSize(final long nativeHandle);
   private native boolean hasPut(final long nativeHandle);
   private native boolean hasDelete(final long nativeHandle);

java/src/test/java/org/rocksdb/RocksDBTest.java

@@ -4,11 +4,9 @@
 //  (found in the LICENSE.Apache file in the root directory).
 package org.rocksdb;
 
-import org.junit.Assume;
 import org.junit.ClassRule;
 import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
 
 import java.util.*;
@@ -145,39 +143,6 @@ public class RocksDBTest {
     }
   }
 
-  @Rule
-  public ExpectedException thrown = ExpectedException.none();
-
-  @Test
-  public void getOutOfArrayMaxSizeValue() throws RocksDBException {
-    final int numberOfValueSplits = 10;
-    final int splitSize = Integer.MAX_VALUE / numberOfValueSplits;
-
-    Runtime runtime = Runtime.getRuntime();
-    long neededMemory = ((long)(splitSize)) * (((long)numberOfValueSplits) + 3);
-    boolean isEnoughMemory = runtime.maxMemory() - runtime.totalMemory() > neededMemory;
-    Assume.assumeTrue(isEnoughMemory);
-
-    final byte[] valueSplit = new byte[splitSize];
-    final byte[] key = "key".getBytes();
-
-    thrown.expect(RocksDBException.class);
-    thrown.expectMessage("Requested array size exceeds VM limit");
-
-    // merge (numberOfValueSplits + 1) valueSplit's to get value size exceeding Integer.MAX_VALUE
-    try (final StringAppendOperator stringAppendOperator = new StringAppendOperator();
-         final Options opt = new Options()
-                 .setCreateIfMissing(true)
-                 .setMergeOperator(stringAppendOperator);
-         final RocksDB db = RocksDB.open(opt, dbFolder.getRoot().getAbsolutePath())) {
-      db.put(key, valueSplit);
-      for (int i = 0; i < numberOfValueSplits; i++) {
-        db.merge(key, valueSplit);
-      }
-      db.get(key);
-    }
-  }
-
   @Test
   public void multiGet() throws RocksDBException, InterruptedException {
     try (final RocksDB db = RocksDB.open(dbFolder.getRoot().getAbsolutePath());

memtable/inlineskiplist.h

@@ -184,10 +184,9 @@ class InlineSkipList {
   const uint16_t kBranching_;
   const uint32_t kScaledInverseBranching_;
 
+  Allocator* const allocator_;  // Allocator used for allocations of nodes
   // Immutable after construction
   Comparator const compare_;
-  Allocator* const allocator_;  // Allocator used for allocations of nodes
-
   Node* const head_;
 
   // Modified only by Insert().  Read racily by readers, but stale
@@ -587,8 +586,8 @@ InlineSkipList<Comparator>::InlineSkipList(const Comparator cmp,
     : kMaxHeight_(static_cast<uint16_t>(max_height)),
       kBranching_(static_cast<uint16_t>(branching_factor)),
       kScaledInverseBranching_((Random::kMaxNext + 1) / kBranching_),
-      compare_(cmp),
       allocator_(allocator),
+      compare_(cmp),
       head_(AllocateNode(0, max_height)),
       max_height_(1),
       seq_splice_(AllocateSplice()) {

memtable/inlineskiplist_test.cc

@@ -309,6 +309,7 @@ TEST_F(InlineSkipTest, InsertWithHint_CompatibleWithInsertWithoutHint) {
   Validate(&list);
 }
 
+#ifndef ROCKSDB_VALGRIND_RUN
 // We want to make sure that with a single writer and multiple
 // concurrent readers (with no synchronization other than when a
 // reader's iterator is created), the reader always observes all the
@@ -635,6 +636,7 @@ TEST_F(InlineSkipTest, ConcurrentInsert1) { RunConcurrentInsert(1); }
 TEST_F(InlineSkipTest, ConcurrentInsert2) { RunConcurrentInsert(2); }
 TEST_F(InlineSkipTest, ConcurrentInsert3) { RunConcurrentInsert(3); }
 
+#endif  // ROCKSDB_VALGRIND_RUN
 }  // namespace rocksdb
 
 int main(int argc, char** argv) {

monitoring/histogram.cc

@@ -235,6 +235,8 @@ void HistogramStat::Data(HistogramData * const data) const {
   data->max = static_cast<double>(max());
   data->average = Average();
   data->standard_deviation = StandardDeviation();
+  data->count = num();
+  data->sum = sum();
 }
 
 void HistogramImpl::Clear() {

monitoring/statistics.cc

@@ -169,11 +169,17 @@ std::string StatisticsImpl::ToString() const {
       char buffer[kTmpStrBufferSize];
       HistogramData hData;
       getHistogramImplLocked(h.first)->Data(&hData);
-      snprintf(
+      // don't handle failures - buffer should always be big enough and arguments
+      // should be provided correctly
+      int ret = snprintf(
           buffer, kTmpStrBufferSize,
-          "%s statistics Percentiles :=> 50 : %f 95 : %f 99 : %f 100 : %f\n",
-          h.second.c_str(), hData.median, hData.percentile95,
-          hData.percentile99, hData.max);
+          "%s P50 : %f P95 : %f P99 : %f P100 : %f COUNT : %" PRIu64 " SUM : %"
+          PRIu64 "\n", h.second.c_str(), hData.median, hData.percentile95,
+          hData.percentile99, hData.max, hData.count, hData.sum);
+      if (ret < 0 || ret >= kTmpStrBufferSize) {
+        assert(false);
+        continue;
+      }
       res.append(buffer);
     }
   }

options/options_parser.cc

@@ -17,6 +17,7 @@
 #include "rocksdb/convenience.h"
 #include "rocksdb/db.h"
 #include "util/cast_util.h"
+#include "util/file_reader_writer.h"
 #include "util/string_util.h"
 #include "util/sync_point.h"
 
@@ -41,12 +42,16 @@ Status PersistRocksDBOptions(const DBOptions& db_opt,
     return Status::InvalidArgument(
         "cf_names.size() and cf_opts.size() must be the same");
   }
-  std::unique_ptr<WritableFile> writable;
+  std::unique_ptr<WritableFile> wf;
 
-  Status s = env->NewWritableFile(file_name, &writable, EnvOptions());
+  Status s = env->NewWritableFile(file_name, &wf, EnvOptions());
   if (!s.ok()) {
     return s;
   }
+  unique_ptr<WritableFileWriter> writable;
+  writable.reset(new WritableFileWriter(std::move(wf), EnvOptions(),
+                                        nullptr /* statistics */));
+
   std::string options_file_content;
 
   writable->Append(option_file_header + "[" +
@@ -93,8 +98,7 @@ Status PersistRocksDBOptions(const DBOptions& db_opt,
       writable->Append(options_file_content + "\n");
     }
   }
-  writable->Flush();
-  writable->Fsync();
+  writable->Sync(true /* use_fsync */);
   writable->Close();
 
   return RocksDBOptionsParser::VerifyRocksDBOptionsFromFile(

options/options_settable_test.cc

@@ -140,7 +140,6 @@ TEST_F(OptionsSettableTest, BlockBasedTableOptionsAllFieldsSettable) {
       "cache_index_and_filter_blocks=1;"
       "cache_index_and_filter_blocks_with_high_priority=true;"
       "pin_l0_filter_and_index_blocks_in_cache=1;"
-      "pin_top_level_index_and_filter=1;"
       "index_type=kHashSearch;"
       "checksum=kxxHash;hash_index_allow_collision=1;no_block_cache=1;"
       "block_cache=1M;block_cache_compressed=1k;block_size=1024;"

port/port_posix.cc

@@ -188,8 +188,6 @@ int GetMaxOpenFiles() {
 void *cacheline_aligned_alloc(size_t size) {
 #if __GNUC__ < 5 && defined(__SANITIZE_ADDRESS__)
   return malloc(size);
-#elif defined(_ISOC11_SOURCE)
-  return aligned_alloc(CACHE_LINE_SIZE, size);
 #elif ( _POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600 || defined(__APPLE__))
   void *m;
   errno = posix_memalign(&m, CACHE_LINE_SIZE, size);

port/win/env_win.cc

@@ -112,6 +112,15 @@ Status WinEnvIO::DeleteFile(const std::string& fname) {
   return result;
 }
 
+Status WinEnvIO::Truncate(const std::string& fname, size_t size) {
+  Status s;
+  int result = truncate(fname.c_str(), size);
+  if (result != 0) {
+    s = IOError("Failed to truncate: " + fname, errno);
+  }
+  return s;
+}
+
 Status WinEnvIO::GetCurrentTime(int64_t* unix_time) {
   time_t time = std::time(nullptr);
   if (time == (time_t)(-1)) {
@@ -344,7 +353,7 @@ Status WinEnvIO::NewRandomRWFile(const std::string & fname,
   // Random access is to disable read-ahead as the system reads too much data
   DWORD desired_access = GENERIC_READ | GENERIC_WRITE;
   DWORD shared_mode = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE;
-  DWORD creation_disposition = OPEN_ALWAYS; // Create if necessary or open existing
+  DWORD creation_disposition = OPEN_EXISTING; // Fail if file does not exist
   DWORD file_flags = FILE_FLAG_RANDOM_ACCESS;
 
   if (options.use_direct_reads && options.use_direct_writes) {
@@ -380,6 +389,84 @@ Status WinEnvIO::NewRandomRWFile(const std::string & fname,
   return s;
 }
 
+Status WinEnvIO::NewMemoryMappedFileBuffer(const std::string & fname,
+  std::unique_ptr<MemoryMappedFileBuffer>* result) {
+  Status s;
+  result->reset();
+
+  DWORD fileFlags = FILE_ATTRIBUTE_READONLY;
+
+  HANDLE hFile = INVALID_HANDLE_VALUE;
+  {
+    IOSTATS_TIMER_GUARD(open_nanos);
+    hFile = CreateFileA(
+      fname.c_str(), GENERIC_READ | GENERIC_WRITE,
+      FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
+      NULL,
+      OPEN_EXISTING,  // Open only if it exists
+      fileFlags,
+      NULL);
+  }
+
+  if (INVALID_HANDLE_VALUE == hFile) {
+    auto lastError = GetLastError();
+    s = IOErrorFromWindowsError("Failed to open NewMemoryMappedFileBuffer: " + fname,
+      lastError);
+    return s;
+  }
+  UniqueCloseHandlePtr fileGuard(hFile, CloseHandleFunc);
+
+  uint64_t fileSize = 0;
+  s = GetFileSize(fname, &fileSize);
+  if (!s.ok()) {
+    return s;
+  }
+  // Will not map empty files
+  if (fileSize == 0) {
+    return Status::NotSupported("NewMemoryMappedFileBuffer can not map zero length files: " + fname);
+  }
+
+  // size_t is 32-bit with 32-bit builds
+  if (fileSize > std::numeric_limits<size_t>::max()) {
+    return Status::NotSupported(
+      "The specified file size does not fit into 32-bit memory addressing: " + fname);
+  }
+
+  HANDLE hMap = CreateFileMappingA(hFile, NULL, PAGE_READWRITE,
+      0,  // Whole file at its present length
+      0,
+      NULL);  // Mapping name
+
+  if (!hMap) {
+    auto lastError = GetLastError();
+    return IOErrorFromWindowsError(
+      "Failed to create file mapping for NewMemoryMappedFileBuffer: " + fname,
+      lastError);
+  }
+  UniqueCloseHandlePtr mapGuard(hMap, CloseHandleFunc);
+
+  void* base = MapViewOfFileEx(hMap, FILE_MAP_WRITE,
+    0,  // High DWORD of access start
+    0,  // Low DWORD
+    fileSize,
+    NULL);  // Let the OS choose the mapping
+
+  if (!base) {
+    auto lastError = GetLastError();
+    return IOErrorFromWindowsError(
+      "Failed to MapViewOfFile for NewMemoryMappedFileBuffer: " + fname,
+      lastError);
+  }
+
+  result->reset(new WinMemoryMappedBuffer(hFile, hMap, 
+    base, static_cast<size_t>(fileSize)));
+
+  mapGuard.release();
+  fileGuard.release();
+
+  return s;
+}
+
 Status WinEnvIO::NewDirectory(const std::string& name,
   std::unique_ptr<Directory>* result) {
   Status s;
@@ -928,27 +1015,33 @@ std::string WinEnvIO::TimeToString(uint64_t secondsSince1970) {
 
 EnvOptions WinEnvIO::OptimizeForLogWrite(const EnvOptions& env_options,
   const DBOptions& db_options) const {
-  EnvOptions optimized = env_options;
+  EnvOptions optimized(env_options);
+  // These two the same as default optimizations
   optimized.bytes_per_sync = db_options.wal_bytes_per_sync;
-  optimized.use_mmap_writes = false;
-  // This is because we flush only whole pages on unbuffered io and
-  // the last records are not guaranteed to be flushed.
-  optimized.use_direct_writes = false;
-  // TODO(icanadi) it's faster if fallocate_with_keep_size is false, but it
-  // breaks TransactionLogIteratorStallAtLastRecord unit test. Fix the unit
-  // test and make this false
-  optimized.fallocate_with_keep_size = true;
   optimized.writable_file_max_buffer_size =
       db_options.writable_file_max_buffer_size;
+
+  // This adversely affects %999 on windows
+  optimized.use_mmap_writes = false;
+  // Direct writes will produce a huge perf impact on
+  // Windows. Pre-allocate space for WAL.
+  optimized.use_direct_writes = false;
   return optimized;
 }
 
 EnvOptions WinEnvIO::OptimizeForManifestWrite(
   const EnvOptions& env_options) const {
-  EnvOptions optimized = env_options;
+  EnvOptions optimized(env_options);
   optimized.use_mmap_writes = false;
-  optimized.use_direct_writes = false;
-  optimized.fallocate_with_keep_size = true;
+  optimized.use_direct_reads = false;
+  return optimized;
+}
+
+EnvOptions WinEnvIO::OptimizeForManifestRead(
+  const EnvOptions& env_options) const {
+  EnvOptions optimized(env_options);
+  optimized.use_mmap_writes = false;
+  optimized.use_direct_reads = false;
   return optimized;
 }
 
@@ -1145,6 +1238,10 @@ Status WinEnv::DeleteFile(const std::string& fname) {
   return winenv_io_.DeleteFile(fname);
 }
 
+Status WinEnv::Truncate(const std::string& fname, size_t size) {
+  return winenv_io_.Truncate(fname, size);
+}
+
 Status WinEnv::GetCurrentTime(int64_t* unix_time) {
   return winenv_io_.GetCurrentTime(unix_time);
 }
@@ -1173,10 +1270,15 @@ Status WinEnv::ReopenWritableFile(const std::string& fname,
 }
 
 Status WinEnv::NewRandomRWFile(const std::string & fname,
-  unique_ptr<RandomRWFile>* result, const EnvOptions & options) {
+  std::unique_ptr<RandomRWFile>* result, const EnvOptions & options) {
   return winenv_io_.NewRandomRWFile(fname, result, options);
 }
 
+Status WinEnv::NewMemoryMappedFileBuffer(const std::string& fname,
+  std::unique_ptr<MemoryMappedFileBuffer>* result) {
+  return winenv_io_.NewMemoryMappedFileBuffer(fname, result);
+}
+
 Status WinEnv::NewDirectory(const std::string& name,
   std::unique_ptr<Directory>* result) {
   return winenv_io_.NewDirectory(name, result);
@@ -1310,6 +1412,11 @@ void  WinEnv::IncBackgroundThreadsIfNeeded(int num, Env::Priority pri) {
   return winenv_threads_.IncBackgroundThreadsIfNeeded(num, pri);
 }
 
+EnvOptions WinEnv::OptimizeForManifestRead(
+  const EnvOptions& env_options) const {
+  return winenv_io_.OptimizeForManifestRead(env_options);
+}
+
 EnvOptions WinEnv::OptimizeForLogWrite(const EnvOptions& env_options,
   const DBOptions& db_options) const {
   return winenv_io_.OptimizeForLogWrite(env_options, db_options);

port/win/env_win.h

@@ -89,6 +89,8 @@ public:
 
   virtual Status DeleteFile(const std::string& fname);
 
+  Status Truncate(const std::string& fname, size_t size);
+
   virtual Status GetCurrentTime(int64_t* unix_time);
 
   virtual Status NewSequentialFile(const std::string& fname,
@@ -110,6 +112,10 @@ public:
     unique_ptr<RandomRWFile>* result,
     const EnvOptions& options);
 
+  virtual Status NewMemoryMappedFileBuffer(
+    const std::string& fname,
+    std::unique_ptr<MemoryMappedFileBuffer>* result);
+
   virtual Status NewDirectory(const std::string& name,
     std::unique_ptr<Directory>* result);
 
@@ -168,6 +174,9 @@ public:
   virtual EnvOptions OptimizeForManifestWrite(
     const EnvOptions& env_options) const;
 
+  virtual EnvOptions OptimizeForManifestRead(
+    const EnvOptions& env_options) const;
+
   size_t GetPageSize() const { return page_size_; }
 
   size_t GetAllocationGranularity() const { return allocation_granularity_; }
@@ -197,6 +206,8 @@ public:
 
   Status DeleteFile(const std::string& fname) override;
 
+  Status Truncate(const std::string& fname, size_t size) override;
+
   Status GetCurrentTime(int64_t* unix_time) override;
 
   Status NewSequentialFile(const std::string& fname,
@@ -224,9 +235,13 @@ public:
 
   // The returned file will only be accessed by one thread at a time.
   Status NewRandomRWFile(const std::string& fname,
-    unique_ptr<RandomRWFile>* result,
+    std::unique_ptr<RandomRWFile>* result,
     const EnvOptions& options) override;
 
+  Status NewMemoryMappedFileBuffer(
+    const std::string& fname,
+    std::unique_ptr<MemoryMappedFileBuffer>* result) override;
+
   Status NewDirectory(const std::string& name,
     std::unique_ptr<Directory>* result) override;
 
@@ -302,12 +317,16 @@ public:
 
   void IncBackgroundThreadsIfNeeded(int num, Env::Priority pri) override;
 
+  EnvOptions OptimizeForManifestRead(
+    const EnvOptions& env_options) const override;
+
   EnvOptions OptimizeForLogWrite(const EnvOptions& env_options,
     const DBOptions& db_options) const override;
 
   EnvOptions OptimizeForManifestWrite(
     const EnvOptions& env_options) const override;
 
+
 private:
 
   WinEnvIO      winenv_io_;

port/win/io_win.cc

@@ -1061,6 +1061,27 @@ Status WinRandomRWFile::Close() {
   return CloseImpl();
 }
 
+//////////////////////////////////////////////////////////////////////////
+/// WinMemoryMappedBufer
+WinMemoryMappedBuffer::~WinMemoryMappedBuffer() {
+  BOOL ret = FALSE;
+  if (base_ != nullptr) {
+    ret = ::UnmapViewOfFile(base_);
+    assert(ret);
+    base_ = nullptr;
+  }
+  if (map_handle_ != NULL && map_handle_ != INVALID_HANDLE_VALUE) {
+    ret = ::CloseHandle(map_handle_);
+    assert(ret);
+    map_handle_ = NULL;
+  }
+  if (file_handle_ != NULL && file_handle_ != INVALID_HANDLE_VALUE) {
+    ret = ::CloseHandle(file_handle_);
+    assert(ret);
+    file_handle_ = NULL;
+  }
+}
+
 //////////////////////////////////////////////////////////////////////////
 /// WinDirectory
 

port/win/io_win.h

@@ -411,6 +411,18 @@ class WinRandomRWFile : private WinFileData,
   virtual Status Close() override;
 };
 
+class WinMemoryMappedBuffer : public MemoryMappedFileBuffer {
+private:
+  HANDLE  file_handle_;
+  HANDLE  map_handle_;
+public:
+  WinMemoryMappedBuffer(HANDLE file_handle, HANDLE map_handle, void* base, size_t size) :
+    MemoryMappedFileBuffer(base, size),
+    file_handle_(file_handle),
+    map_handle_(map_handle) {}
+  ~WinMemoryMappedBuffer() override;
+};
+
 class WinDirectory : public Directory {
   HANDLE handle_;
  public:

src.mk

@@ -208,6 +208,8 @@ LIB_SOURCES =                                                   \
   utilities/transactions/transaction_util.cc                    \
   utilities/transactions/write_prepared_txn.cc                  \
   utilities/transactions/write_prepared_txn_db.cc               \
+  utilities/transactions/write_unprepared_txn.cc                \
+  utilities/transactions/write_unprepared_txn_db.cc             \
   utilities/ttl/db_ttl_impl.cc                                  \
   utilities/write_batch_with_index/write_batch_with_index.cc    \
   utilities/write_batch_with_index/write_batch_with_index_internal.cc    \
@@ -250,6 +252,8 @@ MAIN_SOURCES =                                                          \
   cache/cache_bench.cc                                                  \
   cache/cache_test.cc                                                   \
   db/column_family_test.cc                                              \
+  db/compact_files_test.cc                                              \
+  db/compaction_iterator_test.cc                                        \
   db/compaction_job_stats_test.cc                                       \
   db/compaction_job_test.cc                                             \
   db/compaction_picker_test.cc                                          \
@@ -257,6 +261,7 @@ MAIN_SOURCES =                                                          \
   db/corruption_test.cc                                                 \
   db/cuckoo_table_db_test.cc                                            \
   db/db_basic_test.cc                                                   \
+  db/db_blob_index_test.cc                                              \
   db/db_block_cache_test.cc                                             \
   db/db_bloom_filter_test.cc                                            \
   db/db_compaction_filter_test.cc                                       \
@@ -273,33 +278,50 @@ MAIN_SOURCES =                                                          \
   db/db_memtable_test.cc                                                \
   db/db_merge_operator_test.cc                                          \
   db/db_options_test.cc                                                 \
+  db/db_properties_test.cc                                              \
   db/db_range_del_test.cc                                               \
   db/db_sst_test.cc                                                     \
   db/db_statistics_test.cc                                              \
   db/db_table_properties_test.cc                                        \
   db/db_tailing_iter_test.cc                                            \
   db/db_test.cc                                                         \
+  db/db_test2.cc                                                        \
   db/db_universal_compaction_test.cc                                    \
   db/db_wal_test.cc                                                     \
   db/db_write_test.cc                                                   \
   db/dbformat_test.cc                                                   \
   db/deletefile_test.cc                                                 \
-  db/obsolete_files_test.cc						\
+  db/env_timed_test.cc                                                  \
   db/external_sst_file_basic_test.cc                                    \
   db/external_sst_file_test.cc                                          \
   db/fault_injection_test.cc                                            \
   db/file_indexer_test.cc                                               \
+  db/file_reader_writer_test.cc                                         \
   db/filename_test.cc                                                   \
   db/flush_job_test.cc                                                  \
+  db/hash_table_test.cc                                                 \
+  db/hash_test.cc                                                       \
+  db/heap_test.cc                                                       \
   db/listener_test.cc                                                   \
   db/log_test.cc                                                        \
+  db/lru_cache_test.cc                                                  \
   db/manual_compaction_test.cc                                          \
+  db/memtable_list_test.cc                                              \
+  db/merge_helper_test.cc                                               \
   db/merge_test.cc                                                      \
+  db/obsolete_files_test.cc						\
+  db/options_settable_test.cc                                           \
   db/options_file_test.cc                                               \
+  db/partitioned_filter_block_test.cc                                   \
   db/perf_context_test.cc                                               \
+  db/persistent_cache_test.cc                                           \
   db/plain_table_db_test.cc                                             \
   db/prefix_test.cc                                                     \
+  db/redis_test.cc                                                      \
+  db/repair_test.cc                                                     \
+  db/range_del_aggregator_test.cc                                       \
   db/table_properties_collector_test.cc                                 \
+  db/util_merge_operators_test.cc                                       \
   db/version_builder_test.cc                                            \
   db/version_edit_test.cc                                               \
   db/version_set_test.cc                                                \

table/block.cc

@@ -483,17 +483,9 @@ void Block::SetBlockPrefixIndex(BlockPrefixIndex* prefix_index) {
 
 size_t Block::ApproximateMemoryUsage() const {
   size_t usage = usable_size();
-#ifdef ROCKSDB_MALLOC_USABLE_SIZE
-  usage += malloc_usable_size((void*)this);
-#else
-  usage += sizeof(*this);
-#endif  // ROCKSDB_MALLOC_USABLE_SIZE
   if (prefix_index_) {
     usage += prefix_index_->ApproximateMemoryUsage();
   }
-  if (read_amp_bitmap_) {
-    usage += read_amp_bitmap_->ApproximateMemoryUsage();
-  }
   return usage;
 }
 

table/block.h

@@ -104,13 +104,6 @@ class BlockReadAmpBitmap {
 
   uint32_t GetBytesPerBit() { return 1 << bytes_per_bit_pow_; }
 
-  size_t ApproximateMemoryUsage() const {
-#ifdef ROCKSDB_MALLOC_USABLE_SIZE
-    return malloc_usable_size((void*)this);
-#endif  // ROCKSDB_MALLOC_USABLE_SIZE
-    return sizeof(*this);
-  }
-
  private:
   // Get the current value of bit at `bit_idx` and set it to 1
   inline bool GetAndSet(uint32_t bit_idx) {
@@ -149,8 +142,14 @@ class Block {
   size_t size() const { return size_; }
   const char* data() const { return data_; }
   bool cachable() const { return contents_.cachable; }
-  // The additional memory space taken by the block data.
-  size_t usable_size() const { return contents_.usable_size(); }
+  size_t usable_size() const {
+#ifdef ROCKSDB_MALLOC_USABLE_SIZE
+    if (contents_.allocation.get() != nullptr) {
+      return malloc_usable_size(contents_.allocation.get());
+    }
+#endif  // ROCKSDB_MALLOC_USABLE_SIZE
+    return size_;
+  }
   uint32_t NumRestarts() const;
   CompressionType compression_type() const {
     return contents_.compression_type;
@@ -255,7 +254,7 @@ class BlockIter final : public InternalIterator {
   }
 
   // Makes Valid() return false, status() return `s`, and Seek()/Prev()/etc do
-  // nothing. Calls cleanup functions.
+  // nothing.
   void Invalidate(Status s) {
     // Assert that the BlockIter is never deleted while Pinning is Enabled.
     assert(!pinned_iters_mgr_ ||
@@ -265,9 +264,6 @@ class BlockIter final : public InternalIterator {
     current_ = restarts_;
     status_ = s;
 
-    // Call cleanup callbacks.
-    Cleanable::Reset();
-
     // Clear prev entries cache.
     prev_entries_keys_buff_.clear();
     prev_entries_.clear();

table/block_based_table_builder.cc

@@ -643,7 +643,7 @@ Status BlockBasedTableBuilder::InsertBlockInCache(const Slice& block_contents,
               (end - r->compressed_cache_key_prefix));
 
     // Insert into compressed block cache.
-    block_cache_compressed->Insert(key, block, block->ApproximateMemoryUsage(),
+    block_cache_compressed->Insert(key, block, block->usable_size(),
                                    &DeleteCachedBlock);
 
     // Invalidate OS cache.

table/block_based_table_factory.cc

@@ -151,9 +151,6 @@ std::string BlockBasedTableFactory::GetPrintableTableOptions() const {
            "  pin_l0_filter_and_index_blocks_in_cache: %d\n",
            table_options_.pin_l0_filter_and_index_blocks_in_cache);
   ret.append(buffer);
-  snprintf(buffer, kBufferSize, "  pin_top_level_index_and_filter: %d\n",
-           table_options_.pin_top_level_index_and_filter);
-  ret.append(buffer);
   snprintf(buffer, kBufferSize, "  index_type: %d\n",
            table_options_.index_type);
   ret.append(buffer);

table/block_based_table_factory.h

@@ -158,10 +158,6 @@ static std::unordered_map<std::string, OptionTypeInfo>
           OptionType::kBoolean, OptionVerificationType::kNormal, false, 0}},
         {"block_align",
          {offsetof(struct BlockBasedTableOptions, block_align),
-          OptionType::kBoolean, OptionVerificationType::kNormal, false, 0}},
-        {"pin_top_level_index_and_filter",
-         {offsetof(struct BlockBasedTableOptions,
-                   pin_top_level_index_and_filter),
           OptionType::kBoolean, OptionVerificationType::kNormal, false, 0}}};
 #endif  // !ROCKSDB_LITE
 }  // namespace rocksdb

table/block_based_table_reader.cc

@@ -176,8 +176,9 @@ Cache::Handle* GetEntryFromCache(Cache* block_cache, const Slice& key,
 // For hash based index, return true if prefix_extractor and
 // prefix_extractor_block mismatch, false otherwise. This flag will be used
 // as total_order_seek via NewIndexIterator
-bool PrefixExtractorChanged(const TableProperties* table_properties,
-                            const SliceTransform* prefix_extractor) {
+bool PrefixExtractorChanged(
+    std::shared_ptr<const TableProperties> table_properties,
+    const SliceTransform* prefix_extractor) {
   // BlockBasedTableOptions::kHashSearch requires prefix_extractor to be set.
   // Turn off hash index in prefix_extractor is not set; if  prefix_extractor
   // is set but prefix_extractor_block is not set, also disable hash index
@@ -237,18 +238,16 @@ class PartitionIndexReader : public IndexReader, public Cleanable {
       return NewTwoLevelIterator(
           new BlockBasedTable::PartitionedIndexIteratorState(
               table_, &partition_map_, index_key_includes_seq_),
-          index_block_->NewIterator(icomparator_,
-                                    icomparator_->user_comparator(), nullptr,
-                                    true, nullptr, index_key_includes_seq_));
+          index_block_->NewIterator(
+              icomparator_, icomparator_->user_comparator(), nullptr, true));
     } else {
       auto ro = ReadOptions();
       ro.fill_cache = fill_cache;
       bool kIsIndex = true;
       return new BlockBasedTableIterator(
           table_, ro, *icomparator_,
-          index_block_->NewIterator(icomparator_,
-                                    icomparator_->user_comparator(), nullptr,
-                                    true, nullptr, index_key_includes_seq_),
+          index_block_->NewIterator(
+              icomparator_, icomparator_->user_comparator(), nullptr, true),
           false,
           /* prefix_extractor */ nullptr, kIsIndex, index_key_includes_seq_);
     }
@@ -264,7 +263,7 @@ class PartitionIndexReader : public IndexReader, public Cleanable {
     BlockIter biter;
     BlockHandle handle;
     index_block_->NewIterator(icomparator_, icomparator_->user_comparator(),
-                              &biter, true, nullptr, index_key_includes_seq_);
+                              &biter, true);
     // Index partitions are assumed to be consecuitive. Prefetch them all.
     // Read the first block offset
     biter.SeekToFirst();
@@ -346,14 +345,7 @@ class PartitionIndexReader : public IndexReader, public Cleanable {
 
   virtual size_t ApproximateMemoryUsage() const override {
     assert(index_block_);
-    size_t usage = index_block_->ApproximateMemoryUsage();
-#ifdef ROCKSDB_MALLOC_USABLE_SIZE
-    usage += malloc_usable_size((void*)this);
-#else
-    usage += sizeof(*this);
-#endif  // ROCKSDB_MALLOC_USABLE_SIZE
-    // TODO(myabandeh): more accurate estimate of partition_map_ mem usage
-    return usage;
+    return index_block_->ApproximateMemoryUsage();
   }
 
  private:
@@ -422,13 +414,7 @@ class BinarySearchIndexReader : public IndexReader {
 
   virtual size_t ApproximateMemoryUsage() const override {
     assert(index_block_);
-    size_t usage = index_block_->ApproximateMemoryUsage();
-#ifdef ROCKSDB_MALLOC_USABLE_SIZE
-    usage += malloc_usable_size((void*)this);
-#else
-    usage += sizeof(*this);
-#endif  // ROCKSDB_MALLOC_USABLE_SIZE
-    return usage;
+    return index_block_->ApproximateMemoryUsage();
   }
 
  private:
@@ -513,7 +499,7 @@ class HashIndexReader : public IndexReader {
         file, prefetch_buffer, footer, ReadOptions(), prefixes_meta_handle,
         &prefixes_meta_contents, ioptions, true /* decompress */,
         dummy_comp_dict /*compression dict*/, cache_options);
-    prefixes_meta_block_fetcher.ReadBlockContents();
+    s = prefixes_meta_block_fetcher.ReadBlockContents();
     if (!s.ok()) {
       // TODO: log error
       return Status::OK();
@@ -545,14 +531,8 @@ class HashIndexReader : public IndexReader {
 
   virtual size_t ApproximateMemoryUsage() const override {
     assert(index_block_);
-    size_t usage = index_block_->ApproximateMemoryUsage();
-    usage += prefixes_contents_.usable_size();
-#ifdef ROCKSDB_MALLOC_USABLE_SIZE
-    usage += malloc_usable_size((void*)this);
-#else
-    usage += sizeof(*this);
-#endif  // ROCKSDB_MALLOC_USABLE_SIZE
-    return usage;
+    return index_block_->ApproximateMemoryUsage() +
+           prefixes_contents_.data.size();
   }
 
  private:
@@ -720,17 +700,8 @@ Status BlockBasedTable::Open(const ImmutableCFOptions& ioptions,
 
   std::unique_ptr<FilePrefetchBuffer> prefetch_buffer;
 
-  // prefetch both index and filters, down to all partitions
-  const bool prefetch_all = prefetch_index_and_filter_in_cache || level == 0;
-  const bool preload_all = !table_options.cache_index_and_filter_blocks;
-  // Before read footer, readahead backwards to prefetch data. Do more readahead
-  // if we're going to read index/filter.
-  // TODO: This may incorrectly select small readahead in case partitioned
-  // index/filter is enabled and top-level partition pinning is enabled. That's
-  // because we need to issue readahead before we read the properties, at which
-  // point we don't yet know the index type.
-  const size_t kTailPrefetchSize =
-      prefetch_all || preload_all ? 512 * 1024 : 4 * 1024;
+  // Before read footer, readahead backwards to prefetch data
+  const size_t kTailPrefetchSize = 512 * 1024;
   size_t prefetch_off;
   size_t prefetch_len;
   if (file_size < kTailPrefetchSize) {
@@ -926,43 +897,23 @@ Status BlockBasedTable::Open(const ImmutableCFOptions& ioptions,
                                                 rep->ioptions.info_log);
   }
 
-  BlockBasedTableOptions::IndexType index_type = new_table->UpdateIndexType();
-  // prefetch the first level of index
-  const bool prefetch_index =
-      prefetch_all ||
-      (table_options.pin_top_level_index_and_filter &&
-       index_type == BlockBasedTableOptions::kTwoLevelIndexSearch);
-  // prefetch the first level of filter
-  const bool prefetch_filter =
-      prefetch_all || (table_options.pin_top_level_index_and_filter &&
-                       rep->filter_type == Rep::FilterType::kPartitionedFilter);
-  // Partition fitlers cannot be enabled without partition indexes
-  assert(!prefetch_filter || prefetch_index);
-  // pin both index and filters, down to all partitions
-  const bool pin_all =
+  const bool pin =
       rep->table_options.pin_l0_filter_and_index_blocks_in_cache && level == 0;
-  // pin the first level of index
-  const bool pin_index =
-      pin_all || (table_options.pin_top_level_index_and_filter &&
-                  index_type == BlockBasedTableOptions::kTwoLevelIndexSearch);
-  // pin the first level of filter
-  const bool pin_filter =
-      pin_all || (table_options.pin_top_level_index_and_filter &&
-                  rep->filter_type == Rep::FilterType::kPartitionedFilter);
   // pre-fetching of blocks is turned on
   // Will use block cache for index/filter blocks access
   // Always prefetch index and filter for level 0
   if (table_options.cache_index_and_filter_blocks) {
-    assert(table_options.block_cache != nullptr);
-    if (prefetch_index) {
+    if (prefetch_index_and_filter_in_cache || level == 0) {
+      assert(table_options.block_cache != nullptr);
       // Hack: Call NewIndexIterator() to implicitly add index to the
       // block_cache
+
       CachableEntry<IndexReader> index_entry;
       bool prefix_extractor_changed = false;
       // check prefix_extractor match only if hash based index is used
       if (rep->index_type == BlockBasedTableOptions::kHashSearch) {
-        prefix_extractor_changed = PrefixExtractorChanged(
-            rep->table_properties.get(), prefix_extractor);
+        prefix_extractor_changed =
+            PrefixExtractorChanged(rep->table_properties, prefix_extractor);
       }
       unique_ptr<InternalIterator> iter(new_table->NewIndexIterator(
           ReadOptions(), prefix_extractor_changed, nullptr, &index_entry));
@@ -972,29 +923,27 @@ Status BlockBasedTable::Open(const ImmutableCFOptions& ioptions,
         // On success it should give us ownership of the `CachableEntry` by
         // populating `index_entry`.
         assert(index_entry.value != nullptr);
-        if (prefetch_all) {
-          index_entry.value->CacheDependencies(pin_all);
-        }
-        if (pin_index) {
+        index_entry.value->CacheDependencies(pin);
+        if (pin) {
           rep->index_entry = std::move(index_entry);
         } else {
           index_entry.Release(table_options.block_cache.get());
         }
-      }
-    }
-    if (s.ok() && prefetch_filter) {
-      // Hack: Call GetFilter() to implicitly add filter to the block_cache
-      auto filter_entry = new_table->GetFilter(prefix_extractor);
-      if (filter_entry.value != nullptr && prefetch_all) {
-        filter_entry.value->CacheDependencies(pin_all, prefix_extractor);
-      }
-      // if pin_filter is true then save it in rep_->filter_entry; it will be
-      // released in the destructor only, hence it will be pinned in the
-      // cache while this reader is alive
-      if (pin_filter) {
-        rep->filter_entry = filter_entry;
-      } else {
-        filter_entry.Release(table_options.block_cache.get());
+
+        // Hack: Call GetFilter() to implicitly add filter to the block_cache
+        auto filter_entry = new_table->GetFilter(prefix_extractor);
+        if (filter_entry.value != nullptr) {
+          filter_entry.value->CacheDependencies(pin, prefix_extractor);
+        }
+        // if pin_l0_filter_and_index_blocks_in_cache is true, and this is
+        // a level0 file, then save it in rep_->filter_entry; it will be
+        // released in the destructor only, hence it will be pinned in the
+        // cache while this reader is alive
+        if (pin) {
+          rep->filter_entry = filter_entry;
+        } else {
+          filter_entry.Release(table_options.block_cache.get());
+        }
       }
     }
   } else {
@@ -1010,7 +959,7 @@ Status BlockBasedTable::Open(const ImmutableCFOptions& ioptions,
       // are hence follow the configuration for pin and prefetch regardless of
       // the value of cache_index_and_filter_blocks
       if (prefetch_index_and_filter_in_cache || level == 0) {
-        rep->index_reader->CacheDependencies(pin_all);
+        rep->index_reader->CacheDependencies(pin);
       }
 
       // Set filter block
@@ -1023,7 +972,7 @@ Status BlockBasedTable::Open(const ImmutableCFOptions& ioptions,
         // Refer to the comment above about paritioned indexes always being
         // cached
         if (filter && (prefetch_index_and_filter_in_cache || level == 0)) {
-          filter->CacheDependencies(pin_all, prefix_extractor);
+          filter->CacheDependencies(pin, prefix_extractor);
         }
       }
     } else {
@@ -1168,34 +1117,40 @@ Status BlockBasedTable::GetDataBlockFromCache(
     assert(block->value->compression_type() == kNoCompression);
     if (block_cache != nullptr && block->value->cachable() &&
         read_options.fill_cache) {
-      size_t charge = block->value->ApproximateMemoryUsage();
-      s = block_cache->Insert(block_cache_key, block->value, charge,
-                              &DeleteCachedEntry<Block>,
-                              &(block->cache_handle));
-      block_cache->TEST_mark_as_data_block(block_cache_key, charge);
+      s = block_cache->Insert(
+          block_cache_key, block->value, block->value->usable_size(),
+          &DeleteCachedEntry<Block>, &(block->cache_handle));
+      block_cache->TEST_mark_as_data_block(block_cache_key,
+                                           block->value->usable_size());
       if (s.ok()) {
         if (get_context != nullptr) {
           get_context->RecordCounters(BLOCK_CACHE_ADD, 1);
-          get_context->RecordCounters(BLOCK_CACHE_BYTES_WRITE, charge);
+          get_context->RecordCounters(BLOCK_CACHE_BYTES_WRITE,
+                                      block->value->usable_size());
         } else {
           RecordTick(statistics, BLOCK_CACHE_ADD);
-          RecordTick(statistics, BLOCK_CACHE_BYTES_WRITE, charge);
+          RecordTick(statistics, BLOCK_CACHE_BYTES_WRITE,
+                     block->value->usable_size());
         }
         if (is_index) {
           if (get_context != nullptr) {
             get_context->RecordCounters(BLOCK_CACHE_INDEX_ADD, 1);
-            get_context->RecordCounters(BLOCK_CACHE_INDEX_BYTES_INSERT, charge);
+            get_context->RecordCounters(BLOCK_CACHE_INDEX_BYTES_INSERT,
+                                        block->value->usable_size());
           } else {
             RecordTick(statistics, BLOCK_CACHE_INDEX_ADD);
-            RecordTick(statistics, BLOCK_CACHE_INDEX_BYTES_INSERT, charge);
+            RecordTick(statistics, BLOCK_CACHE_INDEX_BYTES_INSERT,
+                       block->value->usable_size());
           }
         } else {
           if (get_context != nullptr) {
             get_context->RecordCounters(BLOCK_CACHE_DATA_ADD, 1);
-            get_context->RecordCounters(BLOCK_CACHE_DATA_BYTES_INSERT, charge);
+            get_context->RecordCounters(BLOCK_CACHE_DATA_BYTES_INSERT,
+                                        block->value->usable_size());
           } else {
             RecordTick(statistics, BLOCK_CACHE_DATA_ADD);
-            RecordTick(statistics, BLOCK_CACHE_DATA_BYTES_INSERT, charge);
+            RecordTick(statistics, BLOCK_CACHE_DATA_BYTES_INSERT,
+                       block->value->usable_size());
           }
         }
       } else {
@@ -1248,7 +1203,7 @@ Status BlockBasedTable::PutDataBlockToCache(
   if (block_cache_compressed != nullptr && raw_block != nullptr &&
       raw_block->cachable()) {
     s = block_cache_compressed->Insert(compressed_block_cache_key, raw_block,
-                                       raw_block->ApproximateMemoryUsage(),
+                                       raw_block->usable_size(),
                                        &DeleteCachedEntry<Block>);
     if (s.ok()) {
       // Avoid the following code to delete this cached block.
@@ -1263,35 +1218,41 @@ Status BlockBasedTable::PutDataBlockToCache(
   // insert into uncompressed block cache
   assert((block->value->compression_type() == kNoCompression));
   if (block_cache != nullptr && block->value->cachable()) {
-    size_t charge = block->value->ApproximateMemoryUsage();
-    s = block_cache->Insert(block_cache_key, block->value, charge,
-                            &DeleteCachedEntry<Block>, &(block->cache_handle),
-                            priority);
-    block_cache->TEST_mark_as_data_block(block_cache_key, charge);
+    s = block_cache->Insert(
+        block_cache_key, block->value, block->value->usable_size(),
+        &DeleteCachedEntry<Block>, &(block->cache_handle), priority);
+    block_cache->TEST_mark_as_data_block(block_cache_key,
+                                         block->value->usable_size());
     if (s.ok()) {
       assert(block->cache_handle != nullptr);
       if (get_context != nullptr) {
         get_context->RecordCounters(BLOCK_CACHE_ADD, 1);
-        get_context->RecordCounters(BLOCK_CACHE_BYTES_WRITE, charge);
+        get_context->RecordCounters(BLOCK_CACHE_BYTES_WRITE,
+                                    block->value->usable_size());
       } else {
         RecordTick(statistics, BLOCK_CACHE_ADD);
-        RecordTick(statistics, BLOCK_CACHE_BYTES_WRITE, charge);
+        RecordTick(statistics, BLOCK_CACHE_BYTES_WRITE,
+                   block->value->usable_size());
       }
       if (is_index) {
         if (get_context != nullptr) {
           get_context->RecordCounters(BLOCK_CACHE_INDEX_ADD, 1);
-          get_context->RecordCounters(BLOCK_CACHE_INDEX_BYTES_INSERT, charge);
+          get_context->RecordCounters(BLOCK_CACHE_INDEX_BYTES_INSERT,
+                                      block->value->usable_size());
         } else {
           RecordTick(statistics, BLOCK_CACHE_INDEX_ADD);
-          RecordTick(statistics, BLOCK_CACHE_INDEX_BYTES_INSERT, charge);
+          RecordTick(statistics, BLOCK_CACHE_INDEX_BYTES_INSERT,
+                     block->value->usable_size());
         }
       } else {
         if (get_context != nullptr) {
           get_context->RecordCounters(BLOCK_CACHE_DATA_ADD, 1);
-          get_context->RecordCounters(BLOCK_CACHE_DATA_BYTES_INSERT, charge);
+          get_context->RecordCounters(BLOCK_CACHE_DATA_BYTES_INSERT,
+                                      block->value->usable_size());
         } else {
           RecordTick(statistics, BLOCK_CACHE_DATA_ADD);
-          RecordTick(statistics, BLOCK_CACHE_DATA_BYTES_INSERT, charge);
+          RecordTick(statistics, BLOCK_CACHE_DATA_BYTES_INSERT,
+                     block->value->usable_size());
         }
       }
       assert(reinterpret_cast<Block*>(
@@ -1344,9 +1305,7 @@ FilterBlockReader* BlockBasedTable::ReadFilter(
       return new PartitionedFilterBlockReader(
           rep->prefix_filtering ? prefix_extractor : nullptr,
           rep->whole_key_filtering, std::move(block), nullptr,
-          rep->ioptions.statistics, rep->internal_comparator, this,
-          rep_->table_properties == nullptr ||
-              !rep_->table_properties->index_key_is_user_key);
+          rep->ioptions.statistics, rep->internal_comparator, this);
     }
 
     case Rep::FilterType::kBlockFilter:
@@ -1428,23 +1387,24 @@ BlockBasedTable::CachableEntry<FilterBlockReader> BlockBasedTable::GetFilter(
     filter = ReadFilter(prefetch_buffer, filter_blk_handle,
                         is_a_filter_partition, prefix_extractor);
     if (filter != nullptr) {
-      size_t usage = filter->ApproximateMemoryUsage();
       Status s = block_cache->Insert(
-          key, filter, usage, &DeleteCachedFilterEntry, &cache_handle,
+          key, filter, filter->size(), &DeleteCachedFilterEntry, &cache_handle,
           rep_->table_options.cache_index_and_filter_blocks_with_high_priority
               ? Cache::Priority::HIGH
               : Cache::Priority::LOW);
       if (s.ok()) {
         if (get_context != nullptr) {
           get_context->RecordCounters(BLOCK_CACHE_ADD, 1);
-          get_context->RecordCounters(BLOCK_CACHE_BYTES_WRITE, usage);
+          get_context->RecordCounters(BLOCK_CACHE_BYTES_WRITE, filter->size());
           get_context->RecordCounters(BLOCK_CACHE_FILTER_ADD, 1);
-          get_context->RecordCounters(BLOCK_CACHE_FILTER_BYTES_INSERT, usage);
+          get_context->RecordCounters(BLOCK_CACHE_FILTER_BYTES_INSERT,
+                                      filter->size());
         } else {
           RecordTick(statistics, BLOCK_CACHE_ADD);
-          RecordTick(statistics, BLOCK_CACHE_BYTES_WRITE, usage);
+          RecordTick(statistics, BLOCK_CACHE_BYTES_WRITE, filter->size());
           RecordTick(statistics, BLOCK_CACHE_FILTER_ADD);
-          RecordTick(statistics, BLOCK_CACHE_FILTER_BYTES_INSERT, usage);
+          RecordTick(statistics, BLOCK_CACHE_FILTER_BYTES_INSERT,
+                     filter->size());
         }
       } else {
         RecordTick(statistics, BLOCK_CACHE_ADD_FAILURES);
@@ -1510,27 +1470,27 @@ InternalIterator* BlockBasedTable::NewIndexIterator(
     TEST_SYNC_POINT("BlockBasedTable::NewIndexIterator::thread1:1");
     TEST_SYNC_POINT("BlockBasedTable::NewIndexIterator::thread2:3");
     TEST_SYNC_POINT("BlockBasedTable::NewIndexIterator::thread1:4");
-    size_t charge = 0;
     if (s.ok()) {
       assert(index_reader != nullptr);
-      charge = index_reader->ApproximateMemoryUsage();
       s = block_cache->Insert(
-          key, index_reader, charge, &DeleteCachedIndexEntry, &cache_handle,
+          key, index_reader, index_reader->usable_size(),
+          &DeleteCachedIndexEntry, &cache_handle,
           rep_->table_options.cache_index_and_filter_blocks_with_high_priority
               ? Cache::Priority::HIGH
               : Cache::Priority::LOW);
     }
 
     if (s.ok()) {
+      size_t usable_size = index_reader->usable_size();
       if (get_context != nullptr) {
         get_context->RecordCounters(BLOCK_CACHE_ADD, 1);
-        get_context->RecordCounters(BLOCK_CACHE_BYTES_WRITE, charge);
+        get_context->RecordCounters(BLOCK_CACHE_BYTES_WRITE, usable_size);
       } else {
         RecordTick(statistics, BLOCK_CACHE_ADD);
-        RecordTick(statistics, BLOCK_CACHE_BYTES_WRITE, charge);
+        RecordTick(statistics, BLOCK_CACHE_BYTES_WRITE, usable_size);
       }
       RecordTick(statistics, BLOCK_CACHE_INDEX_ADD);
-      RecordTick(statistics, BLOCK_CACHE_INDEX_BYTES_INSERT, charge);
+      RecordTick(statistics, BLOCK_CACHE_INDEX_BYTES_INSERT, usable_size);
     } else {
       if (index_reader != nullptr) {
         delete index_reader;
@@ -1615,12 +1575,11 @@ BlockIter* BlockBasedTable::NewDataBlockIterator(
     {
       StopWatch sw(rep->ioptions.env, rep->ioptions.statistics,
                    READ_BLOCK_GET_MICROS);
-      s = ReadBlockFromFile(
-          rep->file.get(), nullptr /* prefetch_buffer */, rep->footer, ro,
-          handle, &block_value, rep->ioptions, rep->blocks_maybe_compressed,
-          compression_dict, rep->persistent_cache_options,
-          is_index ? kDisableGlobalSequenceNumber : rep->global_seqno,
-          rep->table_options.read_amp_bytes_per_bit);
+      s = ReadBlockFromFile(rep->file.get(), nullptr /* prefetch_buffer */,
+                            rep->footer, ro, handle, &block_value, rep->ioptions,
+                            rep->blocks_maybe_compressed, compression_dict,
+                            rep->persistent_cache_options, rep->global_seqno,
+                            rep->table_options.read_amp_bytes_per_bit);
     }
     if (s.ok()) {
       block.value = block_value.release();
@@ -1657,9 +1616,8 @@ BlockIter* BlockBasedTable::NewDataBlockIterator(
                static_cast<int>(kExtraCacheKeyPrefix + kMaxVarint64Length));
         Slice unique_key =
             Slice(cache_key, static_cast<size_t>(end - cache_key));
-        s = block_cache->Insert(unique_key, nullptr,
-                                block.value->ApproximateMemoryUsage(), nullptr,
-                                &cache_handle);
+        s = block_cache->Insert(unique_key, nullptr, block.value->usable_size(),
+                                nullptr, &cache_handle);
         if (s.ok()) {
           if (cache_handle != nullptr) {
             iter->RegisterCleanup(&ForceReleaseCachedEntry, block_cache,
@@ -1720,8 +1678,7 @@ Status BlockBasedTable::MaybeLoadDataBlockToCache(
             rep->file.get(), prefetch_buffer, rep->footer, ro, handle,
             &raw_block, rep->ioptions,
             block_cache_compressed == nullptr && rep->blocks_maybe_compressed,
-            compression_dict, rep->persistent_cache_options,
-            is_index ? kDisableGlobalSequenceNumber : rep->global_seqno,
+            compression_dict, rep->persistent_cache_options, rep->global_seqno,
             rep->table_options.read_amp_bytes_per_bit);
       }
 
@@ -2103,7 +2060,7 @@ InternalIterator* BlockBasedTable::NewIterator(
     const ReadOptions& read_options, const SliceTransform* prefix_extractor,
     Arena* arena, bool skip_filters) {
   bool prefix_extractor_changed =
-      PrefixExtractorChanged(rep_->table_properties.get(), prefix_extractor);
+      PrefixExtractorChanged(rep_->table_properties, prefix_extractor);
   const bool kIsNotIndex = false;
   if (arena == nullptr) {
     return new BlockBasedTableIterator(
@@ -2211,8 +2168,8 @@ Status BlockBasedTable::Get(const ReadOptions& read_options, const Slice& key,
     // BlockPrefixIndex. Only do this check when index_type is kHashSearch.
     bool prefix_extractor_changed = false;
     if (rep_->index_type == BlockBasedTableOptions::kHashSearch) {
-      prefix_extractor_changed = PrefixExtractorChanged(
-          rep_->table_properties.get(), prefix_extractor);
+      prefix_extractor_changed =
+          PrefixExtractorChanged(rep_->table_properties, prefix_extractor);
     }
     auto iiter = NewIndexIterator(read_options, prefix_extractor_changed,
                                   &iiter_on_stack, /* index_entry */ nullptr,
@@ -2442,11 +2399,18 @@ bool BlockBasedTable::TEST_KeyInCache(const ReadOptions& options,
   return in_cache;
 }
 
-BlockBasedTableOptions::IndexType BlockBasedTable::UpdateIndexType() {
+// REQUIRES: The following fields of rep_ should have already been populated:
+//  1. file
+//  2. index_handle,
+//  3. options
+//  4. internal_comparator
+//  5. index_type
+Status BlockBasedTable::CreateIndexReader(
+    FilePrefetchBuffer* prefetch_buffer, IndexReader** index_reader,
+    InternalIterator* preloaded_meta_index_iter, int level) {
   // Some old version of block-based tables don't have index type present in
   // table properties. If that's the case we can safely use the kBinarySearch.
-  BlockBasedTableOptions::IndexType index_type_on_file =
-      BlockBasedTableOptions::kBinarySearch;
+  auto index_type_on_file = BlockBasedTableOptions::kBinarySearch;
   if (rep_->table_properties) {
     auto& props = rep_->table_properties->user_collected_properties;
     auto pos = props.find(BlockBasedTablePropertyNames::kIndexType);
@@ -2457,19 +2421,6 @@ BlockBasedTableOptions::IndexType BlockBasedTable::UpdateIndexType() {
       rep_->index_type = index_type_on_file;
     }
   }
-  return index_type_on_file;
-}
-
-// REQUIRES: The following fields of rep_ should have already been populated:
-//  1. file
-//  2. index_handle,
-//  3. options
-//  4. internal_comparator
-//  5. index_type
-Status BlockBasedTable::CreateIndexReader(
-    FilePrefetchBuffer* prefetch_buffer, IndexReader** index_reader,
-    InternalIterator* preloaded_meta_index_iter, int level) {
-  auto index_type_on_file = UpdateIndexType();
 
   auto file = rep_->file.get();
   const InternalKeyComparator* icomparator = &rep_->internal_comparator;
@@ -2965,7 +2916,7 @@ void DeleteCachedFilterEntry(const Slice& /*key*/, void* value) {
   FilterBlockReader* filter = reinterpret_cast<FilterBlockReader*>(value);
   if (filter->statistics() != nullptr) {
     RecordTick(filter->statistics(), BLOCK_CACHE_FILTER_BYTES_EVICT,
-               filter->ApproximateMemoryUsage());
+               filter->size());
   }
   delete filter;
 }
@@ -2974,7 +2925,7 @@ void DeleteCachedIndexEntry(const Slice& /*key*/, void* value) {
   IndexReader* index_reader = reinterpret_cast<IndexReader*>(value);
   if (index_reader->statistics() != nullptr) {
     RecordTick(index_reader->statistics(), BLOCK_CACHE_INDEX_BYTES_EVICT,
-               index_reader->ApproximateMemoryUsage());
+               index_reader->usable_size());
   }
   delete index_reader;
 }

table/block_based_table_reader.h

@@ -325,9 +325,6 @@ class BlockBasedTable : public TableReader {
 
   void ReadMeta(const Footer& footer);
 
-  // Figure the index type, update it in rep_, and also return it.
-  BlockBasedTableOptions::IndexType UpdateIndexType();
-
   // Create a index reader based on the index type stored in the table.
   // Optionally, user can pass a preloaded meta_index_iter for the index that
   // need to access extra meta blocks for index construction. This parameter
@@ -484,12 +481,11 @@ struct BlockBasedTable::Rep {
   // block to extract prefix without knowing if a key is internal or not.
   unique_ptr<SliceTransform> internal_prefix_transform;
 
-  // only used in level 0 files when pin_l0_filter_and_index_blocks_in_cache is
-  // true or in all levels when pin_top_level_index_and_filter is set in
-  // combination with partitioned index/filters: then we do use the LRU cache,
-  // but we always keep the filter & index block's handle checked out here (=we
-  // don't call Release()), plus the parsed out objects the LRU cache will never
-  // push flush them out, hence they're pinned
+  // only used in level 0 files:
+  // when pin_l0_filter_and_index_blocks_in_cache is true, we do use the
+  // LRU cache, but we always keep the filter & idndex block's handle checked
+  // out here (=we don't call Release()), plus the parsed out objects
+  // the LRU cache will never push flush them out, hence they're pinned
   CachableEntry<FilterBlockReader> filter_entry;
   CachableEntry<IndexReader> index_entry;
   // range deletion meta-block is pinned through reader's lifetime when LRU

table/format.h

@@ -10,13 +10,6 @@
 #pragma once
 #include <stdint.h>
 #include <string>
-#ifdef ROCKSDB_MALLOC_USABLE_SIZE
-#ifdef OS_FREEBSD
-#include <malloc_np.h>
-#else
-#include <malloc.h>
-#endif
-#endif
 #include "rocksdb/options.h"
 #include "rocksdb/slice.h"
 #include "rocksdb/status.h"
@@ -90,7 +83,7 @@ inline uint32_t GetCompressFormatForVersion(
 }
 
 inline bool BlockBasedTableSupportedVersion(uint32_t version) {
-  return version <= 3;
+  return version <= 2;
 }
 
 // Footer encapsulates the fixed information stored at the tail
@@ -206,19 +199,6 @@ struct BlockContents {
         compression_type(_compression_type),
         allocation(std::move(_data)) {}
 
-  // The additional memory space taken by the block data.
-  size_t usable_size() const {
-    if (allocation.get() != nullptr) {
-#ifdef ROCKSDB_MALLOC_USABLE_SIZE
-      return malloc_usable_size(allocation.get());
-#else
-      return sizeof(*allocation.get());
-#endif  // ROCKSDB_MALLOC_USABLE_SIZE
-    } else {
-      return 0;  // no extra memory is occupied by the data
-    }
-  }
-
   BlockContents(BlockContents&& other) ROCKSDB_NOEXCEPT {
     *this = std::move(other);
   }

table/full_filter_block.cc

@@ -5,14 +5,6 @@
 
 #include "table/full_filter_block.h"
 
-#ifdef ROCKSDB_MALLOC_USABLE_SIZE
-#ifdef OS_FREEBSD
-#include <malloc_np.h>
-#else
-#include <malloc.h>
-#endif
-#endif
-
 #include "monitoring/perf_context_imp.h"
 #include "port/port.h"
 #include "rocksdb/filter_policy.h"
@@ -80,14 +72,8 @@ inline void FullFilterBlockBuilder::AddPrefix(const Slice& key) {
   }
 }
 
-void FullFilterBlockBuilder::Reset() {
-  last_whole_key_recorded_ = false;
-  last_prefix_recorded_ = false;
-}
-
 Slice FullFilterBlockBuilder::Finish(const BlockHandle& /*tmp*/,
                                      Status* status) {
-  Reset();
   // In this impl we ignore BlockHandle
   *status = Status::OK();
   if (num_added_ != 0) {
@@ -159,14 +145,6 @@ bool FullFilterBlockReader::MayMatch(const Slice& entry) {
 }
 
 size_t FullFilterBlockReader::ApproximateMemoryUsage() const {
-  size_t usage = block_contents_.usable_size();
-#ifdef ROCKSDB_MALLOC_USABLE_SIZE
-  usage += malloc_usable_size((void*)this);
-  usage += malloc_usable_size(filter_bits_reader_.get());
-#else
-  usage += sizeof(*this);
-  usage += sizeof(*filter_bits_reader_.get());
-#endif  // ROCKSDB_MALLOC_USABLE_SIZE
-  return usage;
+  return contents_.size();
 }
 }  // namespace rocksdb

table/full_filter_block.h

@@ -52,7 +52,6 @@ class FullFilterBlockBuilder : public FilterBlockBuilder {
  protected:
   virtual void AddKey(const Slice& key);
   std::unique_ptr<FilterBitsBuilder> filter_bits_builder_;
-  virtual void Reset();
 
  private:
   // important: all of these might point to invalid addresses
@@ -114,6 +113,7 @@ class FullFilterBlockReader : public FilterBlockReader {
   Slice contents_;
   std::unique_ptr<FilterBitsReader> filter_bits_reader_;
   BlockContents block_contents_;
+  std::unique_ptr<const char[]> filter_data_;
 
   // No copying allowed
   FullFilterBlockReader(const FullFilterBlockReader&);

table/index_builder.cc

@@ -66,8 +66,6 @@ PartitionedIndexBuilder::PartitionedIndexBuilder(
     : IndexBuilder(comparator),
       index_block_builder_(table_opt.index_block_restart_interval,
                            table_opt.format_version),
-      index_block_builder_without_seq_(table_opt.index_block_restart_interval,
-                                       table_opt.format_version),
       sub_index_builder_(nullptr),
       table_opt_(table_opt),
       seperator_is_key_plus_seq_(false) {}
@@ -151,20 +149,11 @@ Status PartitionedIndexBuilder::Finish(
     std::string handle_encoding;
     last_partition_block_handle.EncodeTo(&handle_encoding);
     index_block_builder_.Add(last_entry.key, handle_encoding);
-    if (!seperator_is_key_plus_seq_) {
-      index_block_builder_without_seq_.Add(ExtractUserKey(last_entry.key),
-                                           handle_encoding);
-    }
     entries_.pop_front();
   }
   // If there is no sub_index left, then return the 2nd level index.
   if (UNLIKELY(entries_.empty())) {
-    if (seperator_is_key_plus_seq_) {
-      index_blocks->index_block_contents = index_block_builder_.Finish();
-    } else {
-      index_blocks->index_block_contents =
-          index_block_builder_without_seq_.Finish();
-    }
+    index_blocks->index_block_contents = index_block_builder_.Finish();
     return Status::OK();
   } else {
     // Finish the next partition index in line and Incomplete() to indicate we
@@ -203,9 +192,7 @@ size_t PartitionedIndexBuilder::EstimateTopLevelIndexSize(
     uint64_t size = it->value->EstimatedSize();
     BlockHandle tmp_block_handle(offset, size);
     tmp_block_handle.EncodeTo(&tmp_handle_encoding);
-    tmp_builder.Add(
-        seperator_is_key_plus_seq_ ? it->key : ExtractUserKey(it->key),
-        tmp_handle_encoding);
+    tmp_builder.Add(it->key, tmp_handle_encoding);
     offset += size;
   }
   return tmp_builder.CurrentSizeEstimate();

table/index_builder.h

@@ -368,7 +368,6 @@ class PartitionedIndexBuilder : public IndexBuilder {
   };
   std::list<Entry> entries_;  // list of partitioned indexes and their keys
   BlockBuilder index_block_builder_;  // top-level index builder
-  BlockBuilder index_block_builder_without_seq_;  // same for user keys
   // the active partition index builder
   ShortenedIndexBuilder* sub_index_builder_;
   // the last key in the active partition index builder

table/merging_iterator.cc

@@ -228,12 +228,28 @@ class MergingIterator : public InternalIterator {
       Slice target = key();
       for (auto& child : children_) {
         if (&child != current_) {
-          child.SeekForPrev(target);
-          TEST_SYNC_POINT_CALLBACK("MergeIterator::Prev:BeforePrev", &child);
-          considerStatus(child.status());
-          if (child.Valid() && comparator_->Equal(target, child.key())) {
-            child.Prev();
+          if (!prefix_seek_mode_) {
+            child.Seek(target);
+            if (child.Valid()) {
+              // Child is at first entry >= key().  Step back one to be < key()
+              TEST_SYNC_POINT_CALLBACK("MergeIterator::Prev:BeforePrev",
+                                       &child);
+              assert(child.status().ok());
+              child.Prev();
+            } else {
+              // Child has no entries >= key().  Position at last entry.
+              TEST_SYNC_POINT("MergeIterator::Prev:BeforeSeekToLast");
+              considerStatus(child.status());
+              child.SeekToLast();
+            }
             considerStatus(child.status());
+          } else {
+            child.SeekForPrev(target);
+            considerStatus(child.status());
+            if (child.Valid() && comparator_->Equal(target, child.key())) {
+              child.Prev();
+              considerStatus(child.status());
+            }
           }
         }
         if (child.Valid()) {

table/partitioned_filter_block.cc

@@ -5,13 +5,6 @@
 
 #include "table/partitioned_filter_block.h"
 
-#ifdef ROCKSDB_MALLOC_USABLE_SIZE
-#ifdef OS_FREEBSD
-#include <malloc_np.h>
-#else
-#include <malloc.h>
-#endif
-#endif
 #include <utility>
 
 #include "monitoring/perf_context_imp.h"
@@ -31,7 +24,6 @@ PartitionedFilterBlockBuilder::PartitionedFilterBlockBuilder(
     : FullFilterBlockBuilder(prefix_extractor, whole_key_filtering,
                              filter_bits_builder),
       index_on_filter_block_builder_(index_block_restart_interval),
-      index_on_filter_block_builder_without_seq_(index_block_restart_interval),
       p_index_builder_(p_index_builder),
       filters_in_partition_(0),
       num_added_(0) {
@@ -56,7 +48,6 @@ void PartitionedFilterBlockBuilder::MaybeCutAFilterBlock() {
   std::string& index_key = p_index_builder_->GetPartitionKey();
   filters.push_back({index_key, filter});
   filters_in_partition_ = 0;
-  Reset();
 }
 
 void PartitionedFilterBlockBuilder::AddKey(const Slice& key) {
@@ -74,10 +65,6 @@ Slice PartitionedFilterBlockBuilder::Finish(
     std::string handle_encoding;
     last_partition_block_handle.EncodeTo(&handle_encoding);
     index_on_filter_block_builder_.Add(last_entry.key, handle_encoding);
-    if (!p_index_builder_->seperator_is_key_plus_seq()) {
-      index_on_filter_block_builder_without_seq_.Add(
-          ExtractUserKey(last_entry.key), handle_encoding);
-    }
     filters.pop_front();
   } else {
     MaybeCutAFilterBlock();
@@ -87,11 +74,7 @@ Slice PartitionedFilterBlockBuilder::Finish(
   if (UNLIKELY(filters.empty())) {
     *status = Status::OK();
     if (finishing_filters) {
-      if (p_index_builder_->seperator_is_key_plus_seq()) {
-        return index_on_filter_block_builder_.Finish();
-      } else {
-        return index_on_filter_block_builder_without_seq_.Finish();
-      }
+      return index_on_filter_block_builder_.Finish();
     } else {
       // This is the rare case where no key was added to the filter
       return Slice();
@@ -108,13 +91,12 @@ Slice PartitionedFilterBlockBuilder::Finish(
 PartitionedFilterBlockReader::PartitionedFilterBlockReader(
     const SliceTransform* prefix_extractor, bool _whole_key_filtering,
     BlockContents&& contents, FilterBitsReader* /*filter_bits_reader*/,
-    Statistics* stats, const InternalKeyComparator comparator,
-    const BlockBasedTable* table, const bool index_key_includes_seq)
+    Statistics* stats, const Comparator& comparator,
+    const BlockBasedTable* table)
     : FilterBlockReader(contents.data.size(), stats, _whole_key_filtering),
       prefix_extractor_(prefix_extractor),
       comparator_(comparator),
-      table_(table),
-      index_key_includes_seq_(index_key_includes_seq) {
+      table_(table) {
   idx_on_fltr_blk_.reset(new Block(std::move(contents),
                                    kDisableGlobalSequenceNumber,
                                    0 /* read_amp_bytes_per_bit */, stats));
@@ -131,8 +113,7 @@ PartitionedFilterBlockReader::~PartitionedFilterBlockReader() {
   char cache_key[BlockBasedTable::kMaxCacheKeyPrefixSize + kMaxVarint64Length];
   BlockIter biter;
   BlockHandle handle;
-  idx_on_fltr_blk_->NewIterator(&comparator_, comparator_.user_comparator(),
-                                &biter, true, nullptr, index_key_includes_seq_);
+  idx_on_fltr_blk_->NewIterator(&comparator_, &comparator_, &biter, true);
   biter.SeekToFirst();
   for (; biter.Valid(); biter.Next()) {
     auto input = biter.value();
@@ -226,8 +207,7 @@ bool PartitionedFilterBlockReader::PrefixMayMatch(
 Slice PartitionedFilterBlockReader::GetFilterPartitionHandle(
     const Slice& entry) {
   BlockIter iter;
-  idx_on_fltr_blk_->NewIterator(&comparator_, comparator_.user_comparator(),
-                                &iter, true, nullptr, index_key_includes_seq_);
+  idx_on_fltr_blk_->NewIterator(&comparator_, &comparator_, &iter, true);
   iter.Seek(entry);
   if (UNLIKELY(!iter.Valid())) {
     return Slice();
@@ -272,14 +252,7 @@ PartitionedFilterBlockReader::GetFilterPartition(
 }
 
 size_t PartitionedFilterBlockReader::ApproximateMemoryUsage() const {
-  size_t usage = idx_on_fltr_blk_->usable_size();
-#ifdef ROCKSDB_MALLOC_USABLE_SIZE
-  usage += malloc_usable_size((void*)this);
-#else
-  usage += sizeof(*this);
-#endif  // ROCKSDB_MALLOC_USABLE_SIZE
-  return usage;
-  // TODO(myabandeh): better estimation for filter_map_ size
+  return idx_on_fltr_blk_->size();
 }
 
 // Release the cached entry and decrement its ref count.
@@ -296,8 +269,7 @@ void PartitionedFilterBlockReader::CacheDependencies(
   auto rep = table_->rep_;
   BlockIter biter;
   BlockHandle handle;
-  idx_on_fltr_blk_->NewIterator(&comparator_, comparator_.user_comparator(),
-                                &biter, true, nullptr, index_key_includes_seq_);
+  idx_on_fltr_blk_->NewIterator(&comparator_, &comparator_, &biter, true);
   // Index partitions are assumed to be consecuitive. Prefetch them all.
   // Read the first block offset
   biter.SeekToFirst();

table/partitioned_filter_block.h

@@ -41,8 +41,6 @@ class PartitionedFilterBlockBuilder : public FullFilterBlockBuilder {
  private:
   // Filter data
   BlockBuilder index_on_filter_block_builder_;  // top-level index builder
-  BlockBuilder
-      index_on_filter_block_builder_without_seq_;  // same for user keys
   struct FilterEntry {
     std::string key;
     Slice filter;
@@ -70,11 +68,13 @@ class PartitionedFilterBlockBuilder : public FullFilterBlockBuilder {
 class PartitionedFilterBlockReader : public FilterBlockReader,
                                      public Cleanable {
  public:
-  explicit PartitionedFilterBlockReader(
-      const SliceTransform* prefix_extractor, bool whole_key_filtering,
-      BlockContents&& contents, FilterBitsReader* filter_bits_reader,
-      Statistics* stats, const InternalKeyComparator comparator,
-      const BlockBasedTable* table, const bool index_key_includes_seq);
+  explicit PartitionedFilterBlockReader(const SliceTransform* prefix_extractor,
+                                        bool whole_key_filtering,
+                                        BlockContents&& contents,
+                                        FilterBitsReader* filter_bits_reader,
+                                        Statistics* stats,
+                                        const Comparator& comparator,
+                                        const BlockBasedTable* table);
   virtual ~PartitionedFilterBlockReader();
 
   virtual bool IsBlockBased() override { return false; }
@@ -98,9 +98,8 @@ class PartitionedFilterBlockReader : public FilterBlockReader,
 
   const SliceTransform* prefix_extractor_;
   std::unique_ptr<Block> idx_on_fltr_blk_;
-  const InternalKeyComparator comparator_;
+  const Comparator& comparator_;
   const BlockBasedTable* table_;
-  const bool index_key_includes_seq_;
   std::unordered_map<uint64_t,
                      BlockBasedTable::CachableEntry<FilterBlockReader>>
       filter_map_;

table/partitioned_filter_block_test.cc

@@ -111,7 +111,7 @@ class PartitionedFilterBlockTest : public testing::Test {
   std::unique_ptr<MockedBlockBasedTable> table;
 
   PartitionedFilterBlockReader* NewReader(
-      PartitionedFilterBlockBuilder* builder, PartitionedIndexBuilder* pib) {
+      PartitionedFilterBlockBuilder* builder) {
     BlockHandle bh;
     Status status;
     Slice slice;
@@ -127,14 +127,13 @@ class PartitionedFilterBlockTest : public testing::Test {
         ioptions, env_options, table_options_, icomp, false)));
     auto reader = new PartitionedFilterBlockReader(
         nullptr, true, BlockContents(slice, false, kNoCompression), nullptr,
-        nullptr, icomp, table.get(), pib->seperator_is_key_plus_seq());
+        nullptr, *icomp.user_comparator(), table.get());
     return reader;
   }
 
   void VerifyReader(PartitionedFilterBlockBuilder* builder,
-                    PartitionedIndexBuilder* pib, bool empty = false) {
-    std::unique_ptr<PartitionedFilterBlockReader> reader(
-        NewReader(builder, pib));
+                    bool empty = false) {
+    std::unique_ptr<PartitionedFilterBlockReader> reader(NewReader(builder));
     // Querying added keys
     const bool no_io = true;
     for (auto key : keys) {
@@ -183,7 +182,7 @@ class PartitionedFilterBlockTest : public testing::Test {
     builder->Add(keys[i]);
     CutABlock(pib.get(), keys[i]);
 
-    VerifyReader(builder.get(), pib.get());
+    VerifyReader(builder.get());
     return CountNumOfIndexPartitions(pib.get());
   }
 
@@ -203,7 +202,7 @@ class PartitionedFilterBlockTest : public testing::Test {
     builder->Add(keys[i]);
     CutABlock(pib.get(), keys[i]);
 
-    VerifyReader(builder.get(), pib.get());
+    VerifyReader(builder.get());
   }
 
   void TestBlockPerAllKeys() {
@@ -221,7 +220,7 @@ class PartitionedFilterBlockTest : public testing::Test {
     builder->Add(keys[i]);
     CutABlock(pib.get(), keys[i]);
 
-    VerifyReader(builder.get(), pib.get());
+    VerifyReader(builder.get());
   }
 
   void CutABlock(PartitionedIndexBuilder* builder,
@@ -262,7 +261,7 @@ TEST_F(PartitionedFilterBlockTest, EmptyBuilder) {
   std::unique_ptr<PartitionedIndexBuilder> pib(NewIndexBuilder());
   std::unique_ptr<PartitionedFilterBlockBuilder> builder(NewBuilder(pib.get()));
   const bool empty = true;
-  VerifyReader(builder.get(), pib.get(), empty);
+  VerifyReader(builder.get(), empty);
 }
 
 TEST_F(PartitionedFilterBlockTest, OneBlock) {

table/table_test.cc

@@ -1049,32 +1049,13 @@ class TableTest : public testing::Test {
 };
 
 class GeneralTableTest : public TableTest {};
-class BlockBasedTableTest
-    : public TableTest,
-      virtual public ::testing::WithParamInterface<uint32_t> {
- public:
-  BlockBasedTableTest() : format_(GetParam()) {}
-
-  BlockBasedTableOptions GetBlockBasedTableOptions() {
-    BlockBasedTableOptions options;
-    options.format_version = format_;
-    return options;
-  }
-
+class BlockBasedTableTest : public TableTest {
  protected:
   uint64_t IndexUncompressedHelper(bool indexCompress);
-
- private:
-  uint32_t format_;
 };
 class PlainTableTest : public TableTest {};
 class TablePropertyTest : public testing::Test {};
 
-INSTANTIATE_TEST_CASE_P(FormatDef, BlockBasedTableTest,
-                        testing::Values(test::kDefaultFormatVersion));
-INSTANTIATE_TEST_CASE_P(FormatLatest, BlockBasedTableTest,
-                        testing::Values(test::kLatestFormatVersion));
-
 // This test serves as the living tutorial for the prefix scan of user collected
 // properties.
 TEST_F(TablePropertyTest, PrefixScanTest) {
@@ -1114,7 +1095,7 @@ TEST_F(TablePropertyTest, PrefixScanTest) {
 
 // This test include all the basic checks except those for index size and block
 // size, which will be conducted in separated unit tests.
-TEST_P(BlockBasedTableTest, BasicBlockBasedTableProperties) {
+TEST_F(BlockBasedTableTest, BasicBlockBasedTableProperties) {
   TableConstructor c(BytewiseComparator(), true /* convert_to_internal_key_ */);
 
   c.Add("a1", "val1");
@@ -1134,7 +1115,7 @@ TEST_P(BlockBasedTableTest, BasicBlockBasedTableProperties) {
   options.compression = kNoCompression;
   options.statistics = CreateDBStatistics();
   options.statistics->stats_level_ = StatsLevel::kAll;
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+  BlockBasedTableOptions table_options;
   table_options.block_restart_interval = 1;
   options.table_factory.reset(NewBlockBasedTableFactory(table_options));
 
@@ -1182,7 +1163,7 @@ uint64_t BlockBasedTableTest::IndexUncompressedHelper(bool compressed) {
   options.compression = kSnappyCompression;
   options.statistics = CreateDBStatistics();
   options.statistics->stats_level_ = StatsLevel::kAll;
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+  BlockBasedTableOptions table_options;
   table_options.block_restart_interval = 1;
   table_options.enable_index_compression = compressed;
   options.table_factory.reset(NewBlockBasedTableFactory(table_options));
@@ -1195,7 +1176,7 @@ uint64_t BlockBasedTableTest::IndexUncompressedHelper(bool compressed) {
   c.ResetTableReader();
   return options.statistics->getTickerCount(NUMBER_BLOCK_COMPRESSED);
 }
-TEST_P(BlockBasedTableTest, IndexUncompressed) {
+TEST_F(BlockBasedTableTest, IndexUncompressed) {
   uint64_t tbl1_compressed_cnt = IndexUncompressedHelper(true);
   uint64_t tbl2_compressed_cnt = IndexUncompressedHelper(false);
   // tbl1_compressed_cnt should include 1 index block
@@ -1203,7 +1184,7 @@ TEST_P(BlockBasedTableTest, IndexUncompressed) {
 }
 #endif  // SNAPPY
 
-TEST_P(BlockBasedTableTest, BlockBasedTableProperties2) {
+TEST_F(BlockBasedTableTest, BlockBasedTableProperties2) {
   TableConstructor c(&reverse_key_comparator);
   std::vector<std::string> keys;
   stl_wrappers::KVMap kvmap;
@@ -1211,7 +1192,7 @@ TEST_P(BlockBasedTableTest, BlockBasedTableProperties2) {
   {
     Options options;
     options.compression = CompressionType::kNoCompression;
-    BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+    BlockBasedTableOptions table_options;
     options.table_factory.reset(NewBlockBasedTableFactory(table_options));
 
     const ImmutableCFOptions ioptions(options);
@@ -1238,7 +1219,7 @@ TEST_P(BlockBasedTableTest, BlockBasedTableProperties2) {
 
   {
     Options options;
-    BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+    BlockBasedTableOptions table_options;
     options.table_factory.reset(NewBlockBasedTableFactory(table_options));
     options.comparator = &reverse_key_comparator;
     options.merge_operator = MergeOperators::CreateUInt64AddOperator();
@@ -1265,7 +1246,7 @@ TEST_P(BlockBasedTableTest, BlockBasedTableProperties2) {
   }
 }
 
-TEST_P(BlockBasedTableTest, RangeDelBlock) {
+TEST_F(BlockBasedTableTest, RangeDelBlock) {
   TableConstructor c(BytewiseComparator());
   std::vector<std::string> keys = {"1pika", "2chu"};
   std::vector<std::string> vals = {"p", "c"};
@@ -1280,7 +1261,7 @@ TEST_P(BlockBasedTableTest, RangeDelBlock) {
   stl_wrappers::KVMap kvmap;
   Options options;
   options.compression = kNoCompression;
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+  BlockBasedTableOptions table_options;
   table_options.block_restart_interval = 1;
   options.table_factory.reset(NewBlockBasedTableFactory(table_options));
 
@@ -1316,12 +1297,12 @@ TEST_P(BlockBasedTableTest, RangeDelBlock) {
   }
 }
 
-TEST_P(BlockBasedTableTest, FilterPolicyNameProperties) {
+TEST_F(BlockBasedTableTest, FilterPolicyNameProperties) {
   TableConstructor c(BytewiseComparator(), true /* convert_to_internal_key_ */);
   c.Add("a1", "val1");
   std::vector<std::string> keys;
   stl_wrappers::KVMap kvmap;
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+  BlockBasedTableOptions table_options;
   table_options.filter_policy.reset(NewBloomFilterPolicy(10));
   Options options;
   options.table_factory.reset(NewBlockBasedTableFactory(table_options));
@@ -1405,14 +1386,14 @@ void PrefetchRange(TableConstructor* c, Options* opt,
   c->ResetTableReader();
 }
 
-TEST_P(BlockBasedTableTest, PrefetchTest) {
+TEST_F(BlockBasedTableTest, PrefetchTest) {
   // The purpose of this test is to test the prefetching operation built into
   // BlockBasedTable.
   Options opt;
   unique_ptr<InternalKeyComparator> ikc;
   ikc.reset(new test::PlainInternalKeyComparator(opt.comparator));
   opt.compression = kNoCompression;
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+  BlockBasedTableOptions table_options;
   table_options.block_size = 1024;
   // big enough so we don't ever lose cached values.
   table_options.block_cache = NewLRUCache(16 * 1024 * 1024, 4);
@@ -1473,8 +1454,8 @@ TEST_P(BlockBasedTableTest, PrefetchTest) {
   c.ResetTableReader();
 }
 
-TEST_P(BlockBasedTableTest, TotalOrderSeekOnHashIndex) {
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+TEST_F(BlockBasedTableTest, TotalOrderSeekOnHashIndex) {
+  BlockBasedTableOptions table_options;
   for (int i = 0; i < 4; ++i) {
     Options options;
     // Make each key/value an individual block
@@ -1565,8 +1546,8 @@ TEST_P(BlockBasedTableTest, TotalOrderSeekOnHashIndex) {
   }
 }
 
-TEST_P(BlockBasedTableTest, NoopTransformSeek) {
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+TEST_F(BlockBasedTableTest, NoopTransformSeek) {
+  BlockBasedTableOptions table_options;
   table_options.filter_policy.reset(NewBloomFilterPolicy(10));
 
   Options options;
@@ -1602,10 +1583,10 @@ TEST_P(BlockBasedTableTest, NoopTransformSeek) {
   }
 }
 
-TEST_P(BlockBasedTableTest, SkipPrefixBloomFilter) {
+TEST_F(BlockBasedTableTest, SkipPrefixBloomFilter) {
   // if DB is opened with a prefix extractor of a different name,
   // prefix bloom is skipped when read the file
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+  BlockBasedTableOptions table_options;
   table_options.filter_policy.reset(NewBloomFilterPolicy(2));
   table_options.whole_key_filtering = false;
 
@@ -1769,24 +1750,24 @@ void TableTest::IndexTest(BlockBasedTableOptions table_options) {
   c.ResetTableReader();
 }
 
-TEST_P(BlockBasedTableTest, BinaryIndexTest) {
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+TEST_F(TableTest, BinaryIndexTest) {
+  BlockBasedTableOptions table_options;
   table_options.index_type = BlockBasedTableOptions::kBinarySearch;
   IndexTest(table_options);
 }
 
-TEST_P(BlockBasedTableTest, HashIndexTest) {
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+TEST_F(TableTest, HashIndexTest) {
+  BlockBasedTableOptions table_options;
   table_options.index_type = BlockBasedTableOptions::kHashSearch;
   IndexTest(table_options);
 }
 
-TEST_P(BlockBasedTableTest, PartitionIndexTest) {
+TEST_F(TableTest, PartitionIndexTest) {
   const int max_index_keys = 5;
   const int est_max_index_key_value_size = 32;
   const int est_max_index_size = max_index_keys * est_max_index_key_value_size;
   for (int i = 1; i <= est_max_index_size + 1; i++) {
-    BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+    BlockBasedTableOptions table_options;
     table_options.index_type = BlockBasedTableOptions::kTwoLevelIndexSearch;
     table_options.metadata_block_size = i;
     IndexTest(table_options);
@@ -1796,7 +1777,7 @@ TEST_P(BlockBasedTableTest, PartitionIndexTest) {
 // It's very hard to figure out the index block size of a block accurately.
 // To make sure we get the index size, we just make sure as key number
 // grows, the filter block size also grows.
-TEST_P(BlockBasedTableTest, IndexSizeStat) {
+TEST_F(BlockBasedTableTest, IndexSizeStat) {
   uint64_t last_index_size = 0;
 
   // we need to use random keys since the pure human readable texts
@@ -1822,7 +1803,7 @@ TEST_P(BlockBasedTableTest, IndexSizeStat) {
     stl_wrappers::KVMap kvmap;
     Options options;
     options.compression = kNoCompression;
-    BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+    BlockBasedTableOptions table_options;
     table_options.block_restart_interval = 1;
     options.table_factory.reset(NewBlockBasedTableFactory(table_options));
 
@@ -1837,12 +1818,12 @@ TEST_P(BlockBasedTableTest, IndexSizeStat) {
   }
 }
 
-TEST_P(BlockBasedTableTest, NumBlockStat) {
+TEST_F(BlockBasedTableTest, NumBlockStat) {
   Random rnd(test::RandomSeed());
   TableConstructor c(BytewiseComparator(), true /* convert_to_internal_key_ */);
   Options options;
   options.compression = kNoCompression;
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+  BlockBasedTableOptions table_options;
   table_options.block_restart_interval = 1;
   table_options.block_size = 1000;
   options.table_factory.reset(NewBlockBasedTableFactory(table_options));
@@ -1929,11 +1910,11 @@ class BlockCachePropertiesSnapshot {
 
 // Make sure, by default, index/filter blocks were pre-loaded (meaning we won't
 // use block cache to store them).
-TEST_P(BlockBasedTableTest, BlockCacheDisabledTest) {
+TEST_F(BlockBasedTableTest, BlockCacheDisabledTest) {
   Options options;
   options.create_if_missing = true;
   options.statistics = CreateDBStatistics();
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+  BlockBasedTableOptions table_options;
   table_options.block_cache = NewLRUCache(1024, 4);
   table_options.filter_policy.reset(NewBloomFilterPolicy(10));
   options.table_factory.reset(new BlockBasedTableFactory(table_options));
@@ -1974,15 +1955,15 @@ TEST_P(BlockBasedTableTest, BlockCacheDisabledTest) {
 
 // Due to the difficulities of the intersaction between statistics, this test
 // only tests the case when "index block is put to block cache"
-TEST_P(BlockBasedTableTest, FilterBlockInBlockCache) {
+TEST_F(BlockBasedTableTest, FilterBlockInBlockCache) {
   // -- Table construction
   Options options;
   options.create_if_missing = true;
   options.statistics = CreateDBStatistics();
 
   // Enable the cache for index/filter blocks
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
-  table_options.block_cache = NewLRUCache(1024, 2);
+  BlockBasedTableOptions table_options;
+  table_options.block_cache = NewLRUCache(1024, 4);
   table_options.cache_index_and_filter_blocks = true;
   options.table_factory.reset(new BlockBasedTableFactory(table_options));
   std::vector<std::string> keys;
@@ -2166,7 +2147,7 @@ void ValidateBlockRestartInterval(int value, int expected) {
   delete factory;
 }
 
-TEST_P(BlockBasedTableTest, InvalidOptions) {
+TEST_F(BlockBasedTableTest, InvalidOptions) {
   // invalid values for block_size_deviation (<0 or >100) are silently set to 0
   ValidateBlockSizeDeviation(-10, 0);
   ValidateBlockSizeDeviation(-1, 0);
@@ -2186,7 +2167,7 @@ TEST_P(BlockBasedTableTest, InvalidOptions) {
   ValidateBlockRestartInterval(1000, 1000);
 }
 
-TEST_P(BlockBasedTableTest, BlockReadCountTest) {
+TEST_F(BlockBasedTableTest, BlockReadCountTest) {
   // bloom_filter_type = 0 -- block-based filter
   // bloom_filter_type = 0 -- full filter
   for (int bloom_filter_type = 0; bloom_filter_type < 2; ++bloom_filter_type) {
@@ -2195,7 +2176,7 @@ TEST_P(BlockBasedTableTest, BlockReadCountTest) {
       Options options;
       options.create_if_missing = true;
 
-      BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+      BlockBasedTableOptions table_options;
       table_options.block_cache = NewLRUCache(1, 0);
       table_options.cache_index_and_filter_blocks = index_and_filter_in_cache;
       table_options.filter_policy.reset(
@@ -2314,7 +2295,7 @@ std::map<std::string, size_t> MockCache::marked_data_in_cache_;
 // object depends on the table to be live, it then must be destructed before the
 // table is closed. This test makes sure that the only items remains in the
 // cache after the table is closed are raw data blocks.
-TEST_P(BlockBasedTableTest, NoObjectInCacheAfterTableClose) {
+TEST_F(BlockBasedTableTest, NoObjectInCacheAfterTableClose) {
   for (int level: {-1, 0, 1, 10}) {
   for (auto index_type :
        {BlockBasedTableOptions::IndexType::kBinarySearch,
@@ -2329,74 +2310,70 @@ TEST_P(BlockBasedTableTest, NoObjectInCacheAfterTableClose) {
         }
         for (bool index_and_filter_in_cache : {true, false}) {
           for (bool pin_l0 : {true, false}) {
-            for (bool pin_top_level : {true, false}) {
-              if (pin_l0 && !index_and_filter_in_cache) {
-                continue;
-              }
-              // Create a table
-              Options opt;
-              unique_ptr<InternalKeyComparator> ikc;
-              ikc.reset(new test::PlainInternalKeyComparator(opt.comparator));
-              opt.compression = kNoCompression;
-              BlockBasedTableOptions table_options =
-                  GetBlockBasedTableOptions();
-              table_options.block_size = 1024;
-              table_options.index_type =
-                  BlockBasedTableOptions::IndexType::kTwoLevelIndexSearch;
-              table_options.pin_l0_filter_and_index_blocks_in_cache = pin_l0;
-              table_options.pin_top_level_index_and_filter = pin_top_level;
-              table_options.partition_filters = partition_filter;
-              table_options.cache_index_and_filter_blocks =
-                  index_and_filter_in_cache;
-              // big enough so we don't ever lose cached values.
-              table_options.block_cache = std::shared_ptr<rocksdb::Cache>(
-                  new MockCache(16 * 1024 * 1024, 4, false, 0.0));
-              table_options.filter_policy.reset(
-                  rocksdb::NewBloomFilterPolicy(10, block_based_filter));
-              opt.table_factory.reset(NewBlockBasedTableFactory(table_options));
+            if (pin_l0 && !index_and_filter_in_cache) {
+              continue;
+            }
+            // Create a table
+            Options opt;
+            unique_ptr<InternalKeyComparator> ikc;
+            ikc.reset(new test::PlainInternalKeyComparator(opt.comparator));
+            opt.compression = kNoCompression;
+            BlockBasedTableOptions table_options;
+            table_options.block_size = 1024;
+            table_options.index_type =
+                BlockBasedTableOptions::IndexType::kTwoLevelIndexSearch;
+            table_options.pin_l0_filter_and_index_blocks_in_cache = pin_l0;
+            table_options.partition_filters = partition_filter;
+            table_options.cache_index_and_filter_blocks =
+                index_and_filter_in_cache;
+            // big enough so we don't ever lose cached values.
+            table_options.block_cache = std::shared_ptr<rocksdb::Cache>(
+                new MockCache(16 * 1024 * 1024, 4, false, 0.0));
+            table_options.filter_policy.reset(
+                rocksdb::NewBloomFilterPolicy(10, block_based_filter));
+            opt.table_factory.reset(NewBlockBasedTableFactory(table_options));
 
-              bool convert_to_internal_key = false;
-              TableConstructor c(BytewiseComparator(), convert_to_internal_key,
-                                 level);
-              std::string user_key = "k01";
-              std::string key =
-                  InternalKey(user_key, 0, kTypeValue).Encode().ToString();
-              c.Add(key, "hello");
-              std::vector<std::string> keys;
-              stl_wrappers::KVMap kvmap;
-              const ImmutableCFOptions ioptions(opt);
-              const MutableCFOptions moptions(opt);
-              c.Finish(opt, ioptions, moptions, table_options, *ikc, &keys,
-                       &kvmap);
+            bool convert_to_internal_key = false;
+            TableConstructor c(BytewiseComparator(), convert_to_internal_key,
+                               level);
+            std::string user_key = "k01";
+            std::string key =
+                InternalKey(user_key, 0, kTypeValue).Encode().ToString();
+            c.Add(key, "hello");
+            std::vector<std::string> keys;
+            stl_wrappers::KVMap kvmap;
+            const ImmutableCFOptions ioptions(opt);
+            const MutableCFOptions moptions(opt);
+            c.Finish(opt, ioptions, moptions, table_options, *ikc, &keys,
+                     &kvmap);
 
-              // Doing a read to make index/filter loaded into the cache
-              auto table_reader =
-                  dynamic_cast<BlockBasedTable*>(c.GetTableReader());
-              PinnableSlice value;
-              GetContext get_context(opt.comparator, nullptr, nullptr, nullptr,
-                                     GetContext::kNotFound, user_key, &value,
-                                     nullptr, nullptr, nullptr, nullptr);
-              InternalKey ikey(user_key, 0, kTypeValue);
-              auto s = table_reader->Get(ReadOptions(), key, &get_context,
-                                         moptions.prefix_extractor.get());
-              ASSERT_EQ(get_context.State(), GetContext::kFound);
-              ASSERT_STREQ(value.data(), "hello");
+            // Doing a read to make index/filter loaded into the cache
+            auto table_reader =
+                dynamic_cast<BlockBasedTable*>(c.GetTableReader());
+            PinnableSlice value;
+            GetContext get_context(opt.comparator, nullptr, nullptr, nullptr,
+                                   GetContext::kNotFound, user_key, &value,
+                                   nullptr, nullptr, nullptr, nullptr);
+            InternalKey ikey(user_key, 0, kTypeValue);
+            auto s = table_reader->Get(ReadOptions(), key, &get_context,
+                                       moptions.prefix_extractor.get());
+            ASSERT_EQ(get_context.State(), GetContext::kFound);
+            ASSERT_STREQ(value.data(), "hello");
 
-              // Close the table
-              c.ResetTableReader();
+            // Close the table
+            c.ResetTableReader();
 
-              auto usage = table_options.block_cache->GetUsage();
-              auto pinned_usage = table_options.block_cache->GetPinnedUsage();
-              // The only usage must be for marked data blocks
-              ASSERT_EQ(usage, MockCache::marked_size_);
-              // There must be some pinned data since PinnableSlice has not
-              // released them yet
-              ASSERT_GT(pinned_usage, 0);
-              // Release pinnable slice reousrces
-              value.Reset();
-              pinned_usage = table_options.block_cache->GetPinnedUsage();
-              ASSERT_EQ(pinned_usage, 0);
-          }
+            auto usage = table_options.block_cache->GetUsage();
+            auto pinned_usage = table_options.block_cache->GetPinnedUsage();
+            // The only usage must be for marked data blocks
+            ASSERT_EQ(usage, MockCache::marked_size_);
+            // There must be some pinned data since PinnableSlice has not
+            // released them yet
+            ASSERT_GT(pinned_usage, 0);
+            // Release pinnable slice reousrces
+            value.Reset();
+            pinned_usage = table_options.block_cache->GetPinnedUsage();
+            ASSERT_EQ(pinned_usage, 0);
           }
         }
       }
@@ -2405,7 +2382,7 @@ TEST_P(BlockBasedTableTest, NoObjectInCacheAfterTableClose) {
   } // level
 }
 
-TEST_P(BlockBasedTableTest, BlockCacheLeak) {
+TEST_F(BlockBasedTableTest, BlockCacheLeak) {
   // Check that when we reopen a table we don't lose access to blocks already
   // in the cache. This test checks whether the Table actually makes use of the
   // unique ID from the file.
@@ -2414,7 +2391,7 @@ TEST_P(BlockBasedTableTest, BlockCacheLeak) {
   unique_ptr<InternalKeyComparator> ikc;
   ikc.reset(new test::PlainInternalKeyComparator(opt.comparator));
   opt.compression = kNoCompression;
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+  BlockBasedTableOptions table_options;
   table_options.block_size = 1024;
   // big enough so we don't ever lose cached values.
   table_options.block_cache = NewLRUCache(16 * 1024 * 1024, 4);
@@ -2469,7 +2446,7 @@ TEST_P(BlockBasedTableTest, BlockCacheLeak) {
   c.ResetTableReader();
 }
 
-TEST_P(BlockBasedTableTest, NewIndexIteratorLeak) {
+TEST_F(BlockBasedTableTest, NewIndexIteratorLeak) {
   // A regression test to avoid data race described in
   // https://github.com/facebook/rocksdb/issues/1267
   TableConstructor c(BytewiseComparator(), true /* convert_to_internal_key_ */);
@@ -2478,7 +2455,7 @@ TEST_P(BlockBasedTableTest, NewIndexIteratorLeak) {
   c.Add("a1", "val1");
   Options options;
   options.prefix_extractor.reset(NewFixedPrefixTransform(1));
-  BlockBasedTableOptions table_options = GetBlockBasedTableOptions();
+  BlockBasedTableOptions table_options;
   table_options.index_type = BlockBasedTableOptions::kHashSearch;
   table_options.cache_index_and_filter_blocks = true;
   table_options.block_cache = NewLRUCache(0);
@@ -2980,7 +2957,7 @@ TEST_F(HarnessTest, FooterTests) {
 }
 
 class IndexBlockRestartIntervalTest
-    : public TableTest,
+    : public BlockBasedTableTest,
       public ::testing::WithParamInterface<int> {
  public:
   static std::vector<int> GetRestartValues() { return {-1, 0, 1, 8, 16, 32}; }
@@ -3125,8 +3102,8 @@ TEST_F(PrefixTest, PrefixAndWholeKeyTest) {
   // rocksdb still works.
 }
 
-TEST_P(BlockBasedTableTest, TableWithGlobalSeqno) {
-  BlockBasedTableOptions bbto = GetBlockBasedTableOptions();
+TEST_F(BlockBasedTableTest, TableWithGlobalSeqno) {
+  BlockBasedTableOptions bbto;
   test::StringSink* sink = new test::StringSink();
   unique_ptr<WritableFileWriter> file_writer(test::GetWritableFileWriter(sink));
   Options options;
@@ -3305,8 +3282,8 @@ TEST_P(BlockBasedTableTest, TableWithGlobalSeqno) {
   delete iter;
 }
 
-TEST_P(BlockBasedTableTest, BlockAlignTest) {
-  BlockBasedTableOptions bbto = GetBlockBasedTableOptions();
+TEST_F(BlockBasedTableTest, BlockAlignTest) {
+  BlockBasedTableOptions bbto;
   bbto.block_align = true;
   test::StringSink* sink = new test::StringSink();
   unique_ptr<WritableFileWriter> file_writer(test::GetWritableFileWriter(sink));
@@ -3394,10 +3371,10 @@ TEST_P(BlockBasedTableTest, BlockAlignTest) {
   table_reader.reset();
 }
 
-TEST_P(BlockBasedTableTest, BadOptions) {
+TEST_F(BlockBasedTableTest, BadOptions) {
   rocksdb::Options options;
   options.compression = kNoCompression;
-  BlockBasedTableOptions bbto = GetBlockBasedTableOptions();
+  rocksdb::BlockBasedTableOptions bbto;
   bbto.block_size = 4000;
   bbto.block_align = true;
 

tools/db_bench_tool.cc

@@ -100,13 +100,13 @@ DEFINE_string(
     "readreverse,"
     "compact,"
     "compactall,"
-    "readrandom,"
     "multireadrandom,"
     "readseq,"
     "readtocache,"
     "readreverse,"
     "readwhilewriting,"
     "readwhilemerging,"
+    "readwhilescanning,"
     "readrandomwriterandom,"
     "updaterandom,"
     "xorupdaterandom,"
@@ -149,6 +149,8 @@ DEFINE_string(
     "reads\n"
     "\treadwhilemerging      -- 1 merger, N threads doing random "
     "reads\n"
+    "\treadwhilescanning     -- 1 thread doing full table scan, "
+    "N threads doing random reads\n"
     "\treadrandomwriterandom -- N threads doing random-read, "
     "random-write\n"
     "\tupdaterandom  -- N threads doing read-modify-write for random "
@@ -430,19 +432,10 @@ DEFINE_int32(ops_between_duration_checks, 1000,
 DEFINE_bool(pin_l0_filter_and_index_blocks_in_cache, false,
             "Pin index/filter blocks of L0 files in block cache.");
 
-DEFINE_bool(
-    pin_top_level_index_and_filter, false,
-    "Pin top-level index of partitioned index/filter blocks in block cache.");
-
 DEFINE_int32(block_size,
              static_cast<int32_t>(rocksdb::BlockBasedTableOptions().block_size),
              "Number of bytes in a block.");
 
-DEFINE_int32(
-    format_version,
-    static_cast<int32_t>(rocksdb::BlockBasedTableOptions().format_version),
-    "Format version of SST files.");
-
 DEFINE_int32(block_restart_interval,
              rocksdb::BlockBasedTableOptions().block_restart_interval,
              "Number of keys between restart points "
@@ -2533,6 +2526,9 @@ void VerifyDBFromDB(std::string& truth_db_name) {
       } else if (name == "readwhilemerging") {
         num_threads++;  // Add extra thread for writing
         method = &Benchmark::ReadWhileMerging;
+      } else if (name == "readwhilescanning") {
+        num_threads++;  // Add extra thread for scaning
+        method = &Benchmark::ReadWhileScanning;
       } else if (name == "readrandomwriterandom") {
         method = &Benchmark::ReadRandomWriteRandom;
       } else if (name == "readrandommergerandom") {
@@ -3141,8 +3137,6 @@ void VerifyDBFromDB(std::string& truth_db_name) {
           FLAGS_cache_index_and_filter_blocks;
       block_based_options.pin_l0_filter_and_index_blocks_in_cache =
           FLAGS_pin_l0_filter_and_index_blocks_in_cache;
-      block_based_options.pin_top_level_index_and_filter =
-          FLAGS_pin_top_level_index_and_filter;
       if (FLAGS_cache_high_pri_pool_ratio > 1e-6) {  // > 0.0 + eps
         block_based_options.cache_index_and_filter_blocks_with_high_priority =
             true;
@@ -3154,8 +3148,7 @@ void VerifyDBFromDB(std::string& truth_db_name) {
       block_based_options.index_block_restart_interval =
           FLAGS_index_block_restart_interval;
       block_based_options.filter_policy = filter_policy_;
-      block_based_options.format_version =
-          static_cast<uint32_t>(FLAGS_format_version);
+      block_based_options.format_version = 2;
       block_based_options.read_amp_bytes_per_bit = FLAGS_read_amp_bytes_per_bit;
       block_based_options.enable_index_compression =
           FLAGS_enable_index_compression;
@@ -4519,6 +4512,45 @@ void VerifyDBFromDB(std::string& truth_db_name) {
     thread->stats.AddBytes(bytes);
   }
 
+  void ReadWhileScanning(ThreadState* thread) {
+    if (thread->tid > 0) {
+      ReadRandom(thread);
+    } else {
+      BGScan(thread);
+    }
+  }
+
+  void BGScan(ThreadState* thread) {
+    if (FLAGS_num_multi_db > 0) {
+      fprintf(stderr, "Not supporting multiple DBs.\n");
+      abort();
+    }
+    assert(db_.db != nullptr);
+    ReadOptions read_options;
+    Iterator* iter = db_.db->NewIterator(read_options);
+
+    fprintf(stderr, "num reads to do %" PRIu64 "\n", reads_);
+    Duration duration(FLAGS_duration, reads_);
+    uint64_t num_seek_to_first = 0;
+    uint64_t num_next = 0;
+    while (!duration.Done(1)) {
+      if (!iter->Valid()) {
+        iter->SeekToFirst();
+        num_seek_to_first++;
+      } else if (!iter->status().ok()) {
+        fprintf(stderr, "Iterator error: %s\n",
+                iter->status().ToString().c_str());
+        abort();
+      } else {
+        iter->Next();
+        num_next++;
+      }
+
+      thread->stats.FinishedOps(&db_, db_.db, 1, kSeek);
+    }
+    delete iter;
+  }
+
   // Given a key K and value V, this puts (K+"0", V), (K+"1", V), (K+"2", V)
   // in DB atomically i.e in a single batch. Also refer GetMany.
   Status PutMany(DB* db, const WriteOptions& writeoptions, const Slice& key,

tools/db_crashtest.py

@@ -48,7 +48,6 @@ default_params = {
     "verify_checksum": 1,
     "write_buffer_size": 4 * 1024 * 1024,
     "writepercent": 35,
-    "format_version": lambda: random.randint(2, 3),
 }
 
 

tools/db_stress.cc

@@ -219,14 +219,10 @@ DEFINE_int32(level0_stop_writes_trigger,
              rocksdb::Options().level0_stop_writes_trigger,
              "Number of files in level-0 that will trigger put stop.");
 
-DEFINE_int32(block_size, rocksdb::BlockBasedTableOptions().block_size,
+DEFINE_int32(block_size,
+             static_cast<int32_t>(rocksdb::BlockBasedTableOptions().block_size),
              "Number of bytes in a block.");
 
-DEFINE_int32(
-    format_version,
-    static_cast<int32_t>(rocksdb::BlockBasedTableOptions().format_version),
-    "Format version of SST files.");
-
 DEFINE_int32(max_background_compactions,
              rocksdb::Options().max_background_compactions,
              "The maximum number of concurrent background compactions "
@@ -526,6 +522,8 @@ DEFINE_uint64(log2_keys_per_lock, 2, "Log2 of number of keys per lock");
 static const bool FLAGS_log2_keys_per_lock_dummy __attribute__((__unused__)) =
     RegisterFlagValidator(&FLAGS_log2_keys_per_lock, &ValidateUint32Range);
 
+DEFINE_uint64(max_manifest_file_size, 16384, "Maximum size of a MANIFEST file");
+
 DEFINE_bool(in_place_update, false, "On true, does inplace update in memtable");
 
 enum RepFactory {
@@ -881,9 +879,9 @@ class SharedState {
             FLAGS_expected_values_path, &expected_mmap_buffer_);
       }
       if (status.ok()) {
-        assert(expected_mmap_buffer_->length == expected_values_size);
+        assert(expected_mmap_buffer_->GetLen() == expected_values_size);
         values_ =
-            static_cast<std::atomic<uint32_t>*>(expected_mmap_buffer_->base);
+            static_cast<std::atomic<uint32_t>*>(expected_mmap_buffer_->GetBase());
         assert(values_ != nullptr);
       } else {
         fprintf(stderr, "Failed opening shared file '%s' with error: %s\n",
@@ -2128,9 +2126,12 @@ class StressTest {
         Slice v(value, sz);
         if (!FLAGS_test_batches_snapshots) {
           // If the chosen key does not allow overwrite and it already
-          // exists, choose another key.
+          // exists, choose another key. Also avoid using merge operands in
+          // no-overwrite positions (where single delete will be called later),
+          // as those features have undefined behavior when used together.
           while (!shared->AllowsOverwrite(rand_column_family, rand_key) &&
-                 shared->Exists(rand_column_family, rand_key)) {
+                 (FLAGS_use_merge ||
+                  shared->Exists(rand_column_family, rand_key))) {
             l.reset();
             rand_key = thread->rand.Next() % max_key;
             rand_column_family = thread->rand.Next() % FLAGS_column_families;
@@ -2481,7 +2482,6 @@ class StressTest {
   void PrintEnv() const {
     fprintf(stdout, "RocksDB version           : %d.%d\n", kMajorVersion,
             kMinorVersion);
-    fprintf(stdout, "Format version            : %d\n", FLAGS_format_version);
     fprintf(stdout, "TransactionDB             : %s\n",
             FLAGS_use_txn ? "true" : "false");
     fprintf(stdout, "Column families           : %d\n", FLAGS_column_families);
@@ -2565,8 +2565,7 @@ class StressTest {
       block_based_options.block_cache_compressed = compressed_cache_;
       block_based_options.checksum = FLAGS_checksum_type_e;
       block_based_options.block_size = FLAGS_block_size;
-      block_based_options.format_version =
-          static_cast<uint32_t>(FLAGS_format_version);
+      block_based_options.format_version = 2;
       block_based_options.filter_policy = filter_policy_;
       options_.table_factory.reset(
           NewBlockBasedTableFactory(block_based_options));
@@ -2607,7 +2606,7 @@ class StressTest {
           FLAGS_level0_file_num_compaction_trigger;
       options_.compression = FLAGS_compression_type_e;
       options_.create_if_missing = true;
-      options_.max_manifest_file_size = 10 * 1024;
+      options_.max_manifest_file_size = FLAGS_max_manifest_file_size;
       options_.inplace_update_support = FLAGS_in_place_update;
       options_.max_subcompactions = static_cast<uint32_t>(FLAGS_subcompactions);
       options_.allow_concurrent_memtable_write =
@@ -2923,6 +2922,12 @@ int main(int argc, char** argv) {
             kValueMaxLen / kRandomValueMaxFactor);
     exit(1);
   }
+  if (FLAGS_use_merge && FLAGS_nooverwritepercent == 100) {
+    fprintf(
+        stderr,
+        "Error: nooverwritepercent must not be 100 when using merge operands");
+    exit(1);
+  }
 
   // Choose a location for the test database if none given with --db=<path>
   if (FLAGS_db.empty()) {

util/compression.h

@@ -254,9 +254,15 @@ inline bool Zlib_Compress(const CompressionOptions& opts,
   // memLevel=9 uses maximum memory for optimal speed.
   // The default value is 8. See zconf.h for more details.
   static const int memLevel = 8;
+  int level;
+  if (opts.level == CompressionOptions::kDefaultCompressionLevel) {
+    level = Z_DEFAULT_COMPRESSION;
+  } else {
+    level = opts.level;
+  }
   z_stream _stream;
   memset(&_stream, 0, sizeof(z_stream));
-  int st = deflateInit2(&_stream, opts.level, Z_DEFLATED, opts.window_bits,
+  int st = deflateInit2(&_stream, level, Z_DEFLATED, opts.window_bits,
                         memLevel, opts.strategy);
   if (st != Z_OK) {
     return false;
@@ -719,9 +725,15 @@ inline bool LZ4HC_Compress(const CompressionOptions& opts,
   output->resize(static_cast<size_t>(output_header_len + compress_bound));
 
   int outlen;
+  int level;
+  if (opts.level == CompressionOptions::kDefaultCompressionLevel) {
+    level = 0;  // lz4hc.h says any value < 1 will be sanitized to default
+  } else {
+    level = opts.level;
+  }
 #if LZ4_VERSION_NUMBER >= 10400  // r124+
   LZ4_streamHC_t* stream = LZ4_createStreamHC();
-  LZ4_resetStreamHC(stream, opts.level);
+  LZ4_resetStreamHC(stream, level);
   const char* compression_dict_data =
       compression_dict.size() > 0 ? compression_dict.data() : nullptr;
   size_t compression_dict_size = compression_dict.size();
@@ -742,7 +754,7 @@ inline bool LZ4HC_Compress(const CompressionOptions& opts,
 #elif LZ4_VERSION_MAJOR  // r113-r123
   outlen = LZ4_compressHC2_limitedOutput(input, &(*output)[output_header_len],
                                          static_cast<int>(length),
-                                         compress_bound, opts.level);
+                                         compress_bound, level);
 #else                    // up to r112
   outlen =
       LZ4_compressHC_limitedOutput(input, &(*output)[output_header_len],
@@ -807,15 +819,23 @@ inline bool ZSTD_Compress(const CompressionOptions& opts, const char* input,
   size_t compressBound = ZSTD_compressBound(length);
   output->resize(static_cast<size_t>(output_header_len + compressBound));
   size_t outlen;
+  int level;
+  if (opts.level == CompressionOptions::kDefaultCompressionLevel) {
+    // 3 is the value of ZSTD_CLEVEL_DEFAULT (not exposed publicly), see
+    // https://github.com/facebook/zstd/issues/1148
+    level = 3;
+  } else {
+    level = opts.level;
+  }
 #if ZSTD_VERSION_NUMBER >= 500  // v0.5.0+
   ZSTD_CCtx* context = ZSTD_createCCtx();
   outlen = ZSTD_compress_usingDict(
       context, &(*output)[output_header_len], compressBound, input, length,
-      compression_dict.data(), compression_dict.size(), opts.level);
+      compression_dict.data(), compression_dict.size(), level);
   ZSTD_freeCCtx(context);
 #else   // up to v0.4.x
   outlen = ZSTD_compress(&(*output)[output_header_len], compressBound, input,
-                         length, opts.level);
+                         length, level);
 #endif  // ZSTD_VERSION_NUMBER >= 500
   if (outlen == 0) {
     return false;

util/testutil.cc

@@ -19,9 +19,6 @@
 namespace rocksdb {
 namespace test {
 
-const uint32_t kDefaultFormatVersion = BlockBasedTableOptions().format_version;
-const uint32_t kLatestFormatVersion = 3u;
-
 Slice RandomString(Random* rnd, int len, std::string* dst) {
   dst->resize(len);
   for (int i = 0; i < len; i++) {
@@ -119,9 +116,14 @@ class Uint64ComparatorImpl : public Comparator {
 };
 }  // namespace
 
+static port::OnceType once;
+static const Comparator* uint64comp;
+
+static void InitModule() { uint64comp = new Uint64ComparatorImpl; }
+
 const Comparator* Uint64Comparator() {
-  static Uint64ComparatorImpl uint64comp;
-  return &uint64comp;
+  port::InitOnce(&once, InitModule);
+  return uint64comp;
 }
 
 WritableFileWriter* GetWritableFileWriter(WritableFile* wf) {
@@ -194,7 +196,6 @@ BlockBasedTableOptions RandomBlockBasedTableOptions(Random* rnd) {
   BlockBasedTableOptions opt;
   opt.cache_index_and_filter_blocks = rnd->Uniform(2);
   opt.pin_l0_filter_and_index_blocks_in_cache = rnd->Uniform(2);
-  opt.pin_top_level_index_and_filter = rnd->Uniform(2);
   opt.index_type = rnd->Uniform(2) ? BlockBasedTableOptions::kBinarySearch
                                    : BlockBasedTableOptions::kHashSearch;
   opt.hash_index_allow_collision = rnd->Uniform(2);

util/testutil.h

@@ -32,9 +32,6 @@ class SequentialFileReader;
 
 namespace test {
 
-extern const uint32_t kDefaultFormatVersion;
-extern const uint32_t kLatestFormatVersion;
-
 // Store in *dst a random string of length "len" and return a Slice that
 // references the generated data.
 extern Slice RandomString(Random* rnd, int len, std::string* dst);
@@ -175,6 +172,9 @@ class VectorIterator : public InternalIterator {
 
   virtual Status status() const override { return Status::OK(); }
 
+  virtual bool IsKeyPinned() const override { return true; }
+  virtual bool IsValuePinned() const override { return true; }
+
  private:
   std::vector<std::string> keys_;
   std::vector<std::string> values_;

utilities/persistent_cache/persistent_cache_test.cc

@@ -157,7 +157,7 @@ TEST_F(PersistentCacheTierTest, DISABLED_BlockCacheInsertWithFileCreateError) {
   rocksdb::SyncPoint::GetInstance()->ClearAllCallBacks();
 }
 
-#ifdef TRAVIS
+#if defined(TRAVIS) || defined(ROCKSDB_VALGRIND_RUN)
 // Travis is unable to handle the normal version of the tests running out of
 // fds, out of space and timeouts. This is an easier version of the test
 // specifically written for Travis
@@ -435,7 +435,7 @@ void PersistentCacheDBTest::RunTest(
   }
 }
 
-#ifdef TRAVIS
+#if defined(TRAVIS) || defined(ROCKSDB_VALGRIND_RUN)
 // Travis is unable to handle the normal version of the tests running out of
 // fds, out of space and timeouts. This is an easier version of the test
 // specifically written for Travis

utilities/transactions/pessimistic_transaction.cc

@@ -46,8 +46,7 @@ PessimisticTransaction::PessimisticTransaction(
       waiting_key_(nullptr),
       lock_timeout_(0),
       deadlock_detect_(false),
-      deadlock_detect_depth_(0),
-      skip_concurrency_control_(false) {
+      deadlock_detect_depth_(0) {
   txn_db_impl_ =
       static_cast_with_check<PessimisticTransactionDB, TransactionDB>(txn_db);
   db_impl_ = static_cast_with_check<DBImpl, DB>(db_);
@@ -62,7 +61,6 @@ void PessimisticTransaction::Initialize(const TransactionOptions& txn_options) {
   deadlock_detect_ = txn_options.deadlock_detect;
   deadlock_detect_depth_ = txn_options.deadlock_detect_depth;
   write_batch_.SetMaxBytes(txn_options.max_write_batch_size);
-  skip_concurrency_control_ = txn_options.skip_concurrency_control;
 
   lock_timeout_ = txn_options.lock_timeout * 1000;
   if (lock_timeout_ < 0) {
@@ -469,14 +467,11 @@ Status PessimisticTransaction::LockBatch(WriteBatch* batch,
 Status PessimisticTransaction::TryLock(ColumnFamilyHandle* column_family,
                                        const Slice& key, bool read_only,
                                        bool exclusive, bool skip_validate) {
-  Status s;
-  if (UNLIKELY(skip_concurrency_control_)) {
-    return s;
-  }
   uint32_t cfh_id = GetColumnFamilyID(column_family);
   std::string key_str = key.ToString();
   bool previously_locked;
   bool lock_upgrade = false;
+  Status s;
 
   // lock this key if this transactions hasn't already locked it
   SequenceNumber tracked_at_seq = kMaxSequenceNumber;

utilities/transactions/pessimistic_transaction.h

@@ -183,9 +183,6 @@ class PessimisticTransaction : public TransactionBaseImpl {
   // Whether to perform deadlock detection or not.
   int64_t deadlock_detect_depth_;
 
-  // Refer to TransactionOptions::skip_concurrency_control
-  bool skip_concurrency_control_;
-
   virtual Status ValidateSnapshot(ColumnFamilyHandle* column_family,
                                   const Slice& key,
                                   SequenceNumber* tracked_at_seq);

utilities/transactions/pessimistic_transaction_db.cc

@@ -26,6 +26,7 @@
 #include "utilities/transactions/pessimistic_transaction.h"
 #include "utilities/transactions/transaction_db_mutex_impl.h"
 #include "utilities/transactions/write_prepared_txn_db.h"
+#include "utilities/transactions/write_unprepared_txn_db.h"
 
 namespace rocksdb {
 
@@ -129,14 +130,6 @@ Status PessimisticTransactionDB::Initialize(
     WriteOptions w_options;
     w_options.sync = true;
     TransactionOptions t_options;
-    // This would help avoiding deadlock for keys that although exist in the WAL
-    // did not go through concurrency control. This includes the merge that
-    // MyRocks uses for auto-inc columns. It is safe to do so, since (i) if
-    // there is a conflict between the keys of two transactions that must be
-    // avoided, it is already avoided by the application, MyRocks, before the
-    // restart (ii) application, MyRocks, guarntees to rollback/commit the
-    // recovered transactions before new transactions start.
-    t_options.skip_concurrency_control = true;
 
     Transaction* real_trx = BeginTransaction(w_options, t_options, nullptr);
     assert(real_trx);
@@ -224,7 +217,7 @@ Status TransactionDB::Open(
   DBOptions db_options_2pc = db_options;
   PrepareWrap(&db_options_2pc, &column_families_copy,
               &compaction_enabled_cf_indices);
-  const bool use_seq_per_batch = txn_db_options.write_policy == WRITE_PREPARED;
+  const bool use_seq_per_batch = txn_db_options.write_policy >= WRITE_PREPARED;
   s = DBImpl::Open(db_options_2pc, dbname, column_families_copy, handles, &db,
                    use_seq_per_batch);
   if (s.ok()) {
@@ -272,7 +265,9 @@ Status TransactionDB::WrapDB(
   std::unique_ptr<PessimisticTransactionDB> txn_db;
   switch (txn_db_options.write_policy) {
     case WRITE_UNPREPARED:
-      return Status::NotSupported("WRITE_UNPREPARED is not implemented yet");
+      txn_db.reset(new WriteUnpreparedTxnDB(
+          db, PessimisticTransactionDB::ValidateTxnDBOptions(txn_db_options)));
+      break;
     case WRITE_PREPARED:
       txn_db.reset(new WritePreparedTxnDB(
           db, PessimisticTransactionDB::ValidateTxnDBOptions(txn_db_options)));
@@ -305,7 +300,9 @@ Status TransactionDB::WrapStackableDB(
 
   switch (txn_db_options.write_policy) {
     case WRITE_UNPREPARED:
-      return Status::NotSupported("WRITE_UNPREPARED is not implemented yet");
+      txn_db.reset(new WriteUnpreparedTxnDB(
+          db, PessimisticTransactionDB::ValidateTxnDBOptions(txn_db_options)));
+      break;
     case WRITE_PREPARED:
       txn_db.reset(new WritePreparedTxnDB(
           db, PessimisticTransactionDB::ValidateTxnDBOptions(txn_db_options)));

utilities/transactions/transaction_lock_mgr.cc

@@ -372,7 +372,7 @@ Status TransactionLockMgr::AcquireWithTimeout(
       if (wait_ids.size() != 0) {
         if (txn->IsDeadlockDetect()) {
           if (IncrementWaiters(txn, wait_ids, key, column_family_id,
-                               lock_info.exclusive, env)) {
+                               lock_info.exclusive)) {
             result = Status::Busy(Status::SubCode::kDeadlock);
             stripe->stripe_mutex->UnLock();
             return result;
@@ -444,7 +444,7 @@ void TransactionLockMgr::DecrementWaitersImpl(
 bool TransactionLockMgr::IncrementWaiters(
     const PessimisticTransaction* txn,
     const autovector<TransactionID>& wait_ids, const std::string& key,
-    const uint32_t& cf_id, const bool& exclusive, Env* const env) {
+    const uint32_t& cf_id, const bool& exclusive) {
   auto id = txn->GetID();
   std::vector<int> queue_parents(txn->GetDeadlockDetectDepth());
   std::vector<TransactionID> queue_values(txn->GetDeadlockDetectDepth());
@@ -468,7 +468,6 @@ bool TransactionLockMgr::IncrementWaiters(
 
   const auto* next_ids = &wait_ids;
   int parent = -1;
-  int64_t deadlock_time = 0;
   for (int tail = 0, head = 0; head < txn->GetDeadlockDetectDepth(); head++) {
     int i = 0;
     if (next_ids) {
@@ -498,10 +497,8 @@ bool TransactionLockMgr::IncrementWaiters(
                         extracted_info.m_exclusive});
         head = queue_parents[head];
       }
-      env->GetCurrentTime(&deadlock_time);
       std::reverse(path.begin(), path.end());
-      dlock_buffer_.AddNewPath(DeadlockPath(path, deadlock_time));
-      deadlock_time = 0;
+      dlock_buffer_.AddNewPath(DeadlockPath(path));
       DecrementWaitersImpl(txn, wait_ids);
       return true;
     } else if (!wait_txn_map_.Contains(next)) {
@@ -514,8 +511,7 @@ bool TransactionLockMgr::IncrementWaiters(
   }
 
   // Wait cycle too big, just assume deadlock.
-  env->GetCurrentTime(&deadlock_time);
-  dlock_buffer_.AddNewPath(DeadlockPath(deadlock_time, true));
+  dlock_buffer_.AddNewPath(DeadlockPath(true));
   DecrementWaitersImpl(txn, wait_ids);
   return true;
 }

utilities/transactions/transaction_lock_mgr.h

@@ -143,7 +143,7 @@ class TransactionLockMgr {
   bool IncrementWaiters(const PessimisticTransaction* txn,
                         const autovector<TransactionID>& wait_ids,
                         const std::string& key, const uint32_t& cf_id,
-                        const bool& exclusive, Env* const env);
+                        const bool& exclusive);
   void DecrementWaiters(const PessimisticTransaction* txn,
                         const autovector<TransactionID>& wait_ids);
   void DecrementWaitersImpl(const PessimisticTransaction* txn,

utilities/transactions/transaction_test.cc

@@ -45,11 +45,17 @@ INSTANTIATE_TEST_CASE_P(
     ::testing::Values(std::make_tuple(false, false, WRITE_COMMITTED),
                       std::make_tuple(false, true, WRITE_COMMITTED),
                       std::make_tuple(false, false, WRITE_PREPARED),
-                      std::make_tuple(false, true, WRITE_PREPARED)));
+                      std::make_tuple(false, true, WRITE_PREPARED),
+                      std::make_tuple(false, false, WRITE_UNPREPARED),
+                      std::make_tuple(false, true, WRITE_UNPREPARED)));
 INSTANTIATE_TEST_CASE_P(
     StackableDBAsBaseDB, TransactionTest,
     ::testing::Values(std::make_tuple(true, true, WRITE_COMMITTED),
-                      std::make_tuple(true, true, WRITE_PREPARED)));
+                      std::make_tuple(true, true, WRITE_PREPARED),
+                      std::make_tuple(true, true, WRITE_UNPREPARED)));
+
+// MySQLStyleTransactionTest takes far too long for valgrind to run.
+#ifndef ROCKSDB_VALGRIND_RUN
 INSTANTIATE_TEST_CASE_P(
     MySQLStyleTransactionTest, MySQLStyleTransactionTest,
     ::testing::Values(std::make_tuple(false, false, WRITE_COMMITTED),
@@ -59,7 +65,12 @@ INSTANTIATE_TEST_CASE_P(
                       std::make_tuple(false, false, WRITE_PREPARED),
                       std::make_tuple(false, true, WRITE_PREPARED),
                       std::make_tuple(true, false, WRITE_PREPARED),
-                      std::make_tuple(true, true, WRITE_PREPARED)));
+                      std::make_tuple(true, true, WRITE_PREPARED),
+                      std::make_tuple(false, false, WRITE_UNPREPARED),
+                      std::make_tuple(false, true, WRITE_UNPREPARED),
+                      std::make_tuple(true, false, WRITE_UNPREPARED),
+                      std::make_tuple(true, true, WRITE_UNPREPARED)));
+#endif  // ROCKSDB_VALGRIND_RUN
 
 TEST_P(TransactionTest, DoubleEmptyWrite) {
   WriteOptions write_options;
@@ -452,14 +463,6 @@ TEST_P(TransactionTest, DeadlockCycleShared) {
     ASSERT_EQ(dlock_buffer.size(), curr_dlock_buffer_len_);
     auto dlock_entry = dlock_buffer[0].path;
     ASSERT_EQ(dlock_entry.size(), kInitialMaxDeadlocks);
-    int64_t pre_deadlock_time = dlock_buffer[0].deadlock_time;
-    int64_t cur_deadlock_time = 0;
-    for (auto const& dl_path_rec : dlock_buffer) {
-      cur_deadlock_time = dl_path_rec.deadlock_time;
-      ASSERT_NE(cur_deadlock_time, 0);
-      ASSERT_TRUE(cur_deadlock_time <= pre_deadlock_time);
-      pre_deadlock_time = cur_deadlock_time;
-    }
 
     int64_t curr_waiting_key = 0;
 
@@ -665,15 +668,6 @@ TEST_P(TransactionTest, DeadlockCycle) {
     ASSERT_EQ(dlock_entry.size(), check_len);
     ASSERT_EQ(dlock_buffer[0].limit_exceeded, check_limit_flag);
 
-    int64_t pre_deadlock_time = dlock_buffer[0].deadlock_time;
-    int64_t cur_deadlock_time = 0;
-    for (auto const& dl_path_rec : dlock_buffer) {
-      cur_deadlock_time = dl_path_rec.deadlock_time;
-      ASSERT_NE(cur_deadlock_time, 0);
-      ASSERT_TRUE(cur_deadlock_time <= pre_deadlock_time);
-      pre_deadlock_time = cur_deadlock_time;
-    }
-
     // Iterates backwards over path verifying decreasing txn_ids.
     for (auto it = dlock_entry.rbegin(); it != dlock_entry.rend(); it++) {
       auto dl_node = *it;
@@ -1792,10 +1786,10 @@ TEST_P(TransactionTest, TwoPhaseLogRollingTest2) {
       ASSERT_EQ(cfh_a->cfd()->GetLogNumber(), db_impl->TEST_LogfileNumber());
       break;
     case WRITE_PREPARED:
+    case WRITE_UNPREPARED:
       // This cf is not flushed yet and should ref the log that has its data
       ASSERT_EQ(cfh_a->cfd()->GetLogNumber(), prepare_log_no);
       break;
-    case WRITE_UNPREPARED:
     default:
       assert(false);
   }
@@ -4820,6 +4814,7 @@ TEST_P(TransactionTest, ExpiredTransactionDataRace1) {
   rocksdb::SyncPoint::GetInstance()->DisableProcessing();
 }
 
+#ifndef ROCKSDB_VALGRIND_RUN
 namespace {
 Status TransactionStressTestInserter(TransactionDB* db,
                                      const size_t num_transactions,
@@ -4907,6 +4902,7 @@ TEST_P(MySQLStyleTransactionTest, TransactionStressTest) {
                                                !TAKE_SNAPSHOT);
   ASSERT_OK(s);
 }
+#endif  // ROCKSDB_VALGRIND_RUN
 
 TEST_P(TransactionTest, MemoryLimitTest) {
   TransactionOptions txn_options;

utilities/transactions/write_prepared_transaction_test.cc

@@ -521,6 +521,7 @@ class WritePreparedTransactionTest
                                          std::get<2>(GetParam())){};
 };
 
+#ifndef ROCKSDB_VALGRIND_RUN
 class SnapshotConcurrentAccessTest
     : public WritePreparedTransactionTestBase,
       virtual public ::testing::WithParamInterface<
@@ -539,6 +540,7 @@ class SnapshotConcurrentAccessTest
   size_t split_id_;
   size_t split_cnt_;
 };
+#endif  // ROCKSDB_VALGRIND_RUN
 
 class SeqAdvanceConcurrentTest
     : public WritePreparedTransactionTestBase,
@@ -562,8 +564,10 @@ class SeqAdvanceConcurrentTest
 INSTANTIATE_TEST_CASE_P(
     WritePreparedTransactionTest, WritePreparedTransactionTest,
     ::testing::Values(std::make_tuple(false, false, WRITE_PREPARED),
-                      std::make_tuple(false, true, WRITE_PREPARED)));
+                      std::make_tuple(false, true, WRITE_PREPARED),
+                      std::make_tuple(false, true, WRITE_UNPREPARED)));
 
+#ifndef ROCKSDB_VALGRIND_RUN
 INSTANTIATE_TEST_CASE_P(
     TwoWriteQueues, SnapshotConcurrentAccessTest,
     ::testing::Values(std::make_tuple(false, true, WRITE_PREPARED, 0, 20),
@@ -585,7 +589,27 @@ INSTANTIATE_TEST_CASE_P(
                       std::make_tuple(false, true, WRITE_PREPARED, 16, 20),
                       std::make_tuple(false, true, WRITE_PREPARED, 17, 20),
                       std::make_tuple(false, true, WRITE_PREPARED, 18, 20),
-                      std::make_tuple(false, true, WRITE_PREPARED, 19, 20)));
+                      std::make_tuple(false, true, WRITE_PREPARED, 19, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 0, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 1, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 2, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 3, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 4, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 5, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 6, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 7, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 8, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 9, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 10, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 11, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 12, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 13, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 14, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 15, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 16, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 17, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 18, 20),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 19, 20)));
 
 INSTANTIATE_TEST_CASE_P(
     OneWriteQueue, SnapshotConcurrentAccessTest,
@@ -608,7 +632,27 @@ INSTANTIATE_TEST_CASE_P(
                       std::make_tuple(false, false, WRITE_PREPARED, 16, 20),
                       std::make_tuple(false, false, WRITE_PREPARED, 17, 20),
                       std::make_tuple(false, false, WRITE_PREPARED, 18, 20),
-                      std::make_tuple(false, false, WRITE_PREPARED, 19, 20)));
+                      std::make_tuple(false, false, WRITE_PREPARED, 19, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 0, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 1, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 2, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 3, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 4, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 5, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 6, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 7, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 8, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 9, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 10, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 11, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 12, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 13, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 14, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 15, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 16, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 17, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 18, 20),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 19, 20)));
 
 INSTANTIATE_TEST_CASE_P(
     TwoWriteQueues, SeqAdvanceConcurrentTest,
@@ -621,7 +665,17 @@ INSTANTIATE_TEST_CASE_P(
                       std::make_tuple(false, true, WRITE_PREPARED, 6, 10),
                       std::make_tuple(false, true, WRITE_PREPARED, 7, 10),
                       std::make_tuple(false, true, WRITE_PREPARED, 8, 10),
-                      std::make_tuple(false, true, WRITE_PREPARED, 9, 10)));
+                      std::make_tuple(false, true, WRITE_PREPARED, 9, 10),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 0, 10),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 1, 10),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 2, 10),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 3, 10),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 4, 10),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 5, 10),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 6, 10),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 7, 10),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 8, 10),
+                      std::make_tuple(false, true, WRITE_UNPREPARED, 9, 10)));
 
 INSTANTIATE_TEST_CASE_P(
     OneWriteQueue, SeqAdvanceConcurrentTest,
@@ -634,7 +688,18 @@ INSTANTIATE_TEST_CASE_P(
                       std::make_tuple(false, false, WRITE_PREPARED, 6, 10),
                       std::make_tuple(false, false, WRITE_PREPARED, 7, 10),
                       std::make_tuple(false, false, WRITE_PREPARED, 8, 10),
-                      std::make_tuple(false, false, WRITE_PREPARED, 9, 10)));
+                      std::make_tuple(false, false, WRITE_PREPARED, 9, 10),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 0, 10),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 1, 10),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 2, 10),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 3, 10),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 4, 10),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 5, 10),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 6, 10),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 7, 10),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 8, 10),
+                      std::make_tuple(false, false, WRITE_UNPREPARED, 9, 10)));
+#endif  // ROCKSDB_VALGRIND_RUN
 
 TEST_P(WritePreparedTransactionTest, CommitMapTest) {
   WritePreparedTxnDB* wp_db = dynamic_cast<WritePreparedTxnDB*>(db);
@@ -810,7 +875,6 @@ TEST_P(WritePreparedTransactionTest, CheckAgainstSnapshotsTest) {
   std::vector<SequenceNumber> snapshots = {100l, 200l, 300l, 400l, 500l,
                                            600l, 700l, 800l, 900l};
   const size_t snapshot_cache_bits = 2;
-  const uint64_t cache_size = 1ul << snapshot_cache_bits;
   // Safety check to express the intended size in the test. Can be adjusted if
   // the snapshots lists changed.
   assert((1ul << snapshot_cache_bits) * 2 + 1 == snapshots.size());
@@ -838,61 +902,11 @@ TEST_P(WritePreparedTransactionTest, CheckAgainstSnapshotsTest) {
                          commit_entry.prep_seq <= snapshots.back();
     ASSERT_EQ(expect_update, !wp_db->old_commit_map_empty_);
   }
-
-  // Test that search will include multiple snapshot from snapshot cache
-  {
-    // exclude first and last item in the cache
-    CommitEntry commit_entry = {snapshots.front() + 1,
-                                snapshots[cache_size - 1] - 1};
-    wp_db->old_commit_map_empty_ = true;  // reset
-    wp_db->old_commit_map_.clear();
-    wp_db->CheckAgainstSnapshots(commit_entry);
-    ASSERT_EQ(wp_db->old_commit_map_.size(), cache_size - 2);
-  }
-
-  // Test that search will include multiple snapshot from old snapshots
-  {
-    // include two in the middle
-    CommitEntry commit_entry = {snapshots[cache_size] + 1,
-                                snapshots[cache_size + 2] + 1};
-    wp_db->old_commit_map_empty_ = true;  // reset
-    wp_db->old_commit_map_.clear();
-    wp_db->CheckAgainstSnapshots(commit_entry);
-    ASSERT_EQ(wp_db->old_commit_map_.size(), 2);
-  }
-
-  // Test that search will include both snapshot cache and old snapshots
-  // Case 1: includes all in snapshot cache
-  {
-    CommitEntry commit_entry = {snapshots.front() - 1, snapshots.back() + 1};
-    wp_db->old_commit_map_empty_ = true;  // reset
-    wp_db->old_commit_map_.clear();
-    wp_db->CheckAgainstSnapshots(commit_entry);
-    ASSERT_EQ(wp_db->old_commit_map_.size(), snapshots.size());
-  }
-
-  // Case 2: includes all snapshot caches except the smallest
-  {
-    CommitEntry commit_entry = {snapshots.front() + 1, snapshots.back() + 1};
-    wp_db->old_commit_map_empty_ = true;  // reset
-    wp_db->old_commit_map_.clear();
-    wp_db->CheckAgainstSnapshots(commit_entry);
-    ASSERT_EQ(wp_db->old_commit_map_.size(), snapshots.size() - 1);
-  }
-
-  // Case 3: includes only the largest of snapshot cache
-  {
-    CommitEntry commit_entry = {snapshots[cache_size - 1] - 1,
-                                snapshots.back() + 1};
-    wp_db->old_commit_map_empty_ = true;  // reset
-    wp_db->old_commit_map_.clear();
-    wp_db->CheckAgainstSnapshots(commit_entry);
-    ASSERT_EQ(wp_db->old_commit_map_.size(), snapshots.size() - cache_size + 1);
-  }
 }
 
 // This test is too slow for travis
 #ifndef TRAVIS
+#ifndef ROCKSDB_VALGRIND_RUN
 // Test that CheckAgainstSnapshots will not miss a live snapshot if it is run in
 // parallel with UpdateSnapshots.
 TEST_P(SnapshotConcurrentAccessTest, SnapshotConcurrentAccessTest) {
@@ -971,6 +985,7 @@ TEST_P(SnapshotConcurrentAccessTest, SnapshotConcurrentAccessTest) {
   }
   printf("\n");
 }
+#endif  // ROCKSDB_VALGRIND_RUN
 #endif  // TRAVIS
 
 // This test clarifies the contract of AdvanceMaxEvictedSeq method
@@ -1345,6 +1360,55 @@ TEST_P(WritePreparedTransactionTest, BasicRecoveryTest) {
   pinnable_val.Reset();
 }
 
+// After recovery the new transactions should still conflict with recovered
+// transactions.
+TEST_P(WritePreparedTransactionTest, ConflictDetectionAfterRecoveryTest) {
+  options.disable_auto_compactions = true;
+  ReOpen();
+
+  TransactionOptions txn_options;
+  WriteOptions write_options;
+  size_t index = 0;
+  Transaction* txn0 = db->BeginTransaction(write_options, txn_options);
+  auto istr0 = std::to_string(index);
+  auto s = txn0->SetName("xid" + istr0);
+  ASSERT_OK(s);
+  s = txn0->Put(Slice("key" + istr0), Slice("bar0" + istr0));
+  ASSERT_OK(s);
+  s = txn0->Prepare();
+
+  // With the same index 0 and key prefix, txn_t0 should conflict with txn0
+  txn_t0_with_status(0, Status::TimedOut());
+  delete txn0;
+
+  auto db_impl = reinterpret_cast<DBImpl*>(db->GetRootDB());
+  db_impl->FlushWAL(true);
+  dynamic_cast<WritePreparedTxnDB*>(db)->TEST_Crash();
+  ReOpenNoDelete();
+
+  // It should still conflict after the recovery
+  txn_t0_with_status(0, Status::TimedOut());
+
+  db_impl = reinterpret_cast<DBImpl*>(db->GetRootDB());
+  db_impl->FlushWAL(true);
+  ReOpenNoDelete();
+
+  // Check that a recovered txn will still cause conflicts after 2nd recovery
+  txn_t0_with_status(0, Status::TimedOut());
+
+  txn0 = db->GetTransactionByName("xid" + istr0);
+  ASSERT_NE(txn0, nullptr);
+  txn0->Commit();
+  delete txn0;
+
+  db_impl = reinterpret_cast<DBImpl*>(db->GetRootDB());
+  db_impl->FlushWAL(true);
+  ReOpenNoDelete();
+
+  // tnx0 is now committed and should no longer cause a conflict
+  txn_t0_with_status(0, Status::OK());
+}
+
 // After recovery the commit map is empty while the max is set. The code would
 // go through a different path which requires a separate test.
 TEST_P(WritePreparedTransactionTest, IsInSnapshotEmptyMapTest) {

utilities/transactions/write_prepared_txn_db.cc

@@ -567,16 +567,14 @@ void WritePreparedTxnDB::ReleaseSnapshotInternal(
     bool need_gc = false;
     {
       WPRecordTick(TXN_OLD_COMMIT_MAP_MUTEX_OVERHEAD);
-      ROCKS_LOG_WARN(info_log_, "old_commit_map_mutex_ overhead for %" PRIu64,
-                     snap_seq);
+      ROCKS_LOG_WARN(info_log_, "old_commit_map_mutex_ overhead");
       ReadLock rl(&old_commit_map_mutex_);
       auto prep_set_entry = old_commit_map_.find(snap_seq);
       need_gc = prep_set_entry != old_commit_map_.end();
     }
     if (need_gc) {
       WPRecordTick(TXN_OLD_COMMIT_MAP_MUTEX_OVERHEAD);
-      ROCKS_LOG_WARN(info_log_, "old_commit_map_mutex_ overhead for %" PRIu64,
-                     snap_seq);
+      ROCKS_LOG_WARN(info_log_, "old_commit_map_mutex_ overhead");
       WriteLock wl(&old_commit_map_mutex_);
       old_commit_map_.erase(snap_seq);
       old_commit_map_empty_.store(old_commit_map_.empty(),
@@ -651,20 +649,13 @@ void WritePreparedTxnDB::CheckAgainstSnapshots(const CommitEntry& evicted) {
   // place before gets overwritten the reader that reads bottom-up will
   // eventully see it.
   const bool next_is_larger = true;
-  // We will set to true if the border line snapshot suggests that.
-  bool search_larger_list = false;
+  SequenceNumber snapshot_seq = kMaxSequenceNumber;
   size_t ip1 = std::min(cnt, SNAPSHOT_CACHE_SIZE);
   for (; 0 < ip1; ip1--) {
-    SequenceNumber snapshot_seq =
-        snapshot_cache_[ip1 - 1].load(std::memory_order_acquire);
+    snapshot_seq = snapshot_cache_[ip1 - 1].load(std::memory_order_acquire);
     TEST_IDX_SYNC_POINT("WritePreparedTxnDB::CheckAgainstSnapshots:p:",
                         ++sync_i);
     TEST_IDX_SYNC_POINT("WritePreparedTxnDB::CheckAgainstSnapshots:s:", sync_i);
-    if (ip1 == SNAPSHOT_CACHE_SIZE) {  // border line snapshot
-      // snapshot_seq < commit_seq => larger_snapshot_seq <= commit_seq
-      // then later also continue the search to larger snapshots
-      search_larger_list = snapshot_seq < evicted.commit_seq;
-    }
     if (!MaybeUpdateOldCommitMap(evicted.prep_seq, evicted.commit_seq,
                                  snapshot_seq, !next_is_larger)) {
       break;
@@ -679,20 +670,17 @@ void WritePreparedTxnDB::CheckAgainstSnapshots(const CommitEntry& evicted) {
 #endif
   TEST_SYNC_POINT("WritePreparedTxnDB::CheckAgainstSnapshots:p:end");
   TEST_SYNC_POINT("WritePreparedTxnDB::CheckAgainstSnapshots:s:end");
-  if (UNLIKELY(SNAPSHOT_CACHE_SIZE < cnt && search_larger_list)) {
+  if (UNLIKELY(SNAPSHOT_CACHE_SIZE < cnt && ip1 == SNAPSHOT_CACHE_SIZE &&
+               snapshot_seq < evicted.prep_seq)) {
     // Then access the less efficient list of snapshots_
     WPRecordTick(TXN_SNAPSHOT_MUTEX_OVERHEAD);
-    ROCKS_LOG_WARN(info_log_,
-                   "snapshots_mutex_ overhead for <%" PRIu64 ",%" PRIu64
-                   "> with %" ROCKSDB_PRIszt " snapshots",
-                   evicted.prep_seq, evicted.commit_seq, cnt);
+    ROCKS_LOG_WARN(info_log_, "snapshots_mutex_ overhead");
     ReadLock rl(&snapshots_mutex_);
     // Items could have moved from the snapshots_ to snapshot_cache_ before
     // accquiring the lock. To make sure that we do not miss a valid snapshot,
     // read snapshot_cache_ again while holding the lock.
     for (size_t i = 0; i < SNAPSHOT_CACHE_SIZE; i++) {
-      SequenceNumber snapshot_seq =
-          snapshot_cache_[i].load(std::memory_order_acquire);
+      snapshot_seq = snapshot_cache_[i].load(std::memory_order_acquire);
       if (!MaybeUpdateOldCommitMap(evicted.prep_seq, evicted.commit_seq,
                                    snapshot_seq, next_is_larger)) {
         break;
@@ -720,10 +708,7 @@ bool WritePreparedTxnDB::MaybeUpdateOldCommitMap(
   // then snapshot_seq < commit_seq
   if (prep_seq <= snapshot_seq) {  // overlapping range
     WPRecordTick(TXN_OLD_COMMIT_MAP_MUTEX_OVERHEAD);
-    ROCKS_LOG_WARN(info_log_,
-                   "old_commit_map_mutex_ overhead for %" PRIu64
-                   " commit entry: <%" PRIu64 ",%" PRIu64 ">",
-                   snapshot_seq, prep_seq, commit_seq);
+    ROCKS_LOG_WARN(info_log_, "old_commit_map_mutex_ overhead");
     WriteLock wl(&old_commit_map_mutex_);
     old_commit_map_empty_.store(false, std::memory_order_release);
     auto& vec = old_commit_map_[snapshot_seq];