update HISTORY.md and version.h for 9.4.1

Summary:
Make `DestroyDB` slowly delete files if it's configured and enabled via `SstFileManager`.

It's currently not available mainly because of DeleteScheduler's logic related to tracked total_size_ and total_trash_size_. These accounting and logic should not be applied to `DestroyDB`. This PR adds a `DeleteUnaccountedDBFile` util for this purpose which deletes files without accounting it.  This util also supports assigning a file to a specified trash bucket so that user can later wait for a specific trash bucket to be empty. For `DestroyDB`, files with more than 1 hard links will be deleted immediately.

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

Test Plan: Added unit tests, existing tests.

Reviewed By: anand1976

Differential Revision: D60300220

Pulled By: jowlyzhang

fbshipit-source-id: 8b18109a177a3a9532f6dc2e40e08310c08ca3c7

HISTORY.md

@@ -1,36 +1,10 @@
 # Rocksdb Change Log
 > NOTE: Entries for next release do not go here. Follow instructions in `unreleased_history/README.txt`
 
-## 9.5.2 (08/13/2024)
-### Bug Fixes
-* Fix a race condition in pessimistic transactions that could allow multiple transactions with the same name to be registered simultaneously, resulting in a crash or other unpredictable behavior.
-
-### Public API Changes
-* Add ticker stats to count file read retries due to checksum mismatch
-
-## 9.5.1 (08/02/2024)
+## 9.4.1 (08/02/2024)
 ### Bug Fixes
 * *Make DestroyDB supports slow deletion when it's configured in `SstFileManager`. The slow deletion is subject to the configured `rate_bytes_per_sec`, but not subject to the `max_trash_db_ratio`.
 
-## 9.5.0 (07/19/2024)
-### Public API Changes
-* Introduced new C API function rocksdb_writebatch_iterate_cf for column family-aware iteration over the contents of a WriteBatch
-* Add support to ingest SST files generated by a DB instead of SstFileWriter. This can be enabled with experimental option `IngestExternalFileOptions::allow_db_generated_files`.
-
-### Behavior Changes
-* When calculating total log size for the `log_size_for_flush` argument in `CreateCheckpoint` API, the size of the archived log will not be included to avoid unnecessary flush
-
-### Bug Fixes
-* Fix a major bug in which an iterator using prefix filtering and SeekForPrev might miss data when the DB is using `whole_key_filtering=false` and `partition_filters=true`.
-* Fixed a bug where `OnErrorRecoveryBegin()` is not called before auto recovery starts.
-* Fixed a bug where event listener reads ErrorHandler's `bg_error_` member without holding db mutex(#12803).
-* Fixed a bug in handling MANIFEST write error that caused the latest valid MANIFEST file to get deleted, resulting in the DB being unopenable.
-* Fixed a race between error recovery due to manifest sync or write failure and external SST file ingestion. Both attempt to write a new manifest file, which causes an assertion failure.
-
-### Performance Improvements
-* Fix an issue where compactions were opening table files and reading table properties while holding db mutex_.
-* Reduce unnecessary filesystem queries and DB mutex acquires in creating backups and checkpoints.
-
 ## 9.4.0 (06/23/2024)
 ### New Features
 * Added a `CompactForTieringCollectorFactory` to auto trigger compaction for tiering use case.

Makefile

@@ -2139,8 +2139,8 @@ ZLIB_DOWNLOAD_BASE ?= http://zlib.net
 BZIP2_VER ?= 1.0.8
 BZIP2_SHA256 ?= ab5a03176ee106d3f0fa90e381da478ddae405918153cca248e682cd0c4a2269
 BZIP2_DOWNLOAD_BASE ?= http://sourceware.org/pub/bzip2
-SNAPPY_VER ?= 1.2.1
-SNAPPY_SHA256 ?= 736aeb64d86566d2236ddffa2865ee5d7a82d26c9016b36218fcc27ea4f09f86
+SNAPPY_VER ?= 1.1.8
+SNAPPY_SHA256 ?= 16b677f07832a612b0836178db7f374e414f94657c138e6993cbfc5dcc58651f
 SNAPPY_DOWNLOAD_BASE ?= https://github.com/google/snappy/archive
 LZ4_VER ?= 1.9.4
 LZ4_SHA256 ?= 0b0e3aa07c8c063ddf40b082bdf7e37a1562bda40a0ff5272957f3e987e0e54b
@@ -2237,7 +2237,7 @@ libsnappy.a: snappy-$(SNAPPY_VER).tar.gz
 	-rm -rf snappy-$(SNAPPY_VER)
 	tar xvzf snappy-$(SNAPPY_VER).tar.gz
 	mkdir snappy-$(SNAPPY_VER)/build
-	cd snappy-$(SNAPPY_VER)/build && CFLAGS='$(ARCHFLAG) ${JAVA_STATIC_DEPS_CCFLAGS} ${EXTRA_CFLAGS}' CXXFLAGS='$(ARCHFLAG) ${JAVA_STATIC_DEPS_CXXFLAGS} ${EXTRA_CXXFLAGS}' LDFLAGS='${JAVA_STATIC_DEPS_LDFLAGS} ${EXTRA_LDFLAGS}' cmake -DCMAKE_POSITION_INDEPENDENT_CODE=ON -DSNAPPY_BUILD_BENCHMARKS=OFF -DSNAPPY_BUILD_TESTS=OFF --compile-no-warning-as-error ${PLATFORM_CMAKE_FLAGS} .. && $(MAKE) ${SNAPPY_MAKE_TARGET}
+	cd snappy-$(SNAPPY_VER)/build && CFLAGS='$(ARCHFLAG) ${JAVA_STATIC_DEPS_CCFLAGS} ${EXTRA_CFLAGS}' CXXFLAGS='$(ARCHFLAG) ${JAVA_STATIC_DEPS_CXXFLAGS} ${EXTRA_CXXFLAGS}' LDFLAGS='${JAVA_STATIC_DEPS_LDFLAGS} ${EXTRA_LDFLAGS}' cmake -DCMAKE_POSITION_INDEPENDENT_CODE=ON ${PLATFORM_CMAKE_FLAGS} .. && $(MAKE) ${SNAPPY_MAKE_TARGET}
 	cp snappy-$(SNAPPY_VER)/build/libsnappy.a .
 
 lz4-$(LZ4_VER).tar.gz:
@@ -2484,11 +2484,13 @@ checkout_folly:
 	fi
 	@# Pin to a particular version for public CI, so that PR authors don't
 	@# need to worry about folly breaking our integration. Update periodically
-	cd third-party/folly && git reset --hard c48fdd205c1c291651749d532b8055fe822bba25
+	cd third-party/folly && git reset --hard 17054c85c2dee0e84cbee7ef28d7bae7d0b1630e
 	@# NOTE: this hack is required for clang in some cases
 	perl -pi -e 's/int rv = syscall/int rv = (int)syscall/' third-party/folly/folly/detail/Futex.cpp
 	@# NOTE: this hack is required for gcc in some cases
 	perl -pi -e 's/(__has_include.<experimental.memory_resource>.)/__cpp_rtti && $$1/' third-party/folly/folly/memory/MemoryResource.h
+	@# NOTE: this hack is only needed if building target build_folly in the future
+	perl -pi -e 's,https://github.com/jedisct1/libsodium/releases/download/1.0.17/libsodium-1.0.17.tar.gz,https://download.libsodium.org/libsodium/releases/old/unsupported/libsodium-1.0.17.tar.gz,' third-party/folly/build/fbcode_builder/manifests/libsodium
 	@# NOTE: boost source will be needed for any build including `USE_FOLLY_LITE` builds as those depend on boost headers
 	cd third-party/folly && $(PYTHON) build/fbcode_builder/getdeps.py fetch boost
 

db/arena_wrapped_db_iter.cc

@@ -138,10 +138,10 @@ Status ArenaWrappedDBIter::Refresh(const Snapshot* snapshot) {
             reinit_internal_iter();
             break;
           } else {
-            *memtable_range_tombstone_iter_ =
-                std::make_unique<TruncatedRangeDelIterator>(
-                    std::unique_ptr<FragmentedRangeTombstoneIterator>(t),
-                    &cfd->internal_comparator(), nullptr, nullptr);
+            delete *memtable_range_tombstone_iter_;
+            *memtable_range_tombstone_iter_ = new TruncatedRangeDelIterator(
+                std::unique_ptr<FragmentedRangeTombstoneIterator>(t),
+                &cfd->internal_comparator(), nullptr, nullptr);
           }
         }
         db_impl->ReturnAndCleanupSuperVersion(cfd, sv);

db/arena_wrapped_db_iter.h

@@ -55,8 +55,7 @@ class ArenaWrappedDBIter : public Iterator {
     db_iter_->SetIter(iter);
   }
 
-  void SetMemtableRangetombstoneIter(
-      std::unique_ptr<TruncatedRangeDelIterator>* iter) {
+  void SetMemtableRangetombstoneIter(TruncatedRangeDelIterator** iter) {
     memtable_range_tombstone_iter_ = iter;
   }
 
@@ -111,8 +110,7 @@ class ArenaWrappedDBIter : public Iterator {
   bool allow_refresh_ = true;
   // If this is nullptr, it means the mutable memtable does not contain range
   // tombstone when added under this DBIter.
-  std::unique_ptr<TruncatedRangeDelIterator>* memtable_range_tombstone_iter_ =
-      nullptr;
+  TruncatedRangeDelIterator** memtable_range_tombstone_iter_ = nullptr;
 };
 
 // Generate the arena wrapped iterator class.

db/c.cc

@@ -532,42 +532,6 @@ struct rocksdb_universal_compaction_options_t {
   ROCKSDB_NAMESPACE::CompactionOptionsUniversal* rep;
 };
 
-struct rocksdb_callback_logger_t : public Logger {
-  static const ssize_t STACK_BUFSZ = 512;
-  rocksdb_callback_logger_t(InfoLogLevel log_level,
-                            void (*logv_cb)(void*, unsigned, char*, size_t),
-                            void* priv)
-      : Logger(log_level), logv_cb_(logv_cb), priv_(priv) {}
-
-  using Logger::Logv;
-  void Logv(const InfoLogLevel level, const char* fmt, va_list ap0) override {
-    char stack_buf[STACK_BUFSZ];
-    char* alloc_buf = nullptr;
-    char* buf = stack_buf;
-    int len = 0;
-    va_list ap1;
-    if (!logv_cb_) return;
-    va_copy(ap1, ap0);
-    len = vsnprintf(buf, STACK_BUFSZ, fmt, ap0);
-    if (len <= 0)
-      goto cleanup;
-    else if (len >= STACK_BUFSZ) {
-      buf = alloc_buf = reinterpret_cast<char*>(malloc(len + 1));
-      if (!buf) goto cleanup;
-      len = vsnprintf(buf, len + 1, fmt, ap1);
-      if (len <= 0) goto cleanup;
-    }
-    logv_cb_(priv_, unsigned(level), buf, size_t(len));
-  cleanup:
-    va_end(ap1);
-    free(alloc_buf);
-  }
-
- private:
-  void (*logv_cb_)(void*, unsigned, char*, size_t) = nullptr;
-  void* priv_ = nullptr;
-};
-
 static bool SaveError(char** errptr, const Status& s) {
   assert(errptr != nullptr);
   if (s.ok()) {
@@ -1838,26 +1802,6 @@ void rocksdb_approximate_sizes_cf(
   delete[] ranges;
 }
 
-extern ROCKSDB_LIBRARY_API void rocksdb_approximate_sizes_cf_with_flags(
-    rocksdb_t* db, rocksdb_column_family_handle_t* column_family,
-    int num_ranges, const char* const* range_start_key,
-    const size_t* range_start_key_len, const char* const* range_limit_key,
-    const size_t* range_limit_key_len, uint8_t include_flags, uint64_t* sizes,
-    char** errptr) {
-  Range* ranges = new Range[num_ranges];
-  for (int i = 0; i < num_ranges; i++) {
-    ranges[i].start = Slice(range_start_key[i], range_start_key_len[i]);
-    ranges[i].limit = Slice(range_limit_key[i], range_limit_key_len[i]);
-  }
-  Status s = db->rep->GetApproximateSizes(
-      column_family->rep, ranges, num_ranges, sizes,
-      static_cast<DB::SizeApproximationFlags>(include_flags));
-  if (!s.ok()) {
-    SaveError(errptr, s);
-  }
-  delete[] ranges;
-}
-
 void rocksdb_delete_file(rocksdb_t* db, const char* name) {
   db->rep->DeleteFile(name);
 }
@@ -2286,32 +2230,6 @@ class H : public WriteBatch::Handler {
   }
 };
 
-class HCF : public WriteBatch::Handler {
- public:
-  void* state_;
-  void (*put_cf_)(void*, uint32_t cfid, const char* k, size_t klen,
-                  const char* v, size_t vlen);
-  void (*deleted_cf_)(void*, uint32_t cfid, const char* k, size_t klen);
-  void (*merge_cf_)(void*, uint32_t cfid, const char* k, size_t klen,
-                    const char* v, size_t vlen);
-  Status PutCF(uint32_t column_family_id, const Slice& key,
-               const Slice& value) override {
-    (*put_cf_)(state_, column_family_id, key.data(), key.size(), value.data(),
-               value.size());
-    return Status::OK();
-  }
-  Status DeleteCF(uint32_t column_family_id, const Slice& key) override {
-    (*deleted_cf_)(state_, column_family_id, key.data(), key.size());
-    return Status::OK();
-  }
-  Status MergeCF(uint32_t column_family_id, const Slice& key,
-                 const Slice& value) override {
-    (*merge_cf_)(state_, column_family_id, key.data(), key.size(), value.data(),
-                 value.size());
-    return Status::OK();
-  }
-};
-
 void rocksdb_writebatch_iterate(rocksdb_writebatch_t* b, void* state,
                                 void (*put)(void*, const char* k, size_t klen,
                                             const char* v, size_t vlen),
@@ -2324,21 +2242,6 @@ void rocksdb_writebatch_iterate(rocksdb_writebatch_t* b, void* state,
   b->rep.Iterate(&handler);
 }
 
-void rocksdb_writebatch_iterate_cf(
-    rocksdb_writebatch_t* b, void* state,
-    void (*put_cf)(void*, uint32_t cfid, const char* k, size_t klen,
-                   const char* v, size_t vlen),
-    void (*deleted_cf)(void*, uint32_t cfid, const char* k, size_t klen),
-    void (*merge_cf)(void*, uint32_t cfid, const char* k, size_t klen,
-                     const char* v, size_t vlen)) {
-  HCF handler;
-  handler.state_ = state;
-  handler.put_cf_ = put_cf;
-  handler.deleted_cf_ = deleted_cf;
-  handler.merge_cf_ = merge_cf;
-  b->rep.Iterate(&handler);
-}
-
 const char* rocksdb_writebatch_data(rocksdb_writebatch_t* b, size_t* size) {
   *size = b->rep.GetDataSize();
   return b->rep.Data().c_str();
@@ -3134,15 +3037,6 @@ rocksdb_logger_t* rocksdb_logger_create_stderr_logger(int log_level,
   return logger;
 }
 
-rocksdb_logger_t* rocksdb_logger_create_callback_logger(
-    int log_level, void (*callback)(void*, unsigned, char*, size_t),
-    void* priv) {
-  rocksdb_logger_t* logger = new rocksdb_logger_t;
-  logger->rep = std::make_shared<rocksdb_callback_logger_t>(
-      static_cast<InfoLogLevel>(log_level), callback, priv);
-  return logger;
-}
-
 void rocksdb_logger_destroy(rocksdb_logger_t* logger) { delete logger; }
 
 void rocksdb_options_set_env(rocksdb_options_t* opt, rocksdb_env_t* env) {

db/c_test.c

@@ -199,79 +199,6 @@ static void CheckDel(void* ptr, const char* k, size_t klen) {
   (*state)++;
 }
 
-// Callback from rocksdb_writebatch_iterate_cf()
-static void CheckPutCF(void* ptr, uint32_t cfid, const char* k, size_t klen,
-                       const char* v, size_t vlen) {
-  int* state = (int*)ptr;
-  switch (*state) {
-    case 0:
-      CheckEqual("bar", k, klen);
-      CheckEqual("b", v, vlen);
-      CheckCondition(cfid == 1);
-      break;
-    case 1:
-      CheckEqual("box", k, klen);
-      CheckEqual("c", v, vlen);
-      CheckCondition(cfid == 1);
-      break;
-    case 4:
-      CheckEqual("foo", k, klen);
-      CheckEqual("f", v, vlen);
-      CheckCondition(cfid == 0);
-      break;
-    case 6:
-      CheckEqual("baz", k, klen);
-      CheckEqual("a", v, vlen);
-      CheckCondition(cfid == 0);
-      break;
-    default:
-      CheckCondition(false);
-      break;
-  }
-  (*state)++;
-}
-
-// Callback from rocksdb_writebatch_iterate_cf()
-static void CheckDelCF(void* ptr, uint32_t cfid, const char* k, size_t klen) {
-  int* state = (int*)ptr;
-  switch (*state) {
-    case 2:
-      CheckEqual("bar", k, klen);
-      CheckCondition(cfid == 1);
-      break;
-    case 5:
-      CheckEqual("foo", k, klen);
-      CheckCondition(cfid == 0);
-      break;
-    default:
-      CheckCondition(false);
-      break;
-  }
-  (*state)++;
-}
-
-// Callback from rocksdb_writebatch_iterate_cf()
-static void CheckMergeCF(void* ptr, uint32_t cfid, const char* k, size_t klen,
-                         const char* v, size_t vlen) {
-  int* state = (int*)ptr;
-  switch (*state) {
-    case 3:
-      CheckEqual("box", k, klen);
-      CheckEqual("cc", v, vlen);
-      CheckCondition(cfid == 1);
-      break;
-    case 7:
-      CheckEqual("baz", k, klen);
-      CheckEqual("aa", v, vlen);
-      CheckCondition(cfid == 0);
-      break;
-    default:
-      CheckCondition(false);
-      break;
-  }
-  (*state)++;
-}
-
 static void CmpDestroy(void* arg) { (void)arg; }
 
 static int CmpCompare(void* arg, const char* a, size_t alen, const char* b,
@@ -1761,21 +1688,6 @@ int main(int argc, char** argv) {
     CheckPinGetCF(db, roptions, handles[1], "bar", NULL);
     CheckPinGetCF(db, roptions, handles[1], "box", "c");
     CheckPinGetCF(db, roptions, handles[1], "buff", "rocksdb");
-    rocksdb_writebatch_clear(wb);
-    // Test WriteBatch iteration with Column Family
-    int pos = 0;
-    rocksdb_writebatch_put_cf(wb, handles[1], "bar", 3, "b", 1);
-    rocksdb_writebatch_put_cf(wb, handles[1], "box", 3, "c", 1);
-    rocksdb_writebatch_delete_cf(wb, handles[1], "bar", 3);
-    rocksdb_writebatch_merge_cf(wb, handles[1], "box", 3, "cc", 2);
-    rocksdb_writebatch_put(wb, "foo", 3, "f", 1);
-    rocksdb_writebatch_delete(wb, "foo", 3);
-    rocksdb_writebatch_put(wb, "baz", 3, "a", 1);
-    rocksdb_writebatch_merge(wb, "baz", 3, "aa", 2);
-    rocksdb_writebatch_iterate_cf(wb, &pos, CheckPutCF, CheckDelCF,
-                                  CheckMergeCF);
-    CheckCondition(pos == 8);
-    rocksdb_writebatch_clear(wb);
     rocksdb_writebatch_destroy(wb);
 
     rocksdb_flush_wal(db, 1, &err);

db/compaction/compaction.cc

@@ -63,9 +63,7 @@ uint64_t TotalFileSize(const std::vector<FileMetaData*>& files) {
   return sum;
 }
 
-// TODO(hx235): consider making this function part of the construction so we
-// don't forget to call it
-void Compaction::FinalizeInputInfo(Version* _input_version) {
+void Compaction::SetInputVersion(Version* _input_version) {
   input_version_ = _input_version;
   cfd_ = input_version_->cfd();
 
@@ -866,11 +864,6 @@ bool Compaction::ShouldFormSubcompactions() const {
     return false;
   }
 
-  if (cfd_->ioptions()->table_factory->Name() ==
-      TableFactory::kPlainTableName()) {
-    return false;
-  }
-
   // Round-Robin pri under leveled compaction allows subcompactions by default
   // and the number of subcompactions can be larger than max_subcompactions_
   if (cfd_->ioptions()->compaction_pri == kRoundRobin &&

db/compaction/compaction.h

@@ -289,7 +289,14 @@ class Compaction {
   // is the sum of all input file sizes.
   uint64_t OutputFilePreallocationSize() const;
 
-  void FinalizeInputInfo(Version* input_version);
+  // TODO(hx235): eventually we should consider `InitInputTableProperties()`'s
+  // status and fail the compaction if needed
+  // TODO(hx235): consider making this function part of the construction so we
+  // don't forget to call it
+  void FinalizeInputInfo(Version* input_version) {
+    SetInputVersion(input_version);
+    InitInputTableProperties().PermitUncheckedError();
+  }
 
   struct InputLevelSummaryBuffer {
     char buffer[128];
@@ -326,16 +333,6 @@ class Compaction {
       int output_level, VersionStorageInfo* vstorage,
       const std::vector<CompactionInputFiles>& inputs);
 
-  // TODO(hx235): eventually we should consider `InitInputTableProperties()`'s
-  // status and fail the compaction if needed
-  //
-  // May open and read table files for table property.
-  // Should not be called while holding mutex_.
-  const TablePropertiesCollection& GetOrInitInputTableProperties() {
-    InitInputTableProperties().PermitUncheckedError();
-    return input_table_properties_;
-  }
-
   const TablePropertiesCollection& GetInputTableProperties() const {
     return input_table_properties_;
   }
@@ -436,6 +433,7 @@ class Compaction {
                                       const int output_level);
 
  private:
+  void SetInputVersion(Version* input_version);
 
   Status InitInputTableProperties();
 

db/compaction/compaction_job.cc

@@ -468,11 +468,6 @@ void CompactionJob::GenSubcompactionBoundaries() {
   ReadOptions read_options(Env::IOActivity::kCompaction);
   read_options.rate_limiter_priority = GetRateLimiterPriority();
   auto* c = compact_->compaction;
-  if (c->immutable_options()->table_factory->Name() ==
-      TableFactory::kPlainTableName()) {
-    return;
-  }
-
   if (c->max_subcompactions() <= 1 &&
       !(c->immutable_options()->compaction_pri == kRoundRobin &&
         c->immutable_options()->compaction_style == kCompactionStyleLevel)) {
@@ -626,7 +621,6 @@ Status CompactionJob::Run() {
   const size_t num_threads = compact_->sub_compact_states.size();
   assert(num_threads > 0);
   const uint64_t start_micros = db_options_.clock->NowMicros();
-  compact_->compaction->GetOrInitInputTableProperties();
 
   // Launch a thread for each of subcompactions 1...num_threads-1
   std::vector<port::Thread> thread_pool;

db/compaction/compaction_service_job.cc

@@ -275,8 +275,6 @@ Status CompactionServiceCompactionJob::Run() {
   log_buffer_->FlushBufferToLog();
   LogCompaction();
   const uint64_t start_micros = db_options_.clock->NowMicros();
-  c->GetOrInitInputTableProperties();
-
   // Pick the only sub-compaction we should have
   assert(compact_->sub_compact_states.size() == 1);
   SubcompactionState* sub_compact = compact_->sub_compact_states.data();

db/db_bloom_filter_test.cc

@@ -322,67 +322,6 @@ TEST_F(DBBloomFilterTest, GetFilterByPrefixBloom) {
   }
 }
 
-TEST_F(DBBloomFilterTest, FilterNumEntriesCoalesce) {
-  for (bool partition_filters : {true, false}) {
-    SCOPED_TRACE("partition_filters=" + std::to_string(partition_filters));
-    for (bool prefix : {true, false}) {
-      SCOPED_TRACE("prefix=" + std::to_string(prefix));
-      for (bool whole : {true, false}) {
-        SCOPED_TRACE("whole=" + std::to_string(whole));
-        Options options = last_options_;
-        options.prefix_extractor.reset();
-        if (prefix) {
-          options.prefix_extractor.reset(NewFixedPrefixTransform(3));
-        }
-        BlockBasedTableOptions bbto;
-        bbto.filter_policy.reset(NewBloomFilterPolicy(10));
-        bbto.whole_key_filtering = whole;
-        if (partition_filters) {
-          bbto.partition_filters = true;
-          bbto.index_type =
-              BlockBasedTableOptions::IndexType::kTwoLevelIndexSearch;
-        }
-        options.table_factory.reset(NewBlockBasedTableFactory(bbto));
-        DestroyAndReopen(options);
-
-        // Need a snapshot to allow keeping multiple entries for the same key
-        std::vector<const Snapshot*> snapshots;
-        for (int i = 1; i <= 3; ++i) {
-          std::string val = "val" + std::to_string(i);
-          ASSERT_OK(Put("foo1", val));
-          ASSERT_OK(Put("foo2", val));
-          ASSERT_OK(Put("bar1", val));
-          ASSERT_OK(Put("bar2", val));
-          ASSERT_OK(Put("bar3", val));
-          snapshots.push_back(db_->GetSnapshot());
-        }
-        ASSERT_OK(Flush());
-
-        TablePropertiesCollection tpc;
-        ASSERT_OK(db_->GetPropertiesOfAllTables(&tpc));
-        // sanity checks
-        ASSERT_EQ(tpc.size(), 1U);
-        auto& tp = *tpc.begin()->second;
-        EXPECT_EQ(tp.num_entries, 3U * 5U);
-
-        // test checks
-        unsigned ex_filter_entries = 0;
-        if (whole) {
-          ex_filter_entries += 5;  // unique keys
-        }
-        if (prefix) {
-          ex_filter_entries += 2;  // unique prefixes
-        }
-        EXPECT_EQ(tp.num_filter_entries, ex_filter_entries);
-
-        for (auto* sn : snapshots) {
-          db_->ReleaseSnapshot(sn);
-        }
-      }
-    }
-  }
-}
-
 TEST_F(DBBloomFilterTest, WholeKeyFilterProp) {
   for (bool partition_filters : {true, false}) {
     Options options = last_options_;
@@ -3204,55 +3143,52 @@ TEST_F(DBBloomFilterTest, DynamicBloomFilterOptions) {
 }
 
 TEST_F(DBBloomFilterTest, SeekForPrevWithPartitionedFilters) {
-  for (bool wkf : {true, false}) {
-    SCOPED_TRACE("whole_key_filtering=" + std::to_string(wkf));
-    Options options = CurrentOptions();
-    constexpr size_t kNumKeys = 10000;
-    static_assert(kNumKeys <= 10000, "kNumKeys have to be <= 10000");
-    options.memtable_factory.reset(
-        test::NewSpecialSkipListFactory(kNumKeys + 10));
-    options.create_if_missing = true;
-    constexpr size_t kPrefixLength = 4;
-    options.prefix_extractor.reset(NewFixedPrefixTransform(kPrefixLength));
-    options.compression = kNoCompression;
-    BlockBasedTableOptions bbto;
-    bbto.filter_policy.reset(NewBloomFilterPolicy(50));
-    bbto.index_shortening =
-        BlockBasedTableOptions::IndexShorteningMode::kNoShortening;
-    bbto.block_size = 128;
-    bbto.metadata_block_size = 128;
-    bbto.partition_filters = true;
-    bbto.index_type = BlockBasedTableOptions::IndexType::kTwoLevelIndexSearch;
-    bbto.whole_key_filtering = wkf;
-    options.table_factory.reset(NewBlockBasedTableFactory(bbto));
-    DestroyAndReopen(options);
+  Options options = CurrentOptions();
+  constexpr size_t kNumKeys = 10000;
+  static_assert(kNumKeys <= 10000, "kNumKeys have to be <= 10000");
+  options.memtable_factory.reset(
+      test::NewSpecialSkipListFactory(kNumKeys + 10));
+  options.create_if_missing = true;
+  constexpr size_t kPrefixLength = 4;
+  options.prefix_extractor.reset(NewFixedPrefixTransform(kPrefixLength));
+  options.compression = kNoCompression;
+  BlockBasedTableOptions bbto;
+  bbto.filter_policy.reset(NewBloomFilterPolicy(50));
+  bbto.index_shortening =
+      BlockBasedTableOptions::IndexShorteningMode::kNoShortening;
+  bbto.block_size = 128;
+  bbto.metadata_block_size = 128;
+  bbto.partition_filters = true;
+  bbto.index_type = BlockBasedTableOptions::IndexType::kTwoLevelIndexSearch;
+  options.table_factory.reset(NewBlockBasedTableFactory(bbto));
+  DestroyAndReopen(options);
 
-    const std::string value(64, '\0');
+  const std::string value(64, '\0');
 
-    WriteOptions write_opts;
-    write_opts.disableWAL = true;
-    for (size_t i = 0; i < kNumKeys; ++i) {
-      std::ostringstream oss;
-      oss << std::setfill('0') << std::setw(4) << std::fixed << i;
-      ASSERT_OK(db_->Put(write_opts, oss.str(), value));
-    }
-    ASSERT_OK(Flush());
-
-    ReadOptions read_opts;
-    // Use legacy, implicit prefix seek
-    read_opts.total_order_seek = false;
-    read_opts.auto_prefix_mode = false;
-    std::unique_ptr<Iterator> it(db_->NewIterator(read_opts));
-    for (size_t i = 0; i < kNumKeys; ++i) {
-      // Seek with a key after each one added but with same prefix. One will
-      // surely cross a partition boundary.
-      std::ostringstream oss;
-      oss << std::setfill('0') << std::setw(4) << std::fixed << i << "a";
-      it->SeekForPrev(oss.str());
-      ASSERT_OK(it->status());
-      ASSERT_TRUE(it->Valid());
-    }
+  WriteOptions write_opts;
+  write_opts.disableWAL = true;
+  for (size_t i = 0; i < kNumKeys; ++i) {
+    std::ostringstream oss;
+    oss << std::setfill('0') << std::setw(4) << std::fixed << i;
+    ASSERT_OK(db_->Put(write_opts, oss.str(), value));
   }
+  ASSERT_OK(Flush());
+
+  ReadOptions read_opts;
+  // Use legacy, implicit prefix seek
+  read_opts.total_order_seek = false;
+  read_opts.auto_prefix_mode = false;
+  std::unique_ptr<Iterator> it(db_->NewIterator(read_opts));
+  for (size_t i = 0; i < kNumKeys; ++i) {
+    // Seek with a key after each one added but with same prefix. One will
+    // surely cross a partition boundary.
+    std::ostringstream oss;
+    oss << std::setfill('0') << std::setw(4) << std::fixed << i << "a";
+    it->SeekForPrev(oss.str());
+    ASSERT_OK(it->status());
+    ASSERT_TRUE(it->Valid());
+  }
+  it.reset();
 }
 
 namespace {

db/db_filesnapshot.cc

@@ -21,7 +21,6 @@
 #include "rocksdb/db.h"
 #include "rocksdb/env.h"
 #include "rocksdb/metadata.h"
-#include "rocksdb/transaction_log.h"
 #include "rocksdb/types.h"
 #include "test_util/sync_point.h"
 #include "util/file_checksum_helper.h"
@@ -93,11 +92,6 @@ Status DBImpl::GetLiveFiles(std::vector<std::string>& ret,
 }
 
 Status DBImpl::GetSortedWalFiles(VectorWalPtr& files) {
-  return GetSortedWalFilesImpl(files,
-                               /*need_seqnos*/ true);
-}
-
-Status DBImpl::GetSortedWalFilesImpl(VectorWalPtr& files, bool need_seqnos) {
   // Record tracked WALs as a (minimum) cross-check for directory scan
   std::vector<uint64_t> required_by_manifest;
 
@@ -123,10 +117,7 @@ Status DBImpl::GetSortedWalFilesImpl(VectorWalPtr& files, bool need_seqnos) {
     }
   }
 
-  // NOTE: need to include archived WALs because needed WALs might have been
-  // archived since getting required_by_manifest set
-  Status s = wal_manager_.GetSortedWalFiles(files, need_seqnos,
-                                            /*include_archived*/ true);
+  Status s = wal_manager_.GetSortedWalFiles(files);
 
   // DisableFileDeletions / EnableFileDeletions not supported in read-only DB
   if (deletions_disabled.ok()) {
@@ -215,12 +206,7 @@ Status DBImpl::GetLiveFilesStorageInfo(
     } else if (opts.wal_size_for_flush > 0) {
       // FIXME: avoid querying the filesystem for current WAL state
       // If the outstanding WAL files are small, we skip the flush.
-      // Don't take archived log size into account when calculating wal
-      // size for flush, and don't need to verify consistency with manifest
-      // here & now.
-      s = wal_manager_.GetSortedWalFiles(live_wal_files,
-                                         /* need_seqnos */ false,
-                                         /*include_archived*/ false);
+      s = GetSortedWalFiles(live_wal_files);
 
       if (!s.ok()) {
         return s;
@@ -231,7 +217,6 @@ Status DBImpl::GetLiveFilesStorageInfo(
       // We may be able to cover 2PC case too.
       uint64_t total_wal_size = 0;
       for (auto& wal : live_wal_files) {
-        assert(wal->Type() == kAliveLogFile);
         total_wal_size += wal->SizeFileBytes();
       }
       if (total_wal_size < opts.wal_size_for_flush) {
@@ -443,8 +428,7 @@ Status DBImpl::GetLiveFilesStorageInfo(
   // WAL files.
   if (s.ok()) {
     // FIXME: avoid querying the filesystem for current WAL state
-    s = GetSortedWalFilesImpl(live_wal_files,
-                              /* need_seqnos */ false);
+    s = GetSortedWalFiles(live_wal_files);
   }
   if (!s.ok()) {
     return s;

db/db_impl/db_impl.cc

@@ -392,8 +392,8 @@ Status DBImpl::ResumeImpl(DBRecoverContext context) {
       if (!s.ok()) {
         io_s = versions_->io_status();
         if (!io_s.ok()) {
-          error_handler_.SetBGError(io_s,
-                                    BackgroundErrorReason::kManifestWrite);
+          s = error_handler_.SetBGError(io_s,
+                                        BackgroundErrorReason::kManifestWrite);
         }
       }
     }
@@ -917,8 +917,8 @@ Status DBImpl::RegisterRecordSeqnoTimeWorker(const ReadOptions& read_options,
             read_options, write_options, &edit, &mutex_,
             directories_.GetDbDir());
         if (!s.ok() && versions_->io_status().IsIOError()) {
-          error_handler_.SetBGError(versions_->io_status(),
-                                    BackgroundErrorReason::kManifestWrite);
+          s = error_handler_.SetBGError(versions_->io_status(),
+                                        BackgroundErrorReason::kManifestWrite);
         }
       }
 
@@ -1725,8 +1725,8 @@ Status DBImpl::ApplyWALToManifest(const ReadOptions& read_options,
       read_options, write_options, synced_wals, &mutex_,
       directories_.GetDbDir());
   if (!status.ok() && versions_->io_status().IsIOError()) {
-    error_handler_.SetBGError(versions_->io_status(),
-                              BackgroundErrorReason::kManifestWrite);
+    status = error_handler_.SetBGError(versions_->io_status(),
+                                       BackgroundErrorReason::kManifestWrite);
   }
   return status;
 }
@@ -2065,19 +2065,19 @@ InternalIterator* DBImpl::NewInternalIterator(
       read_options, super_version->GetSeqnoToTimeMapping(), arena);
   Status s;
   if (!read_options.ignore_range_deletions) {
-    std::unique_ptr<TruncatedRangeDelIterator> mem_tombstone_iter;
+    TruncatedRangeDelIterator* mem_tombstone_iter = nullptr;
     auto range_del_iter = super_version->mem->NewRangeTombstoneIterator(
         read_options, sequence, false /* immutable_memtable */);
     if (range_del_iter == nullptr || range_del_iter->empty()) {
       delete range_del_iter;
     } else {
-      mem_tombstone_iter = std::make_unique<TruncatedRangeDelIterator>(
+      mem_tombstone_iter = new TruncatedRangeDelIterator(
           std::unique_ptr<FragmentedRangeTombstoneIterator>(range_del_iter),
           &cfd->ioptions()->internal_comparator, nullptr /* smallest */,
           nullptr /* largest */);
     }
-    merge_iter_builder.AddPointAndTombstoneIterator(
-        mem_iter, std::move(mem_tombstone_iter));
+    merge_iter_builder.AddPointAndTombstoneIterator(mem_iter,
+                                                    mem_tombstone_iter);
   } else {
     merge_iter_builder.AddIterator(mem_iter);
   }
@@ -5828,18 +5828,6 @@ Status DBImpl::IngestExternalFiles(
             "timestamps enabled doesn't support ingest behind.");
       }
     }
-    if (ingest_opts.allow_db_generated_files) {
-      if (ingest_opts.write_global_seqno) {
-        return Status::NotSupported(
-            "write_global_seqno is deprecated and does not work with "
-            "allow_db_generated_files.");
-      }
-      if (ingest_opts.move_files) {
-        return Status::NotSupported(
-            "Options move_files and allow_db_generated_files are not "
-            "compatible.");
-      }
-    }
   }
 
   // TODO (yanqin) maybe handle the case in which column_families have

db/db_impl/db_impl.h

@@ -508,8 +508,6 @@ class DBImpl : public DB {
   Status GetLiveFiles(std::vector<std::string>&, uint64_t* manifest_file_size,
                       bool flush_memtable = true) override;
   Status GetSortedWalFiles(VectorWalPtr& files) override;
-  Status GetSortedWalFilesImpl(VectorWalPtr& files, bool need_seqnos);
-
   // Get the known flushed sizes of WALs that might still be written to
   // or have pending sync.
   // NOTE: unlike alive_log_files_, this function includes WALs that might
@@ -2350,9 +2348,6 @@ class DBImpl : public DB {
   bool HaveManualCompaction(ColumnFamilyData* cfd);
   bool MCOverlap(ManualCompactionState* m, ManualCompactionState* m1);
   void UpdateDeletionCompactionStats(const std::unique_ptr<Compaction>& c);
-
-  // May open and read table files for table property.
-  // Should not be called while holding mutex_.
   void BuildCompactionJobInfo(const ColumnFamilyData* cfd, Compaction* c,
                               const Status& st,
                               const CompactionJobStats& compaction_job_stats,

db/db_impl/db_impl_compaction_flush.cc

@@ -1595,12 +1595,10 @@ Status DBImpl::CompactFilesImpl(
 
   ReleaseFileNumberFromPendingOutputs(pending_outputs_inserted_elem);
 
-  mutex_.Unlock();
   if (compaction_job_info != nullptr) {
     BuildCompactionJobInfo(cfd, c.get(), s, compaction_job_stats,
                            job_context->job_id, compaction_job_info);
   }
-  mutex_.Lock();
 
   if (status.ok()) {
     // Done
@@ -4163,7 +4161,7 @@ void DBImpl::BuildCompactionJobInfo(
   compaction_job_info->base_input_level = c->start_level();
   compaction_job_info->output_level = c->output_level();
   compaction_job_info->stats = compaction_job_stats;
-  const auto& input_table_properties = c->GetOrInitInputTableProperties();
+  const auto& input_table_properties = c->GetInputTableProperties();
   const auto& output_table_properties = c->GetOutputTableProperties();
   compaction_job_info->table_properties.insert(input_table_properties.begin(),
                                                input_table_properties.end());

db/db_impl/db_impl_open.cc

@@ -530,12 +530,6 @@ Status DBImpl::Recover(
                            /*no_error_if_files_missing=*/false, is_retry,
                            &desc_status);
     desc_status.PermitUncheckedError();
-    if (is_retry) {
-      RecordTick(stats_, FILE_READ_CORRUPTION_RETRY_COUNT);
-      if (desc_status.ok()) {
-        RecordTick(stats_, FILE_READ_CORRUPTION_RETRY_SUCCESS_COUNT);
-      }
-    }
     if (can_retry) {
       // If we're opening for the first time and the failure is likely due to
       // a corrupt MANIFEST file (could result in either the log::Reader
@@ -646,7 +640,7 @@ Status DBImpl::Recover(
                            f->fd.smallest_seqno, f->fd.largest_seqno,
                            f->marked_for_compaction,
                            f->temperature,  // this can be different from
-                           // `last_level_temperature`
+                                            // `last_level_temperature`
                            f->oldest_blob_file_number, f->oldest_ancester_time,
                            f->file_creation_time, f->epoch_number,
                            f->file_checksum, f->file_checksum_func_name,

db/db_impl/db_impl_write.cc

@@ -556,35 +556,6 @@ Status DBImpl::WriteImpl(const WriteOptions& write_options,
     const SequenceNumber current_sequence = last_sequence + 1;
     last_sequence += seq_inc;
 
-    if (log_context.need_log_sync) {
-      VersionEdit synced_wals;
-      log_write_mutex_.Lock();
-      if (status.ok()) {
-        MarkLogsSynced(logfile_number_, log_context.need_log_dir_sync,
-                       &synced_wals);
-      } else {
-        MarkLogsNotSynced(logfile_number_);
-      }
-      log_write_mutex_.Unlock();
-      if (status.ok() && synced_wals.IsWalAddition()) {
-        InstrumentedMutexLock l(&mutex_);
-        // TODO: plumb Env::IOActivity, Env::IOPriority
-        const ReadOptions read_options;
-        status = ApplyWALToManifest(read_options, write_options, &synced_wals);
-      }
-
-      // Requesting sync with two_write_queues_ is expected to be very rare. We
-      // hence provide a simple implementation that is not necessarily
-      // efficient.
-      if (status.ok() && two_write_queues_) {
-        if (manual_wal_flush_) {
-          status = FlushWAL(true);
-        } else {
-          status = SyncWAL();
-        }
-      }
-    }
-
     // PreReleaseCallback is called after WAL write and before memtable write
     if (status.ok()) {
       SequenceNumber next_sequence = current_sequence;
@@ -668,6 +639,34 @@ Status DBImpl::WriteImpl(const WriteOptions& write_options,
     assert(pre_release_cb_status.ok());
   }
 
+  if (log_context.need_log_sync) {
+    VersionEdit synced_wals;
+    log_write_mutex_.Lock();
+    if (status.ok()) {
+      MarkLogsSynced(logfile_number_, log_context.need_log_dir_sync,
+                     &synced_wals);
+    } else {
+      MarkLogsNotSynced(logfile_number_);
+    }
+    log_write_mutex_.Unlock();
+    if (status.ok() && synced_wals.IsWalAddition()) {
+      InstrumentedMutexLock l(&mutex_);
+      // TODO: plumb Env::IOActivity, Env::IOPriority
+      const ReadOptions read_options;
+      status = ApplyWALToManifest(read_options, write_options, &synced_wals);
+    }
+
+    // Requesting sync with two_write_queues_ is expected to be very rare. We
+    // hence provide a simple implementation that is not necessarily efficient.
+    if (two_write_queues_) {
+      if (manual_wal_flush_) {
+        status = FlushWAL(true);
+      } else {
+        status = SyncWAL();
+      }
+    }
+  }
+
   bool should_exit_batch_group = true;
   if (in_parallel_group) {
     // CompleteParallelWorker returns true if this thread should
@@ -1205,7 +1204,8 @@ void DBImpl::MemTableInsertStatusCheck(const Status& status) {
     mutex_.Lock();
     assert(!error_handler_.IsBGWorkStopped());
     // Maybe change the return status to void?
-    error_handler_.SetBGError(status, BackgroundErrorReason::kMemTable);
+    error_handler_.SetBGError(status, BackgroundErrorReason::kMemTable)
+        .PermitUncheckedError();
     mutex_.Unlock();
   }
 }
@@ -2363,8 +2363,8 @@ Status DBImpl::SwitchMemtable(ColumnFamilyData* cfd, WriteContext* context) {
           read_options, write_options, &wal_deletion, &mutex_,
           directories_.GetDbDir());
       if (!s.ok() && versions_->io_status().IsIOError()) {
-        error_handler_.SetBGError(versions_->io_status(),
-                                  BackgroundErrorReason::kManifestWrite);
+        s = error_handler_.SetBGError(versions_->io_status(),
+                                      BackgroundErrorReason::kManifestWrite);
       }
       if (!s.ok()) {
         return s;

db/db_io_failure_test.cc

@@ -705,7 +705,6 @@ class DBIOCorruptionTest
   DBIOCorruptionTest() : DBIOFailureTest() {
     BlockBasedTableOptions bbto;
     options_ = CurrentOptions();
-    options_.statistics = CreateDBStatistics();
 
     base_env_ = env_;
     EXPECT_NE(base_env_, nullptr);
@@ -728,8 +727,6 @@ class DBIOCorruptionTest
 
   Status ReopenDB() { return TryReopen(options_); }
 
-  Statistics* stats() { return options_.statistics.get(); }
-
  protected:
   std::unique_ptr<Env> env_guard_;
   std::shared_ptr<CorruptionFS> fs_;
@@ -752,12 +749,8 @@ TEST_P(DBIOCorruptionTest, GetReadCorruptionRetry) {
   if (std::get<2>(GetParam())) {
     ASSERT_OK(s);
     ASSERT_EQ(val, "val1");
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_COUNT), 1);
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_SUCCESS_COUNT),
-              1);
   } else {
     ASSERT_TRUE(s.IsCorruption());
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_COUNT), 0);
   }
 }
 
@@ -780,12 +773,8 @@ TEST_P(DBIOCorruptionTest, IterReadCorruptionRetry) {
   }
   if (std::get<2>(GetParam())) {
     ASSERT_OK(iter->status());
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_COUNT), 1);
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_SUCCESS_COUNT),
-              1);
   } else {
     ASSERT_TRUE(iter->status().IsCorruption());
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_COUNT), 0);
   }
   delete iter;
 }
@@ -810,13 +799,9 @@ TEST_P(DBIOCorruptionTest, MultiGetReadCorruptionRetry) {
   if (std::get<2>(GetParam())) {
     ASSERT_EQ(values[0].ToString(), "val1");
     ASSERT_EQ(values[1].ToString(), "val2");
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_COUNT), 1);
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_SUCCESS_COUNT),
-              1);
   } else {
     ASSERT_TRUE(statuses[0].IsCorruption());
     ASSERT_TRUE(statuses[1].IsCorruption());
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_COUNT), 0);
   }
 }
 
@@ -833,9 +818,6 @@ TEST_P(DBIOCorruptionTest, CompactionReadCorruptionRetry) {
   Status s = dbfull()->CompactRange(CompactRangeOptions(), nullptr, nullptr);
   if (std::get<2>(GetParam())) {
     ASSERT_OK(s);
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_COUNT), 1);
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_SUCCESS_COUNT),
-              1);
 
     std::string val;
     ReadOptions ro;
@@ -844,7 +826,6 @@ TEST_P(DBIOCorruptionTest, CompactionReadCorruptionRetry) {
     ASSERT_EQ(val, "val1");
   } else {
     ASSERT_TRUE(s.IsCorruption());
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_COUNT), 0);
   }
 }
 
@@ -857,9 +838,6 @@ TEST_P(DBIOCorruptionTest, FlushReadCorruptionRetry) {
   Status s = Flush();
   if (std::get<2>(GetParam())) {
     ASSERT_OK(s);
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_COUNT), 1);
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_SUCCESS_COUNT),
-              1);
 
     std::string val;
     ReadOptions ro;
@@ -868,7 +846,6 @@ TEST_P(DBIOCorruptionTest, FlushReadCorruptionRetry) {
     ASSERT_EQ(val, "val1");
   } else {
     ASSERT_NOK(s);
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_COUNT), 0);
   }
 }
 
@@ -885,12 +862,8 @@ TEST_P(DBIOCorruptionTest, ManifestCorruptionRetry) {
 
   if (std::get<2>(GetParam())) {
     ASSERT_OK(ReopenDB());
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_COUNT), 1);
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_SUCCESS_COUNT),
-              1);
   } else {
     ASSERT_EQ(ReopenDB(), Status::Corruption());
-    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_COUNT), 0);
   }
   SyncPoint::GetInstance()->DisableProcessing();
 }

db/db_rate_limiter_test.cc

@@ -245,13 +245,7 @@ TEST_P(DBRateLimiterOnReadTest, VerifyChecksum) {
   // In DirectIO, where we support tail prefetching, during table open, we only
   // do 1 read instead of 4 as described above. Actual checksum verification
   // reads stay the same.
-#ifdef OS_WIN
-  // No file system prefetch implemented for OS Win. During table open,
-  // we only do 1 read for BufferedIO.
-  int num_read_per_file = 4;
-#else
   int num_read_per_file = (!use_direct_io_) ? 7 : 4;
-#endif
   int expected = kNumFiles * num_read_per_file;
 
   ASSERT_EQ(expected, options_.rate_limiter->GetTotalRequests(Env::IO_USER));

db/db_sst_test.cc

@@ -921,7 +921,7 @@ TEST_P(DBWALTestWithParam, WALTrashCleanupOnOpen) {
       // before restarting the DB.
       // We have to set this on the 2nd to last file for it to delay deletion
       // on the last file. (Quirk of DeleteScheduler::BackgroundEmptyTrash())
-      options.sst_file_manager->SetDeleteRateBytesPerSecond(1);
+      options.sst_file_manager->SetDeleteRateBytesPerSecond(1024 * 1024);
     }
     ASSERT_OK(Put("Key2", DummyString(1024, v)));
     ASSERT_OK(Put("Key3", DummyString(1024, v)));

db/db_test_util.cc

@@ -565,11 +565,6 @@ Options DBTestBase::GetOptions(
       options.unordered_write = false;
       break;
     }
-    case kBlockBasedTableWithBinarySearchWithFirstKeyIndex: {
-      table_options.index_type =
-          BlockBasedTableOptions::kBinarySearchWithFirstKey;
-      break;
-    }
 
     default:
       break;

db/db_test_util.h

@@ -1041,7 +1041,6 @@ class DBTestBase : public testing::Test {
     kPartitionedFilterWithNewTableReaderForCompactions,
     kUniversalSubcompactions,
     kUnorderedWrite,
-    kBlockBasedTableWithBinarySearchWithFirstKeyIndex,
     // This must be the last line
     kEnd,
   };

db/error_handler.cc

@@ -250,6 +250,8 @@ void ErrorHandler::CancelErrorRecovery() {
   EndAutoRecovery();
 }
 
+STATIC_AVOID_DESTRUCTION(const Status, kOkStatus){Status::OK()};
+
 // This is the main function for looking at an error during a background
 // operation and deciding the severity, and error recovery strategy. The high
 // level algorithm is as follows -
@@ -268,11 +270,11 @@ void ErrorHandler::CancelErrorRecovery() {
 // This can also get called as part of a recovery operation. In that case, we
 // also track the error separately in recovery_error_ so we can tell in the
 // end whether recovery succeeded or not
-void ErrorHandler::HandleKnownErrors(const Status& bg_err,
-                                     BackgroundErrorReason reason) {
+const Status& ErrorHandler::HandleKnownErrors(const Status& bg_err,
+                                              BackgroundErrorReason reason) {
   db_mutex_->AssertHeld();
   if (bg_err.ok()) {
-    return;
+    return kOkStatus;
   }
 
   ROCKS_LOG_INFO(db_options_.info_log,
@@ -337,7 +339,7 @@ void ErrorHandler::HandleKnownErrors(const Status& bg_err,
     } else {
       // This error is less severe than previously encountered error. Don't
       // take any further action
-      return;
+      return bg_error_;
     }
   }
 
@@ -354,6 +356,7 @@ void ErrorHandler::HandleKnownErrors(const Status& bg_err,
   if (bg_error_.severity() >= Status::Severity::kHardError) {
     is_db_stopped_.store(true, std::memory_order_release);
   }
+  return bg_error_;
 }
 
 // This is the main function for looking at IO related error during the
@@ -380,14 +383,14 @@ void ErrorHandler::HandleKnownErrors(const Status& bg_err,
 // 3) for other cases, HandleKnownErrors(const Status& bg_err,
 //    BackgroundErrorReason reason) will be called to handle other error cases
 //    such as delegating to SstFileManager to handle no space error.
-void ErrorHandler::SetBGError(const Status& bg_status,
-                              BackgroundErrorReason reason) {
+const Status& ErrorHandler::SetBGError(const Status& bg_status,
+                                       BackgroundErrorReason reason) {
   db_mutex_->AssertHeld();
   Status tmp_status = bg_status;
   IOStatus bg_io_err = status_to_io_status(std::move(tmp_status));
 
   if (bg_io_err.ok()) {
-    return;
+    return kOkStatus;
   }
   ROCKS_LOG_WARN(db_options_.info_log, "Background IO error %s",
                  bg_io_err.ToString().c_str());
@@ -410,11 +413,11 @@ void ErrorHandler::SetBGError(const Status& bg_status,
     EventHelpers::NotifyOnBackgroundError(db_options_.listeners, reason,
                                           &bg_err, db_mutex_, &auto_recovery);
     recover_context_ = context;
-    return;
-  }
-  if (bg_io_err.subcode() != IOStatus::SubCode::kNoSpace &&
-      (bg_io_err.GetScope() == IOStatus::IOErrorScope::kIOErrorScopeFile ||
-       bg_io_err.GetRetryable())) {
+    return bg_error_;
+  } else if (bg_io_err.subcode() != IOStatus::SubCode::kNoSpace &&
+             (bg_io_err.GetScope() ==
+                  IOStatus::IOErrorScope::kIOErrorScopeFile ||
+              bg_io_err.GetRetryable())) {
     // Second, check if the error is a retryable IO error (file scope IO error
     // is also treated as retryable IO error in RocksDB write path). if it is
     // retryable error and its severity is higher than bg_error_, overwrite the
@@ -423,6 +426,10 @@ void ErrorHandler::SetBGError(const Status& bg_status,
     // IO error as hard error. Note that, all the NoSpace error should be
     // handled by the SstFileManager::StartErrorRecovery(). Therefore, no matter
     // it is retryable or file scope, this logic will be bypassed.
+    bool auto_recovery = false;
+    EventHelpers::NotifyOnBackgroundError(db_options_.listeners, reason,
+                                          &new_bg_io_err, db_mutex_,
+                                          &auto_recovery);
 
     RecordStats({ERROR_HANDLER_BG_RETRYABLE_IO_ERROR_COUNT},
                 {} /* int_histograms */);
@@ -438,13 +445,9 @@ void ErrorHandler::SetBGError(const Status& bg_status,
       ROCKS_LOG_INFO(
           db_options_.info_log,
           "ErrorHandler: Compaction will schedule by itself to resume\n");
-      bool auto_recovery = false;
-      EventHelpers::NotifyOnBackgroundError(db_options_.listeners, reason,
-                                            &new_bg_io_err, db_mutex_,
-                                            &auto_recovery);
       // Not used in this code path.
       new_bg_io_err.PermitUncheckedError();
-      return;
+      return bg_error_;
     }
 
     Status::Severity severity;
@@ -466,14 +469,10 @@ void ErrorHandler::SetBGError(const Status& bg_status,
     Status bg_err(new_bg_io_err, severity);
     CheckAndSetRecoveryAndBGError(bg_err);
     recover_context_ = context;
-    bool auto_recovery = db_options_.max_bgerror_resume_count > 0;
-    EventHelpers::NotifyOnBackgroundError(db_options_.listeners, reason,
-                                          &new_bg_io_err, db_mutex_,
-                                          &auto_recovery);
-    StartRecoverFromRetryableBGIOError(bg_io_err);
-    return;
+    return StartRecoverFromRetryableBGIOError(bg_io_err);
+  } else {
+    return HandleKnownErrors(new_bg_io_err, reason);
   }
-  HandleKnownErrors(new_bg_io_err, reason);
 }
 
 void ErrorHandler::AddFilesToQuarantine(
@@ -621,23 +620,23 @@ Status ErrorHandler::RecoverFromBGError(bool is_manual) {
   return s;
 }
 
-void ErrorHandler::StartRecoverFromRetryableBGIOError(
+const Status& ErrorHandler::StartRecoverFromRetryableBGIOError(
     const IOStatus& io_error) {
   db_mutex_->AssertHeld();
-  if (bg_error_.ok() || io_error.ok()) {
-    return;
-  }
-  if (db_options_.max_bgerror_resume_count <= 0 || recovery_in_prog_) {
-    // Auto resume BG error is not enabled
-    return;
-  }
-  if (end_recovery_) {
+  if (bg_error_.ok()) {
+    return bg_error_;
+  } else if (io_error.ok()) {
+    return kOkStatus;
+  } else if (db_options_.max_bgerror_resume_count <= 0 || recovery_in_prog_) {
+    // Auto resume BG error is not enabled, directly return bg_error_.
+    return bg_error_;
+  } else if (end_recovery_) {
     // Can temporarily release db mutex
     EventHelpers::NotifyOnErrorRecoveryEnd(db_options_.listeners, bg_error_,
                                            Status::ShutdownInProgress(),
                                            db_mutex_);
     db_mutex_->AssertHeld();
-    return;
+    return bg_error_;
   }
   RecordStats({ERROR_HANDLER_AUTORESUME_COUNT}, {} /* int_histograms */);
   ROCKS_LOG_INFO(
@@ -665,6 +664,12 @@ void ErrorHandler::StartRecoverFromRetryableBGIOError(
 
   recovery_thread_.reset(
       new port::Thread(&ErrorHandler::RecoverFromRetryableBGIOError, this));
+
+  if (recovery_error_.ok()) {
+    return recovery_error_;
+  } else {
+    return bg_error_;
+  }
 }
 
 // Automatic recover from Retryable BG IO error. Must be called after db

db/error_handler.h

@@ -56,7 +56,7 @@ class ErrorHandler {
   Status::Severity GetErrorSeverity(BackgroundErrorReason reason,
                                     Status::Code code, Status::SubCode subcode);
 
-  void SetBGError(const Status& bg_err, BackgroundErrorReason reason);
+  const Status& SetBGError(const Status& bg_err, BackgroundErrorReason reason);
 
   Status GetBGError() const { return bg_error_; }
 
@@ -135,10 +135,11 @@ class ErrorHandler {
   // unsorted.
   autovector<uint64_t> files_to_quarantine_;
 
-  void HandleKnownErrors(const Status& bg_err, BackgroundErrorReason reason);
+  const Status& HandleKnownErrors(const Status& bg_err,
+                                  BackgroundErrorReason reason);
   Status OverrideNoSpaceError(const Status& bg_error, bool* auto_recovery);
   void RecoverFromNoSpace();
-  void StartRecoverFromRetryableBGIOError(const IOStatus& io_error);
+  const Status& StartRecoverFromRetryableBGIOError(const IOStatus& io_error);
   void RecoverFromRetryableBGIOError();
   // First, if it is in recovery and the recovery_error is ok. Set the
   // recovery_error_ to bg_err. Second, if the severity is higher than the

db/error_handler_fs_test.cc

@@ -851,17 +851,13 @@ TEST_F(DBErrorHandlingFSTest, DoubleManifestWriteError) {
       });
   SyncPoint::GetInstance()->EnableProcessing();
   s = Flush();
-  ASSERT_TRUE(s.IsNoSpace());
-  ASSERT_EQ(dbfull()->TEST_GetBGError().severity(),
-            ROCKSDB_NAMESPACE::Status::Severity::kHardError);
+  ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kHardError);
   ASSERT_FALSE(dbfull()->TEST_GetFilesToQuarantine().empty());
   fault_fs_->SetFilesystemActive(true);
 
   // This Resume() will attempt to create a new manifest file and fail again
   s = dbfull()->Resume();
-  ASSERT_TRUE(s.IsNoSpace());
-  ASSERT_EQ(dbfull()->TEST_GetBGError().severity(),
-            ROCKSDB_NAMESPACE::Status::Severity::kHardError);
+  ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kHardError);
   ASSERT_FALSE(dbfull()->TEST_GetFilesToQuarantine().empty());
   fault_fs_->SetFilesystemActive(true);
   SyncPoint::GetInstance()->ClearAllCallBacks();

db/event_helpers.cc

@@ -228,18 +228,15 @@ void EventHelpers::NotifyOnErrorRecoveryEnd(
     InstrumentedMutex* db_mutex) {
   if (!listeners.empty()) {
     db_mutex->AssertHeld();
-    // Make copies before releasing mutex to avoid race.
-    Status old_bg_error_cp = old_bg_error;
-    Status new_bg_error_cp = new_bg_error;
     // release lock while notifying events
     db_mutex->Unlock();
     TEST_SYNC_POINT("NotifyOnErrorRecoveryEnd:MutexUnlocked:1");
     TEST_SYNC_POINT("NotifyOnErrorRecoveryEnd:MutexUnlocked:2");
     for (auto& listener : listeners) {
       BackgroundErrorRecoveryInfo info;
-      info.old_bg_error = old_bg_error_cp;
-      info.new_bg_error = new_bg_error_cp;
-      listener->OnErrorRecoveryCompleted(old_bg_error_cp);
+      info.old_bg_error = old_bg_error;
+      info.new_bg_error = new_bg_error;
+      listener->OnErrorRecoveryCompleted(old_bg_error);
       listener->OnErrorRecoveryEnd(info);
       info.old_bg_error.PermitUncheckedError();
       info.new_bg_error.PermitUncheckedError();

db/external_sst_file_ingestion_job.cc

@@ -44,12 +44,9 @@ Status ExternalSstFileIngestionJob::Prepare(
       return status;
     }
 
-    // Files generated in another DB or CF may have a different column family
-    // ID, so we let it pass here.
     if (file_to_ingest.cf_id !=
             TablePropertiesCollectorFactory::Context::kUnknownColumnFamily &&
-        file_to_ingest.cf_id != cfd_->GetID() &&
-        !ingestion_options_.allow_db_generated_files) {
+        file_to_ingest.cf_id != cfd_->GetID()) {
       return Status::InvalidArgument(
           "External file column family id don't match");
     }
@@ -114,7 +111,6 @@ Status ExternalSstFileIngestionJob::Prepare(
     const std::string path_inside_db = TableFileName(
         cfd_->ioptions()->cf_paths, f.fd.GetNumber(), f.fd.GetPathId());
     if (ingestion_options_.move_files) {
-      assert(!ingestion_options_.allow_db_generated_files);
       status =
           fs_->LinkFile(path_outside_db, path_inside_db, IOOptions(), nullptr);
       if (status.ok()) {
@@ -708,16 +704,9 @@ Status ExternalSstFileIngestionJob::SanityCheckTableProperties(
   // Get table version
   auto version_iter = uprops.find(ExternalSstFilePropertyNames::kVersion);
   if (version_iter == uprops.end()) {
-    if (!ingestion_options_.allow_db_generated_files) {
-      return Status::Corruption("External file version not found");
-    } else {
-      // 0 is special version for when a file from live DB does not have the
-      // version table property
-      file_to_ingest->version = 0;
-    }
-  } else {
-    file_to_ingest->version = DecodeFixed32(version_iter->second.c_str());
+    return Status::Corruption("External file version not found");
   }
+  file_to_ingest->version = DecodeFixed32(version_iter->second.c_str());
 
   auto seqno_iter = uprops.find(ExternalSstFilePropertyNames::kGlobalSeqno);
   if (file_to_ingest->version == 2) {
@@ -744,15 +733,8 @@ Status ExternalSstFileIngestionJob::SanityCheckTableProperties(
       return Status::InvalidArgument(
           "External SST file V1 does not support global seqno");
     }
-  } else if (file_to_ingest->version == 0) {
-    // allow_db_generated_files is true
-    assert(seqno_iter == uprops.end());
-    file_to_ingest->original_seqno = 0;
-    file_to_ingest->global_seqno_offset = 0;
   } else {
-    return Status::InvalidArgument("External file version " +
-                                   std::to_string(file_to_ingest->version) +
-                                   " is not supported");
+    return Status::InvalidArgument("External file version is not supported");
   }
 
   file_to_ingest->cf_id = static_cast<uint32_t>(props->column_family_id);
@@ -914,25 +896,6 @@ Status ExternalSstFileIngestionJob::GetIngestedFileInfo(
   } else if (!iter->status().ok()) {
     return iter->status();
   }
-  if (ingestion_options_.allow_db_generated_files) {
-    // Verify that all keys have seqno zero.
-    // TODO: store largest seqno in table property and validate it instead.
-    for (iter->SeekToFirst(); iter->Valid(); iter->Next()) {
-      Status pik_status =
-          ParseInternalKey(iter->key(), &key, allow_data_in_errors);
-      if (!pik_status.ok()) {
-        return Status::Corruption("Corrupted key in external file. ",
-                                  pik_status.getState());
-      }
-      if (key.sequence != 0) {
-        return Status::NotSupported(
-            "External file has a key with non zero sequence number.");
-      }
-    }
-    if (!iter->status().ok()) {
-      return iter->status();
-    }
-  }
 
   std::unique_ptr<InternalIterator> range_del_iter(
       table_reader->NewRangeTombstoneIterator(ro));
@@ -948,11 +911,6 @@ Status ExternalSstFileIngestionJob::GetIngestedFileInfo(
         return Status::Corruption("Corrupted key in external file. ",
                                   pik_status.getState());
       }
-      if (key.sequence != 0) {
-        return Status::Corruption(
-            "External file has a range deletion with non zero sequence "
-            "number.");
-      }
       RangeTombstone tombstone(key, range_del_iter->value());
 
       InternalKey start_key = tombstone.SerializeKey();
@@ -1087,18 +1045,11 @@ Status ExternalSstFileIngestionJob::AssignLevelAndSeqnoForIngestedFile(
           "Column family enables user-defined timestamps, please make sure the "
           "key range (without timestamp) of external file does not overlap "
           "with key range (without timestamp) in the db");
-      return status;
     }
     if (*assigned_seqno == 0) {
       *assigned_seqno = last_seqno + 1;
     }
   }
-
-  if (ingestion_options_.allow_db_generated_files && *assigned_seqno != 0) {
-    return Status::InvalidArgument(
-        "An ingested file is assigned to a non-zero sequence number, which is "
-        "incompatible with ingestion option allow_db_generated_files.");
-  }
   return status;
 }
 

db/external_sst_file_test.cc

@@ -3719,7 +3719,10 @@ TEST_F(ExternalSSTFileWithTimestampTest, TimestampsNotPersistedBasic) {
 }
 
 INSTANTIATE_TEST_CASE_P(ExternalSSTFileTest, ExternalSSTFileTest,
-                        testing::Combine(testing::Bool(), testing::Bool()));
+                        testing::Values(std::make_tuple(false, false),
+                                        std::make_tuple(false, true),
+                                        std::make_tuple(true, false),
+                                        std::make_tuple(true, true)));
 
 INSTANTIATE_TEST_CASE_P(ExternSSTFileLinkFailFallbackTest,
                         ExternSSTFileLinkFailFallbackTest,
@@ -3727,339 +3730,6 @@ INSTANTIATE_TEST_CASE_P(ExternSSTFileLinkFailFallbackTest,
                                         std::make_tuple(true, true),
                                         std::make_tuple(false, false)));
 
-class IngestDBGeneratedFileTest : public ExternalSSTFileTestBase,
-                                  public ::testing::WithParamInterface<bool> {
- public:
-  IngestDBGeneratedFileTest() {
-    ingest_opts.allow_db_generated_files = true;
-    ingest_opts.move_files = false;
-    ingest_opts.verify_checksums_before_ingest = GetParam();
-    ingest_opts.snapshot_consistency = false;
-  }
-
- protected:
-  IngestExternalFileOptions ingest_opts;
-};
-
-INSTANTIATE_TEST_CASE_P(BasicMultiConfig, IngestDBGeneratedFileTest,
-                        testing::Bool());
-
-TEST_P(IngestDBGeneratedFileTest, FailureCase) {
-  // Ingesting overlapping data should always fail.
-  do {
-    SCOPED_TRACE("option_config_ = " + std::to_string(option_config_));
-
-    Options options = CurrentOptions();
-    CreateAndReopenWithCF({"toto"}, options);
-    // Fill CFs with overlapping keys. Will try to ingest CF1 into default CF.
-    for (int k = 0; k < 50; ++k) {
-      ASSERT_OK(Put(Key(k), "default_cf_" + Key(k)));
-    }
-    for (int k = 49; k < 100; ++k) {
-      ASSERT_OK(Put(1, Key(k), "cf1_" + Key(k)));
-    }
-    ASSERT_OK(Flush(/*cf=*/1));
-    {
-      // Verify that largest key of the file has non-zero seqno.
-      std::vector<std::vector<FileMetaData>> metadata;
-      dbfull()->TEST_GetFilesMetaData(handles_[1], &metadata, nullptr);
-      const FileMetaData& file = metadata[0][0];
-      ValueType vtype;
-      SequenceNumber seq;
-      UnPackSequenceAndType(ExtractInternalKeyFooter(file.largest.Encode()),
-                            &seq, &vtype);
-      ASSERT_GE(seq, 0);
-    }
-    std::vector<LiveFileMetaData> live_meta;
-    db_->GetLiveFilesMetaData(&live_meta);
-    ASSERT_EQ(live_meta.size(), 1);
-    std::vector<std::string> to_ingest_files;
-    to_ingest_files.emplace_back(live_meta[0].directory + "/" +
-                                 live_meta[0].relative_filename);
-    // Ingesting a file whose boundary key has non-zero seqno.
-    Status s = db_->IngestExternalFile(to_ingest_files, ingest_opts);
-    ASSERT_TRUE(
-        s.ToString().find("External file has non zero sequence number") !=
-        std::string::npos);
-    ASSERT_NOK(s);
-
-    {
-      // Only non-boundary key with non-zero seqno.
-      const Snapshot* snapshot = db_->GetSnapshot();
-      ASSERT_OK(Put(1, Key(70), "cf1_" + Key(70)));
-      ASSERT_OK(Flush(1));
-      CompactRangeOptions cro;
-      cro.bottommost_level_compaction =
-          BottommostLevelCompaction::kForceOptimized;
-      ASSERT_OK(db_->CompactRange(cro, handles_[1], nullptr, nullptr));
-
-      // Verify that only the non-boundary key of the file has non-zero seqno.
-      std::vector<std::vector<FileMetaData>> metadata;
-      // File may be at different level for different options.
-      dbfull()->TEST_GetFilesMetaData(handles_[1], &metadata, nullptr);
-      bool found_file = false;
-      for (const auto& level : metadata) {
-        if (level.empty()) {
-          continue;
-        }
-        ASSERT_FALSE(found_file);
-        found_file = true;
-        ASSERT_EQ(1, level.size());
-        const FileMetaData& file = level[0];
-        ValueType vtype;
-        SequenceNumber seq;
-        UnPackSequenceAndType(ExtractInternalKeyFooter(file.largest.Encode()),
-                              &seq, &vtype);
-        ASSERT_EQ(seq, 0);
-        UnPackSequenceAndType(ExtractInternalKeyFooter(file.smallest.Encode()),
-                              &seq, &vtype);
-        ASSERT_EQ(seq, 0);
-        ASSERT_GT(file.fd.largest_seqno, 0);
-      }
-      ASSERT_TRUE(found_file);
-      live_meta.clear();
-      db_->GetLiveFilesMetaData(&live_meta);
-      ASSERT_EQ(live_meta.size(), 1);
-      to_ingest_files[0] =
-          live_meta[0].directory + "/" + live_meta[0].relative_filename;
-      s = db_->IngestExternalFile(to_ingest_files, ingest_opts);
-      ASSERT_NOK(s);
-      ASSERT_TRUE(
-          s.ToString().find(
-              "External file has a key with non zero sequence number") !=
-          std::string::npos);
-      db_->ReleaseSnapshot(snapshot);
-    }
-
-    CompactRangeOptions cro;
-    cro.bottommost_level_compaction =
-        BottommostLevelCompaction::kForceOptimized;
-    ASSERT_OK(db_->CompactRange(cro, handles_[1], nullptr, nullptr));
-    live_meta.clear();
-    db_->GetLiveFilesMetaData(&live_meta);
-    ASSERT_EQ(live_meta.size(), 1);
-    ASSERT_EQ(0, live_meta[0].largest_seqno);
-    to_ingest_files[0] =
-        live_meta[0].directory + "/" + live_meta[0].relative_filename;
-
-    ingest_opts.allow_db_generated_files = false;
-    // Ingesting a DB genrate file with allow_db_generated_files = false;
-    s = db_->IngestExternalFile(to_ingest_files, ingest_opts);
-    ASSERT_TRUE(s.ToString().find("External file version not found") !=
-                std::string::npos);
-    ASSERT_NOK(s);
-
-    const std::string err =
-        "An ingested file is assigned to a non-zero sequence number, which is "
-        "incompatible with ingestion option allow_db_generated_files";
-    ingest_opts.allow_db_generated_files = true;
-    s = db_->IngestExternalFile(to_ingest_files, ingest_opts);
-    ASSERT_TRUE(s.ToString().find(err) != std::string::npos);
-    ASSERT_NOK(s);
-    if (options.compaction_style != kCompactionStyleUniversal) {
-      // FIXME: after fixing ingestion with universal compaction, currently
-      //  will always ingest into L0.
-      ingest_opts.fail_if_not_bottommost_level = true;
-      s = db_->IngestExternalFile(to_ingest_files, ingest_opts);
-      ASSERT_NOK(s);
-      ASSERT_TRUE(s.ToString().find("Files cannot be ingested to Lmax") !=
-                  std::string::npos);
-      ingest_opts.fail_if_not_bottommost_level = false;
-    }
-    ingest_opts.write_global_seqno = true;
-    s = db_->IngestExternalFile(to_ingest_files, ingest_opts);
-    ASSERT_TRUE(s.ToString().find("write_global_seqno is deprecated and does "
-                                  "not work with allow_db_generated_files") !=
-                std::string::npos);
-    ASSERT_NOK(s);
-    ingest_opts.write_global_seqno = false;
-
-    // Delete the overlapping key.
-    ASSERT_OK(db_->Delete(WriteOptions(), handles_[1], Key(49)));
-    ASSERT_OK(db_->CompactRange(cro, handles_[1], nullptr, nullptr));
-    live_meta.clear();
-    db_->GetLiveFilesMetaData(&live_meta);
-    bool cf1_file_found = false;
-    for (const auto& f : live_meta) {
-      if (f.column_family_name == "toto") {
-        ASSERT_FALSE(cf1_file_found);
-        cf1_file_found = true;
-        ASSERT_EQ(0, f.largest_seqno);
-        to_ingest_files[0] = f.directory + "/" + f.relative_filename;
-      }
-    }
-    ASSERT_TRUE(cf1_file_found);
-
-    const Snapshot* snapshot = db_->GetSnapshot();
-    ingest_opts.snapshot_consistency = true;
-    s = db_->IngestExternalFile(to_ingest_files, ingest_opts);
-    // snapshot_consistency with snapshot will assign a newest sequence number.
-    ASSERT_TRUE(s.ToString().find(err) != std::string::npos);
-    ASSERT_NOK(s);
-
-    ingest_opts.move_files = true;
-    s = db_->IngestExternalFile(to_ingest_files, ingest_opts);
-    ingest_opts.move_files = false;
-    ASSERT_TRUE(
-        s.ToString().find("Options move_files and allow_db_generated_files are "
-                          "not compatible") != std::string::npos);
-    ASSERT_NOK(s);
-
-    ingest_opts.snapshot_consistency = false;
-    ASSERT_OK(db_->IngestExternalFile(to_ingest_files, ingest_opts));
-    db_->ReleaseSnapshot(snapshot);
-
-    // Verify default CF content.
-    std::string val;
-    for (int k = 0; k < 100; ++k) {
-      ASSERT_OK(db_->Get(ReadOptions(), Key(k), &val));
-      if (k < 50) {
-        ASSERT_EQ(val, "default_cf_" + Key(k));
-      } else {
-        ASSERT_EQ(val, "cf1_" + Key(k));
-      }
-    }
-  } while (ChangeOptions(kSkipPlainTable | kSkipFIFOCompaction));
-}
-
-class IngestDBGeneratedFileTest2
-    : public ExternalSSTFileTestBase,
-      public ::testing::WithParamInterface<std::tuple<bool, bool, bool, bool>> {
- public:
-  IngestDBGeneratedFileTest2() = default;
-};
-
-INSTANTIATE_TEST_CASE_P(VaryingOptions, IngestDBGeneratedFileTest2,
-                        testing::Combine(testing::Bool(), testing::Bool(),
-                                         testing::Bool(), testing::Bool()));
-
-TEST_P(IngestDBGeneratedFileTest2, NotOverlapWithDB) {
-  // Use a separate column family to sort some data, generate multiple SST
-  // files. Then ingest these files into another column family or DB. The data
-  // to be ingested does not overlap with existing data.
-  IngestExternalFileOptions ingest_opts;
-  ingest_opts.allow_db_generated_files = true;
-  ingest_opts.move_files = false;
-  ingest_opts.snapshot_consistency = std::get<0>(GetParam());
-  ingest_opts.allow_global_seqno = std::get<1>(GetParam());
-  ingest_opts.allow_blocking_flush = std::get<2>(GetParam());
-  ingest_opts.fail_if_not_bottommost_level = std::get<3>(GetParam());
-
-  do {
-    SCOPED_TRACE("option_config_ = " + std::to_string(option_config_));
-    Options options = CurrentOptions();
-    // vector memtable for temp CF does not support concurrent write
-    options.allow_concurrent_memtable_write = false;
-    CreateAndReopenWithCF({"toto"}, options);
-
-    // non-empty bottommost level
-    WriteOptions wo;
-    for (int k = 0; k < 50; ++k) {
-      ASSERT_OK(db_->Put(wo, handles_[1], Key(k), "base_val_" + Key(k)));
-    }
-    ASSERT_OK(Flush());
-    CompactRangeOptions cro;
-    cro.bottommost_level_compaction =
-        BottommostLevelCompaction::kForceOptimized;
-    ASSERT_OK(db_->CompactRange(cro, handles_[1], nullptr, nullptr));
-    // non-empty memtable
-    for (int k = 50; k < 100; ++k) {
-      ASSERT_OK(db_->Put(wo, handles_[1], Key(k), "base_val_" + Key(k)));
-    }
-
-    // load external data to sort, generate multiple files
-    Options temp_cf_opts;
-    ColumnFamilyHandle* temp_cfh;
-    temp_cf_opts.target_file_size_base = 4 << 10;
-    temp_cf_opts.memtable_factory.reset(new VectorRepFactory());
-    temp_cf_opts.allow_concurrent_memtable_write = false;
-    temp_cf_opts.compaction_style = kCompactionStyleUniversal;
-    ASSERT_OK(db_->CreateColumnFamily(temp_cf_opts, "temp_cf", &temp_cfh));
-
-    Random rnd(301);
-    std::vector<std::string> expected_value;
-    expected_value.resize(100);
-    // Out of order insertion of keys from 100 to 199.
-    for (int k = 99; k >= 0; --k) {
-      expected_value[k] = rnd.RandomString(200);
-      ASSERT_OK(db_->Put(wo, temp_cfh, Key(k + 100), expected_value[k]));
-    }
-    ASSERT_OK(db_->CompactRange(cro, temp_cfh, nullptr, nullptr));
-    std::vector<std::string> sst_file_paths;
-    ColumnFamilyMetaData cf_meta;
-    db_->GetColumnFamilyMetaData(temp_cfh, &cf_meta);
-    ASSERT_GT(cf_meta.file_count, 1);
-    for (const auto& level_meta : cf_meta.levels) {
-      if (level_meta.level + 1 < temp_cf_opts.num_levels) {
-        ASSERT_EQ(0, level_meta.files.size());
-      } else {
-        ASSERT_GT(level_meta.files.size(), 1);
-        for (const auto& meta : level_meta.files) {
-          ASSERT_EQ(0, meta.largest_seqno);
-          sst_file_paths.emplace_back(meta.directory + "/" +
-                                      meta.relative_filename);
-        }
-      }
-    }
-
-    ASSERT_OK(
-        db_->IngestExternalFile(handles_[1], sst_file_paths, ingest_opts));
-    // Verify state of the CF1
-    ReadOptions ro;
-    std::string val;
-    for (int k = 0; k < 100; ++k) {
-      ASSERT_OK(db_->Get(ro, handles_[1], Key(k), &val));
-      ASSERT_EQ(val, "base_val_" + Key(k));
-      ASSERT_OK(db_->Get(ro, handles_[1], Key(100 + k), &val));
-      ASSERT_EQ(val, expected_value[k]);
-    }
-
-    // Ingest into another DB.
-    if (!encrypted_env_) {
-      // Ingestion between encrypted env and non-encrypted env won't work.
-      std::string db2_path = test::PerThreadDBPath("DB2");
-      Options db2_options;
-      db2_options.create_if_missing = true;
-      DB* db2 = nullptr;
-      ASSERT_OK(DB::Open(db2_options, db2_path, &db2));
-      // Write some base data.
-      expected_value.emplace_back(rnd.RandomString(100));
-      ASSERT_OK(db2->Put(WriteOptions(), Key(200), expected_value.back()));
-      ASSERT_OK(db2->CompactRange(cro, nullptr, nullptr));
-      expected_value.emplace_back(rnd.RandomString(100));
-      ASSERT_OK(db2->Put(WriteOptions(), Key(201), expected_value.back()));
-
-      ASSERT_OK(db2->IngestExternalFile({sst_file_paths}, ingest_opts));
-      {
-        std::unique_ptr<Iterator> iter{db2->NewIterator(ReadOptions())};
-        iter->SeekToFirst();
-        // The DB should have keys 100-199 from ingested files, and keys 200 and
-        // 201 from itself.
-        for (int k = 100; k <= 201; ++k, iter->Next()) {
-          ASSERT_TRUE(iter->Valid());
-          ASSERT_EQ(iter->key(), Key(k));
-          ASSERT_EQ(iter->value(), expected_value[k - 100]);
-        }
-        ASSERT_FALSE(iter->Valid());
-        ASSERT_OK(iter->status());
-      }
-
-      // Dropping the original CF should not affect db2, reopening it should not
-      // miss SST files.
-      ASSERT_OK(db_->DropColumnFamily(temp_cfh));
-      ASSERT_OK(db_->DestroyColumnFamilyHandle(temp_cfh));
-      ASSERT_OK(db2->Close());
-      delete db2;
-      ASSERT_OK(DB::Open(db2_options, db2_path, &db2));
-      ASSERT_OK(db2->Close());
-      delete db2;
-      ASSERT_OK(DestroyDB(db2_path, db2_options));
-    } else {
-      ASSERT_OK(db_->DropColumnFamily(temp_cfh));
-      ASSERT_OK(db_->DestroyColumnFamilyHandle(temp_cfh));
-    }
-  } while (ChangeOptions(kSkipPlainTable | kSkipFIFOCompaction));
-}
 }  // namespace ROCKSDB_NAMESPACE
 
 int main(int argc, char** argv) {

db/fault_injection_test.cc

@@ -567,44 +567,34 @@ class FaultInjectionDBTest : public DBTestBase {
 TEST(FaultInjectionFSTest, ReadUnsyncedData) {
   std::shared_ptr<FaultInjectionTestFS> fault_fs =
       std::make_shared<FaultInjectionTestFS>(FileSystem::Default());
-  fault_fs->SetInjectUnsyncedDataLoss(true);
   ASSERT_TRUE(fault_fs->ReadUnsyncedData());
-  ASSERT_TRUE(fault_fs->InjectUnsyncedDataLoss());
 
-  // This is a randomized mini-stress test, to reduce the chances of bugs in
-  // FaultInjectionTestFS being caught only in db_stress, where they are
-  // difficult to debug. ~1000 iterations might be needed to debug relevant
-  // code changes. Limiting to 10 for each regular unit test run.
-  auto seed = Random::GetTLSInstance()->Next();
-  for (int i = 0; i < 10; i++, seed++) {
-    Random rnd(seed);
-    uint32_t len = rnd.Uniform(10000) + 1;
+  uint32_t len = Random::GetTLSInstance()->Uniform(10000) + 1;
+  Random rnd(len);
 
-    std::string f =
-        test::PerThreadDBPath("read_unsynced." + std::to_string(seed));
-    std::string data = rnd.RandomString(len);
-
-    // Create partially synced file
+  // Create partially synced file
+  std::string f = test::PerThreadDBPath("read_unsynced.data");
+  std::string data = rnd.RandomString(len);
+  {
     std::unique_ptr<FSWritableFile> w;
     ASSERT_OK(fault_fs->NewWritableFile(f, {}, &w, nullptr));
     uint32_t synced_len = rnd.Uniform(len + 1);
     ASSERT_OK(w->Append(Slice(data.data(), synced_len), {}, nullptr));
-    if (synced_len > 0) {
-      ASSERT_OK(w->Sync({}, nullptr));
-    }
+    ASSERT_OK(w->Sync({}, nullptr));
     ASSERT_OK(w->Append(Slice(data.data() + synced_len, len - synced_len), {},
                         nullptr));
-
-    // Test file size includes unsynced data
-    {
-      uint64_t file_size;
-      ASSERT_OK(fault_fs->GetFileSize(f, {}, &file_size, nullptr));
-      ASSERT_EQ(len, file_size);
-    }
-
-    // Test read file contents, with two reads that probably don't
-    // align with the unsynced split. And maybe a sync or write between
-    // the two reads.
+    // no sync here
+    ASSERT_OK(w->Close({}, nullptr));
+  }
+  // Test file size includes unsynced data
+  {
+    uint64_t file_size;
+    ASSERT_OK(fault_fs->GetFileSize(f, {}, &file_size, nullptr));
+    ASSERT_EQ(len, file_size);
+  }
+  // Test read file contents, with two reads that probably don't
+  // align with the unsynced split.
+  {
     std::unique_ptr<FSSequentialFile> r;
     ASSERT_OK(fault_fs->NewSequentialFile(f, {}, &r, nullptr));
     uint32_t first_read_len = rnd.Uniform(len + 1);
@@ -613,36 +603,12 @@ TEST(FaultInjectionFSTest, ReadUnsyncedData) {
     ASSERT_OK(r->Read(first_read_len, {}, &sl, scratch.get(), nullptr));
     ASSERT_EQ(first_read_len, sl.size());
     ASSERT_EQ(0, sl.compare(Slice(data.data(), first_read_len)));
-
-    // Maybe a sync and/or write and/or close between the two reads.
-    if (rnd.OneIn(2)) {
-      ASSERT_OK(w->Sync({}, nullptr));
-    }
-    if (rnd.OneIn(2)) {
-      uint32_t more_len = rnd.Uniform(1000) + 1;
-      std::string more_data = rnd.RandomString(more_len);
-      ASSERT_OK(w->Append(more_data, {}, nullptr));
-      data += more_data;
-      len += more_len;
-    }
-    if (rnd.OneIn(2)) {
-      ASSERT_OK(w->Sync({}, nullptr));
-    }
-    if (rnd.OneIn(2)) {
-      ASSERT_OK(w->Close({}, nullptr));
-      w.reset();
-    }
-
-    // Second read some of, all of, or more than rest of file
-    uint32_t second_read_len = rnd.Uniform(len + 1);
+    uint32_t second_read_len = len - first_read_len;
     scratch.reset(new char[second_read_len]);
     ASSERT_OK(r->Read(second_read_len, {}, &sl, scratch.get(), nullptr));
-    if (len - first_read_len < second_read_len) {
-      ASSERT_EQ(len - first_read_len, sl.size());
-    } else {
-      ASSERT_EQ(second_read_len, sl.size());
-    }
-    ASSERT_EQ(0, sl.compare(Slice(data.data() + first_read_len, sl.size())));
+    ASSERT_EQ(second_read_len, sl.size());
+    ASSERT_EQ(0,
+              sl.compare(Slice(data.data() + first_read_len, second_read_len)));
   }
 }
 }  // namespace ROCKSDB_NAMESPACE

db/memtable_list.cc

@@ -235,19 +235,19 @@ void MemTableListVersion::AddIterators(
       SequenceNumber read_seq = options.snapshot != nullptr
                                     ? options.snapshot->GetSequenceNumber()
                                     : kMaxSequenceNumber;
-      std::unique_ptr<TruncatedRangeDelIterator> mem_tombstone_iter;
+      TruncatedRangeDelIterator* mem_tombstone_iter = nullptr;
       auto range_del_iter = m->NewRangeTombstoneIterator(
           options, read_seq, true /* immutale_memtable */);
       if (range_del_iter == nullptr || range_del_iter->empty()) {
         delete range_del_iter;
       } else {
-        mem_tombstone_iter = std::make_unique<TruncatedRangeDelIterator>(
+        mem_tombstone_iter = new TruncatedRangeDelIterator(
             std::unique_ptr<FragmentedRangeTombstoneIterator>(range_del_iter),
             &m->GetInternalKeyComparator(), nullptr /* smallest */,
             nullptr /* largest */);
       }
-      merge_iter_builder->AddPointAndTombstoneIterator(
-          mem_iter, std::move(mem_tombstone_iter));
+      merge_iter_builder->AddPointAndTombstoneIterator(mem_iter,
+                                                       mem_tombstone_iter);
     }
   }
 }

db/table_cache.cc

@@ -230,7 +230,7 @@ InternalIterator* TableCache::NewIterator(
     const InternalKey* smallest_compaction_key,
     const InternalKey* largest_compaction_key, bool allow_unprepared_value,
     uint8_t block_protection_bytes_per_key, const SequenceNumber* read_seqno,
-    std::unique_ptr<TruncatedRangeDelIterator>* range_del_iter) {
+    TruncatedRangeDelIterator** range_del_iter) {
   PERF_TIMER_GUARD(new_table_iterator_nanos);
 
   Status s;
@@ -285,7 +285,7 @@ InternalIterator* TableCache::NewIterator(
         delete new_range_del_iter;
         *range_del_iter = nullptr;
       } else {
-        *range_del_iter = std::make_unique<TruncatedRangeDelIterator>(
+        *range_del_iter = new TruncatedRangeDelIterator(
             std::unique_ptr<FragmentedRangeTombstoneIterator>(
                 new_range_del_iter),
             &icomparator, &file_meta.smallest, &file_meta.largest);

db/table_cache.h

@@ -100,7 +100,7 @@ class TableCache {
       const InternalKey* largest_compaction_key, bool allow_unprepared_value,
       uint8_t protection_bytes_per_key,
       const SequenceNumber* range_del_read_seqno = nullptr,
-      std::unique_ptr<TruncatedRangeDelIterator>* range_del_iter = nullptr);
+      TruncatedRangeDelIterator** range_del_iter = nullptr);
 
   // If a seek to internal key "k" in specified file finds an entry,
   // call get_context->SaveValue() repeatedly until

db/version_edit.cc

@@ -255,6 +255,7 @@ bool VersionEdit::EncodeTo(std::string* dst,
       char p = static_cast<char>(0);
       PutLengthPrefixedSlice(dst, Slice(&p, 1));
     }
+
     TEST_SYNC_POINT_CALLBACK("VersionEdit::EncodeTo:NewFile4:CustomizeFields",
                              dst);
 

db/version_set.cc

@@ -976,8 +976,7 @@ class LevelIterator final : public InternalIterator {
       const std::vector<AtomicCompactionUnitBoundary>* compaction_boundaries =
           nullptr,
       bool allow_unprepared_value = false,
-      std::unique_ptr<TruncatedRangeDelIterator>*** range_tombstone_iter_ptr_ =
-          nullptr)
+      TruncatedRangeDelIterator**** range_tombstone_iter_ptr_ = nullptr)
       : table_cache_(table_cache),
         read_options_(read_options),
         file_options_(file_options),
@@ -1117,8 +1116,9 @@ class LevelIterator final : public InternalIterator {
   }
 
   void ClearRangeTombstoneIter() {
-    if (range_tombstone_iter_) {
-      range_tombstone_iter_->reset();
+    if (range_tombstone_iter_ && *range_tombstone_iter_) {
+      delete *range_tombstone_iter_;
+      *range_tombstone_iter_ = nullptr;
     }
   }
 
@@ -1201,7 +1201,7 @@ class LevelIterator final : public InternalIterator {
   // iterator end).
   //
   // *range_tombstone_iter_ points to range tombstones of the current SST file
-  std::unique_ptr<TruncatedRangeDelIterator>* range_tombstone_iter_;
+  TruncatedRangeDelIterator** range_tombstone_iter_;
 
   // The sentinel key to be returned
   Slice sentinel_;
@@ -1280,9 +1280,11 @@ void LevelIterator::Seek(const Slice& target) {
                                           ts_sz);
       if (prefix_extractor_->InDomain(target_user_key_without_ts) &&
           (!prefix_extractor_->InDomain(next_file_first_user_key_without_ts) ||
-           prefix_extractor_->Transform(target_user_key_without_ts)
-                   .compare(prefix_extractor_->Transform(
-                       next_file_first_user_key_without_ts)) != 0)) {
+           user_comparator_.CompareWithoutTimestamp(
+               prefix_extractor_->Transform(target_user_key_without_ts), false,
+               prefix_extractor_->Transform(
+                   next_file_first_user_key_without_ts),
+               false) != 0)) {
         // SkipEmptyFileForward() will not advance to next file when this flag
         // is set for reason detailed below.
         //
@@ -1927,7 +1929,7 @@ InternalIterator* Version::TEST_GetLevelIterator(
     int level, bool allow_unprepared_value) {
   auto* arena = merge_iter_builder->GetArena();
   auto* mem = arena->AllocateAligned(sizeof(LevelIterator));
-  std::unique_ptr<TruncatedRangeDelIterator>** tombstone_iter_ptr = nullptr;
+  TruncatedRangeDelIterator*** tombstone_iter_ptr = nullptr;
   auto level_iter = new (mem) LevelIterator(
       cfd_->table_cache(), read_options, file_options_,
       cfd_->internal_comparator(), &storage_info_.LevelFilesBrief(level),
@@ -2027,7 +2029,7 @@ void Version::AddIteratorsForLevel(const ReadOptions& read_options,
   auto* arena = merge_iter_builder->GetArena();
   if (level == 0) {
     // Merge all level zero files together since they may overlap
-    std::unique_ptr<TruncatedRangeDelIterator> tombstone_iter = nullptr;
+    TruncatedRangeDelIterator* tombstone_iter = nullptr;
     for (size_t i = 0; i < storage_info_.LevelFilesBrief(0).num_files; i++) {
       const auto& file = storage_info_.LevelFilesBrief(0).files[i];
       auto table_iter = cfd_->table_cache()->NewIterator(
@@ -2044,8 +2046,8 @@ void Version::AddIteratorsForLevel(const ReadOptions& read_options,
       if (read_options.ignore_range_deletions) {
         merge_iter_builder->AddIterator(table_iter);
       } else {
-        merge_iter_builder->AddPointAndTombstoneIterator(
-            table_iter, std::move(tombstone_iter));
+        merge_iter_builder->AddPointAndTombstoneIterator(table_iter,
+                                                         tombstone_iter);
       }
     }
     if (should_sample) {
@@ -2062,7 +2064,7 @@ void Version::AddIteratorsForLevel(const ReadOptions& read_options,
     // walks through the non-overlapping files in the level, opening them
     // lazily.
     auto* mem = arena->AllocateAligned(sizeof(LevelIterator));
-    std::unique_ptr<TruncatedRangeDelIterator>** tombstone_iter_ptr = nullptr;
+    TruncatedRangeDelIterator*** tombstone_iter_ptr = nullptr;
     auto level_iter = new (mem) LevelIterator(
         cfd_->table_cache(), read_options, soptions,
         cfd_->internal_comparator(), &storage_info_.LevelFilesBrief(level),
@@ -5508,7 +5510,6 @@ Status VersionSet::ProcessManifestWrites(
   Status s;
   IOStatus io_s;
   IOStatus manifest_io_status;
-  std::unique_ptr<log::Writer> new_desc_log_ptr;
   {
     FileOptions opt_file_opts = fs_->OptimizeForManifestWrite(file_options_);
     mu->Unlock();
@@ -5537,7 +5538,6 @@ Status VersionSet::ProcessManifestWrites(
       }
     }
 
-    log::Writer* raw_desc_log_ptr = descriptor_log_.get();
     if (s.ok() && new_descriptor_log) {
       // This is fine because everything inside of this block is serialized --
       // only one thread can be here at the same time
@@ -5559,14 +5559,12 @@ Status VersionSet::ProcessManifestWrites(
             db_options_->listeners, nullptr,
             tmp_set.Contains(FileType::kDescriptorFile),
             tmp_set.Contains(FileType::kDescriptorFile)));
-        new_desc_log_ptr.reset(
+        descriptor_log_.reset(
             new log::Writer(std::move(file_writer), 0, false));
-        raw_desc_log_ptr = new_desc_log_ptr.get();
         s = WriteCurrentStateToManifest(write_options, curr_state,
-                                        wal_additions, raw_desc_log_ptr, io_s);
-        assert(s == io_s);
-      }
-      if (!io_s.ok()) {
+                                        wal_additions, descriptor_log_.get(),
+                                        io_s);
+      } else {
         manifest_io_status = io_s;
         s = io_s;
       }
@@ -5609,7 +5607,7 @@ Status VersionSet::ProcessManifestWrites(
         }
         ++idx;
 #endif /* !NDEBUG */
-        io_s = raw_desc_log_ptr->AddRecord(write_options, record);
+        io_s = descriptor_log_->AddRecord(write_options, record);
         if (!io_s.ok()) {
           s = io_s;
           manifest_io_status = io_s;
@@ -5619,7 +5617,7 @@ Status VersionSet::ProcessManifestWrites(
 
       if (s.ok()) {
         io_s =
-            SyncManifest(db_options_, write_options, raw_desc_log_ptr->file());
+            SyncManifest(db_options_, write_options, descriptor_log_->file());
         manifest_io_status = io_s;
         TEST_SYNC_POINT_CALLBACK(
             "VersionSet::ProcessManifestWrites:AfterSyncManifest", &io_s);
@@ -5652,7 +5650,7 @@ Status VersionSet::ProcessManifestWrites(
 
     if (s.ok()) {
       // find offset in manifest file where this version is stored.
-      new_manifest_file_size = raw_desc_log_ptr->file()->GetFileSize();
+      new_manifest_file_size = descriptor_log_->file()->GetFileSize();
     }
 
     if (first_writer.edit_list.front()->IsColumnFamilyDrop()) {
@@ -5698,7 +5696,6 @@ Status VersionSet::ProcessManifestWrites(
   // Append the old manifest file to the obsolete_manifest_ list to be deleted
   // by PurgeObsoleteFiles later.
   if (s.ok() && new_descriptor_log) {
-    descriptor_log_ = std::move(new_desc_log_ptr);
     obsolete_manifests_.emplace_back(
         DescriptorFileName("", manifest_file_number_));
   }
@@ -5776,7 +5773,6 @@ Status VersionSet::ProcessManifestWrites(
     // corrupted. So we need to force the next version update to start a
     // new manifest file.
     descriptor_log_.reset();
-    new_desc_log_ptr.reset();
     // If manifest operations failed, then we know the CURRENT file still
     // points to the original MANIFEST. Therefore, we can safely delete the
     // new MANIFEST.
@@ -7082,8 +7078,8 @@ InternalIterator* VersionSet::MakeInputIterator(
   // that will be initialized to where CompactionMergingIterator stores
   // pointer to its range tombstones. This is used by LevelIterator
   // to update pointer to range tombstones as it traverse different SST files.
-  std::vector<std::pair<std::unique_ptr<TruncatedRangeDelIterator>,
-                        std::unique_ptr<TruncatedRangeDelIterator>**>>
+  std::vector<
+      std::pair<TruncatedRangeDelIterator*, TruncatedRangeDelIterator***>>
       range_tombstones;
   size_t num = 0;
   for (size_t which = 0; which < c->num_input_levels(); which++) {
@@ -7105,8 +7101,7 @@ InternalIterator* VersionSet::MakeInputIterator(
                   *end, fmd.smallest.user_key()) < 0) {
             continue;
           }
-          std::unique_ptr<TruncatedRangeDelIterator> range_tombstone_iter =
-              nullptr;
+          TruncatedRangeDelIterator* range_tombstone_iter = nullptr;
           list[num++] = cfd->table_cache()->NewIterator(
               read_options, file_options_compactions,
               cfd->internal_comparator(), fmd, range_del_agg,
@@ -7123,13 +7118,11 @@ InternalIterator* VersionSet::MakeInputIterator(
               c->mutable_cf_options()->block_protection_bytes_per_key,
               /*range_del_read_seqno=*/nullptr,
               /*range_del_iter=*/&range_tombstone_iter);
-          range_tombstones.emplace_back(std::move(range_tombstone_iter),
-                                        nullptr);
+          range_tombstones.emplace_back(range_tombstone_iter, nullptr);
         }
       } else {
         // Create concatenating iterator for the files from this level
-        std::unique_ptr<TruncatedRangeDelIterator>** tombstone_iter_ptr =
-            nullptr;
+        TruncatedRangeDelIterator*** tombstone_iter_ptr = nullptr;
         list[num++] = new LevelIterator(
             cfd->table_cache(), read_options, file_options_compactions,
             cfd->internal_comparator(), c->input_levels(which),

db/wal_manager.cc

@@ -43,17 +43,16 @@ Status WalManager::DeleteFile(const std::string& fname, uint64_t number) {
   }
   return s;
 }
-Status WalManager::GetSortedWalFiles(VectorWalPtr& files, bool need_seqnos,
-                                     bool include_archived) {
+
+Status WalManager::GetSortedWalFiles(VectorWalPtr& files) {
   // First get sorted files in db dir, then get sorted files from archived
   // dir, to avoid a race condition where a log file is moved to archived
   // dir in between.
   Status s;
   // list wal files in main db dir.
   VectorWalPtr logs;
-  s = GetSortedWalsOfType(wal_dir_, logs, kAliveLogFile, need_seqnos);
-
-  if (!include_archived || !s.ok()) {
+  s = GetSortedWalsOfType(wal_dir_, logs, kAliveLogFile);
+  if (!s.ok()) {
     return s;
   }
 
@@ -68,7 +67,7 @@ Status WalManager::GetSortedWalFiles(VectorWalPtr& files, bool need_seqnos,
   std::string archivedir = ArchivalDirectory(wal_dir_);
   Status exists = env_->FileExists(archivedir);
   if (exists.ok()) {
-    s = GetSortedWalsOfType(archivedir, files, kArchivedLogFile, need_seqnos);
+    s = GetSortedWalsOfType(archivedir, files, kArchivedLogFile);
     if (!s.ok()) {
       return s;
     }
@@ -250,8 +249,7 @@ void WalManager::PurgeObsoleteWALFiles() {
 
   size_t files_del_num = log_files_num - files_keep_num;
   VectorWalPtr archived_logs;
-  s = GetSortedWalsOfType(archival_dir, archived_logs, kArchivedLogFile,
-                          /*need_seqno=*/false);
+  s = GetSortedWalsOfType(archival_dir, archived_logs, kArchivedLogFile);
   if (!s.ok()) {
     ROCKS_LOG_WARN(db_options_.info_log,
                    "Unable to get archived WALs from: %s: %s",
@@ -286,9 +284,6 @@ void WalManager::ArchiveWALFile(const std::string& fname, uint64_t number) {
   Status s = env_->RenameFile(fname, archived_log_name);
   // The sync point below is used in (DBTest,TransactionLogIteratorRace)
   TEST_SYNC_POINT("WalManager::PurgeObsoleteFiles:2");
-  // The sync point below is used in
-  // (CheckPointTest, CheckpointWithArchievedLog)
-  TEST_SYNC_POINT("WalManager::ArchiveWALFile");
   ROCKS_LOG_INFO(db_options_.info_log, "Move log file %s to %s -- %s\n",
                  fname.c_str(), archived_log_name.c_str(),
                  s.ToString().c_str());
@@ -296,7 +291,7 @@ void WalManager::ArchiveWALFile(const std::string& fname, uint64_t number) {
 
 Status WalManager::GetSortedWalsOfType(const std::string& path,
                                        VectorWalPtr& log_files,
-                                       WalFileType log_type, bool need_seqnos) {
+                                       WalFileType log_type) {
   std::vector<std::string> all_files;
   const Status status = env_->GetChildren(path, &all_files);
   if (!status.ok()) {
@@ -308,17 +303,13 @@ Status WalManager::GetSortedWalsOfType(const std::string& path,
     FileType type;
     if (ParseFileName(f, &number, &type) && type == kWalFile) {
       SequenceNumber sequence;
-      if (need_seqnos) {
-        Status s = ReadFirstRecord(log_type, number, &sequence);
-        if (!s.ok()) {
-          return s;
-        }
-        if (sequence == 0) {
-          // empty file
-          continue;
-        }
-      } else {
-        sequence = 0;
+      Status s = ReadFirstRecord(log_type, number, &sequence);
+      if (!s.ok()) {
+        return s;
+      }
+      if (sequence == 0) {
+        // empty file
+        continue;
       }
 
       // Reproduce the race condition where a log file is moved
@@ -328,7 +319,7 @@ Status WalManager::GetSortedWalsOfType(const std::string& path,
       TEST_SYNC_POINT("WalManager::GetSortedWalsOfType:2");
 
       uint64_t size_bytes;
-      Status s = env_->GetFileSize(LogFileName(path, number), &size_bytes);
+      s = env_->GetFileSize(LogFileName(path, number), &size_bytes);
       // re-try in case the alive log file has been moved to archive.
       if (!s.ok() && log_type == kAliveLogFile) {
         std::string archived_file = ArchivedLogFileName(path, number);

db/wal_manager.h

@@ -49,8 +49,7 @@ class WalManager {
         wal_in_db_path_(db_options_.IsWalDirSameAsDBPath()),
         io_tracer_(io_tracer) {}
 
-  Status GetSortedWalFiles(VectorWalPtr& files, bool need_seqnos = true,
-                           bool include_archived = true);
+  Status GetSortedWalFiles(VectorWalPtr& files);
 
   // Allow user to tail transaction log to find all recent changes to the
   // database that are newer than `seq_number`.
@@ -79,7 +78,7 @@ class WalManager {
 
  private:
   Status GetSortedWalsOfType(const std::string& path, VectorWalPtr& log_files,
-                             WalFileType type, bool need_seqnos);
+                             WalFileType type);
   // Requires: all_logs should be sorted with earliest log file first
   // Retains all log files in all_logs which contain updates with seq no.
   // Greater Than or Equal to the requested SequenceNumber.

db_stress_tool/cf_consistency_stress.cc

@@ -73,13 +73,16 @@ class CfConsistencyStressTest : public StressTest {
       status = db_->Write(write_opts, &batch);
     }
 
-    if (status.ok()) {
-      auto num = static_cast<long>(rand_column_families.size());
-      thread->stats.AddBytesForWrites(num, (sz + 1) * num);
-    } else if (!IsErrorInjectedAndRetryable(status)) {
+    if (IsRetryableInjectedError(status)) {
+      fprintf(stdout, "multi put or merge error: %s\n",
+              status.ToString().c_str());
+    } else if (!status.ok()) {
       fprintf(stderr, "multi put or merge error: %s\n",
               status.ToString().c_str());
       thread->stats.AddErrors(1);
+    } else {
+      auto num = static_cast<long>(rand_column_families.size());
+      thread->stats.AddBytesForWrites(num, (sz + 1) * num);
     }
 
     return status;
@@ -96,11 +99,13 @@ class CfConsistencyStressTest : public StressTest {
       batch.Delete(cfh, key);
     }
     Status s = db_->Write(write_opts, &batch);
-    if (s.ok()) {
-      thread->stats.AddDeletes(static_cast<long>(rand_column_families.size()));
-    } else if (!IsErrorInjectedAndRetryable(s)) {
+    if (IsRetryableInjectedError(s)) {
+      fprintf(stdout, "multidel error: %s\n", s.ToString().c_str());
+    } else if (!s.ok()) {
       fprintf(stderr, "multidel error: %s\n", s.ToString().c_str());
       thread->stats.AddErrors(1);
+    } else {
+      thread->stats.AddDeletes(static_cast<long>(rand_column_families.size()));
     }
     return s;
   }
@@ -125,12 +130,14 @@ class CfConsistencyStressTest : public StressTest {
       batch.DeleteRange(cfh, key, end_key);
     }
     Status s = db_->Write(write_opts, &batch);
-    if (s.ok()) {
-      thread->stats.AddRangeDeletions(
-          static_cast<long>(rand_column_families.size()));
-    } else if (!IsErrorInjectedAndRetryable(s)) {
+    if (IsRetryableInjectedError(s)) {
+      fprintf(stdout, "multi del range error: %s\n", s.ToString().c_str());
+    } else if (!s.ok()) {
       fprintf(stderr, "multi del range error: %s\n", s.ToString().c_str());
       thread->stats.AddErrors(1);
+    } else {
+      thread->stats.AddRangeDeletions(
+          static_cast<long>(rand_column_families.size()));
     }
     return s;
   }
@@ -241,7 +248,9 @@ class CfConsistencyStressTest : public StressTest {
       thread->stats.AddGets(1, 1);
     } else if (s.IsNotFound()) {
       thread->stats.AddGets(1, 0);
-    } else if (!IsErrorInjectedAndRetryable(s)) {
+    } else if (IsRetryableInjectedError(s)) {
+      fprintf(stdout, "TestGet error: %s\n", s.ToString().c_str());
+    } else {
       fprintf(stderr, "TestGet error: %s\n", s.ToString().c_str());
       thread->stats.AddErrors(1);
     }
@@ -277,7 +286,9 @@ class CfConsistencyStressTest : public StressTest {
       } else if (s.IsNotFound()) {
         // not found case
         thread->stats.AddGets(1, 0);
-      } else if (!IsErrorInjectedAndRetryable(s)) {
+      } else if (IsRetryableInjectedError(s)) {
+        fprintf(stdout, "MultiGet error: %s\n", s.ToString().c_str());
+      } else {
         // errors case
         fprintf(stderr, "MultiGet error: %s\n", s.ToString().c_str());
         thread->stats.AddErrors(1);
@@ -313,7 +324,12 @@ class CfConsistencyStressTest : public StressTest {
       PinnableWideColumns result;
       s = db_->GetEntity(read_opts, cfh, key, &result);
 
-      if (s.ok()) {
+      if (IsRetryableInjectedError(s)) {
+        fprintf(stdout,
+                "GetEntity error: inconsistent columns for key %s, entity %s\n",
+                StringToHex(key).c_str(),
+                WideColumnsToHex(result.columns()).c_str());
+      } else if (s.ok()) {
         if (!VerifyWideColumns(result.columns())) {
           fprintf(
               stderr,
@@ -343,10 +359,10 @@ class CfConsistencyStressTest : public StressTest {
       if (fault_fs_guard) {
         fault_fs_guard->DisableThreadLocalErrorInjection(
             FaultInjectionIOType::kRead);
+
         fault_fs_guard->DisableThreadLocalErrorInjection(
             FaultInjectionIOType::kMetadataRead);
       }
-
       if (s.ok() || s.IsNotFound()) {
         const bool cmp_found = s.ok();
 
@@ -501,7 +517,9 @@ class CfConsistencyStressTest : public StressTest {
       thread->stats.AddGets(1, 1);
     } else if (s.IsNotFound()) {
       thread->stats.AddGets(1, 0);
-    } else if (!IsErrorInjectedAndRetryable(s)) {
+    } else if (IsRetryableInjectedError(s)) {
+      fprintf(stdout, "TestGetEntity error: %s\n", s.ToString().c_str());
+    } else {
       fprintf(stderr, "TestGetEntity error: %s\n", s.ToString().c_str());
       thread->stats.AddErrors(1);
     }
@@ -572,7 +590,9 @@ class CfConsistencyStressTest : public StressTest {
         for (size_t j = 0; j < num_cfs; ++j) {
           const Status& s = result[j].status();
           const WideColumns& columns = result[j].columns();
-          if (!s.ok() && IsErrorInjectedAndRetryable(s)) {
+          if (IsRetryableInjectedError(s)) {
+            fprintf(stdout, "TestMultiGetEntity (AttributeGroup) error: %s\n",
+                    s.ToString().c_str());
             break;
           } else if (!s.ok() && !s.IsNotFound()) {
             fprintf(stderr, "TestMultiGetEntity (AttributeGroup) error: %s\n",
@@ -679,7 +699,9 @@ class CfConsistencyStressTest : public StressTest {
           const Status& s = statuses[j];
           const WideColumns& columns = results[j].columns();
 
-          if (!s.ok() && IsErrorInjectedAndRetryable(s)) {
+          if (IsRetryableInjectedError(s)) {
+            fprintf(stdout, "TestMultiGetEntity error: %s\n",
+                    s.ToString().c_str());
             break;
           } else if (!s.ok() && !s.IsNotFound()) {
             fprintf(stderr, "TestMultiGetEntity error: %s\n",
@@ -816,7 +838,9 @@ class CfConsistencyStressTest : public StressTest {
       s = iter->status();
     }
 
-    if (!s.ok() && !IsErrorInjectedAndRetryable(s)) {
+    if (IsRetryableInjectedError(s)) {
+      fprintf(stdout, "TestPrefixScan error: %s\n", s.ToString().c_str());
+    } else if (!s.ok()) {
       fprintf(stderr, "TestPrefixScan error: %s\n", s.ToString().c_str());
       thread->stats.AddErrors(1);
 

db_stress_tool/db_stress_common.cc

@@ -19,6 +19,8 @@
 
 ROCKSDB_NAMESPACE::Env* db_stress_listener_env = nullptr;
 ROCKSDB_NAMESPACE::Env* db_stress_env = nullptr;
+// If non-null, injects read error at a rate specified by the
+// read_fault_one_in or write_fault_one_in flag
 std::shared_ptr<ROCKSDB_NAMESPACE::FaultInjectionTestFS> fault_fs_guard;
 std::shared_ptr<ROCKSDB_NAMESPACE::SecondaryCache> compressed_secondary_cache;
 std::shared_ptr<ROCKSDB_NAMESPACE::Cache> block_cache;

db_stress_tool/db_stress_driver.cc

@@ -159,13 +159,13 @@ bool RunStressTestImpl(SharedState* shared) {
         stress->TrackExpectedState(shared);
       }
 
+      // Since wrie fault and sync fault implementations are coupled with each
+      // other in `TestFSWritableFile()`, we can not enable or disable only one
+      // of the two.
+      // TODO(hx235): decouple implementations of write fault injection and sync
+      // fault injection.
       if (FLAGS_sync_fault_injection || FLAGS_write_fault_one_in > 0) {
         fault_fs_guard->SetFilesystemDirectWritable(false);
-        fault_fs_guard->SetInjectUnsyncedDataLoss(FLAGS_sync_fault_injection);
-        if (FLAGS_exclude_wal_from_write_fault_injection) {
-          fault_fs_guard->SetFileTypesExcludedFromWriteFaultInjection(
-              {FileType::kWalFile});
-        }
       }
       now = clock->NowMicros();
       fprintf(stdout, "%s Starting database operations\n",

db_stress_tool/db_stress_env_wrapper.h

@@ -13,20 +13,6 @@
 #include "monitoring/thread_status_util.h"
 
 namespace ROCKSDB_NAMESPACE {
-namespace {
-void CheckIOActivity(const IOOptions& options) {
-#ifndef NDEBUG
-  const ThreadStatus::OperationType thread_op =
-      ThreadStatusUtil::GetThreadOperation();
-  Env::IOActivity io_activity =
-      ThreadStatusUtil::TEST_GetExpectedIOActivity(thread_op);
-  assert(io_activity == Env::IOActivity::kUnknown ||
-         io_activity == options.io_activity);
-#else
-  (void)options;
-#endif
-}
-}  // namespace
 class DbStressRandomAccessFileWrapper : public FSRandomAccessFileOwnerWrapper {
  public:
   explicit DbStressRandomAccessFileWrapper(
@@ -97,67 +83,144 @@ class DbStressWritableFileWrapper : public FSWritableFileOwnerWrapper {
 
   IOStatus Append(const Slice& data, const IOOptions& options,
                   IODebugContext* dbg) override {
-    CheckIOActivity(options);
+#ifndef NDEBUG
+    const ThreadStatus::OperationType thread_op =
+        ThreadStatusUtil::GetThreadOperation();
+    Env::IOActivity io_activity =
+        ThreadStatusUtil::TEST_GetExpectedIOActivity(thread_op);
+    assert(io_activity == Env::IOActivity::kUnknown ||
+           io_activity == options.io_activity);
+#endif
     return target()->Append(data, options, dbg);
   }
   IOStatus Append(const Slice& data, const IOOptions& options,
                   const DataVerificationInfo& verification_info,
                   IODebugContext* dbg) override {
-    CheckIOActivity(options);
+#ifndef NDEBUG
+    const ThreadStatus::OperationType thread_op =
+        ThreadStatusUtil::GetThreadOperation();
+    Env::IOActivity io_activity =
+        ThreadStatusUtil::TEST_GetExpectedIOActivity(thread_op);
+    assert(io_activity == Env::IOActivity::kUnknown ||
+           io_activity == options.io_activity);
+#endif
     return target()->Append(data, options, verification_info, dbg);
   }
   IOStatus PositionedAppend(const Slice& data, uint64_t offset,
                             const IOOptions& options,
                             IODebugContext* dbg) override {
-    CheckIOActivity(options);
+#ifndef NDEBUG
+    const ThreadStatus::OperationType thread_op =
+        ThreadStatusUtil::GetThreadOperation();
+    Env::IOActivity io_activity =
+        ThreadStatusUtil::TEST_GetExpectedIOActivity(thread_op);
+    assert(io_activity == Env::IOActivity::kUnknown ||
+           io_activity == options.io_activity);
+#endif
     return target()->PositionedAppend(data, offset, options, dbg);
   }
   IOStatus PositionedAppend(const Slice& data, uint64_t offset,
                             const IOOptions& options,
                             const DataVerificationInfo& verification_info,
                             IODebugContext* dbg) override {
-    CheckIOActivity(options);
+#ifndef NDEBUG
+    const ThreadStatus::OperationType thread_op =
+        ThreadStatusUtil::GetThreadOperation();
+    Env::IOActivity io_activity =
+        ThreadStatusUtil::TEST_GetExpectedIOActivity(thread_op);
+    assert(io_activity == Env::IOActivity::kUnknown ||
+           io_activity == options.io_activity);
+#endif
     return target()->PositionedAppend(data, offset, options, verification_info,
                                       dbg);
   }
 
   IOStatus Truncate(uint64_t size, const IOOptions& options,
                     IODebugContext* dbg) override {
-    CheckIOActivity(options);
+#ifndef NDEBUG
+    const ThreadStatus::OperationType thread_op =
+        ThreadStatusUtil::GetThreadOperation();
+    Env::IOActivity io_activity =
+        ThreadStatusUtil::TEST_GetExpectedIOActivity(thread_op);
+    assert(io_activity == Env::IOActivity::kUnknown ||
+           io_activity == options.io_activity);
+#endif
     return target()->Truncate(size, options, dbg);
   }
 
   IOStatus Close(const IOOptions& options, IODebugContext* dbg) override {
-    CheckIOActivity(options);
+#ifndef NDEBUG
+    const ThreadStatus::OperationType thread_op =
+        ThreadStatusUtil::GetThreadOperation();
+    Env::IOActivity io_activity =
+        ThreadStatusUtil::TEST_GetExpectedIOActivity(thread_op);
+    assert(io_activity == Env::IOActivity::kUnknown ||
+           io_activity == options.io_activity);
+#endif
     return target()->Close(options, dbg);
   }
 
   IOStatus Flush(const IOOptions& options, IODebugContext* dbg) override {
-    CheckIOActivity(options);
+#ifndef NDEBUG
+    const ThreadStatus::OperationType thread_op =
+        ThreadStatusUtil::GetThreadOperation();
+    Env::IOActivity io_activity =
+        ThreadStatusUtil::TEST_GetExpectedIOActivity(thread_op);
+    assert(io_activity == Env::IOActivity::kUnknown ||
+           io_activity == options.io_activity);
+#endif
     return target()->Flush(options, dbg);
   }
 
   IOStatus Sync(const IOOptions& options, IODebugContext* dbg) override {
-    CheckIOActivity(options);
+#ifndef NDEBUG
+    const ThreadStatus::OperationType thread_op =
+        ThreadStatusUtil::GetThreadOperation();
+    Env::IOActivity io_activity =
+        ThreadStatusUtil::TEST_GetExpectedIOActivity(thread_op);
+    assert(io_activity == Env::IOActivity::kUnknown ||
+           io_activity == options.io_activity);
+#endif
     return target()->Sync(options, dbg);
   }
 
   IOStatus Fsync(const IOOptions& options, IODebugContext* dbg) override {
-    CheckIOActivity(options);
+#ifndef NDEBUG
+    const ThreadStatus::OperationType thread_op =
+        ThreadStatusUtil::GetThreadOperation();
+    Env::IOActivity io_activity =
+        ThreadStatusUtil::TEST_GetExpectedIOActivity(thread_op);
+    assert(io_activity == Env::IOActivity::kUnknown ||
+           io_activity == options.io_activity);
+#endif
     return target()->Fsync(options, dbg);
   }
 
 #ifdef ROCKSDB_FALLOCATE_PRESENT
   IOStatus Allocate(uint64_t offset, uint64_t len, const IOOptions& options,
                     IODebugContext* dbg) override {
-    CheckIOActivity(options);
+#ifndef NDEBUG
+    const ThreadStatus::OperationType thread_op =
+        ThreadStatusUtil::GetThreadOperation();
+    Env::IOActivity io_activity =
+        ThreadStatusUtil::TEST_GetExpectedIOActivity(thread_op);
+    assert(io_activity == Env::IOActivity::kUnknown ||
+           io_activity == options.io_activity);
+#endif
     return target()->Allocate(offset, len, options, dbg);
   }
 #endif
 
   IOStatus RangeSync(uint64_t offset, uint64_t nbytes, const IOOptions& options,
                      IODebugContext* dbg) override {
-    CheckIOActivity(options);
+#ifndef NDEBUG
+    const ThreadStatus::OperationType thread_op =
+        ThreadStatusUtil::GetThreadOperation();
+    Env::IOActivity io_activity =
+        ThreadStatusUtil::TEST_GetExpectedIOActivity(thread_op);
+    assert(io_activity == Env::IOActivity::kUnknown ||
+           io_activity == options.io_activity);
+#endif
     return target()->RangeSync(offset, nbytes, options, dbg);
   }
 };

db_stress_tool/db_stress_gflags.cc

@@ -1095,9 +1095,6 @@ DEFINE_int32(write_fault_one_in, 0,
              "On non-zero, enables fault injection on write. Currently only"
              "injects write error when writing to SST files.");
 
-DEFINE_bool(exclude_wal_from_write_fault_injection, false,
-            "If true, we won't inject write fault when writing to WAL file");
-
 DEFINE_int32(metadata_write_fault_one_in, 1000,
              "On non-zero, enables fault injection on metadata write (i.e, "
              "directory and file metadata write)");

db_stress_tool/db_stress_listener.h

@@ -284,13 +284,12 @@ class DbStressListener : public EventListener {
           FaultInjectionIOType::kMetadataWrite);
       // TODO(hx235): only exempt the flush thread during error recovery instead
       // of all the flush threads from error injection
-      fault_fs_guard->SetIOActivtiesExcludedFromFaultInjection(
+      fault_fs_guard->SetIOActivtiesExemptedFromFaultInjection(
           {Env::IOActivity::kFlush});
     }
   }
 
-  void OnErrorRecoveryEnd(
-      const BackgroundErrorRecoveryInfo& /*info*/) override {
+  void OnErrorRecoveryCompleted(Status /* old_bg_error */) override {
     RandomSleep();
     if (FLAGS_error_recovery_with_no_fault_injection && fault_fs_guard) {
       fault_fs_guard->EnableThreadLocalErrorInjection(
@@ -301,7 +300,7 @@ class DbStressListener : public EventListener {
           FaultInjectionIOType::kMetadataRead);
       fault_fs_guard->EnableThreadLocalErrorInjection(
           FaultInjectionIOType::kMetadataWrite);
-      fault_fs_guard->SetIOActivtiesExcludedFromFaultInjection({});
+      fault_fs_guard->SetIOActivtiesExemptedFromFaultInjection({});
     }
   }
 

db_stress_tool/db_stress_shared_state.h

@@ -37,7 +37,6 @@ DECLARE_int32(metadata_read_fault_one_in);
 DECLARE_int32(metadata_write_fault_one_in);
 DECLARE_int32(read_fault_one_in);
 DECLARE_int32(write_fault_one_in);
-DECLARE_bool(exclude_wal_from_write_fault_injection);
 DECLARE_int32(open_metadata_read_fault_one_in);
 DECLARE_int32(open_metadata_write_fault_one_in);
 DECLARE_int32(open_write_fault_one_in);

db_stress_tool/db_stress_test_base.cc

@@ -103,16 +103,12 @@ StressTest::StressTest()
   }
 }
 
-void StressTest::CleanUp() {
+StressTest::~StressTest() {
   for (auto cf : column_families_) {
     delete cf;
   }
   column_families_.clear();
-  if (db_) {
-    db_->Close();
-  }
   delete db_;
-  db_ = nullptr;
 
   for (auto* cf : cmp_cfhs_) {
     delete cf;
@@ -290,12 +286,19 @@ bool StressTest::BuildOptionsTable() {
            std::to_string(options_.write_buffer_size / 8),
        }},
       {"memtable_huge_page_size", {"0", std::to_string(2 * 1024 * 1024)}},
+      {"max_successive_merges", {"0", "2", "4"}},
       {"strict_max_successive_merges", {"false", "true"}},
       {"inplace_update_num_locks", {"100", "200", "300"}},
       // TODO: re-enable once internal task T124324915 is fixed.
       // {"experimental_mempurge_threshold", {"0.0", "1.0"}},
       // TODO(ljin): enable test for this option
       // {"disable_auto_compactions", {"100", "200", "300"}},
+      {"level0_file_num_compaction_trigger",
+       {
+           std::to_string(options_.level0_file_num_compaction_trigger),
+           std::to_string(options_.level0_file_num_compaction_trigger + 2),
+           std::to_string(options_.level0_file_num_compaction_trigger + 4),
+       }},
       {"level0_slowdown_writes_trigger",
        {
            std::to_string(options_.level0_slowdown_writes_trigger),
@@ -340,35 +343,6 @@ bool StressTest::BuildOptionsTable() {
        }},
       {"max_sequential_skip_in_iterations", {"4", "8", "12"}},
   };
-  if (FLAGS_compaction_style == kCompactionStyleUniversal &&
-      FLAGS_universal_max_read_amp > 0) {
-    // level0_file_num_compaction_trigger needs to be at most max_read_amp
-    options_tbl.emplace(
-        "level0_file_num_compaction_trigger",
-        std::vector<std::string>{
-            std::to_string(options_.level0_file_num_compaction_trigger),
-            std::to_string(
-                std::min(options_.level0_file_num_compaction_trigger + 2,
-                         FLAGS_universal_max_read_amp)),
-            std::to_string(
-                std::min(options_.level0_file_num_compaction_trigger + 4,
-                         FLAGS_universal_max_read_amp)),
-        });
-  } else {
-    options_tbl.emplace(
-        "level0_file_num_compaction_trigger",
-        std::vector<std::string>{
-            std::to_string(options_.level0_file_num_compaction_trigger),
-            std::to_string(options_.level0_file_num_compaction_trigger + 2),
-            std::to_string(options_.level0_file_num_compaction_trigger + 4),
-        });
-  }
-  if (FLAGS_unordered_write) {
-    options_tbl.emplace("max_successive_merges", std::vector<std::string>{"0"});
-  } else {
-    options_tbl.emplace("max_successive_merges",
-                        std::vector<std::string>{"0", "2", "4"});
-  }
 
   if (FLAGS_allow_setting_blob_options_dynamically) {
     options_tbl.emplace("enable_blob_files",
@@ -462,7 +436,7 @@ void StressTest::FinishInitDb(SharedState* shared) {
 void StressTest::TrackExpectedState(SharedState* shared) {
   // When data loss is simulated, recovery from potential data loss is a prefix
   // recovery that requires tracing
-  if (MightHaveUnsyncedDataLoss() && IsStateTracked()) {
+  if (MightHaveDataLoss() && IsStateTracked()) {
     Status s = shared->SaveAtAndAfter(db_);
     if (!s.ok()) {
       fprintf(stderr, "Error enabling history tracing: %s\n",
@@ -542,7 +516,9 @@ void StressTest::ProcessStatus(SharedState* shared, std::string opname,
   if (s.ok()) {
     return;
   }
-  if (!ignore_injected_error || !IsErrorInjectedAndRetryable(s)) {
+  if (ignore_injected_error && IsRetryableInjectedError(s)) {
+    fprintf(stdout, "%s failed: %s\n", opname.c_str(), s.ToString().c_str());
+  } else {
     std::ostringstream oss;
     oss << opname << " failed: " << s.ToString();
     VerificationAbort(shared, oss.str());
@@ -1042,8 +1018,10 @@ void StressTest::OperateDb(ThreadState* thread) {
       if (thread->rand.OneInOpt(FLAGS_manual_wal_flush_one_in)) {
         bool sync = thread->rand.OneIn(2) ? true : false;
         Status s = db_->FlushWAL(sync);
-        if (!s.ok() && !IsErrorInjectedAndRetryable(s) &&
-            !(sync && s.IsNotSupported())) {
+        if (IsRetryableInjectedError(s)) {
+          fprintf(stdout, "FlushWAL(sync=%s) failed: %s\n",
+                  (sync ? "true" : "false"), s.ToString().c_str());
+        } else if (!s.ok() && !(sync && s.IsNotSupported())) {
           fprintf(stderr, "FlushWAL(sync=%s) failed: %s\n",
                   (sync ? "true" : "false"), s.ToString().c_str());
         }
@@ -1051,19 +1029,17 @@ void StressTest::OperateDb(ThreadState* thread) {
 
       if (thread->rand.OneInOpt(FLAGS_lock_wal_one_in)) {
         Status s = db_->LockWAL();
-        if (!s.ok() && !IsErrorInjectedAndRetryable(s)) {
+        if (IsRetryableInjectedError(s)) {
+          fprintf(stdout, "LockWAL() failed: %s\n", s.ToString().c_str());
+        } else if (!s.ok()) {
           fprintf(stderr, "LockWAL() failed: %s\n", s.ToString().c_str());
-        } else if (s.ok()) {
+        } else {
           //  Temporarily disable error injection for verification
           if (fault_fs_guard) {
             fault_fs_guard->DisableThreadLocalErrorInjection(
                 FaultInjectionIOType::kRead);
             fault_fs_guard->DisableThreadLocalErrorInjection(
                 FaultInjectionIOType::kMetadataRead);
-            fault_fs_guard->DisableThreadLocalErrorInjection(
-                FaultInjectionIOType::kWrite);
-            fault_fs_guard->DisableThreadLocalErrorInjection(
-                FaultInjectionIOType::kMetadataWrite);
           }
 
           // Verify no writes during LockWAL
@@ -1122,17 +1098,13 @@ void StressTest::OperateDb(ThreadState* thread) {
                 FaultInjectionIOType::kRead);
             fault_fs_guard->EnableThreadLocalErrorInjection(
                 FaultInjectionIOType::kMetadataRead);
-            fault_fs_guard->EnableThreadLocalErrorInjection(
-                FaultInjectionIOType::kWrite);
-            fault_fs_guard->EnableThreadLocalErrorInjection(
-                FaultInjectionIOType::kMetadataWrite);
           }
         }
       }
 
       if (thread->rand.OneInOpt(FLAGS_sync_wal_one_in)) {
         Status s = db_->SyncWAL();
-        if (!s.ok() && !s.IsNotSupported() && !IsErrorInjectedAndRetryable(s)) {
+        if (!s.ok() && !s.IsNotSupported() && !IsRetryableInjectedError(s)) {
           fprintf(stderr, "SyncWAL() failed: %s\n", s.ToString().c_str());
         }
       }
@@ -1172,12 +1144,8 @@ void StressTest::OperateDb(ThreadState* thread) {
         ProcessStatus(shared, "GetLiveFiles", s_1);
         Status s_2 = TestGetLiveFilesMetaData();
         ProcessStatus(shared, "GetLiveFilesMetaData", s_2);
-        // TODO: enable again after making `GetLiveFilesStorageInfo()`
-        // compatible with `Options::recycle_log_file_num`
-        if (FLAGS_recycle_log_file_num == 0) {
-          Status s_3 = TestGetLiveFilesStorageInfo();
-          ProcessStatus(shared, "GetLiveFilesStorageInfo", s_3);
-        }
+        Status s_3 = TestGetLiveFilesStorageInfo();
+        ProcessStatus(shared, "GetLiveFilesStorageInfo", s_3);
       }
 
       if (thread->rand.OneInOpt(FLAGS_get_all_column_family_metadata_one_in)) {
@@ -1234,32 +1202,7 @@ void StressTest::OperateDb(ThreadState* thread) {
       }
 
       if (thread->rand.OneInOpt(FLAGS_get_property_one_in)) {
-        // TestGetProperty doesn't return status for us to tell whether it has
-        // failed due to injected error. So we disable fault injection to avoid
-        // false positive
-        if (fault_fs_guard) {
-          fault_fs_guard->DisableThreadLocalErrorInjection(
-              FaultInjectionIOType::kMetadataRead);
-          fault_fs_guard->DisableThreadLocalErrorInjection(
-              FaultInjectionIOType::kMetadataWrite);
-          fault_fs_guard->DisableThreadLocalErrorInjection(
-              FaultInjectionIOType::kRead);
-          fault_fs_guard->DisableThreadLocalErrorInjection(
-              FaultInjectionIOType::kWrite);
-        }
-
         TestGetProperty(thread);
-
-        if (fault_fs_guard) {
-          fault_fs_guard->EnableThreadLocalErrorInjection(
-              FaultInjectionIOType::kMetadataWrite);
-          fault_fs_guard->EnableThreadLocalErrorInjection(
-              FaultInjectionIOType::kMetadataRead);
-          fault_fs_guard->EnableThreadLocalErrorInjection(
-              FaultInjectionIOType::kRead);
-          fault_fs_guard->EnableThreadLocalErrorInjection(
-              FaultInjectionIOType::kWrite);
-        }
       }
 
       if (thread->rand.OneInOpt(FLAGS_get_properties_of_all_tables_one_in)) {
@@ -1286,29 +1229,17 @@ void StressTest::OperateDb(ThreadState* thread) {
         }
 
         if (total_size <= FLAGS_backup_max_size) {
-          // TODO(hx235): enable error injection with
+          // TODO(hx235): enable metadata error injection with
           // backup/restore after fixing the various issues it surfaces
-          if (fault_fs_guard) {
-            fault_fs_guard->DisableThreadLocalErrorInjection(
-                FaultInjectionIOType::kMetadataRead);
-            fault_fs_guard->DisableThreadLocalErrorInjection(
-                FaultInjectionIOType::kMetadataWrite);
-            fault_fs_guard->DisableThreadLocalErrorInjection(
-                FaultInjectionIOType::kRead);
-            fault_fs_guard->DisableThreadLocalErrorInjection(
-                FaultInjectionIOType::kWrite);
-          }
+          fault_fs_guard->DisableThreadLocalErrorInjection(
+              FaultInjectionIOType::kMetadataRead);
+          fault_fs_guard->DisableThreadLocalErrorInjection(
+              FaultInjectionIOType::kMetadataWrite);
           Status s = TestBackupRestore(thread, rand_column_families, rand_keys);
-          if (fault_fs_guard) {
-            fault_fs_guard->EnableThreadLocalErrorInjection(
-                FaultInjectionIOType::kMetadataWrite);
-            fault_fs_guard->EnableThreadLocalErrorInjection(
-                FaultInjectionIOType::kMetadataRead);
-            fault_fs_guard->EnableThreadLocalErrorInjection(
-                FaultInjectionIOType::kRead);
-            fault_fs_guard->EnableThreadLocalErrorInjection(
-                FaultInjectionIOType::kWrite);
-          }
+          fault_fs_guard->EnableThreadLocalErrorInjection(
+              FaultInjectionIOType::kMetadataWrite);
+          fault_fs_guard->EnableThreadLocalErrorInjection(
+              FaultInjectionIOType::kMetadataRead);
           ProcessStatus(shared, "Backup/restore", s);
         }
       }
@@ -1743,7 +1674,7 @@ Status StressTest::TestIterateImpl(ThreadState* thread,
     if (!(FLAGS_use_txn && FLAGS_txn_write_policy != 0 /* write committed */) &&
         !FLAGS_use_multi_cf_iterator && thread->rand.OneIn(4)) {
       Status s = iter->Refresh(snapshot_guard.snapshot());
-      if (!s.ok() && IsErrorInjectedAndRetryable(s)) {
+      if (!s.ok() && IsRetryableInjectedError(s)) {
         return s;
       }
       assert(s.ok());
@@ -1773,13 +1704,10 @@ Status StressTest::TestIterateImpl(ThreadState* thread,
       op_logs += "S " + key.ToString(true) + " ";
     }
 
-    if (!iter->status().ok() && IsErrorInjectedAndRetryable(iter->status())) {
-      return iter->status();
-    } else if (!cmp_iter->status().ok() &&
-               IsErrorInjectedAndRetryable(cmp_iter->status())) {
-      return cmp_iter->status();
+    if (IsRetryableInjectedError(iter->status()) ||
+        IsRetryableInjectedError(cmp_iter->status())) {
+      return !iter->status().ok() ? iter->status() : cmp_iter->status();
     }
-
     VerifyIterator(thread, cmp_cfh, ro, iter.get(), cmp_iter.get(), last_op,
                    key, op_logs, verify_func, &diverged);
 
@@ -1804,13 +1732,10 @@ Status StressTest::TestIterateImpl(ThreadState* thread,
 
       last_op = kLastOpNextOrPrev;
 
-      if (!iter->status().ok() && IsErrorInjectedAndRetryable(iter->status())) {
-        return iter->status();
-      } else if (!cmp_iter->status().ok() &&
-                 IsErrorInjectedAndRetryable(cmp_iter->status())) {
-        return cmp_iter->status();
+      if (IsRetryableInjectedError(iter->status()) ||
+          IsRetryableInjectedError(cmp_iter->status())) {
+        return !iter->status().ok() ? iter->status() : cmp_iter->status();
       }
-
       VerifyIterator(thread, cmp_cfh, ro, iter.get(), cmp_iter.get(), last_op,
                      key, op_logs, verify_func, &diverged);
     }
@@ -2354,7 +2279,7 @@ Status StressTest::TestBackupRestore(
         FaultInjectionIOType::kMetadataWrite);
   }
 
-  if (s.ok() || IsErrorInjectedAndRetryable(s)) {
+  if (s.ok() || IsRetryableInjectedError(s)) {
     // Preserve directories on failure, or allowed persistent backup
     if (!allow_persistent) {
       s = DestroyDir(db_stress_env, backup_dir);
@@ -2364,7 +2289,7 @@ Status StressTest::TestBackupRestore(
     }
   }
 
-  if (s.ok() || IsErrorInjectedAndRetryable(s)) {
+  if (s.ok() || IsRetryableInjectedError(s)) {
     s = DestroyDir(db_stress_env, restore_dir);
     if (!s.ok()) {
       from = "Destroy restore dir";
@@ -2383,7 +2308,7 @@ Status StressTest::TestBackupRestore(
         FaultInjectionIOType::kMetadataWrite);
   }
 
-  if (!s.ok() && !IsErrorInjectedAndRetryable(s)) {
+  if (!s.ok() && !IsRetryableInjectedError(s)) {
     fprintf(stderr,
             "Failure in %s with: %s under specified BackupEngineOptions: %s, "
             "CreateBackupOptions: %s, RestoreOptions: %s (Empty string or "
@@ -2545,32 +2470,23 @@ Status StressTest::TestCheckpoint(ThreadState* thread,
   Status s = Checkpoint::Create(db_, &checkpoint);
   if (s.ok()) {
     s = checkpoint->CreateCheckpoint(checkpoint_dir);
-    if (!s.ok() && !IsErrorInjectedAndRetryable(s)) {
+    if (!s.ok() && !IsRetryableInjectedError(s)) {
       fprintf(stderr, "Fail to create checkpoint to %s\n",
               checkpoint_dir.c_str());
       std::vector<std::string> files;
 
       // Temporarily disable error injection to print debugging information
-      if (fault_fs_guard) {
-        fault_fs_guard->DisableThreadLocalErrorInjection(
-            FaultInjectionIOType::kMetadataRead);
-      }
-
+      fault_fs_guard->DisableThreadLocalErrorInjection(
+          FaultInjectionIOType::kMetadataRead);
       Status my_s = db_stress_env->GetChildren(checkpoint_dir, &files);
-
       // Enable back disable error injection disabled for printing debugging
       // information
-      if (fault_fs_guard) {
-        fault_fs_guard->EnableThreadLocalErrorInjection(
-            FaultInjectionIOType::kMetadataRead);
-      }
-      if (!my_s.ok()) {
-        fprintf(stderr, "Fail to GetChildren under %s due to %s\n",
-                checkpoint_dir.c_str(), my_s.ToString().c_str());
-      } else {
-        for (const auto& f : files) {
-          fprintf(stderr, " %s\n", f.c_str());
-        }
+      fault_fs_guard->EnableThreadLocalErrorInjection(
+          FaultInjectionIOType::kMetadataRead);
+      assert(my_s.ok());
+
+      for (const auto& f : files) {
+        fprintf(stderr, " %s\n", f.c_str());
       }
     }
   }
@@ -2657,7 +2573,7 @@ Status StressTest::TestCheckpoint(ThreadState* thread,
         FaultInjectionIOType::kMetadataWrite);
   }
 
-  if (!s.ok() && !IsErrorInjectedAndRetryable(s)) {
+  if (!s.ok() && !IsRetryableInjectedError(s)) {
     fprintf(stderr, "A checkpoint operation failed with: %s\n",
             s.ToString().c_str());
   } else {
@@ -2808,18 +2724,18 @@ void StressTest::TestCompactFiles(ThreadState* thread,
       auto s = db_->CompactFiles(compact_options, column_family, input_files,
                                  static_cast<int>(output_level));
       if (!s.ok()) {
-        thread->stats.AddNumCompactFilesFailed(1);
         // TOOD (hx235): allow an exact list of tolerable failures under stress
         // test
         bool non_ok_status_allowed =
-            s.IsManualCompactionPaused() || IsErrorInjectedAndRetryable(s) ||
+            s.IsManualCompactionPaused() || (IsRetryableInjectedError(s)) ||
             s.IsAborted() || s.IsInvalidArgument() || s.IsNotSupported();
+        fprintf(non_ok_status_allowed ? stdout : stderr,
+                "Unable to perform CompactFiles(): %s under specified "
+                "CompactionOptions: %s (Empty string or "
+                "missing field indicates default option or value is used)\n",
+                s.ToString().c_str(), compact_opt_oss.str().c_str());
+        thread->stats.AddNumCompactFilesFailed(1);
         if (!non_ok_status_allowed) {
-          fprintf(stderr,
-                  "Unable to perform CompactFiles(): %s under specified "
-                  "CompactionOptions: %s (Empty string or "
-                  "missing field indicates default option or value is used)\n",
-                  s.ToString().c_str(), compact_opt_oss.str().c_str());
           thread->shared->SafeTerminate();
         }
       } else {
@@ -2843,12 +2759,11 @@ void StressTest::TestPromoteL0(ThreadState* thread,
          s.ToString().find("VersionBuilder: Cannot delete table file") !=
              std::string::npos &&
          s.ToString().find("since it is on level") != std::string::npos);
-
+    fprintf(non_ok_status_allowed ? stdout : stderr,
+            "Unable to perform PromoteL0(): %s under specified "
+            "target_level: %d.\n",
+            s.ToString().c_str(), target_level);
     if (!non_ok_status_allowed) {
-      fprintf(stderr,
-              "Unable to perform PromoteL0(): %s under specified "
-              "target_level: %d.\n",
-              s.ToString().c_str(), target_level);
       thread->shared->SafeTerminate();
     }
   }
@@ -2937,8 +2852,10 @@ void StressTest::TestAcquireSnapshot(ThreadState* thread,
   // will later read the same key before releasing the snapshot and
   // verify that the results are the same.
   Status status_at = db_->Get(ropt, column_family, key, &value_at);
-  if (!status_at.ok() && IsErrorInjectedAndRetryable(status_at)) {
-    db_->ReleaseSnapshot(snapshot);
+  if (IsRetryableInjectedError(status_at)) {
+    fprintf(stdout,
+            "Unable to perform Get() during TestAcquireSnapshot(): %s)\n",
+            status_at.ToString().c_str());
     return;
   }
   std::vector<bool>* key_vec = nullptr;
@@ -3047,33 +2964,19 @@ void StressTest::TestCompactRange(ThreadState* thread, int64_t rand_key,
   uint32_t pre_hash = 0;
   if (thread->rand.OneIn(2)) {
     // Temporarily disable error injection to for validation
-    if (fault_fs_guard) {
-      fault_fs_guard->DisableThreadLocalErrorInjection(
-          FaultInjectionIOType::kMetadataRead);
-      fault_fs_guard->DisableThreadLocalErrorInjection(
-          FaultInjectionIOType::kRead);
-      fault_fs_guard->DisableThreadLocalErrorInjection(
-          FaultInjectionIOType::kMetadataWrite);
-      fault_fs_guard->DisableThreadLocalErrorInjection(
-          FaultInjectionIOType::kWrite);
-    }
-
     // Declare a snapshot and compare the data before and after the compaction
+    fault_fs_guard->DisableThreadLocalErrorInjection(
+        FaultInjectionIOType::kMetadataRead);
+    fault_fs_guard->DisableThreadLocalErrorInjection(
+        FaultInjectionIOType::kRead);
     pre_snapshot = db_->GetSnapshot();
     pre_hash =
         GetRangeHash(thread, pre_snapshot, column_family, start_key, end_key);
-
     // Enable back error injection disabled for validation
-    if (fault_fs_guard) {
-      fault_fs_guard->EnableThreadLocalErrorInjection(
-          FaultInjectionIOType::kMetadataRead);
-      fault_fs_guard->EnableThreadLocalErrorInjection(
-          FaultInjectionIOType::kRead);
-      fault_fs_guard->EnableThreadLocalErrorInjection(
-          FaultInjectionIOType::kMetadataWrite);
-      fault_fs_guard->EnableThreadLocalErrorInjection(
-          FaultInjectionIOType::kWrite);
-    }
+    fault_fs_guard->EnableThreadLocalErrorInjection(
+        FaultInjectionIOType::kMetadataRead);
+    fault_fs_guard->EnableThreadLocalErrorInjection(
+        FaultInjectionIOType::kRead);
   }
   std::ostringstream compact_range_opt_oss;
   compact_range_opt_oss << "exclusive_manual_compaction: "
@@ -3093,15 +2996,15 @@ void StressTest::TestCompactRange(ThreadState* thread, int64_t rand_key,
   if (!status.ok()) {
     // TOOD (hx235): allow an exact list of tolerable failures under stress test
     bool non_ok_status_allowed =
-        status.IsManualCompactionPaused() ||
-        IsErrorInjectedAndRetryable(status) || status.IsAborted() ||
-        status.IsInvalidArgument() || status.IsNotSupported();
+        status.IsManualCompactionPaused() || IsRetryableInjectedError(status) ||
+        status.IsAborted() || status.IsInvalidArgument() ||
+        status.IsNotSupported();
+    fprintf(non_ok_status_allowed ? stdout : stderr,
+            "Unable to perform CompactRange(): %s under specified "
+            "CompactRangeOptions: %s (Empty string or "
+            "missing field indicates default option or value is used)\n",
+            status.ToString().c_str(), compact_range_opt_oss.str().c_str());
     if (!non_ok_status_allowed) {
-      fprintf(stderr,
-              "Unable to perform CompactRange(): %s under specified "
-              "CompactRangeOptions: %s (Empty string or "
-              "missing field indicates default option or value is used)\n",
-              status.ToString().c_str(), compact_range_opt_oss.str().c_str());
       // Fail fast to preserve the DB state.
       thread->shared->SetVerificationFailure();
     }
@@ -3109,16 +3012,11 @@ void StressTest::TestCompactRange(ThreadState* thread, int64_t rand_key,
 
   if (pre_snapshot != nullptr) {
     // Temporarily disable error injection for validation
-    if (fault_fs_guard) {
-      fault_fs_guard->DisableThreadLocalErrorInjection(
-          FaultInjectionIOType::kMetadataRead);
-      fault_fs_guard->DisableThreadLocalErrorInjection(
-          FaultInjectionIOType::kRead);
-      fault_fs_guard->DisableThreadLocalErrorInjection(
-          FaultInjectionIOType::kMetadataWrite);
-      fault_fs_guard->DisableThreadLocalErrorInjection(
-          FaultInjectionIOType::kWrite);
-    }
+    // Declaring a snapshot and compare the data before and after the compaction
+    fault_fs_guard->DisableThreadLocalErrorInjection(
+        FaultInjectionIOType::kMetadataRead);
+    fault_fs_guard->DisableThreadLocalErrorInjection(
+        FaultInjectionIOType::kRead);
     uint32_t post_hash =
         GetRangeHash(thread, pre_snapshot, column_family, start_key, end_key);
     if (pre_hash != post_hash) {
@@ -3134,17 +3032,11 @@ void StressTest::TestCompactRange(ThreadState* thread, int64_t rand_key,
       thread->shared->SetVerificationFailure();
     }
     db_->ReleaseSnapshot(pre_snapshot);
-    if (fault_fs_guard) {
-      // Enable back error injection disabled for validation
-      fault_fs_guard->EnableThreadLocalErrorInjection(
-          FaultInjectionIOType::kMetadataRead);
-      fault_fs_guard->EnableThreadLocalErrorInjection(
-          FaultInjectionIOType::kRead);
-      fault_fs_guard->EnableThreadLocalErrorInjection(
-          FaultInjectionIOType::kMetadataWrite);
-      fault_fs_guard->EnableThreadLocalErrorInjection(
-          FaultInjectionIOType::kWrite);
-    }
+    // Enable back error injection disabled for validation
+    fault_fs_guard->EnableThreadLocalErrorInjection(
+        FaultInjectionIOType::kMetadataRead);
+    fault_fs_guard->EnableThreadLocalErrorInjection(
+        FaultInjectionIOType::kRead);
   }
 }
 
@@ -3534,23 +3426,17 @@ void StressTest::Open(SharedState* shared, bool reopen) {
     }
     // TODO; test transaction DB Open with fault injection
     if (!FLAGS_use_txn) {
-      bool inject_sync_fault = FLAGS_sync_fault_injection;
       bool inject_open_meta_read_error =
           FLAGS_open_metadata_read_fault_one_in > 0;
       bool inject_open_meta_write_error =
           FLAGS_open_metadata_write_fault_one_in > 0;
       bool inject_open_read_error = FLAGS_open_read_fault_one_in > 0;
       bool inject_open_write_error = FLAGS_open_write_fault_one_in > 0;
-      if ((inject_sync_fault || inject_open_meta_read_error ||
-           inject_open_meta_write_error || inject_open_read_error ||
-           inject_open_write_error) &&
+      if ((inject_open_meta_read_error || inject_open_meta_write_error ||
+           inject_open_read_error || inject_open_write_error) &&
           fault_fs_guard
               ->FileExists(FLAGS_db + "/CURRENT", IOOptions(), nullptr)
               .ok()) {
-        if (inject_sync_fault || inject_open_write_error) {
-          fault_fs_guard->SetFilesystemDirectWritable(false);
-          fault_fs_guard->SetInjectUnsyncedDataLoss(inject_sync_fault);
-        }
         fault_fs_guard->SetThreadLocalErrorContext(
             FaultInjectionIOType::kMetadataRead,
             static_cast<uint32_t>(FLAGS_seed),
@@ -3574,6 +3460,7 @@ void StressTest::Open(SharedState* shared, bool reopen) {
         fault_fs_guard->EnableThreadLocalErrorInjection(
             FaultInjectionIOType::kRead);
 
+        fault_fs_guard->SetFilesystemDirectWritable(false);
         fault_fs_guard->SetThreadLocalErrorContext(
             FaultInjectionIOType::kWrite, static_cast<uint32_t>(FLAGS_seed),
             FLAGS_open_write_fault_one_in, false /* retryable */,
@@ -3608,11 +3495,11 @@ void StressTest::Open(SharedState* shared, bool reopen) {
           }
         }
 
-        if (inject_sync_fault || inject_open_meta_read_error ||
-            inject_open_meta_write_error || inject_open_read_error ||
-            inject_open_write_error) {
+        if (inject_open_meta_read_error || inject_open_meta_write_error ||
+            inject_open_read_error || inject_open_write_error) {
           fault_fs_guard->DisableThreadLocalErrorInjection(
               FaultInjectionIOType::kRead);
+          fault_fs_guard->SetFilesystemDirectWritable(true);
           fault_fs_guard->DisableThreadLocalErrorInjection(
               FaultInjectionIOType::kWrite);
           fault_fs_guard->DisableThreadLocalErrorInjection(
@@ -3635,10 +3522,9 @@ void StressTest::Open(SharedState* shared, bool reopen) {
             }
           }
           if (!s.ok()) {
-            // After failure to opening a DB due to IO error or unsynced data
-            // loss, retry should successfully open the DB with correct data if
-            // no IO error shows up.
-            inject_sync_fault = false;
+            // After failure to opening a DB due to IO error, retry should
+            // successfully open the DB with correct data if no IO error shows
+            // up.
             inject_open_meta_read_error = false;
             inject_open_meta_write_error = false;
             inject_open_read_error = false;
@@ -3798,7 +3684,7 @@ void StressTest::Reopen(ThreadState* thread) {
   // crash-recovery verification does. Therefore it always expects no data loss
   // and we should ensure no data loss in testing.
   // TODO(hx235): eliminate the FlushWAL(true /* sync */)/SyncWAL() below
-  if (!FLAGS_disable_wal && !FLAGS_avoid_flush_during_shutdown) {
+  if (!FLAGS_disable_wal) {
     Status s;
     if (FLAGS_manual_wal_flush_one_in > 0) {
       s = db_->FlushWAL(/*sync=*/true);
@@ -3835,7 +3721,7 @@ void StressTest::Reopen(ThreadState* thread) {
           clock_->TimeToString(now / 1000000).c_str(), num_times_reopened_);
   Open(thread->shared, /*reopen=*/true);
 
-  if (thread->shared->GetStressTest()->MightHaveUnsyncedDataLoss() &&
+  if (thread->shared->GetStressTest()->MightHaveDataLoss() &&
       IsStateTracked()) {
     Status s = thread->shared->SaveAtAndAfter(db_);
     if (!s.ok()) {

db_stress_tool/db_stress_test_base.h

@@ -27,7 +27,7 @@ class StressTest {
  public:
   StressTest();
 
-  virtual ~StressTest() {}
+  virtual ~StressTest();
 
   std::shared_ptr<Cache> NewCache(size_t capacity, int32_t num_shard_bits);
 
@@ -44,13 +44,12 @@ class StressTest {
   virtual void VerifyDb(ThreadState* thread) const = 0;
   virtual void ContinuouslyVerifyDb(ThreadState* /*thread*/) const = 0;
   void PrintStatistics();
-  bool MightHaveUnsyncedDataLoss() {
-    return FLAGS_sync_fault_injection || FLAGS_disable_wal ||
+  bool MightHaveDataLoss() {
+    return FLAGS_sync_fault_injection || FLAGS_write_fault_one_in > 0 ||
+           FLAGS_metadata_write_fault_one_in > 0 || FLAGS_disable_wal ||
            FLAGS_manual_wal_flush_one_in > 0;
   }
 
-  void CleanUp();
-
  protected:
   static int GetMinInjectedErrorCount(int error_count_1, int error_count_2) {
     if (error_count_1 > 0 && error_count_2 > 0) {
@@ -281,10 +280,9 @@ class StressTest {
     return Status::NotSupported("TestCustomOperations() must be overridden");
   }
 
-  bool IsErrorInjectedAndRetryable(const Status& error_s) const {
-    assert(!error_s.ok());
-    return error_s.getState() && std::strstr(error_s.getState(), "inject") &&
-           !status_to_io_status(Status(error_s)).GetDataLoss();
+  bool IsRetryableInjectedError(const Status& s) const {
+    return s.getState() && std::strstr(s.getState(), "inject") &&
+           !status_to_io_status(Status(s)).GetDataLoss();
   }
 
   void ProcessStatus(SharedState* shared, std::string msg, const Status& s,

db_stress_tool/db_stress_tool.cc

@@ -90,10 +90,10 @@ int db_stress_tool(int argc, char** argv) {
     FaultInjectionTestFS* fs =
         new FaultInjectionTestFS(raw_env->GetFileSystem());
     fault_fs_guard.reset(fs);
-    // Set it to direct writable here to initially bypass any fault injection
-    // during DB open This will correspondingly be overwritten in
-    // StressTest::Open() for open fault injection and in RunStressTestImpl()
-    // for proper fault injection setup.
+    // Set it to direct writable here to not lose files created during DB open
+    // when no open fault injection is not enabled.
+    // This will be overwritten in StressTest::Open() for open fault injection
+    // and in RunStressTestImpl() for proper write fault injection setup.
     fault_fs_guard->SetFilesystemDirectWritable(true);
     fault_env_guard =
         std::make_shared<CompositeEnvWrapper>(raw_env, fault_fs_guard);
@@ -362,11 +362,11 @@ int db_stress_tool(int argc, char** argv) {
   // Initialize the Zipfian pre-calculated array
   InitializeHotKeyGenerator(FLAGS_hot_key_alpha);
   shared.reset(new SharedState(db_stress_env, stress.get()));
-  bool run_stress_test = RunStressTest(shared.get());
-  // Close DB in CleanUp() before destructor to prevent race between destructor
-  // and operations in listener callbacks (e.g. MultiOpsTxnsStressListener).
-  stress->CleanUp();
-  return run_stress_test ? 0 : 1;
+  if (RunStressTest(shared.get())) {
+    return 0;
+  } else {
+    return 1;
+  }
 }
 
 }  // namespace ROCKSDB_NAMESPACE

db_stress_tool/no_batched_ops_stress.cc

@@ -599,7 +599,10 @@ class NonBatchedOpsStressTest : public StressTest {
                   key.ToString(true).c_str());
         }
       }
-    } else if (injected_error_count == 0 || !IsErrorInjectedAndRetryable(s)) {
+    } else if (injected_error_count > 0 && IsRetryableInjectedError(s)) {
+      fprintf(stdout, "error : Get() returns %s for key: %s.\n",
+              s.ToString().c_str(), key.ToString(true).c_str());
+    } else {
       thread->shared->SetVerificationFailure();
       fprintf(stderr, "error : Get() returns %s for key: %s.\n",
               s.ToString().c_str(), key.ToString(true).c_str());
@@ -873,10 +876,15 @@ class NonBatchedOpsStressTest : public StressTest {
       } else if (s.IsMergeInProgress() && use_txn) {
         // With txn this is sometimes expected.
         thread->stats.AddGets(1, 1);
-      } else if (injected_error_count == 0 || !IsErrorInjectedAndRetryable(s)) {
-        fprintf(stderr, "MultiGet error: %s\n", s.ToString().c_str());
-        thread->stats.AddErrors(1);
-        shared->SetVerificationFailure();
+      } else {
+        if (injected_error_count > 0 && IsRetryableInjectedError(s)) {
+          fprintf(stdout, "MultiGet error: %s\n", s.ToString().c_str());
+          thread->stats.AddVerifiedErrors(1);
+        } else {
+          fprintf(stderr, "MultiGet error: %s\n", s.ToString().c_str());
+          thread->stats.AddErrors(1);
+          shared->SetVerificationFailure();
+        }
       }
 
       // Enable back error injection disbled for checking results
@@ -1072,7 +1080,10 @@ class NonBatchedOpsStressTest : public StressTest {
                   StringToHex(key_str).c_str());
         }
       }
-    } else if (injected_error_count == 0 || !IsErrorInjectedAndRetryable(s)) {
+    } else if (injected_error_count > 0 && IsRetryableInjectedError(s)) {
+      fprintf(stdout, "error : GetEntity() returns %s for key: %s.\n",
+              s.ToString().c_str(), StringToHex(key_str).c_str());
+    } else {
       fprintf(stderr, "error : GetEntity() returns %s for key: %s.\n",
               s.ToString().c_str(), StringToHex(key_str).c_str());
       thread->shared->SetVerificationFailure();
@@ -1268,11 +1279,15 @@ class NonBatchedOpsStressTest : public StressTest {
           thread->stats.AddGets(1, 1);
         } else if (s.IsNotFound()) {
           thread->stats.AddGets(1, 0);
-        } else if (injected_error_count == 0 ||
-                   !IsErrorInjectedAndRetryable(s)) {
-          fprintf(stderr, "MultiGetEntity error: %s\n", s.ToString().c_str());
-          thread->stats.AddErrors(1);
-          thread->shared->SetVerificationFailure();
+        } else {
+          if (injected_error_count > 0 && IsRetryableInjectedError(s)) {
+            fprintf(stderr, "MultiGetEntity error: %s\n", s.ToString().c_str());
+            thread->stats.AddVerifiedErrors(1);
+          } else {
+            fprintf(stderr, "MultiGetEntity error: %s\n", s.ToString().c_str());
+            thread->stats.AddErrors(1);
+            thread->shared->SetVerificationFailure();
+          }
         }
       }
       // Enable back error injection disbled for checking results
@@ -1534,7 +1549,13 @@ class NonBatchedOpsStressTest : public StressTest {
 
     if (s.ok()) {
       thread->stats.AddPrefixes(1, count);
-    } else if (injected_error_count == 0 || !IsErrorInjectedAndRetryable(s)) {
+    } else if (injected_error_count > 0 && IsRetryableInjectedError(s)) {
+      fprintf(stdout, "TestPrefixScan error: %s\n", s.ToString().c_str());
+    } else {
+      fprintf(stderr, "TestPrefixScan is retryable error? %s\n",
+              IsRetryableInjectedError(s) ? "true" : "false");
+      fprintf(stderr, "TestPrefixScan injected_error_count: %d\n",
+              injected_error_count);
       fprintf(stderr, "TestPrefixScan error: %s\n", s.ToString().c_str());
       thread->shared->SetVerificationFailure();
     }
@@ -1586,13 +1607,14 @@ class NonBatchedOpsStressTest : public StressTest {
 
     if (FLAGS_verify_before_write) {
       // Temporarily disable error injection for preparation
-      if (fault_fs_guard) {
-        fault_fs_guard->DisableThreadLocalErrorInjection(
-            FaultInjectionIOType::kRead);
-        fault_fs_guard->DisableThreadLocalErrorInjection(
-            FaultInjectionIOType::kMetadataRead);
-      }
-
+      fault_fs_guard->DisableThreadLocalErrorInjection(
+          FaultInjectionIOType::kRead);
+      fault_fs_guard->DisableThreadLocalErrorInjection(
+          FaultInjectionIOType::kWrite);
+      fault_fs_guard->DisableThreadLocalErrorInjection(
+          FaultInjectionIOType::kMetadataRead);
+      fault_fs_guard->DisableThreadLocalErrorInjection(
+          FaultInjectionIOType::kMetadataWrite);
       std::string from_db;
       Status s = db_->Get(read_opts, cfh, k, &from_db);
       bool res = VerifyOrSyncValue(
@@ -1600,12 +1622,14 @@ class NonBatchedOpsStressTest : public StressTest {
           /* msg_prefix */ "Pre-Put Get verification", from_db, s);
 
       // Enable back error injection disabled for preparation
-      if (fault_fs_guard) {
-        fault_fs_guard->EnableThreadLocalErrorInjection(
-            FaultInjectionIOType::kRead);
-        fault_fs_guard->EnableThreadLocalErrorInjection(
-            FaultInjectionIOType::kMetadataRead);
-      }
+      fault_fs_guard->EnableThreadLocalErrorInjection(
+          FaultInjectionIOType::kRead);
+      fault_fs_guard->EnableThreadLocalErrorInjection(
+          FaultInjectionIOType::kWrite);
+      fault_fs_guard->EnableThreadLocalErrorInjection(
+          FaultInjectionIOType::kMetadataRead);
+      fault_fs_guard->EnableThreadLocalErrorInjection(
+          FaultInjectionIOType::kMetadataWrite);
       if (!res) {
         return s;
       }
@@ -1671,7 +1695,8 @@ class NonBatchedOpsStressTest : public StressTest {
 
     if (!s.ok()) {
       pending_expected_value.Rollback();
-      if (IsErrorInjectedAndRetryable(s)) {
+      if (IsRetryableInjectedError(s)) {
+        fprintf(stdout, "put or merge error: %s\n", s.ToString().c_str());
         return s;
       } else if (FLAGS_inject_error_severity == 2) {
         if (!is_db_stopped_ && s.severity() >= Status::Severity::kFatalError) {
@@ -1731,7 +1756,8 @@ class NonBatchedOpsStressTest : public StressTest {
 
       if (!s.ok()) {
         pending_expected_value.Rollback();
-        if (IsErrorInjectedAndRetryable(s)) {
+        if (IsRetryableInjectedError(s)) {
+          fprintf(stdout, "delete error: %s\n", s.ToString().c_str());
           return s;
         } else if (FLAGS_inject_error_severity == 2) {
           if (!is_db_stopped_ &&
@@ -1766,7 +1792,8 @@ class NonBatchedOpsStressTest : public StressTest {
 
       if (!s.ok()) {
         pending_expected_value.Rollback();
-        if (IsErrorInjectedAndRetryable(s)) {
+        if (IsRetryableInjectedError(s)) {
+          fprintf(stdout, "delete error: %s\n", s.ToString().c_str());
           return s;
         } else if (FLAGS_inject_error_severity == 2) {
           if (!is_db_stopped_ &&
@@ -1836,7 +1863,8 @@ class NonBatchedOpsStressTest : public StressTest {
            pending_expected_values) {
         pending_expected_value.Rollback();
       }
-      if (IsErrorInjectedAndRetryable(s)) {
+      if (IsRetryableInjectedError(s)) {
+        fprintf(stdout, "delete range error: %s\n", s.ToString().c_str());
         return s;
       } else if (FLAGS_inject_error_severity == 2) {
         if (!is_db_stopped_ && s.severity() >= Status::Severity::kFatalError) {
@@ -1869,24 +1897,19 @@ class NonBatchedOpsStressTest : public StressTest {
     std::ostringstream ingest_options_oss;
 
     // Temporarily disable error injection for preparation
-    if (fault_fs_guard) {
-      fault_fs_guard->DisableThreadLocalErrorInjection(
-          FaultInjectionIOType::kMetadataRead);
-      fault_fs_guard->DisableThreadLocalErrorInjection(
-          FaultInjectionIOType::kMetadataWrite);
-    }
-
+    fault_fs_guard->DisableThreadLocalErrorInjection(
+        FaultInjectionIOType::kMetadataRead);
+    fault_fs_guard->DisableThreadLocalErrorInjection(
+        FaultInjectionIOType::kMetadataWrite);
     if (db_stress_env->FileExists(sst_filename).ok()) {
       // Maybe we terminated abnormally before, so cleanup to give this file
       // ingestion a clean slate
       s = db_stress_env->DeleteFile(sst_filename);
     }
-    if (fault_fs_guard) {
-      fault_fs_guard->EnableThreadLocalErrorInjection(
-          FaultInjectionIOType::kMetadataRead);
-      fault_fs_guard->EnableThreadLocalErrorInjection(
-          FaultInjectionIOType::kMetadataWrite);
-    }
+    fault_fs_guard->EnableThreadLocalErrorInjection(
+        FaultInjectionIOType::kMetadataRead);
+    fault_fs_guard->EnableThreadLocalErrorInjection(
+        FaultInjectionIOType::kMetadataWrite);
 
     SstFileWriter sst_file_writer(EnvOptions(options_), options_);
     if (s.ok()) {
@@ -1969,7 +1992,9 @@ class NonBatchedOpsStressTest : public StressTest {
         pending_expected_value.Rollback();
       }
 
-      if (!IsErrorInjectedAndRetryable(s)) {
+      if (IsRetryableInjectedError(s)) {
+        fprintf(stdout, "file ingestion error: %s \n", s.ToString().c_str());
+      } else {
         fprintf(stderr,
                 "file ingestion error: %s under specified "
                 "IngestExternalFileOptions: %s (Empty string or "
@@ -2158,7 +2183,11 @@ class NonBatchedOpsStressTest : public StressTest {
       assert(last_key < ub);
       if (!iter->Valid()) {
         if (!iter->status().ok()) {
-          if (IsErrorInjectedAndRetryable(iter->status())) {
+          if (IsRetryableInjectedError(iter->status())) {
+            fprintf(
+                stdout,
+                "TestIterate against expected state forward scan error: %s\n",
+                iter->status().ToString().c_str());
             return iter->status();
           } else {
             thread->shared->SetVerificationFailure();
@@ -2220,7 +2249,11 @@ class NonBatchedOpsStressTest : public StressTest {
       assert(lb < last_key);
       if (!iter->Valid()) {
         if (!iter->status().ok()) {
-          if (IsErrorInjectedAndRetryable(iter->status())) {
+          if (IsRetryableInjectedError(iter->status())) {
+            fprintf(
+                stdout,
+                "TestIterate against expected state backward scan error: %s\n",
+                iter->status().ToString().c_str());
             return iter->status();
           } else {
             thread->shared->SetVerificationFailure();
@@ -2284,7 +2317,10 @@ class NonBatchedOpsStressTest : public StressTest {
             shared->Get(rand_column_family, i + lb));
       }
       Status rs = iter->Refresh();
-      if (!rs.ok() && IsErrorInjectedAndRetryable(rs)) {
+      if (!rs.ok() && IsRetryableInjectedError(rs)) {
+        fprintf(stdout,
+                "TestIterate against expected state refresh error: %s\n",
+                iter->status().ToString().c_str());
         return rs;
       }
       assert(rs.ok());
@@ -2448,7 +2484,11 @@ class NonBatchedOpsStressTest : public StressTest {
     }
 
     if (!iter->status().ok()) {
-      if (IsErrorInjectedAndRetryable(iter->status())) {
+      if (IsRetryableInjectedError(iter->status())) {
+        fprintf(
+            stdout,
+            "TestIterate against expected state scan from middle error: %s\n",
+            iter->status().ToString().c_str());
         return iter->status();
       } else {
         thread->shared->SetVerificationFailure();

env/composite_env_wrapper.h

@@ -18,13 +18,6 @@
 
 namespace ROCKSDB_NAMESPACE {
 
-// This class supports abstracting different types of an `Env`'s functionality
-// into separate interfaces. It is constructed with a `FileSystem` and a
-// `SystemClock` and delegates:
-// * File system operations to member `file_system_`.
-// * Time related misc operations to member `clock_`.
-// A subclass needs to inherit `CompositeEnv` and provide implementations for
-// the thread management related APIs.
 class CompositeEnv : public Env {
  public:
   // Initialize a CompositeEnvWrapper that delegates all thread/time related
@@ -257,20 +250,6 @@ class CompositeEnv : public Env {
   }
 };
 
-// A `CompositeEnvWrapper` is constructed with a target `Env` object, an
-// optional `FileSystem` object and an optional `SystemClock` object.
-// `Env::GetFileSystem()` is a fallback file system if no such object is
-// explicitly provided. Similarly, `Env::GetSystemClock()` is a fallback system
-// clock.
-// Besides delegating corresponding functionality to `file_system_` and `clock_`
-// which is inherited from `CompositeEnv`, it also implements the thread
-// management APIs by delegating them to the target `Env` object.
-//
-// Effectively, this class helps to support using customized file system
-// implementations such as a remote file system instead of the default file
-// system provided by the operating system.
-//
-// Also see public API `NewCompositeEnv` in rocksdb/include/env.h
 class CompositeEnvWrapper : public CompositeEnv {
  public:
   // Initialize a CompositeEnvWrapper that delegates all thread/time related

env/env.cc

@@ -355,12 +355,6 @@ class LegacyDirectoryWrapper : public FSDirectory {
   std::unique_ptr<Directory> target_;
 };
 
-// A helper class to make legacy `Env` implementations be backward compatible
-// now that all `Env` implementations are expected to have a `FileSystem` type
-// member `file_system_` and a `SystemClock` type member `clock_`.
-// This class wraps a legacy `Env` object and expose its file system related
-// APIs as a `FileSystem` interface. Also check `LegacySystemClock` that does
-// the same thing for the clock related APIs.
 class LegacyFileSystemWrapper : public FileSystem {
  public:
   // Initialize an EnvWrapper that delegates all calls to *t

file/writable_file_writer.h

@@ -233,17 +233,13 @@ class WritableFileWriter {
   WritableFileWriter& operator=(const WritableFileWriter&) = delete;
 
   ~WritableFileWriter() {
-    IOOptions io_options;
-#ifndef NDEBUG
-    // This is needed to pass the IOActivity related checks in stress test.
-    // See DbStressWritableFileWrapper.
-    ThreadStatus::OperationType op_type =
+    ThreadStatus::OperationType cur_op_type =
         ThreadStatusUtil::GetThreadOperation();
-    io_options.io_activity =
-        ThreadStatusUtil::TEST_GetExpectedIOActivity(op_type);
-#endif
-    auto s = Close(io_options);
+    ThreadStatusUtil::SetThreadOperation(
+        ThreadStatus::OperationType::OP_UNKNOWN);
+    auto s = Close(IOOptions());
     s.PermitUncheckedError();
+    ThreadStatusUtil::SetThreadOperation(cur_op_type);
   }
 
   std::string file_name() const { return file_name_; }
@@ -308,9 +304,6 @@ class WritableFileWriter {
   // operating on the file after an error happens.
   void reset_seen_error() {
     seen_error_.store(false, std::memory_order_relaxed);
-#ifndef NDEBUG
-    seen_injected_error_.store(false, std::memory_order_relaxed);
-#endif  // NDEBUG
   }
   void set_seen_error(const Status& s) {
     seen_error_.store(true, std::memory_order_relaxed);

include/rocksdb/c.h

@@ -650,19 +650,6 @@ extern ROCKSDB_LIBRARY_API void rocksdb_approximate_sizes_cf(
     const size_t* range_start_key_len, const char* const* range_limit_key,
     const size_t* range_limit_key_len, uint64_t* sizes, char** errptr);
 
-enum {
-  rocksdb_size_approximation_flags_none = 0,
-  rocksdb_size_approximation_flags_include_memtable = 1 << 0,
-  rocksdb_size_approximation_flags_include_files = 1 << 1,
-};
-
-extern ROCKSDB_LIBRARY_API void rocksdb_approximate_sizes_cf_with_flags(
-    rocksdb_t* db, rocksdb_column_family_handle_t* column_family,
-    int num_ranges, const char* const* range_start_key,
-    const size_t* range_start_key_len, const char* const* range_limit_key,
-    const size_t* range_limit_key_len, uint8_t include_flags, uint64_t* sizes,
-    char** errptr);
-
 extern ROCKSDB_LIBRARY_API void rocksdb_compact_range(rocksdb_t* db,
                                                       const char* start_key,
                                                       size_t start_key_len,
@@ -863,13 +850,6 @@ extern ROCKSDB_LIBRARY_API void rocksdb_writebatch_iterate(
     rocksdb_writebatch_t*, void* state,
     void (*put)(void*, const char* k, size_t klen, const char* v, size_t vlen),
     void (*deleted)(void*, const char* k, size_t klen));
-extern ROCKSDB_LIBRARY_API void rocksdb_writebatch_iterate_cf(
-    rocksdb_writebatch_t*, void* state,
-    void (*put_cf)(void*, uint32_t cfid, const char* k, size_t klen,
-                   const char* v, size_t vlen),
-    void (*deleted_cf)(void*, uint32_t cfid, const char* k, size_t klen),
-    void (*merge_cf)(void*, uint32_t cfid, const char* k, size_t klen,
-                     const char* v, size_t vlen));
 extern ROCKSDB_LIBRARY_API const char* rocksdb_writebatch_data(
     rocksdb_writebatch_t*, size_t* size);
 extern ROCKSDB_LIBRARY_API void rocksdb_writebatch_set_save_point(
@@ -1225,11 +1205,6 @@ extern ROCKSDB_LIBRARY_API int rocksdb_options_get_info_log_level(
     rocksdb_options_t*);
 extern ROCKSDB_LIBRARY_API rocksdb_logger_t*
 rocksdb_logger_create_stderr_logger(int log_level, const char* prefix);
-extern ROCKSDB_LIBRARY_API rocksdb_logger_t*
-rocksdb_logger_create_callback_logger(int log_level,
-                                      void (*)(void* priv, unsigned lev,
-                                               char* msg, size_t len),
-                                      void* priv);
 extern ROCKSDB_LIBRARY_API void rocksdb_logger_destroy(
     rocksdb_logger_t* logger);
 extern ROCKSDB_LIBRARY_API void rocksdb_options_set_write_buffer_size(

include/rocksdb/db.h

@@ -1334,10 +1334,9 @@ class DB {
 
   // DB implementations export properties about their state via this method.
   // If "property" is a valid "string" property understood by this DB
-  // implementation (see Properties struct above for valid options) and the DB
-  // is able to get and fill "*value" with its current value, then return true.
-  // In all the other cases (e.g, "property" is an invalid "string" property, IO
-  // errors ..), it returns false.
+  // implementation (see Properties struct above for valid options), fills
+  // "*value" with its current value and returns true.  Otherwise, returns
+  // false.
   virtual bool GetProperty(ColumnFamilyHandle* column_family,
                            const Slice& property, std::string* value) = 0;
   virtual bool GetProperty(const Slice& property, std::string* value) {

include/rocksdb/env.h

@@ -145,11 +145,6 @@ struct EnvOptions {
 // Exceptions MUST NOT propagate out of overridden functions into RocksDB,
 // because RocksDB is not exception-safe. This could cause undefined behavior
 // including data loss, unreported corruption, deadlocks, and more.
-// An interface that abstracts RocksDB's interactions with the operating system
-// environment. There are three main types of APIs:
-// 1) File system operations, like creating a file, writing to a file, etc.
-// 2) Thread management.
-// 3) Misc functions, like getting the current time.
 class Env : public Customizable {
  public:
   static const char* kDefaultName() { return "DefaultEnv"; }

include/rocksdb/options.h

@@ -645,7 +645,9 @@ struct DBOptions {
   bool verify_sst_unique_id_in_manifest = true;
 
   // Use the specified object to interact with the environment,
-  // e.g. to read/write files, schedule background work, etc.
+  // e.g. to read/write files, schedule background work, etc. In the near
+  // future, support for doing storage operations such as read/write files
+  // through env will be deprecated in favor of file_system (see below)
   // Default: Env::Default()
   Env* env = Env::Default();
 
@@ -2179,24 +2181,6 @@ struct IngestExternalFileOptions {
   //
   // XXX: "bottommost" is obsolete/confusing terminology to refer to last level
   bool fail_if_not_bottommost_level = false;
-  // EXPERIMENTAL
-  // If set to true, ingestion will
-  // - allow the files to not be generated by SstFileWriter, and
-  // - ignore cf_id mismatch between cf_id in the files and the CF they are
-  // being ingested into.
-  //
-  // REQUIRES:
-  // - files to be ingested do not overlap with existing keys.
-  // - write_global_seqno = false
-  // - move_files = false
-  //
-  // Warning: This ONLY works for SST files where all keys have sequence number
-  // zero and with no duplicated user keys (this should be guaranteed if the
-  // file is generated by a DB with zero as the largest sequence number).
-  // We scan the entire SST files to validate sequence numbers.
-  // Warning: If a DB contains ingested files generated by another DB/CF,
-  // RepairDB() may not correctly recover these files. It may lose these files.
-  bool allow_db_generated_files = false;
 };
 
 enum TraceFilterType : uint64_t {

include/rocksdb/statistics.h

@@ -529,11 +529,6 @@ enum Tickers : uint32_t {
   // Footer corruption detected when opening an SST file for reading
   SST_FOOTER_CORRUPTION_COUNT,
 
-  // Counters for file read retries with the verify_and_reconstruct_read
-  // file system option after detecting a checksum mismatch
-  FILE_READ_CORRUPTION_RETRY_COUNT,
-  FILE_READ_CORRUPTION_RETRY_SUCCESS_COUNT,
-
   TICKER_ENUM_MAX
 };
 

include/rocksdb/utilities/checkpoint.h

@@ -32,10 +32,8 @@ class Checkpoint {
   // same filesystem as the database, and copied otherwise.
   // (2) other required files (like MANIFEST) are always copied.
   // log_size_for_flush: if the total log file size is equal or larger than
-  // this value, then a flush is triggered for all the column families.
-  // The archived log size will not be included when calculating the total log
-  // size.
-  // The default value is 0, which means flush is always triggered. If you move
+  // this value, then a flush is triggered for all the column families. The
+  // default value is 0, which means flush is always triggered. If you move
   // away from the default, the checkpoint may not contain up-to-date data
   // if WAL writing is not always enabled.
   // Flush will always trigger if it is 2PC.

include/rocksdb/version.h

@@ -12,8 +12,8 @@
 // NOTE: in 'main' development branch, this should be the *next*
 // minor or major version number planned for release.
 #define ROCKSDB_MAJOR 9
-#define ROCKSDB_MINOR 5
-#define ROCKSDB_PATCH 2
+#define ROCKSDB_MINOR 4
+#define ROCKSDB_PATCH 1
 
 // 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/portal.h

@@ -5269,10 +5269,6 @@ class TickerTypeJni {
         return -0x53;
       case ROCKSDB_NAMESPACE::Tickers::SST_FOOTER_CORRUPTION_COUNT:
         return -0x55;
-      case ROCKSDB_NAMESPACE::Tickers::FILE_READ_CORRUPTION_RETRY_COUNT:
-        return -0x56;
-      case ROCKSDB_NAMESPACE::Tickers::FILE_READ_CORRUPTION_RETRY_SUCCESS_COUNT:
-        return -0x57;
       case ROCKSDB_NAMESPACE::Tickers::TICKER_ENUM_MAX:
         // -0x54 is the max value at this time. Since these values are exposed
         // directly to Java clients, we'll keep the value the same till the next
@@ -5730,11 +5726,6 @@ class TickerTypeJni {
         return ROCKSDB_NAMESPACE::Tickers::PREFETCH_HITS;
       case -0x55:
         return ROCKSDB_NAMESPACE::Tickers::SST_FOOTER_CORRUPTION_COUNT;
-      case -0x56:
-        return ROCKSDB_NAMESPACE::Tickers::FILE_READ_CORRUPTION_RETRY_COUNT;
-      case -0x57:
-        return ROCKSDB_NAMESPACE::Tickers::
-            FILE_READ_CORRUPTION_RETRY_SUCCESS_COUNT;
       case -0x54:
         // -0x54 is the max value at this time. Since these values are exposed
         // directly to Java clients, we'll keep the value the same till the next

java/src/main/java/org/rocksdb/RateLimiter.java

@@ -45,7 +45,7 @@ public class RateLimiter extends RocksObject {
    *     when rate_bytes_per_sec is set to 10MB/s and refill_period_us is set to
    *     100ms, then 1MB is refilled every 100ms internally. Larger value can
    *     lead to burstier writes while smaller value introduces more CPU
-   *     overhead. The default of 100,000us should work for most cases.
+   *     overhead. The default of 100,000ms should work for most cases.
    */
   public RateLimiter(final long rateBytesPerSecond,
       final long refillPeriodMicros) {
@@ -66,7 +66,7 @@ public class RateLimiter extends RocksObject {
    *     when rate_bytes_per_sec is set to 10MB/s and refill_period_us is set to
    *     100ms, then 1MB is refilled every 100ms internally. Larger value can
    *     lead to burstier writes while smaller value introduces more CPU
-   *     overhead. The default of 100,000us should work for most cases.
+   *     overhead. The default of 100,000ms should work for most cases.
    * @param fairness RateLimiter accepts high-pri requests and low-pri requests.
    *     A low-pri request is usually blocked in favor of hi-pri request.
    *     Currently, RocksDB assigns low-pri to request from compaction and
@@ -95,7 +95,7 @@ public class RateLimiter extends RocksObject {
    *     when rate_bytes_per_sec is set to 10MB/s and refill_period_us is set to
    *     100ms, then 1MB is refilled every 100ms internally. Larger value can
    *     lead to burstier writes while smaller value introduces more CPU
-   *     overhead. The default of 100,000us should work for most cases.
+   *     overhead. The default of 100,000ms should work for most cases.
    * @param fairness RateLimiter accepts high-pri requests and low-pri requests.
    *     A low-pri request is usually blocked in favor of hi-pri request.
    *     Currently, RocksDB assigns low-pri to request from compaction and
@@ -127,7 +127,7 @@ public class RateLimiter extends RocksObject {
    *     when rate_bytes_per_sec is set to 10MB/s and refill_period_us is set to
    *     100ms, then 1MB is refilled every 100ms internally. Larger value can
    *     lead to burstier writes while smaller value introduces more CPU
-   *     overhead. The default of 100,000us should work for most cases.
+   *     overhead. The default of 100,000ms should work for most cases.
    * @param fairness RateLimiter accepts high-pri requests and low-pri requests.
    *     A low-pri request is usually blocked in favor of hi-pri request.
    *     Currently, RocksDB assigns low-pri to request from compaction and

java/src/main/java/org/rocksdb/TickerType.java

@@ -878,10 +878,6 @@ public enum TickerType {
 
     SST_FOOTER_CORRUPTION_COUNT((byte) -0x55),
 
-    FILE_READ_CORRUPTION_RETRY_COUNT((byte) -0x56),
-
-    FILE_READ_CORRUPTION_RETRY_SUCCESS_COUNT((byte) -0x57),
-
     TICKER_ENUM_MAX((byte) -0x54);
 
     private final byte value;

monitoring/statistics.cc

@@ -266,10 +266,6 @@ const std::vector<std::pair<Tickers, std::string>> TickersNameMap = {
     {PREFETCH_BYTES_USEFUL, "rocksdb.prefetch.bytes.useful"},
     {PREFETCH_HITS, "rocksdb.prefetch.hits"},
     {SST_FOOTER_CORRUPTION_COUNT, "rocksdb.footer.corruption.count"},
-    {FILE_READ_CORRUPTION_RETRY_COUNT,
-     "rocksdb.file.read.corruption.retry.count"},
-    {FILE_READ_CORRUPTION_RETRY_SUCCESS_COUNT,
-     "rocksdb.file.read.corruption.retry.success.count"},
 };
 
 const std::vector<std::pair<Histograms, std::string>> HistogramsNameMap = {

options/db_options.h

@@ -98,20 +98,17 @@ struct ImmutableDBOptions {
   std::string db_host_id;
   FileTypeSet checksum_handoff_file_types;
   CacheTier lowest_used_cache_tier;
+  // Convenience/Helper objects that are not part of the base DBOptions
+  std::shared_ptr<FileSystem> fs;
+  SystemClock* clock;
+  Statistics* stats;
+  Logger* logger;
   std::shared_ptr<CompactionService> compaction_service;
   bool enforce_single_del_contracts;
   uint64_t follower_refresh_catchup_period_ms;
   uint64_t follower_catchup_retry_count;
   uint64_t follower_catchup_retry_wait_ms;
 
-  // Beginning convenience/helper objects that are not part of the base
-  // DBOptions
-  std::shared_ptr<FileSystem> fs;
-  SystemClock* clock;
-  Statistics* stats;
-  Logger* logger;
-  // End of convenience/helper objects.
-
   bool IsWalDirSameAsDBPath() const;
   bool IsWalDirSameAsDBPath(const std::string& path) const;
   const std::string& GetWalDir() const;

port/jemalloc_helper.h

@@ -6,7 +6,7 @@
 #pragma once
 
 #if defined(__clang__) && defined(__GLIBC__)
-// glibc's `posix_memalign()` declaration specifies `noexcept` while clang's
+// glibc's `posix_memalign()` declaration specifies `throw()` while clang's
 // declaration does not. There is a hack in clang to make its re-declaration
 // compatible with glibc's if they are declared consecutively. That hack breaks
 // if yet another `posix_memalign()` declaration comes between glibc's and
@@ -14,7 +14,7 @@
 // declarations both come before "jemalloc.h"'s `posix_memalign()` declaration.
 //
 // This problem could also be avoided if "jemalloc.h"'s `posix_memalign()`
-// declaration did not specify `noexcept` when built with clang.
+// declaration did not specify `throw()` when built with clang.
 #include <mm_malloc.h>
 #endif
 

port/win/port_win.cc

@@ -101,9 +101,8 @@ bool CondVar::TimedWait(uint64_t abs_time_us) {
   std::unique_lock<std::mutex> lk(mu_->getLock(), std::adopt_lock);
 
   // Work around https://github.com/microsoft/STL/issues/369
-  // std::condition_variable_any::wait_for had a fix, but
-  // std::condition_variable still doesn't have a fix in STL yet
-#if defined(_MSC_VER)
+#if defined(_MSC_VER) && \
+    (!defined(_MSVC_STL_UPDATE) || _MSVC_STL_UPDATE < 202008L)
   if (relTimeUs == std::chrono::microseconds::zero()) {
     lk.unlock();
     lk.lock();

table/block_based/block_based_table_iterator.cc

@@ -823,12 +823,6 @@ void BlockBasedTableIterator::BlockCacheLookupForReadAheadSize(
         read_options_, block_handle,
         &(block_handle_info.cachable_entry_).As<Block_kData>());
     if (!s.ok()) {
-#ifndef NDEBUG
-      // To allow fault injection verification to pass since non-okay status in
-      // `BlockCacheLookupForReadAheadSize()` won't fail the read but to have
-      // less or no readahead
-      IGNORE_STATUS_IF_ERROR(s);
-#endif
       break;
     }
 
@@ -858,15 +852,6 @@ void BlockBasedTableIterator::BlockCacheLookupForReadAheadSize(
     is_index_at_curr_block_ = false;
   }
 
-#ifndef NDEBUG
-  // To allow fault injection verification to pass since non-okay status in
-  // `BlockCacheLookupForReadAheadSize()` won't fail the read but to have less
-  // or no readahead
-  if (!index_iter_->status().ok()) {
-    IGNORE_STATUS_IF_ERROR(index_iter_->status());
-  }
-#endif
-
   if (found_first_miss_block) {
     // Iterate cache hit block handles from the end till a Miss is there, to
     // truncate and update the end offset till that Miss.

table/block_based/block_based_table_reader.cc

@@ -478,7 +478,6 @@ bool IsFeatureSupported(const TableProperties& table_properties,
 }
 
 // Caller has to ensure seqno is not nullptr.
-// Set *seqno to the global sequence number for reading this file.
 Status GetGlobalSequenceNumber(const TableProperties& table_properties,
                                SequenceNumber largest_seqno,
                                SequenceNumber* seqno) {
@@ -501,17 +500,12 @@ Status GetGlobalSequenceNumber(const TableProperties& table_properties,
   }
 
   uint32_t version = DecodeFixed32(version_pos->second.c_str());
-  if (version != 2) {
-    std::array<char, 200> msg_buf;
-    if (version != 1) {
-      snprintf(msg_buf.data(), msg_buf.max_size(),
-               "An external sst file has corrupted version %u.", version);
-      return Status::Corruption(msg_buf.data());
-    }
-    if (seqno_pos != props.end()) {
+  if (version < 2) {
+    if (seqno_pos != props.end() || version != 1) {
+      std::array<char, 200> msg_buf;
       // This is a v1 external sst file, global_seqno is not supported.
       snprintf(msg_buf.data(), msg_buf.max_size(),
-               "An external sst file with version %u has global seqno "
+               "An external sst file with version %u have global seqno "
                "property with value %s",
                version, seqno_pos->second.c_str());
       return Status::Corruption(msg_buf.data());
@@ -693,10 +687,6 @@ Status BlockBasedTable::Open(
       s = ReadFooterFromFile(retry_opts, file.get(), *ioptions.fs,
                              prefetch_buffer.get(), file_size, &footer,
                              kBlockBasedTableMagicNumber);
-      RecordTick(ioptions.stats, FILE_READ_CORRUPTION_RETRY_COUNT);
-      if (s.ok()) {
-        RecordTick(ioptions.stats, FILE_READ_CORRUPTION_RETRY_SUCCESS_COUNT);
-      }
     }
   }
   if (!s.ok()) {

table/block_based/block_based_table_reader_sync_and_async.h

@@ -223,16 +223,13 @@ DEFINE_SYNC_AND_ASYNC(void, BlockBasedTable::RetrieveMultipleBlocks)
         s = VerifyBlockChecksum(footer, data, handle.size(),
                                 rep_->file->file_name(), handle.offset());
         RecordTick(ioptions.stats, BLOCK_CHECKSUM_COMPUTE_COUNT);
-        if (!s.ok()) {
-          RecordTick(ioptions.stats, BLOCK_CHECKSUM_MISMATCH_COUNT);
-        }
         TEST_SYNC_POINT_CALLBACK("RetrieveMultipleBlocks:VerifyChecksum", &s);
         if (!s.ok() &&
             CheckFSFeatureSupport(ioptions.fs.get(),
                                   FSSupportedOps::kVerifyAndReconstructRead)) {
           assert(s.IsCorruption());
           assert(!ioptions.allow_mmap_reads);
-          RecordTick(ioptions.stats, FILE_READ_CORRUPTION_RETRY_COUNT);
+          RecordTick(ioptions.stats, BLOCK_CHECKSUM_MISMATCH_COUNT);
 
           // Repeat the read for this particular block using the regular
           // synchronous Read API. We can use the same chunk of memory
@@ -249,10 +246,6 @@ DEFINE_SYNC_AND_ASYNC(void, BlockBasedTable::RetrieveMultipleBlocks)
             assert(result.size() == BlockSizeWithTrailer(handle));
             s = VerifyBlockChecksum(footer, data, handle.size(),
                                     rep_->file->file_name(), handle.offset());
-            if (s.ok()) {
-              RecordTick(ioptions.stats,
-                         FILE_READ_CORRUPTION_RETRY_SUCCESS_COUNT);
-            }
           } else {
             s = io_s;
           }

table/block_based/full_filter_block.cc

@@ -20,8 +20,7 @@ namespace ROCKSDB_NAMESPACE {
 FullFilterBlockBuilder::FullFilterBlockBuilder(
     const SliceTransform* _prefix_extractor, bool whole_key_filtering,
     FilterBitsBuilder* filter_bits_builder)
-    : need_last_prefix_(whole_key_filtering && _prefix_extractor != nullptr),
-      prefix_extractor_(_prefix_extractor),
+    : prefix_extractor_(_prefix_extractor),
       whole_key_filtering_(whole_key_filtering),
       last_whole_key_recorded_(false),
       last_prefix_recorded_(false),
@@ -39,7 +38,7 @@ void FullFilterBlockBuilder::Add(const Slice& key_without_ts) {
   const bool add_prefix =
       prefix_extractor_ && prefix_extractor_->InDomain(key_without_ts);
 
-  if (need_last_prefix_ && !last_prefix_recorded_ && last_key_in_domain_) {
+  if (!last_prefix_recorded_ && last_key_in_domain_) {
     // We can reach here when a new filter partition starts in partitioned
     // filter. The last prefix in the previous partition should be added if
     // necessary regardless of key_without_ts, to support prefix SeekForPrev.
@@ -83,15 +82,7 @@ inline void FullFilterBlockBuilder::AddKey(const Slice& key) {
 void FullFilterBlockBuilder::AddPrefix(const Slice& key) {
   assert(prefix_extractor_ && prefix_extractor_->InDomain(key));
   Slice prefix = prefix_extractor_->Transform(key);
-  if (need_last_prefix_) {
-    // WART/FIXME: Because last_prefix_str_ is needed above to make
-    // SeekForPrev work with partitioned + prefix filters, we are currently
-    // use this inefficient code in that case (in addition to prefix+whole
-    // key). Hopefully this can be optimized with some refactoring up the call
-    // chain to BlockBasedTableBuilder. Even in PartitionedFilterBlockBuilder,
-    // we don't currently have access to the previous key/prefix by the time we
-    // know we are starting a new partition.
-
+  if (whole_key_filtering_) {
     // if both whole_key and prefix are added to bloom then we will have whole
     // key and prefix addition being interleaved and thus cannot rely on the
     // bits builder to properly detect the duplicates by comparing with the last

table/block_based/full_filter_block.h

@@ -69,7 +69,6 @@ class FullFilterBlockBuilder : public FilterBlockBuilder {
   void AddPrefix(const Slice& key);
   const SliceTransform* prefix_extractor() { return prefix_extractor_; }
   const std::string& last_prefix_str() const { return last_prefix_str_; }
-  bool need_last_prefix_;
 
  private:
   // important: all of these might point to invalid addresses

table/block_based/partitioned_filter_block.cc

@@ -45,9 +45,6 @@ PartitionedFilterBlockBuilder::PartitionedFilterBlockBuilder(
       p_index_builder_(p_index_builder),
       keys_added_to_partition_(0),
       total_added_in_built_(0) {
-  // See FullFilterBlockBuilder::AddPrefix
-  need_last_prefix_ = prefix_extractor() != nullptr;
-  // Compute keys_per_partition_
   keys_per_partition_ = static_cast<uint32_t>(
       filter_bits_builder_->ApproximateNumEntries(partition_size));
   if (keys_per_partition_ < 1) {
@@ -89,7 +86,7 @@ void PartitionedFilterBlockBuilder::MaybeCutAFilterBlock(
   }
 
   // Add the prefix of the next key before finishing the partition without
-  // updating last_prefix_str_. This hack fixes a bug with format_verison=3
+  // updating last_prefix_str_. This hack, fixes a bug with format_verison=3
   // where seeking for the prefix would lead us to the previous partition.
   const bool maybe_add_prefix =
       next_key && prefix_extractor() && prefix_extractor()->InDomain(*next_key);

table/block_fetcher.cc

@@ -84,9 +84,7 @@ inline bool BlockFetcher::TryGetFromPrefetchBuffer() {
         if (io_status_.ok()) {
           got_from_prefetch_buffer_ = true;
           used_buf_ = const_cast<char*>(slice_.data());
-        } else if (io_status_.IsCorruption()) {
-          // Returning true apparently indicates we either got some data from
-          // the prefetch buffer, or we tried and encountered an error.
+        } else if (!(io_status_.IsCorruption() && retry_corrupt_read_)) {
           return true;
         }
       }
@@ -336,15 +334,9 @@ void BlockFetcher::ReadBlock(bool retry) {
     ProcessTrailerIfPresent();
   }
 
-  if (retry) {
-    RecordTick(ioptions_.stats, FILE_READ_CORRUPTION_RETRY_COUNT);
-  }
   if (io_status_.ok()) {
     InsertCompressedBlockToPersistentCacheIfNeeded();
     fs_buf_ = std::move(read_req.fs_scratch);
-    if (retry) {
-      RecordTick(ioptions_.stats, FILE_READ_CORRUPTION_RETRY_SUCCESS_COUNT);
-    }
   } else {
     ReleaseFileSystemProvidedBuffer(&read_req);
     direct_io_buf_.reset();
@@ -363,11 +355,7 @@ IOStatus BlockFetcher::ReadBlockContents() {
     return IOStatus::OK();
   }
   if (TryGetFromPrefetchBuffer()) {
-    if (io_status_.IsCorruption() && retry_corrupt_read_) {
-      ReadBlock(/*retry=*/true);
-    }
     if (!io_status_.ok()) {
-      assert(!fs_buf_);
       return io_status_;
     }
   } else if (!TryGetSerializedBlockFromPersistentCache()) {

table/compaction_merging_iterator.cc

@@ -11,8 +11,8 @@ class CompactionMergingIterator : public InternalIterator {
   CompactionMergingIterator(
       const InternalKeyComparator* comparator, InternalIterator** children,
       int n, bool is_arena_mode,
-      std::vector<std::pair<std::unique_ptr<TruncatedRangeDelIterator>,
-                            std::unique_ptr<TruncatedRangeDelIterator>**>>&
+      std::vector<
+          std::pair<TruncatedRangeDelIterator*, TruncatedRangeDelIterator***>>
           range_tombstones)
       : is_arena_mode_(is_arena_mode),
         comparator_(comparator),
@@ -27,7 +27,7 @@ class CompactionMergingIterator : public InternalIterator {
     }
     assert(range_tombstones.size() == static_cast<size_t>(n));
     for (auto& p : range_tombstones) {
-      range_tombstone_iters_.push_back(std::move(p.first));
+      range_tombstone_iters_.push_back(p.first);
     }
     pinned_heap_item_.resize(n);
     for (int i = 0; i < n; ++i) {
@@ -47,7 +47,10 @@ class CompactionMergingIterator : public InternalIterator {
   }
 
   ~CompactionMergingIterator() override {
-    range_tombstone_iters_.clear();
+    // TODO: use unique_ptr for range_tombstone_iters_
+    for (auto child : range_tombstone_iters_) {
+      delete child;
+    }
 
     for (auto& child : children_) {
       child.iter.DeleteIter(is_arena_mode_);
@@ -194,8 +197,7 @@ class CompactionMergingIterator : public InternalIterator {
   // nullptr means the sorted run of children_[i] does not have range
   // tombstones (or the current SSTable does not have range tombstones in the
   // case of LevelIterator).
-  std::vector<std::unique_ptr<TruncatedRangeDelIterator>>
-      range_tombstone_iters_;
+  std::vector<TruncatedRangeDelIterator*> range_tombstone_iters_;
   // Used as value for range tombstone keys
   std::string dummy_tombstone_val{};
 
@@ -347,9 +349,8 @@ void CompactionMergingIterator::AddToMinHeapOrCheckStatus(HeapItem* child) {
 
 InternalIterator* NewCompactionMergingIterator(
     const InternalKeyComparator* comparator, InternalIterator** children, int n,
-    std::vector<std::pair<std::unique_ptr<TruncatedRangeDelIterator>,
-                          std::unique_ptr<TruncatedRangeDelIterator>**>>&
-        range_tombstone_iters,
+    std::vector<std::pair<TruncatedRangeDelIterator*,
+                          TruncatedRangeDelIterator***>>& range_tombstone_iters,
     Arena* arena) {
   assert(n >= 0);
   if (n == 0) {

table/compaction_merging_iterator.h

@@ -38,8 +38,7 @@ class CompactionMergingIterator;
 
 InternalIterator* NewCompactionMergingIterator(
     const InternalKeyComparator* comparator, InternalIterator** children, int n,
-    std::vector<std::pair<std::unique_ptr<TruncatedRangeDelIterator>,
-                          std::unique_ptr<TruncatedRangeDelIterator>**>>&
-        range_tombstone_iters,
+    std::vector<std::pair<TruncatedRangeDelIterator*,
+                          TruncatedRangeDelIterator***>>& range_tombstone_iters,
     Arena* arena = nullptr);
 }  // namespace ROCKSDB_NAMESPACE

table/merging_iterator.cc

@@ -96,9 +96,8 @@ class MergingIterator : public InternalIterator {
   // could be updated. In that case, this merging iterator is only responsible
   // for freeing the new range tombstone iterator that it has pointers to in
   // range_tombstone_iters_.
-  void AddRangeTombstoneIterator(
-      std::unique_ptr<TruncatedRangeDelIterator>&& iter) {
-    range_tombstone_iters_.emplace_back(std::move(iter));
+  void AddRangeTombstoneIterator(TruncatedRangeDelIterator* iter) {
+    range_tombstone_iters_.emplace_back(iter);
   }
 
   // Called by MergingIteratorBuilder when all point iterators and range
@@ -126,7 +125,9 @@ class MergingIterator : public InternalIterator {
   }
 
   ~MergingIterator() override {
-    range_tombstone_iters_.clear();
+    for (auto child : range_tombstone_iters_) {
+      delete child;
+    }
 
     for (auto& child : children_) {
       child.iter.DeleteIter(is_arena_mode_);
@@ -623,8 +624,7 @@ class MergingIterator : public InternalIterator {
   // Invariant(rti): pinned_heap_item_[i] is in minHeap_ iff
   // range_tombstone_iters_[i]->Valid() and at most one pinned_heap_item_[i] is
   // in minHeap_.
-  std::vector<std::unique_ptr<TruncatedRangeDelIterator>>
-      range_tombstone_iters_;
+  std::vector<TruncatedRangeDelIterator*> range_tombstone_iters_;
 
   // Levels (indices into range_tombstone_iters_/children_ ) that currently have
   // "active" range tombstones. See comments above MergingIterator for meaning
@@ -841,8 +841,7 @@ void MergingIterator::SeekImpl(const Slice& target, size_t starting_level,
         prefetched_target.emplace_back(
             level, current_search_key.GetInternalKey().ToString());
       }
-      UnownedPtr<TruncatedRangeDelIterator> range_tombstone_iter =
-          range_tombstone_iters_[level].get();
+      auto range_tombstone_iter = range_tombstone_iters_[level];
       if (range_tombstone_iter) {
         range_tombstone_iter->SeekInternalKey(
             current_search_key.GetInternalKey());
@@ -1126,8 +1125,7 @@ void MergingIterator::SeekForPrevImpl(const Slice& target,
         prefetched_target.emplace_back(
             level, current_search_key.GetInternalKey().ToString());
       }
-      UnownedPtr<TruncatedRangeDelIterator> range_tombstone_iter =
-          range_tombstone_iters_[level].get();
+      auto range_tombstone_iter = range_tombstone_iters_[level];
       if (range_tombstone_iter) {
         range_tombstone_iter->SeekForPrev(current_search_key.GetUserKey());
         if (range_tombstone_iter->Valid()) {
@@ -1351,8 +1349,7 @@ void MergingIterator::SwitchToForward() {
     ParseInternalKey(target, &pik, false /* log_err_key */)
         .PermitUncheckedError();
     for (size_t i = 0; i < range_tombstone_iters_.size(); ++i) {
-      UnownedPtr<TruncatedRangeDelIterator> iter =
-          range_tombstone_iters_[i].get();
+      auto iter = range_tombstone_iters_[i];
       if (iter) {
         iter->Seek(pik.user_key);
         // The while loop is needed as the Seek() call above is only for user
@@ -1398,8 +1395,7 @@ void MergingIterator::SwitchToBackward() {
   ParseInternalKey(target, &pik, false /* log_err_key */)
       .PermitUncheckedError();
   for (size_t i = 0; i < range_tombstone_iters_.size(); ++i) {
-    UnownedPtr<TruncatedRangeDelIterator> iter =
-        range_tombstone_iters_[i].get();
+    auto iter = range_tombstone_iters_[i];
     if (iter) {
       iter->SeekForPrev(pik.user_key);
       // Since the SeekForPrev() call above is only for user key,
@@ -1694,9 +1690,8 @@ void MergeIteratorBuilder::AddIterator(InternalIterator* iter) {
 }
 
 void MergeIteratorBuilder::AddPointAndTombstoneIterator(
-    InternalIterator* point_iter,
-    std::unique_ptr<TruncatedRangeDelIterator>&& tombstone_iter,
-    std::unique_ptr<TruncatedRangeDelIterator>** tombstone_iter_ptr) {
+    InternalIterator* point_iter, TruncatedRangeDelIterator* tombstone_iter,
+    TruncatedRangeDelIterator*** tombstone_iter_ptr) {
   // tombstone_iter_ptr != nullptr means point_iter is a LevelIterator.
   bool add_range_tombstone = tombstone_iter ||
                              !merge_iter->range_tombstone_iters_.empty() ||
@@ -1716,7 +1711,7 @@ void MergeIteratorBuilder::AddPointAndTombstoneIterator(
              merge_iter->children_.size() - 1) {
         merge_iter->AddRangeTombstoneIterator(nullptr);
       }
-      merge_iter->AddRangeTombstoneIterator(std::move(tombstone_iter));
+      merge_iter->AddRangeTombstoneIterator(tombstone_iter);
     }
 
     if (tombstone_iter_ptr) {

table/merging_iterator.h

@@ -70,10 +70,8 @@ class MergeIteratorBuilder {
   // point iterators are not LevelIterator, then range tombstone iterator is
   // only added to the merging iter if there is a non-null `tombstone_iter`.
   void AddPointAndTombstoneIterator(
-      InternalIterator* point_iter,
-      std::unique_ptr<TruncatedRangeDelIterator>&& tombstone_iter,
-      std::unique_ptr<TruncatedRangeDelIterator>** tombstone_iter_ptr =
-          nullptr);
+      InternalIterator* point_iter, TruncatedRangeDelIterator* tombstone_iter,
+      TruncatedRangeDelIterator*** tombstone_iter_ptr = nullptr);
 
   // Get arena used to build the merging iterator. It is called one a child
   // iterator needs to be allocated.
@@ -93,7 +91,7 @@ class MergeIteratorBuilder {
   Arena* arena;
   // Used to set LevelIterator.range_tombstone_iter_.
   // See AddRangeTombstoneIterator() implementation for more detail.
-  std::vector<std::pair<size_t, std::unique_ptr<TruncatedRangeDelIterator>**>>
+  std::vector<std::pair<size_t, TruncatedRangeDelIterator***>>
       range_del_iter_ptrs_;
 };
 

tools/check_format_compatible.sh

@@ -125,7 +125,7 @@ EOF
 
 # To check for DB forward compatibility with loading options (old version
 # reading data from new), as well as backward compatibility
-declare -a db_forward_with_options_refs=("8.6.fb" "8.7.fb" "8.8.fb" "8.9.fb" "8.10.fb" "8.11.fb" "9.0.fb" "9.1.fb" "9.2.fb" "9.3.fb" "9.4.fb")
+declare -a db_forward_with_options_refs=("8.6.fb" "8.7.fb" "8.8.fb" "8.9.fb" "8.10.fb" "8.11.fb" "9.0.fb" "9.1.fb" "9.2.fb" "9.3.fb")
 # To check for DB forward compatibility without loading options (in addition
 # to the "with loading options" set), as well as backward compatibility
 declare -a db_forward_no_options_refs=() # N/A at the moment

tools/db_crashtest.py

@@ -48,15 +48,13 @@ default_params = {
     "charge_filter_construction": lambda: random.choice([0, 1]),
     "charge_table_reader": lambda: random.choice([0, 1]),
     "charge_file_metadata": lambda: random.choice([0, 1]),
-    "checkpoint_one_in": lambda: random.choice([10000, 1000000]),
+    "checkpoint_one_in":  lambda: random.choice([10000, 1000000]),
     "compression_type": lambda: random.choice(
         ["none", "snappy", "zlib", "lz4", "lz4hc", "xpress", "zstd"]
     ),
-    "bottommost_compression_type": lambda: (
-        "disable"
-        if random.randint(0, 1) == 0
-        else random.choice(["none", "snappy", "zlib", "lz4", "lz4hc", "xpress", "zstd"])
-    ),
+    "bottommost_compression_type": lambda: "disable"
+    if random.randint(0, 1) == 0
+    else random.choice(["none", "snappy", "zlib", "lz4", "lz4hc", "xpress", "zstd"]),
     "checksum_type": lambda: random.choice(
         ["kCRC32c", "kxxHash", "kxxHash64", "kXXH3"]
     ),
@@ -67,13 +65,13 @@ default_params = {
     "compression_use_zstd_dict_trainer": lambda: random.randint(0, 1),
     "compression_checksum": lambda: random.randint(0, 1),
     "clear_column_family_one_in": 0,
-    "compact_files_one_in": lambda: random.choice([1000, 1000000]),
-    "compact_range_one_in": lambda: random.choice([1000, 1000000]),
+    "compact_files_one_in":  lambda: random.choice([1000, 1000000]),
+    "compact_range_one_in":  lambda: random.choice([1000, 1000000]),
     # Disabled because of various likely related failures with
     # "Cannot delete table file #N from level 0 since it is on level X"
     "promote_l0_one_in": 0,
     "compaction_pri": random.randint(0, 4),
-    "key_may_exist_one_in": lambda: random.choice([100, 100000]),
+    "key_may_exist_one_in":  lambda: random.choice([100, 100000]),
     "data_block_index_type": lambda: random.choice([0, 1]),
     "delpercent": 4,
     "delrangepercent": 1,
@@ -149,16 +147,10 @@ default_params = {
     "use_sqfc_for_range_queries": lambda: random.choice([0, 1, 1, 1]),
     "mock_direct_io": False,
     "cache_type": lambda: random.choice(
-        [
-            "lru_cache",
-            "fixed_hyper_clock_cache",
-            "auto_hyper_clock_cache",
-            "auto_hyper_clock_cache",
-            "tiered_lru_cache",
-            "tiered_fixed_hyper_clock_cache",
-            "tiered_auto_hyper_clock_cache",
-            "tiered_auto_hyper_clock_cache",
-        ]
+        ["lru_cache", "fixed_hyper_clock_cache", "auto_hyper_clock_cache",
+         "auto_hyper_clock_cache", "tiered_lru_cache",
+         "tiered_fixed_hyper_clock_cache", "tiered_auto_hyper_clock_cache",
+         "tiered_auto_hyper_clock_cache"]
     ),
     "uncache_aggressiveness": lambda: int(math.pow(10, 4.0 * random.random()) - 1.0),
     "use_full_merge_v1": lambda: random.randint(0, 1),
@@ -168,9 +160,7 @@ default_params = {
     "use_attribute_group": lambda: random.randint(0, 1),
     "use_multi_cf_iterator": lambda: random.randint(0, 1),
     # 999 -> use Bloom API
-    "bloom_before_level": lambda: random.choice(
-        [random.randint(-1, 2), random.randint(-1, 10), 0x7FFFFFFF - 1, 0x7FFFFFFF]
-    ),
+    "bloom_before_level": lambda: random.choice([random.randint(-1, 2), random.randint(-1, 10), 0x7fffffff - 1, 0x7fffffff]),
     "value_size_mult": 32,
     "verification_only": 0,
     "verify_checksum": 1,
@@ -183,7 +173,13 @@ default_params = {
     "use_multi_get_entity": lambda: random.choice([0] * 7 + [1]),
     "periodic_compaction_seconds": lambda: random.choice([0, 0, 1, 2, 10, 100, 1000]),
     "daily_offpeak_time_utc": lambda: random.choice(
-        ["", "", "00:00-23:59", "04:00-08:00", "23:30-03:15"]
+        [
+            "",
+            "",
+            "00:00-23:59",
+            "04:00-08:00",
+            "23:30-03:15"
+        ]
     ),
     # 0 = never (used by some), 10 = often (for threading bugs), 600 = default
     "stats_dump_period_sec": lambda: random.choice([0, 10, 600]),
@@ -200,7 +196,8 @@ default_params = {
     # TODO(hx235): Enable `wal_bytes_per_sync` after fixing the DB recovery such
     # that it won't recover past the WAL data hole created by this option
     "wal_bytes_per_sync": 0,
-    "compaction_readahead_size": lambda: random.choice([0, 0, 1024 * 1024]),
+    "compaction_readahead_size": lambda: random.choice(
+        [0, 0, 1024 * 1024]),
     "db_write_buffer_size": lambda: random.choice(
         [0, 0, 0, 1024 * 1024, 8 * 1024 * 1024, 128 * 1024 * 1024]
     ),
@@ -225,14 +222,13 @@ default_params = {
     "metadata_write_fault_one_in": lambda: random.choice([0, 128, 1000]),
     "read_fault_one_in": lambda: random.choice([0, 32, 1000]),
     "write_fault_one_in": lambda: random.choice([0, 128, 1000]),
-    "exclude_wal_from_write_fault_injection": 0,
     "open_metadata_write_fault_one_in": lambda: random.choice([0, 0, 8]),
     "open_metadata_read_fault_one_in": lambda: random.choice([0, 0, 8]),
     "open_write_fault_one_in": lambda: random.choice([0, 0, 16]),
     "open_read_fault_one_in": lambda: random.choice([0, 0, 32]),
     "sync_fault_injection": lambda: random.randint(0, 1),
-    "get_property_one_in": lambda: random.choice([100000, 1000000]),
-    "get_properties_of_all_tables_one_in": lambda: random.choice([100000, 1000000]),
+    "get_property_one_in":  lambda: random.choice([100000, 1000000]),
+    "get_properties_of_all_tables_one_in":  lambda: random.choice([100000, 1000000]),
     "paranoid_file_checks": lambda: random.choice([0, 1, 1, 1]),
     "max_write_buffer_size_to_maintain": lambda: random.choice(
         [0, 1024 * 1024, 2 * 1024 * 1024, 4 * 1024 * 1024, 8 * 1024 * 1024]
@@ -269,16 +265,14 @@ default_params = {
     "bottommost_file_compaction_delay": lambda: random.choice(
         [0, 0, 0, 600, 3600, 86400]
     ),
-    "auto_readahead_size": lambda: random.choice([0, 1]),
+    "auto_readahead_size" : lambda: random.choice([0, 1]),
     "verify_iterator_with_expected_state_one_in": 5,
     "allow_fallocate": lambda: random.choice([0, 1]),
     "table_cache_numshardbits": lambda: random.choice([6] * 3 + [-1] * 2 + [0]),
     "enable_write_thread_adaptive_yield": lambda: random.choice([0, 1]),
     "log_readahead_size": lambda: random.choice([0, 16 * 1024 * 1024]),
     "bgerror_resume_retry_interval": lambda: random.choice([100, 1000000]),
-    "delete_obsolete_files_period_micros": lambda: random.choice(
-        [6 * 60 * 60 * 1000000, 30 * 1000000]
-    ),
+    "delete_obsolete_files_period_micros": lambda: random.choice([6 * 60 * 60 * 1000000, 30 * 1000000]),
     "max_log_file_size": lambda: random.choice([0, 1024 * 1024]),
     "log_file_time_to_roll": lambda: random.choice([0, 60]),
     "use_adaptive_mutex": lambda: random.choice([0, 1]),
@@ -310,34 +304,24 @@ default_params = {
     "max_total_wal_size": lambda: random.choice([0] * 4 + [64 * 1024 * 1024]),
     "high_pri_pool_ratio": lambda: random.choice([0, 0.5]),
     "low_pri_pool_ratio": lambda: random.choice([0, 0.5]),
-    "soft_pending_compaction_bytes_limit": lambda: random.choice(
-        [1024 * 1024] + [64 * 1073741824] * 4
-    ),
-    "hard_pending_compaction_bytes_limit": lambda: random.choice(
-        [2 * 1024 * 1024] + [256 * 1073741824] * 4
-    ),
+    "soft_pending_compaction_bytes_limit" : lambda: random.choice([1024 * 1024] + [64 * 1073741824] * 4),
+    "hard_pending_compaction_bytes_limit" : lambda: random.choice([2 * 1024 * 1024] + [256 * 1073741824] * 4),
     "enable_sst_partitioner_factory": lambda: random.choice([0, 1]),
     "enable_do_not_compress_roles": lambda: random.choice([0, 1]),
     "block_align": lambda: random.choice([0, 1]),
     "lowest_used_cache_tier": lambda: random.choice([0, 1, 2]),
     "enable_custom_split_merge": lambda: random.choice([0, 1]),
     "adm_policy": lambda: random.choice([0, 1, 2, 3]),
-    "last_level_temperature": lambda: random.choice(
-        ["kUnknown", "kHot", "kWarm", "kCold"]
-    ),
-    "default_write_temperature": lambda: random.choice(
-        ["kUnknown", "kHot", "kWarm", "kCold"]
-    ),
-    "default_temperature": lambda: random.choice(
-        ["kUnknown", "kHot", "kWarm", "kCold"]
-    ),
+    "last_level_temperature": lambda: random.choice(["kUnknown", "kHot", "kWarm", "kCold"]),
+    "default_write_temperature": lambda: random.choice(["kUnknown", "kHot", "kWarm", "kCold"]),
+    "default_temperature": lambda: random.choice(["kUnknown", "kHot", "kWarm", "kCold"]),
     # TODO(hx235): enable `enable_memtable_insert_with_hint_prefix_extractor`
     # after fixing the surfaced issue with delete range
     "enable_memtable_insert_with_hint_prefix_extractor": 0,
     "check_multiget_consistency": lambda: random.choice([0, 0, 0, 1]),
     "check_multiget_entity_consistency": lambda: random.choice([0, 0, 0, 1]),
     "use_timed_put_one_in": lambda: random.choice([0] * 7 + [1, 5, 10]),
-    "universal_max_read_amp": lambda: random.choice([-1] * 3 + [0, 4, 10]),
+    "universal_max_read_amp": lambda : random.choice([-1] * 3 + [0, 4, 10]),
 }
 _TEST_DIR_ENV_VAR = "TEST_TMPDIR"
 # If TEST_TMPDIR_EXPECTED is not specified, default value will be TEST_TMPDIR
@@ -523,8 +507,6 @@ txn_params = {
     "inplace_update_support": 0,
     # TimedPut is not supported in transaction
     "use_timed_put_one_in": 0,
-    # AttributeGroup not yet supported
-    "use_attribute_group": 0,
 }
 
 # For optimistic transaction db
@@ -538,8 +520,6 @@ optimistic_txn_params = {
     "inplace_update_support": 0,
     # TimedPut is not supported in transaction
     "use_timed_put_one_in": 0,
-    # AttributeGroup not yet supported
-    "use_attribute_group": 0,
 }
 
 best_efforts_recovery_params = {
@@ -547,7 +527,7 @@ best_efforts_recovery_params = {
     "disable_wal": 1,
     "column_families": 1,
     "skip_verifydb": 1,
-    "verify_db_one_in": 0,
+    "verify_db_one_in": 0
 }
 
 blob_params = {
@@ -643,11 +623,10 @@ multiops_txn_default_params = {
     "create_timestamped_snapshot_one_in": 50,
     "sync_fault_injection": 0,
     "metadata_write_fault_one_in": 0,
-    "manual_wal_flush_one_in": 0,
+    "manual_wal_flush": 0,
     # This test has aggressive flush frequency and small write buffer size.
     # Disabling write fault to avoid writes being stopped.
     "write_fault_one_in": 0,
-    "metadata_write_fault_one_in": 0,
     # PutEntity in transactions is not yet implemented
     "use_put_entity_one_in": 0,
     "use_get_entity": 0,
@@ -687,7 +666,6 @@ multiops_wp_txn_params = {
     "lock_wal_one_in": 0,
 }
 
-
 def finalize_and_sanitize(src_params):
     dest_params = {k: v() if callable(v) else v for (k, v) in src_params.items()}
     if is_release_mode():
@@ -749,19 +727,23 @@ def finalize_and_sanitize(src_params):
     # inplace_update_support based on other option values, which may change
     # across runs.
     if dest_params["inplace_update_support"] == 1:
-        dest_params["delpercent"] += dest_params["delrangepercent"]
-        dest_params["delrangepercent"] = 0
-        dest_params["readpercent"] += dest_params["prefixpercent"]
-        dest_params["prefixpercent"] = 0
-        dest_params["allow_concurrent_memtable_write"] = 0
-        # inplace_update_support does not update sequence number. Our stress test recovery
-        # logic for unsynced data loss relies on max sequence number stored
-        # in MANIFEST, so they don't work together.
-        dest_params["sync_fault_injection"] = 0
-        dest_params["disable_wal"] = 0
-        dest_params["manual_wal_flush_one_in"] = 0
+       dest_params["delpercent"] += dest_params["delrangepercent"]
+       dest_params["delrangepercent"] = 0
+       dest_params["readpercent"] += dest_params["prefixpercent"]
+       dest_params["prefixpercent"] = 0
+       dest_params["allow_concurrent_memtable_write"] = 0
+       # inplace_update_support does not update sequence number. Our stress test recovery
+       # logic for unsynced data loss relies on max sequence number stored
+       # in MANIFEST, so they don't work together.
+       dest_params["sync_fault_injection"] = 0
+       dest_params["write_fault_one_in"] = 0
+       dest_params["metadata_write_fault_one_in"] = 0
+       dest_params["disable_wal"] = 0
+       dest_params["manual_wal_flush_one_in"] = 0
     if (
         dest_params.get("sync_fault_injection") == 1
+        or dest_params.get("write_fault_one_in") > 0
+        or dest_params.get("metadata_write_fault_one_in") > 0
         or dest_params.get("disable_wal") == 1
         or dest_params.get("manual_wal_flush_one_in") > 0
     ):
@@ -777,14 +759,6 @@ def finalize_and_sanitize(src_params):
         # files, which would be problematic when unsynced data can be lost in
         # crash recoveries.
         dest_params["enable_compaction_filter"] = 0
-        # Prefix-recoverability relies on tracing successful user writes.
-        # Currently we trace all user writes regardless of whether it later succeeds or not.
-        # To simplify, we disable any user write failure injection.
-        # TODO(hx235): support tracing user writes with failure injection.
-        # TODO(hx235): support excluding WAL from metadata write fault injection so we don't
-        # have to disable metadata write fault injection to other file
-        dest_params["metadata_write_fault_one_in"] = 0
-        dest_params["exclude_wal_from_write_fault_injection"] = 1
     # Only under WritePrepared txns, unordered_write would provide the same guarnatees as vanilla rocksdb
     # unordered_write is only enabled with --txn, and txn_params disables inplace_update_support, so
     # setting allow_concurrent_memtable_write=1 won't conflcit with inplace_update_support.
@@ -839,6 +813,8 @@ def finalize_and_sanitize(src_params):
     # compatible with only write committed policy
     if dest_params.get("use_txn") == 1 and dest_params.get("txn_write_policy", 0) != 0:
         dest_params["sync_fault_injection"] = 0
+        dest_params["write_fault_one_in"] = 0
+        dest_params["metadata_write_fault_one_in"] = 0
         dest_params["disable_wal"] = 0
         dest_params["manual_wal_flush_one_in"] = 0
         # Wide-column pessimistic transaction APIs are initially supported for
@@ -846,10 +822,10 @@ def finalize_and_sanitize(src_params):
         dest_params["use_put_entity_one_in"] = 0
     # TODO(hx235): enable test_multi_ops_txns with fault injection after stabilizing the CI
     if dest_params.get("test_multi_ops_txns") == 1:
-        dest_params["write_fault_one_in"] = 0
-        dest_params["metadata_write_fault_one_in"] = 0
-        dest_params["read_fault_one_in"] = 0
-        dest_params["metadata_read_fault_one_in"] = 0
+         dest_params["write_fault_one_in"] = 0
+         dest_params["metadata_write_fault_one_in"] = 0
+         dest_params["read_fault_one_in"] = 0
+         dest_params["metadata_read_fault_one_in"] = 0
     # Wide column stress tests require FullMergeV3
     if dest_params["use_put_entity_one_in"] != 0:
         dest_params["use_full_merge_v1"] = 0
@@ -857,21 +833,15 @@ def finalize_and_sanitize(src_params):
         dest_params["verify_file_checksums_one_in"] = 0
     if dest_params["write_fault_one_in"] > 0:
         # background work may be disabled while DB is resuming after some error
-        dest_params["max_write_buffer_number"] = max(
-            dest_params["max_write_buffer_number"], 10
-        )
+        dest_params["max_write_buffer_number"] = max(dest_params["max_write_buffer_number"], 10)
     if dest_params["secondary_cache_uri"].find("compressed_secondary_cache") >= 0:
         dest_params["compressed_secondary_cache_size"] = 0
         dest_params["compressed_secondary_cache_ratio"] = 0.0
     if dest_params["cache_type"].find("tiered_") >= 0:
         if dest_params["compressed_secondary_cache_size"] > 0:
-            dest_params["compressed_secondary_cache_ratio"] = float(
-                dest_params["compressed_secondary_cache_size"]
-                / (
-                    dest_params["cache_size"]
-                    + dest_params["compressed_secondary_cache_size"]
-                )
-            )
+            dest_params["compressed_secondary_cache_ratio"] = \
+                    float(dest_params["compressed_secondary_cache_size"]/ \
+                    (dest_params["cache_size"] + dest_params["compressed_secondary_cache_size"]))
             dest_params["compressed_secondary_cache_size"] = 0
         else:
             dest_params["compressed_secondary_cache_ratio"] = 0.0
@@ -881,12 +851,10 @@ def finalize_and_sanitize(src_params):
             dest_params["compressed_secondary_cache_size"] = 0
             dest_params["compressed_secondary_cache_ratio"] = 0.0
     if dest_params["use_write_buffer_manager"]:
-        if dest_params["cache_size"] <= 0 or dest_params["db_write_buffer_size"] <= 0:
+        if (dest_params["cache_size"] <= 0
+            or dest_params["db_write_buffer_size"] <= 0):
             dest_params["use_write_buffer_manager"] = 0
-    if (
-        dest_params["user_timestamp_size"] > 0
-        and dest_params["persist_user_defined_timestamps"] == 0
-    ):
+    if dest_params["user_timestamp_size"] > 0 and dest_params["persist_user_defined_timestamps"] == 0:
         # Features that are not compatible with UDT in memtable only feature.
         dest_params["enable_blob_files"] = 0
         dest_params["allow_setting_blob_options_dynamically"] = 0
@@ -906,36 +874,22 @@ def finalize_and_sanitize(src_params):
         # Only best efforts recovery test support disabling wal and
         # disable atomic flush.
         if dest_params["test_best_efforts_recovery"] == 0:
-            dest_params["disable_wal"] = 0
+          dest_params["disable_wal"] = 0
     if dest_params.get("allow_concurrent_memtable_write", 1) == 1:
         dest_params["memtablerep"] = "skip_list"
-    if (
-        dest_params.get("enable_compaction_filter", 0) == 1
-        or dest_params.get("inplace_update_support", 0) == 1
-    ):
+    if (dest_params.get("enable_compaction_filter", 0) == 1
+        or dest_params.get("inplace_update_support", 0) == 1):
         # Compaction filter, inplace update support are incompatible with snapshots. Need to avoid taking
         # snapshots, as well as avoid operations that use snapshots for
         # verification.
         dest_params["acquire_snapshot_one_in"] = 0
         dest_params["compact_range_one_in"] = 0
         # Redistribute to maintain 100% total
-        dest_params["readpercent"] += dest_params.get(
-            "iterpercent", 10
-        ) + dest_params.get("prefixpercent", 20)
+        dest_params["readpercent"] += dest_params.get("iterpercent", 10) + dest_params.get("prefixpercent", 20)
         dest_params["iterpercent"] = 0
         dest_params["prefixpercent"] = 0
         dest_params["check_multiget_consistency"] = 0
         dest_params["check_multiget_entity_consistency"] = 0
-    if dest_params.get("disable_wal") == 0 and dest_params.get("reopen") > 0:
-        # Reopen with WAL currently requires persisting WAL data before closing for reopen.
-        # Previous injected WAL write errors may not be cleared by the time of closing and ready
-        # for persisting WAL.
-        # To simplify, we disable any WAL write error injection.
-        # TODO(hx235): support WAL write error injection with reopen
-        # TODO(hx235): support excluding WAL from metadata write fault injection so we don't
-        # have to disable metadata write fault injection to other file
-        dest_params["exclude_wal_from_write_fault_injection"] = 1
-        dest_params["metadata_write_fault_one_in"] = 0
     if dest_params.get("disable_wal") == 1:
         # disableWAL and recycle_log_file_num options are not mutually
         # compatible at the moment
@@ -951,14 +905,11 @@ def finalize_and_sanitize(src_params):
     # `use_timed_put_one_in` option so that they make sense together.
     if dest_params.get("use_put_entity_one_in") == 1:
         dest_params["use_timed_put_one_in"] = 0
-    elif (
-        dest_params.get("use_put_entity_one_in") > 1
-        and dest_params.get("use_timed_put_one_in") == 1
-    ):
+    elif (dest_params.get("use_put_entity_one_in") > 1 and
+        dest_params.get("use_timed_put_one_in") == 1):
         dest_params["use_timed_put_one_in"] = 3
     return dest_params
 
-
 def gen_cmd_params(args):
     params = {}
 
@@ -1033,7 +984,7 @@ def gen_cmd(params, unknown_params):
                 "test_tiered_storage",
                 "cleanup_cmd",
                 "skip_tmpdir_check",
-                "print_stderr_separately",
+                "print_stderr_separately"
             }
             and v is not None
         ]
@@ -1071,7 +1022,6 @@ def print_output_and_exit_on_error(stdout, stderr, print_stderr_separately=False
 
     sys.exit(2)
 
-
 def cleanup_after_success(dbname):
     shutil.rmtree(dbname, True)
     if cleanup_cmd is not None:
@@ -1081,7 +1031,6 @@ def cleanup_after_success(dbname):
             print("TEST FAILED. DB cleanup returned error %d\n" % ret)
             sys.exit(1)
 
-
 # This script runs and kills db_stress multiple times. It checks consistency
 # in case of unsafe crashes in RocksDB.
 def blackbox_crash_main(args, unknown_args):
@@ -1273,9 +1222,7 @@ def whitebox_crash_main(args, unknown_args):
         )
 
         print(msg)
-        print_output_and_exit_on_error(
-            stdoutdata, stderrdata, args.print_stderr_separately
-        )
+        print_output_and_exit_on_error(stdoutdata, stderrdata, args.print_stderr_separately)
 
         if hit_timeout:
             print("Killing the run for running too long")
@@ -1310,6 +1257,7 @@ def whitebox_crash_main(args, unknown_args):
 
         time.sleep(1)  # time to stabilize after a kill
 
+
     # If successfully finished or timed out (we currently treat timed out test as passing)
     # Clean up after ourselves
     if succeeded or hit_timeout:

util/stderr_logger.h

@@ -17,7 +17,7 @@ namespace ROCKSDB_NAMESPACE {
 class StderrLogger : public Logger {
  public:
   explicit StderrLogger(const InfoLogLevel log_level = InfoLogLevel::INFO_LEVEL)
-      : Logger(log_level), log_prefix(nullptr), log_prefix_len(0) {}
+      : Logger(log_level), log_prefix(nullptr) {}
   explicit StderrLogger(const InfoLogLevel log_level, const std::string prefix)
       : Logger(log_level),
         log_prefix(strdup(prefix.c_str())),

util/xxhash.h

@@ -1303,7 +1303,7 @@ struct XXH3_state_s {
  * Note that this doesn't prepare the state for a streaming operation,
  * it's still necessary to use XXH3_NNbits_reset*() afterwards.
  */
-#define XXH3_INITSTATE(XXH3_state_ptr)   do { (XXH3_state_ptr)->seed = 0; } while (0)
+#define XXH3_INITSTATE(XXH3_state_ptr)   { (XXH3_state_ptr)->seed = 0; }
 
 
 /*!

utilities/backup/backup_engine_test.cc

@@ -3962,7 +3962,7 @@ TEST_P(BackupEngineTestWithParam, BackupUsingDirectIO) {
   OpenDBAndBackupEngine(true /* destroy_old_data */);
   for (int i = 0; i < kNumBackups; ++i) {
     FillDB(db_.get(), i * kNumKeysPerBackup /* from */,
-           (i + 1) * kNumKeysPerBackup /* to */);
+           (i + 1) * kNumKeysPerBackup /* to */, kFlushAll);
 
     // Clear the file open counters and then do a bunch of backup engine ops.
     // For all ops, files should be opened in direct mode.

utilities/checkpoint/checkpoint_test.cc

@@ -13,7 +13,6 @@
 #ifndef OS_WIN
 #include <unistd.h>
 #endif
-#include <cstdlib>
 #include <iostream>
 #include <thread>
 #include <utility>
@@ -995,43 +994,6 @@ TEST_F(CheckpointTest, CheckpointWithDbPath) {
   delete checkpoint;
 }
 
-TEST_F(CheckpointTest, CheckpointWithArchievedLog) {
-  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->LoadDependency(
-      {{"WalManager::ArchiveWALFile",
-        "CheckpointTest:CheckpointWithArchievedLog"}});
-  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing();
-  Options options = CurrentOptions();
-  options.WAL_ttl_seconds = 3600;
-  options.disable_auto_compactions = true;
-  DestroyAndReopen(options);
-
-  ASSERT_OK(Put("key1", std::string(1024 * 1024, 'a')));
-  // flush and archive the first log
-  ASSERT_OK(Flush());
-  ASSERT_OK(Put("key2", std::string(1024, 'a')));
-
-  Checkpoint* checkpoint;
-  ASSERT_OK(Checkpoint::Create(db_, &checkpoint));
-  TEST_SYNC_POINT("CheckpointTest:CheckpointWithArchievedLog");
-  ASSERT_OK(checkpoint->CreateCheckpoint(snapshot_name_, 1024 * 1024));
-  // unflushed log size < 1024 * 1024 < total file size including archived log,
-  // so flush shouldn't occur, there is only one file at level 0
-  ASSERT_EQ(NumTableFilesAtLevel(0), 1);
-  delete checkpoint;
-  checkpoint = nullptr;
-
-  DB* snapshot_db;
-  ASSERT_OK(DB::Open(options, snapshot_name_, &snapshot_db));
-  ReadOptions read_opts;
-  std::string get_result;
-  ASSERT_OK(snapshot_db->Get(read_opts, "key1", &get_result));
-  ASSERT_EQ(std::string(1024 * 1024, 'a'), get_result);
-  get_result.clear();
-  ASSERT_OK(snapshot_db->Get(read_opts, "key2", &get_result));
-  ASSERT_EQ(std::string(1024, 'a'), get_result);
-  delete snapshot_db;
-}
-
 class CheckpointDestroyTest : public CheckpointTest,
                               public testing::WithParamInterface<bool> {};
 

utilities/fault_injection_fs.cc

@@ -148,11 +148,9 @@ TestFSWritableFile::TestFSWritableFile(const std::string& fname,
       file_opts_(file_opts),
       target_(std::move(f)),
       writable_file_opened_(true),
-      fs_(fs),
-      unsync_data_loss_(fs_->InjectUnsyncedDataLoss()) {
+      fs_(fs) {
   assert(target_ != nullptr);
-  assert(state_.pos_at_last_append_ == 0);
-  assert(state_.pos_at_last_sync_ == 0);
+  state_.pos_at_last_append_ = 0;
 }
 
 TestFSWritableFile::~TestFSWritableFile() {
@@ -168,13 +166,13 @@ IOStatus TestFSWritableFile::Append(const Slice& data, const IOOptions& options,
     return fs_->GetError();
   }
 
-  IOStatus s = fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite,
-                                                options, state_.filename_);
+  IOStatus s =
+      fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite, options);
   if (!s.ok()) {
     return s;
   }
 
-  if (target_->use_direct_io() || !unsync_data_loss_) {
+  if (target_->use_direct_io()) {
     // TODO(hx235): buffer data for direct IO write to simulate data loss like
     // non-direct IO write
     s = target_->Append(data, options, dbg);
@@ -203,8 +201,8 @@ IOStatus TestFSWritableFile::Append(
     return IOStatus::Corruption("Data is corrupted!");
   }
 
-  IOStatus s = fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite,
-                                                options, state_.filename_);
+  IOStatus s =
+      fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite, options);
   if (!s.ok()) {
     return s;
   }
@@ -222,7 +220,7 @@ IOStatus TestFSWritableFile::Append(
     return IOStatus::Corruption(msg);
   }
 
-  if (target_->use_direct_io() || !unsync_data_loss_) {
+  if (target_->use_direct_io()) {
     // TODO(hx235): buffer data for direct IO write to simulate data loss like
     // non-direct IO write
     s = target_->Append(data, options, dbg);
@@ -242,8 +240,8 @@ IOStatus TestFSWritableFile::Truncate(uint64_t size, const IOOptions& options,
   if (!fs_->IsFilesystemActive()) {
     return fs_->GetError();
   }
-  IOStatus s = fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite,
-                                                options, state_.filename_);
+  IOStatus s =
+      fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite, options);
   if (!s.ok()) {
     return s;
   }
@@ -266,8 +264,8 @@ IOStatus TestFSWritableFile::PositionedAppend(const Slice& data,
   if (fs_->ShouldDataCorruptionBeforeWrite()) {
     return IOStatus::Corruption("Data is corrupted!");
   }
-  IOStatus s = fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite,
-                                                options, state_.filename_);
+  IOStatus s =
+      fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite, options);
   if (!s.ok()) {
     return s;
   }
@@ -292,8 +290,8 @@ IOStatus TestFSWritableFile::PositionedAppend(
   if (fs_->ShouldDataCorruptionBeforeWrite()) {
     return IOStatus::Corruption("Data is corrupted!");
   }
-  IOStatus s = fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite,
-                                                options, state_.filename_);
+  IOStatus s =
+      fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite, options);
   if (!s.ok()) {
     return s;
   }
@@ -378,13 +376,15 @@ IOStatus TestFSWritableFile::RangeSync(uint64_t offset, uint64_t nbytes,
   }
   // Assumes caller passes consecutive byte ranges.
   uint64_t sync_limit = offset + nbytes;
+  uint64_t buf_begin =
+      state_.pos_at_last_sync_ < 0 ? 0 : state_.pos_at_last_sync_;
 
   IOStatus io_s;
-  if (sync_limit < state_.pos_at_last_sync_) {
+  if (sync_limit < buf_begin) {
     return io_s;
   }
   uint64_t num_to_sync = std::min(static_cast<uint64_t>(state_.buffer_.size()),
-                                  sync_limit - state_.pos_at_last_sync_);
+                                  sync_limit - buf_begin);
   Slice buf_to_sync(state_.buffer_.data(), num_to_sync);
   io_s = target_->Append(buf_to_sync, options, dbg);
   state_.buffer_ = state_.buffer_.substr(num_to_sync);
@@ -468,7 +468,7 @@ IOStatus TestFSRandomAccessFile::Read(uint64_t offset, size_t n,
     return fs_->GetError();
   }
   IOStatus s = fs_->MaybeInjectThreadLocalError(
-      FaultInjectionIOType::kRead, options, "",
+      FaultInjectionIOType::kRead, options,
       FaultInjectionTestFS::ErrorOperation::kRead, result, use_direct_io(),
       scratch, /*need_count_increase=*/true,
       /*fault_injected=*/nullptr);
@@ -485,35 +485,26 @@ IOStatus TestFSRandomAccessFile::ReadAsync(
     FSReadRequest& req, const IOOptions& opts,
     std::function<void(FSReadRequest&, void*)> cb, void* cb_arg,
     void** io_handle, IOHandleDeleter* del_fn, IODebugContext* /*dbg*/) {
-  IOStatus res_status;
-  FSReadRequest res;
   IOStatus s;
+  FSReadRequest res;
   if (!fs_->IsFilesystemActive()) {
-    res_status = fs_->GetError();
+    s = fs_->GetError();
   }
-  if (res_status.ok()) {
-    res_status = fs_->MaybeInjectThreadLocalError(
-        FaultInjectionIOType::kRead, opts, "",
+  if (s.ok()) {
+    s = fs_->MaybeInjectThreadLocalError(
+        FaultInjectionIOType::kRead, opts,
         FaultInjectionTestFS::ErrorOperation::kRead, &res.result,
         use_direct_io(), req.scratch, /*need_count_increase=*/true,
         /*fault_injected=*/nullptr);
   }
-  if (res_status.ok()) {
+  if (s.ok()) {
     s = target_->ReadAsync(req, opts, cb, cb_arg, io_handle, del_fn, nullptr);
     // TODO (low priority): fs_->ReadUnsyncedData()
-  } else {
-    // If there’s no injected error, then cb will be called asynchronously when
-    // target_ actually finishes the read. But if there’s an injected error, it
-    // needs to immediately call cb(res, cb_arg) s since target_->ReadAsync()
-    // isn’t invoked at all.
-    res.status = res_status;
+  }
+  if (!s.ok()) {
+    res.status = s;
     cb(res, cb_arg);
   }
-  // We return ReadAsync()'s status intead of injected error status here since
-  // the return status is not supposed to be the status of the actual IO (i.e,
-  // the actual async read). The actual status of the IO will be passed to cb()
-  // callback upon the actual read finishes or like above when injected error
-  // happens.
   return s;
 }
 
@@ -533,7 +524,7 @@ IOStatus TestFSRandomAccessFile::MultiRead(FSReadRequest* reqs, size_t num_reqs,
     }
     bool this_injected_error;
     reqs[i].status = fs_->MaybeInjectThreadLocalError(
-        FaultInjectionIOType::kRead, options, "",
+        FaultInjectionIOType::kRead, options,
         FaultInjectionTestFS::ErrorOperation::kRead, &(reqs[i].result),
         use_direct_io(), reqs[i].scratch,
         /*need_count_increase=*/true,
@@ -542,7 +533,7 @@ IOStatus TestFSRandomAccessFile::MultiRead(FSReadRequest* reqs, size_t num_reqs,
   }
   if (s.ok()) {
     s = fs_->MaybeInjectThreadLocalError(
-        FaultInjectionIOType::kRead, options, "",
+        FaultInjectionIOType::kRead, options,
         FaultInjectionTestFS::ErrorOperation::kMultiRead, nullptr,
         use_direct_io(), nullptr, /*need_count_increase=*/!injected_error,
         /*fault_injected=*/nullptr);
@@ -558,44 +549,38 @@ size_t TestFSRandomAccessFile::GetUniqueId(char* id, size_t max_size) const {
   }
 }
 
-namespace {
-// Modifies `result` to start at the beginning of `scratch` if not already,
-// copying data there if needed.
-void MoveToScratchIfNeeded(Slice* result, char* scratch) {
-  if (result->data() != scratch) {
-    // NOTE: might overlap, where result is later in scratch
-    std::copy(result->data(), result->data() + result->size(), scratch);
-    *result = Slice(scratch, result->size());
-  }
-}
-}  // namespace
-
-void FaultInjectionTestFS::ReadUnsynced(const std::string& fname,
-                                        uint64_t offset, size_t n,
-                                        Slice* result, char* scratch,
-                                        int64_t* pos_at_last_sync) {
-  *result = Slice(scratch, 0);  // default empty result
-  assert(*pos_at_last_sync == -1);  // default "unknown"
+void FaultInjectionTestFS::AddUnsyncedToRead(const std::string& fname,
+                                             size_t pos, size_t n,
+                                             Slice* result, char* scratch) {
+  // Should be checked prior
+  assert(result->size() < n);
+  size_t pos_after = pos + result->size();
 
   MutexLock l(&mutex_);
   auto it = db_file_state_.find(fname);
   if (it != db_file_state_.end()) {
     auto& st = it->second;
-    *pos_at_last_sync = static_cast<int64_t>(st.pos_at_last_sync_);
-    // Find overlap between [offset, offset + n) and
-    // [*pos_at_last_sync, *pos_at_last_sync + st.buffer_.size())
-    int64_t begin = std::max(static_cast<int64_t>(offset), *pos_at_last_sync);
-    int64_t end =
-        std::min(static_cast<int64_t>(offset + n),
-                 *pos_at_last_sync + static_cast<int64_t>(st.buffer_.size()));
-
-    // Copy and return overlap if there is any
-    if (begin < end) {
-      size_t offset_in_buffer = static_cast<size_t>(begin - *pos_at_last_sync);
-      size_t offset_in_scratch = static_cast<size_t>(begin - offset);
-      std::copy_n(st.buffer_.data() + offset_in_buffer, end - begin,
-                  scratch + offset_in_scratch);
-      *result = Slice(scratch + offset_in_scratch, end - begin);
+    if (st.pos_at_last_append_ > static_cast<ssize_t>(pos_after)) {
+      size_t remaining_requested = n - result->size();
+      size_t to_copy =
+          std::min(remaining_requested,
+                   static_cast<size_t>(st.pos_at_last_append_) - pos_after);
+      size_t buffer_offset = pos_after - static_cast<size_t>(std::max(
+                                             st.pos_at_last_sync_, ssize_t{0}));
+      // Data might have been dropped from buffer
+      if (st.buffer_.size() > buffer_offset) {
+        to_copy = std::min(to_copy, st.buffer_.size() - buffer_offset);
+        if (result->data() != scratch) {
+          // TODO: this will be needed when supporting random reads
+          // but not currently used
+          abort();
+          // NOTE: might overlap
+          // std::copy_n(result->data(), result->size(), scratch);
+        }
+        std::copy_n(st.buffer_.data() + buffer_offset, to_copy,
+                    scratch + result->size());
+        *result = Slice(scratch, result->size() + to_copy);
+      }
     }
   }
 }
@@ -604,146 +589,20 @@ IOStatus TestFSSequentialFile::Read(size_t n, const IOOptions& options,
                                     Slice* result, char* scratch,
                                     IODebugContext* dbg) {
   IOStatus s = fs_->MaybeInjectThreadLocalError(
-      FaultInjectionIOType::kRead, options, "",
+      FaultInjectionIOType::kRead, options,
       FaultInjectionTestFS::ErrorOperation::kRead, result, use_direct_io(),
       scratch, true /*need_count_increase=*/, nullptr /* fault_injected*/);
   if (!s.ok()) {
     return s;
   }
 
-  // Some complex logic is needed to deal with concurrent write to the same
-  // file, while keeping good performance (e.g. not holding FS mutex during
-  // I/O op), especially in common cases.
-
-  if (read_pos_ == target_read_pos_) {
-    // Normal case: start by reading from underlying file
-    s = target()->Read(n, options, result, scratch, dbg);
-    if (!s.ok()) {
-      return s;
-    }
-    target_read_pos_ += result->size();
-  } else {
-    // We must have previously read buffered data (unsynced) not written to
-    // target. Deal with this case (and more) below.
-    *result = {};
+  s = target()->Read(n, options, result, scratch, dbg);
+  if (!s.ok()) {
+    return s;
   }
 
   if (fs_->ReadUnsyncedData() && result->size() < n) {
-    // We need to check if there's unsynced data to fill out the rest of the
-    // read.
-
-    // First, ensure target read data is in scratch for easy handling.
-    MoveToScratchIfNeeded(result, scratch);
-    assert(result->data() == scratch);
-
-    // If we just did a target Read, we only want unsynced data after it
-    // (target_read_pos_). Otherwise (e.g. if target is behind because of
-    // unsynced data) we want unsynced data starting at the current read pos
-    // (read_pos_, not yet updated).
-    const uint64_t unsynced_read_pos = std::max(target_read_pos_, read_pos_);
-    const size_t offset_from_read_pos =
-        static_cast<size_t>(unsynced_read_pos - read_pos_);
-    Slice unsynced_result;
-    int64_t pos_at_last_sync = -1;
-    fs_->ReadUnsynced(fname_, unsynced_read_pos, n - offset_from_read_pos,
-                      &unsynced_result, scratch + offset_from_read_pos,
-                      &pos_at_last_sync);
-    assert(unsynced_result.data() >= scratch + offset_from_read_pos);
-    assert(unsynced_result.data() < scratch + n);
-    // Now, there are several cases to consider (some grouped together):
-    if (pos_at_last_sync <= static_cast<int64_t>(unsynced_read_pos)) {
-      // 1. We didn't get any unsynced data because nothing has been written
-      // to the file beyond unsynced_read_pos (including untracked
-      // pos_at_last_sync == -1)
-      // 2. We got some unsynced data starting at unsynced_read_pos (possibly
-      // on top of some synced data from target). We don't need to try reading
-      // any more from target because we established a "point in time" for
-      // completing this Read in which we read as much tail data (unsynced) as
-      // we could.
-
-      // We got pos_at_last_sync info if we got any unsynced data.
-      assert(pos_at_last_sync >= 0 || unsynced_result.size() == 0);
-
-      // Combined data is already lined up in scratch.
-      assert(result->data() + result->size() == unsynced_result.data());
-      assert(result->size() + unsynced_result.size() <= n);
-      // Combine results
-      *result = Slice(result->data(), result->size() + unsynced_result.size());
-    } else {
-      // 3. Any unsynced data we got was after unsynced_read_pos because the
-      // file was synced some time since our last target Read (either from this
-      // Read or a prior Read). We need to read more data from target to ensure
-      // this Read is filled out, even though we might have already read some
-      // (but not all due to a race). This code handles:
-      //
-      // * Catching up target after prior read(s) of unsynced data
-      // * Racing Sync in another thread since we called target Read above
-      //
-      // And merging potentially three results together for this Read:
-      // * The original target Read above
-      // * The following (non-throw-away) target Read
-      // * The ReadUnsynced above, which is always last if it returned data,
-      // so that we have a "point in time" for completing this Read in which we
-      // read as much tail data (unsynced) as we could.
-      //
-      // Deeper note about the race: we cannot just treat the original target
-      // Read as a "point in time" view of available data in the file, because
-      // there might have been unsynced data at that time, which became synced
-      // data by the time we read unsynced data. That is the race we are
-      // resolving with this "double check"-style code.
-      const size_t supplemental_read_pos = unsynced_read_pos;
-
-      // First, if there's any data from target that we know we would need to
-      // throw away to catch up, try to do it.
-      if (target_read_pos_ < supplemental_read_pos) {
-        Slice throw_away_result;
-        size_t throw_away_n = supplemental_read_pos - target_read_pos_;
-        std::unique_ptr<char[]> throw_away_scratch{new char[throw_away_n]};
-        s = target()->Read(throw_away_n, options, &throw_away_result,
-                           throw_away_scratch.get(), dbg);
-        if (!s.ok()) {
-          read_pos_ += result->size();
-          return s;
-        }
-        target_read_pos_ += throw_away_result.size();
-        if (target_read_pos_ < supplemental_read_pos) {
-          // Because of pos_at_last_sync > supplemental_read_pos, we should
-          // have been able to catch up
-          read_pos_ += result->size();
-          return IOStatus::IOError(
-              "Unexpected truncation or short read of file " + fname_);
-        }
-      }
-      // Now we can do a productive supplemental Read from target
-      assert(target_read_pos_ == supplemental_read_pos);
-      Slice supplemental_result;
-      size_t supplemental_n =
-          unsynced_result.size() == 0
-              ? n - offset_from_read_pos
-              : unsynced_result.data() - (scratch + offset_from_read_pos);
-      s = target()->Read(supplemental_n, options, &supplemental_result,
-                         scratch + offset_from_read_pos, dbg);
-      if (!s.ok()) {
-        read_pos_ += result->size();
-        return s;
-      }
-      target_read_pos_ += supplemental_result.size();
-      MoveToScratchIfNeeded(&supplemental_result,
-                            scratch + offset_from_read_pos);
-
-      // Combined data is already lined up in scratch.
-      assert(result->data() + result->size() == supplemental_result.data());
-      assert(unsynced_result.size() == 0 ||
-             supplemental_result.data() + supplemental_result.size() ==
-                 unsynced_result.data());
-      assert(result->size() + supplemental_result.size() +
-                 unsynced_result.size() <=
-             n);
-      // Combine results
-      *result =
-          Slice(result->data(), result->size() + supplemental_result.size() +
-                                    unsynced_result.size());
-    }
+    fs_->AddUnsyncedToRead(fname_, read_pos_, n, result, scratch);
   }
   read_pos_ += result->size();
 
@@ -755,7 +614,7 @@ IOStatus TestFSSequentialFile::PositionedRead(uint64_t offset, size_t n,
                                               Slice* result, char* scratch,
                                               IODebugContext* dbg) {
   IOStatus s = fs_->MaybeInjectThreadLocalError(
-      FaultInjectionIOType::kRead, options, "",
+      FaultInjectionIOType::kRead, options,
       FaultInjectionTestFS::ErrorOperation::kRead, result, use_direct_io(),
       scratch, true /*need_count_increase=*/, nullptr /* fault_injected */);
   if (!s.ok()) {
@@ -839,12 +698,12 @@ IOStatus FaultInjectionTestFS::NewWritableFile(
     return GetError();
   }
 
-  if (IsFilesystemDirectWritable()) {
+  if (ShouldUseDiretWritable(fname)) {
     return target()->NewWritableFile(fname, file_opts, result, dbg);
   }
 
-  IOStatus io_s = MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite,
-                                              file_opts.io_options, fname);
+  IOStatus io_s = MaybeInjectThreadLocalError(
+      FaultInjectionIOType::kMetadataWrite, file_opts.io_options);
   if (!io_s.ok()) {
     return io_s;
   }
@@ -876,11 +735,11 @@ IOStatus FaultInjectionTestFS::ReopenWritableFile(
   if (!IsFilesystemActive()) {
     return GetError();
   }
-  if (IsFilesystemDirectWritable()) {
+  if (ShouldUseDiretWritable(fname)) {
     return target()->ReopenWritableFile(fname, file_opts, result, dbg);
   }
-  IOStatus io_s = MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite,
-                                              file_opts.io_options, fname);
+  IOStatus io_s = MaybeInjectThreadLocalError(
+      FaultInjectionIOType::kMetadataWrite, file_opts.io_options);
   if (!io_s.ok()) {
     return io_s;
   }
@@ -952,11 +811,11 @@ IOStatus FaultInjectionTestFS::NewRandomRWFile(
   if (!IsFilesystemActive()) {
     return GetError();
   }
-  if (IsFilesystemDirectWritable()) {
+  if (ShouldUseDiretWritable(fname)) {
     return target()->NewRandomRWFile(fname, file_opts, result, dbg);
   }
-  IOStatus io_s = MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite,
-                                              file_opts.io_options, fname);
+  IOStatus io_s = MaybeInjectThreadLocalError(
+      FaultInjectionIOType::kMetadataWrite, file_opts.io_options);
   if (!io_s.ok()) {
     return io_s;
   }
@@ -988,10 +847,7 @@ IOStatus FaultInjectionTestFS::NewRandomAccessFile(
     return GetError();
   }
   IOStatus io_s = MaybeInjectThreadLocalError(
-      FaultInjectionIOType::kRead, file_opts.io_options, fname,
-      ErrorOperation::kOpen, nullptr /* result */, false /* direct_io */,
-      nullptr /* scratch */, true /*need_count_increase*/,
-      nullptr /*fault_injected*/);
+      FaultInjectionIOType::kMetadataWrite, file_opts.io_options);
   if (!io_s.ok()) {
     return io_s;
   }
@@ -1011,10 +867,7 @@ IOStatus FaultInjectionTestFS::NewSequentialFile(
     return GetError();
   }
   IOStatus io_s = MaybeInjectThreadLocalError(
-      FaultInjectionIOType::kRead, file_opts.io_options, fname,
-      ErrorOperation::kOpen, nullptr /* result */, false /* direct_io */,
-      nullptr /* scratch */, true /*need_count_increase*/,
-      nullptr /*fault_injected*/);
+      FaultInjectionIOType::kMetadataWrite, file_opts.io_options);
   if (!io_s.ok()) {
     return io_s;
   }
@@ -1132,6 +985,7 @@ IOStatus FaultInjectionTestFS::RenameFile(const std::string& s,
       auto tdn = TestFSGetDirAndName(t);
       if (dir_to_new_files_since_last_sync_[sdn.first].erase(sdn.second) != 0) {
         auto& tlist = dir_to_new_files_since_last_sync_[tdn.first];
+        assert(tlist.find(tdn.second) == tlist.end());
         tlist[tdn.second] = previous_contents;
       }
     }
@@ -1374,7 +1228,8 @@ IOStatus FaultInjectionTestFS::MaybeInjectThreadLocalReadError(
   ErrorContext* ctx =
       static_cast<ErrorContext*>(injected_thread_local_read_error_.Get());
   if (ctx == nullptr || !ctx->enable_error_injection || !ctx->one_in ||
-      ShouldIOActivtiesExcludedFromFaultInjection(io_options.io_activity)) {
+      io_activties_exempted_from_fault_injection.find(io_options.io_activity) !=
+          io_activties_exempted_from_fault_injection.end()) {
     return IOStatus::OK();
   }
 
@@ -1440,9 +1295,8 @@ bool FaultInjectionTestFS::TryParseFileName(const std::string& file_name,
 }
 
 IOStatus FaultInjectionTestFS::MaybeInjectThreadLocalError(
-    FaultInjectionIOType type, const IOOptions& io_options,
-    const std::string& file_name, ErrorOperation op, Slice* result,
-    bool direct_io, char* scratch, bool need_count_increase,
+    FaultInjectionIOType type, const IOOptions& io_options, ErrorOperation op,
+    Slice* result, bool direct_io, char* scratch, bool need_count_increase,
     bool* fault_injected) {
   if (type == FaultInjectionIOType::kRead) {
     return MaybeInjectThreadLocalReadError(io_options, op, result, direct_io,
@@ -1452,9 +1306,8 @@ IOStatus FaultInjectionTestFS::MaybeInjectThreadLocalError(
 
   ErrorContext* ctx = GetErrorContextFromFaultInjectionIOType(type);
   if (ctx == nullptr || !ctx->enable_error_injection || !ctx->one_in ||
-      ShouldIOActivtiesExcludedFromFaultInjection(io_options.io_activity) ||
-      (type == FaultInjectionIOType::kWrite &&
-       ShouldExcludeFromWriteFaultInjection(file_name))) {
+      io_activties_exempted_from_fault_injection.find(io_options.io_activity) !=
+          io_activties_exempted_from_fault_injection.end()) {
     return IOStatus::OK();
   }
 

utilities/fault_injection_fs.h

@@ -41,15 +41,17 @@ enum class FaultInjectionIOType {
 
 struct FSFileState {
   std::string filename_;
-  uint64_t pos_at_last_append_ = 0;
-  uint64_t pos_at_last_sync_ = 0;
+  ssize_t pos_at_last_append_;
+  ssize_t pos_at_last_sync_;
   std::string buffer_;
 
-  explicit FSFileState(const std::string& filename = {})
-      : filename_(filename) {}
+  explicit FSFileState(const std::string& filename)
+      : filename_(filename), pos_at_last_append_(-1), pos_at_last_sync_(-1) {}
+
+  FSFileState() : pos_at_last_append_(-1), pos_at_last_sync_(-1) {}
 
   bool IsFullySynced() const {
-    return pos_at_last_append_ == pos_at_last_sync_;
+    return pos_at_last_append_ <= 0 || pos_at_last_append_ == pos_at_last_sync_;
   }
 
   IOStatus DropUnsyncedData();
@@ -103,7 +105,6 @@ class TestFSWritableFile : public FSWritableFile {
   bool writable_file_opened_;
   FaultInjectionTestFS* fs_;
   port::Mutex mutex_;
-  const bool unsync_data_loss_;
 };
 
 // A wrapper around WritableFileWriter* file
@@ -176,8 +177,7 @@ class TestFSSequentialFile : public FSSequentialFileOwnerWrapper {
  private:
   FaultInjectionTestFS* fs_;
   std::string fname_;
-  uint64_t read_pos_ = 0;
-  uint64_t target_read_pos_ = 0;
+  size_t read_pos_ = 0;
 };
 
 class TestFSDirectory : public FSDirectory {
@@ -207,7 +207,6 @@ class FaultInjectionTestFS : public FileSystemWrapper {
       : FileSystemWrapper(base),
         filesystem_active_(true),
         filesystem_writable_(false),
-        inject_unsynced_data_loss_(false),
         read_unsynced_data_(true),
         allow_link_open_file_(false),
         injected_thread_local_read_error_(DeleteThreadLocalErrorContext),
@@ -360,6 +359,19 @@ class FaultInjectionTestFS : public FileSystemWrapper {
     MutexLock l(&mutex_);
     return filesystem_writable_;
   }
+  bool ShouldUseDiretWritable(const std::string& file_name) {
+    MutexLock l(&mutex_);
+    if (filesystem_writable_) {
+      return true;
+    }
+    FileType file_type = kTempFile;
+    uint64_t file_number = 0;
+    if (!TryParseFileName(file_name, &file_number, &file_type)) {
+      return false;
+    }
+    return direct_writable_types_.find(file_type) !=
+           direct_writable_types_.end();
+  }
   void SetFilesystemActiveNoLock(
       bool active, IOStatus error = IOStatus::Corruption("Not active")) {
     error.PermitUncheckedError();
@@ -379,18 +391,6 @@ class FaultInjectionTestFS : public FileSystemWrapper {
     filesystem_writable_ = writable;
   }
 
-  // If true, we buffer write data in memory to simulate data loss upon system
-  // crash by only having process crashes
-  void SetInjectUnsyncedDataLoss(bool inject) {
-    MutexLock l(&mutex_);
-    inject_unsynced_data_loss_ = inject;
-  }
-
-  bool InjectUnsyncedDataLoss() {
-    MutexLock l(&mutex_);
-    return inject_unsynced_data_loss_;
-  }
-
   // In places (e.g. GetSortedWals()) RocksDB relies on querying the file size
   // or even reading the contents of files currently open for writing, and
   // as in POSIX semantics, expects to see the flushed size and contents
@@ -414,10 +414,15 @@ class FaultInjectionTestFS : public FileSystemWrapper {
     allow_link_open_file_ = allow_link_open_file;
   }
 
-  bool ShouldIOActivtiesExcludedFromFaultInjection(Env::IOActivity io_activty) {
+  void SetDirectWritableTypes(const std::set<FileType>& types) {
     MutexLock l(&mutex_);
-    return io_activties_excluded_from_fault_injection.find(io_activty) !=
-           io_activties_excluded_from_fault_injection.end();
+    direct_writable_types_ = types;
+  }
+
+  void SetIOActivtiesExemptedFromFaultInjection(
+      const std::set<Env::IOActivity>& io_activties) {
+    MutexLock l(&mutex_);
+    io_activties_exempted_from_fault_injection = io_activties;
   }
 
   void AssertNoOpenFile() { assert(open_managed_files_.empty()); }
@@ -494,8 +499,8 @@ class FaultInjectionTestFS : public FileSystemWrapper {
 
   IOStatus MaybeInjectThreadLocalError(
       FaultInjectionIOType type, const IOOptions& io_options,
-      const std::string& file_name = "", ErrorOperation op = kUnknown,
-      Slice* slice = nullptr, bool direct_io = false, char* scratch = nullptr,
+      ErrorOperation op = kUnknown, Slice* slice = nullptr,
+      bool direct_io = false, char* scratch = nullptr,
       bool need_count_increase = false, bool* fault_injected = nullptr);
 
   int GetAndResetInjectedThreadLocalErrorCount(FaultInjectionIOType type) {
@@ -508,29 +513,6 @@ class FaultInjectionTestFS : public FileSystemWrapper {
     return count;
   }
 
-  void SetIOActivtiesExcludedFromFaultInjection(
-      const std::set<Env::IOActivity>& io_activties) {
-    MutexLock l(&mutex_);
-    io_activties_excluded_from_fault_injection = io_activties;
-  }
-
-  void SetFileTypesExcludedFromWriteFaultInjection(
-      const std::set<FileType>& types) {
-    MutexLock l(&mutex_);
-    file_types_excluded_from_write_fault_injection_ = types;
-  }
-
-  bool ShouldExcludeFromWriteFaultInjection(const std::string& file_name) {
-    MutexLock l(&mutex_);
-    FileType file_type = kTempFile;
-    uint64_t file_number = 0;
-    if (!TryParseFileName(file_name, &file_number, &file_type)) {
-      return false;
-    }
-    return file_types_excluded_from_write_fault_injection_.find(file_type) !=
-           file_types_excluded_from_write_fault_injection_.end();
-  }
-
   void EnableThreadLocalErrorInjection(FaultInjectionIOType type) {
     ErrorContext* ctx = GetErrorContextFromFaultInjectionIOType(type);
     if (ctx) {
@@ -547,14 +529,8 @@ class FaultInjectionTestFS : public FileSystemWrapper {
 
   void PrintInjectedThreadLocalErrorBacktrace(FaultInjectionIOType type);
 
-  // If there is unsynced data in the specified file within the specified
-  // range [offset, offset + n), return the unsynced data overlapping with
-  // that range, in a corresponding range of scratch. When known, also return
-  // the position of the last sync, so that the caller can determine whether
-  // more data is available from the target file when not available from
-  // unsynced.
-  void ReadUnsynced(const std::string& fname, uint64_t offset, size_t n,
-                    Slice* result, char* scratch, int64_t* pos_at_last_sync);
+  void AddUnsyncedToRead(const std::string& fname, size_t offset, size_t n,
+                         Slice* result, char* scratch);
 
  private:
   port::Mutex mutex_;
@@ -569,7 +545,6 @@ class FaultInjectionTestFS : public FileSystemWrapper {
   bool filesystem_active_;    // Record flushes, syncs, writes
   bool filesystem_writable_;  // Bypass FaultInjectionTestFS and go directly
                               // to underlying FS for writable files
-  bool inject_unsynced_data_loss_;  // See InjectUnsyncedDataLoss()
   bool read_unsynced_data_;   // See SetReadUnsyncedData()
   bool allow_link_open_file_;  // See SetAllowLinkOpenFile()
   IOStatus fs_error_;
@@ -607,8 +582,8 @@ class FaultInjectionTestFS : public FileSystemWrapper {
     }
   };
 
-  std::set<FileType> file_types_excluded_from_write_fault_injection_;
-  std::set<Env::IOActivity> io_activties_excluded_from_fault_injection;
+  std::set<FileType> direct_writable_types_;
+  std::set<Env::IOActivity> io_activties_exempted_from_fault_injection;
   ThreadLocalPtr injected_thread_local_read_error_;
   ThreadLocalPtr injected_thread_local_write_error_;
   ThreadLocalPtr injected_thread_local_metadata_read_error_;

utilities/table_properties_collectors/compact_for_tiering_collector.cc

@@ -139,6 +139,8 @@ std::shared_ptr<CompactForTieringCollectorFactory>
 NewCompactForTieringCollectorFactory(double compaction_trigger_ratio) {
   return std::make_shared<CompactForTieringCollectorFactory>(
       compaction_trigger_ratio);
+  return std::shared_ptr<CompactForTieringCollectorFactory>(
+      new CompactForTieringCollectorFactory(compaction_trigger_ratio));
 }
 
 }  // namespace ROCKSDB_NAMESPACE

utilities/transactions/pessimistic_transaction.cc

@@ -1227,15 +1227,14 @@ Status PessimisticTransaction::SetName(const TransactionName& name) {
   if (txn_state_ == STARTED) {
     if (name_.length()) {
       s = Status::InvalidArgument("Transaction has already been named.");
+    } else if (txn_db_impl_->GetTransactionByName(name) != nullptr) {
+      s = Status::InvalidArgument("Transaction name must be unique.");
     } else if (name.length() < 1 || name.length() > 512) {
       s = Status::InvalidArgument(
           "Transaction name length must be between 1 and 512 chars.");
     } else {
       name_ = name;
-      s = txn_db_impl_->RegisterTransaction(this);
-      if (!s.ok()) {
-        name_.clear();
-      }
+      txn_db_impl_->RegisterTransaction(this);
     }
   } else {
     s = Status::InvalidArgument("Transaction is beyond state for naming.");

utilities/transactions/pessimistic_transaction_db.cc

@@ -723,11 +723,6 @@ void PessimisticTransactionDB::ReinitializeTransaction(
 Transaction* PessimisticTransactionDB::GetTransactionByName(
     const TransactionName& name) {
   std::lock_guard<std::mutex> lock(name_map_mutex_);
-  return GetTransactionByNameLocked(name);
-}
-
-Transaction* PessimisticTransactionDB::GetTransactionByNameLocked(
-    const TransactionName& name) {
   auto it = transactions_.find(name);
   if (it == transactions_.end()) {
     return nullptr;
@@ -760,15 +755,13 @@ void PessimisticTransactionDB::SetDeadlockInfoBufferSize(uint32_t target_size) {
   lock_manager_->Resize(target_size);
 }
 
-Status PessimisticTransactionDB::RegisterTransaction(Transaction* txn) {
+void PessimisticTransactionDB::RegisterTransaction(Transaction* txn) {
   assert(txn);
   assert(txn->GetName().length() > 0);
+  assert(GetTransactionByName(txn->GetName()) == nullptr);
   assert(txn->GetState() == Transaction::STARTED);
   std::lock_guard<std::mutex> lock(name_map_mutex_);
-  if (!transactions_.insert({txn->GetName(), txn}).second) {
-    return Status::InvalidArgument("Duplicate txn name " + txn->GetName());
-  }
-  return Status::OK();
+  transactions_[txn->GetName()] = txn;
 }
 
 void PessimisticTransactionDB::UnregisterTransaction(Transaction* txn) {

utilities/transactions/pessimistic_transaction_db.h

@@ -173,7 +173,7 @@ class PessimisticTransactionDB : public TransactionDB {
 
   Transaction* GetTransactionByName(const TransactionName& name) override;
 
-  Status RegisterTransaction(Transaction* txn);
+  void RegisterTransaction(Transaction* txn);
   void UnregisterTransaction(Transaction* txn);
 
   // not thread safe. current use case is during recovery (single thread)
@@ -239,7 +239,6 @@ class PessimisticTransactionDB : public TransactionDB {
   friend class WriteUnpreparedTransactionTest_MarkLogWithPrepSection_Test;
 
   Transaction* BeginInternalTransaction(const WriteOptions& options);
-  Transaction* GetTransactionByNameLocked(const TransactionName& name);
 
   std::shared_ptr<LockManager> lock_manager_;
 

utilities/transactions/transaction_base.cc

@@ -832,13 +832,11 @@ Status TransactionBaseImpl::RebuildFromWriteBatch(WriteBatch* src_batch) {
     }
 
     Status PutCF(uint32_t cf, const Slice& key, const Slice& val) override {
-      Slice user_key = GetUserKey(cf, key);
-      return txn_->Put(db_->GetColumnFamilyHandle(cf), user_key, val);
+      return txn_->Put(db_->GetColumnFamilyHandle(cf), key, val);
     }
 
     Status PutEntityCF(uint32_t cf, const Slice& key,
                        const Slice& entity) override {
-      Slice user_key = GetUserKey(cf, key);
       Slice entity_copy = entity;
       WideColumns columns;
       const Status s =
@@ -847,22 +845,19 @@ Status TransactionBaseImpl::RebuildFromWriteBatch(WriteBatch* src_batch) {
         return s;
       }
 
-      return txn_->PutEntity(db_->GetColumnFamilyHandle(cf), user_key, columns);
+      return txn_->PutEntity(db_->GetColumnFamilyHandle(cf), key, columns);
     }
 
     Status DeleteCF(uint32_t cf, const Slice& key) override {
-      Slice user_key = GetUserKey(cf, key);
-      return txn_->Delete(db_->GetColumnFamilyHandle(cf), user_key);
+      return txn_->Delete(db_->GetColumnFamilyHandle(cf), key);
     }
 
     Status SingleDeleteCF(uint32_t cf, const Slice& key) override {
-      Slice user_key = GetUserKey(cf, key);
-      return txn_->SingleDelete(db_->GetColumnFamilyHandle(cf), user_key);
+      return txn_->SingleDelete(db_->GetColumnFamilyHandle(cf), key);
     }
 
     Status MergeCF(uint32_t cf, const Slice& key, const Slice& val) override {
-      Slice user_key = GetUserKey(cf, key);
-      return txn_->Merge(db_->GetColumnFamilyHandle(cf), user_key, val);
+      return txn_->Merge(db_->GetColumnFamilyHandle(cf), key, val);
     }
 
     // this is used for reconstructing prepared transactions upon
@@ -885,21 +880,6 @@ Status TransactionBaseImpl::RebuildFromWriteBatch(WriteBatch* src_batch) {
     Status MarkRollback(const Slice&) override {
       return Status::InvalidArgument();
     }
-    size_t GetTimestampSize(uint32_t cf_id) {
-      auto cfd = db_->versions_->GetColumnFamilySet()->GetColumnFamily(cf_id);
-      const Comparator* ucmp = cfd->user_comparator();
-      assert(ucmp);
-      return ucmp->timestamp_size();
-    }
-
-    Slice GetUserKey(uint32_t cf_id, const Slice& key) {
-      size_t ts_sz = GetTimestampSize(cf_id);
-      if (ts_sz == 0) {
-        return key;
-      }
-      assert(key.size() >= ts_sz);
-      return Slice(key.data(), key.size() - ts_sz);
-    }
   };
 
   IndexedWriteBatchBuilder copycat(this, dbimpl_);

utilities/transactions/write_committed_transaction_ts_test.cc

@@ -288,25 +288,12 @@ TEST_P(WriteCommittedTxnWithTsTest, RecoverFromWal) {
 
   txn0.reset();
 
-  std::unique_ptr<Transaction> txn3(
-      NewTxn(WriteOptions(), TransactionOptions()));
-  assert(txn3);
-  ASSERT_OK(txn3->Put(handles_[1], "baz", "baz_value"));
-  ASSERT_OK(txn3->SetName("txn3"));
-  ASSERT_OK(txn3->Prepare());
-  txn3.reset();
-
   ASSERT_OK(ReOpenNoDelete(cf_descs, &handles_));
 
   {
-    Transaction* recovered_txn0 = db->GetTransactionByName("txn0");
-    ASSERT_OK(recovered_txn0->SetCommitTimestamp(23));
-    ASSERT_OK(recovered_txn0->Commit());
-    delete recovered_txn0;
     std::string value;
     Status s = GetFromDb(ReadOptions(), handles_[1], "foo", /*ts=*/23, &value);
-    ASSERT_OK(s);
-    ASSERT_EQ("foo_value", value);
+    ASSERT_TRUE(s.IsNotFound());
 
     s = db->Get(ReadOptions(), handles_[0], "bar", &value);
     ASSERT_OK(s);
@@ -327,9 +314,6 @@ TEST_P(WriteCommittedTxnWithTsTest, RecoverFromWal) {
     s = GetFromDb(ReadOptions(), handles_[1], "key1", /*ts=*/24, &value);
     ASSERT_OK(s);
     ASSERT_EQ("value_3", value);
-
-    s = GetFromDb(ReadOptions(), handles_[1], "baz", /*ts=*/24, &value);
-    ASSERT_TRUE(s.IsNotFound());
   }
 }