Update HISTORY.md and version.h for 9.5.2

Summary:
Add a couple of ticker stats for corruption retry count and successful retries. This PR also eliminates an extra read attempt when there's a checksum mismatch in a block read from the prefetch buffer.

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

Test Plan: Update existing tests

Reviewed By: jowlyzhang

Differential Revision: D61024687

Pulled By: anand1976

fbshipit-source-id: 3a08403580ab244000e0d480b7ee0f5a03d76b06

HISTORY.md

@@ -1,10 +1,36 @@
 # Rocksdb Change Log
 > NOTE: Entries for next release do not go here. Follow instructions in `unreleased_history/README.txt`
 
-## 9.4.1 (08/02/2024)
+## 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)
 ### 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.1.8
-SNAPPY_SHA256 ?= 16b677f07832a612b0836178db7f374e414f94657c138e6993cbfc5dcc58651f
+SNAPPY_VER ?= 1.2.1
+SNAPPY_SHA256 ?= 736aeb64d86566d2236ddffa2865ee5d7a82d26c9016b36218fcc27ea4f09f86
 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 ${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 -DSNAPPY_BUILD_BENCHMARKS=OFF -DSNAPPY_BUILD_TESTS=OFF --compile-no-warning-as-error ${PLATFORM_CMAKE_FLAGS} .. && $(MAKE) ${SNAPPY_MAKE_TARGET}
 	cp snappy-$(SNAPPY_VER)/build/libsnappy.a .
 
 lz4-$(LZ4_VER).tar.gz:
@@ -2484,13 +2484,11 @@ 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 17054c85c2dee0e84cbee7ef28d7bae7d0b1630e
+	cd third-party/folly && git reset --hard c48fdd205c1c291651749d532b8055fe822bba25
 	@# 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 {
-            delete *memtable_range_tombstone_iter_;
-            *memtable_range_tombstone_iter_ = new TruncatedRangeDelIterator(
-                std::unique_ptr<FragmentedRangeTombstoneIterator>(t),
-                &cfd->internal_comparator(), nullptr, nullptr);
+            *memtable_range_tombstone_iter_ =
+                std::make_unique<TruncatedRangeDelIterator>(
+                    std::unique_ptr<FragmentedRangeTombstoneIterator>(t),
+                    &cfd->internal_comparator(), nullptr, nullptr);
           }
         }
         db_impl->ReturnAndCleanupSuperVersion(cfd, sv);

db/arena_wrapped_db_iter.h

@@ -55,7 +55,8 @@ class ArenaWrappedDBIter : public Iterator {
     db_iter_->SetIter(iter);
   }
 
-  void SetMemtableRangetombstoneIter(TruncatedRangeDelIterator** iter) {
+  void SetMemtableRangetombstoneIter(
+      std::unique_ptr<TruncatedRangeDelIterator>* iter) {
     memtable_range_tombstone_iter_ = iter;
   }
 
@@ -110,7 +111,8 @@ 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.
-  TruncatedRangeDelIterator** memtable_range_tombstone_iter_ = nullptr;
+  std::unique_ptr<TruncatedRangeDelIterator>* memtable_range_tombstone_iter_ =
+      nullptr;
 };
 
 // Generate the arena wrapped iterator class.

db/c.cc

@@ -532,6 +532,42 @@ 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()) {
@@ -1802,6 +1838,26 @@ 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);
 }
@@ -2230,6 +2286,32 @@ 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),
@@ -2242,6 +2324,21 @@ 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();
@@ -3037,6 +3134,15 @@ 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,6 +199,79 @@ 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,
@@ -1688,6 +1761,21 @@ 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,7 +63,9 @@ uint64_t TotalFileSize(const std::vector<FileMetaData*>& files) {
   return sum;
 }
 
-void Compaction::SetInputVersion(Version* _input_version) {
+// TODO(hx235): consider making this function part of the construction so we
+// don't forget to call it
+void Compaction::FinalizeInputInfo(Version* _input_version) {
   input_version_ = _input_version;
   cfd_ = input_version_->cfd();
 
@@ -864,6 +866,11 @@ 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,14 +289,7 @@ class Compaction {
   // is the sum of all input file sizes.
   uint64_t OutputFilePreallocationSize() const;
 
-  // 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();
-  }
+  void FinalizeInputInfo(Version* input_version);
 
   struct InputLevelSummaryBuffer {
     char buffer[128];
@@ -333,6 +326,16 @@ 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_;
   }
@@ -433,7 +436,6 @@ class Compaction {
                                       const int output_level);
 
  private:
-  void SetInputVersion(Version* input_version);
 
   Status InitInputTableProperties();
 

db/compaction/compaction_job.cc

@@ -468,6 +468,11 @@ 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)) {
@@ -621,6 +626,7 @@ 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,6 +275,8 @@ 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,6 +322,67 @@ 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_;
@@ -3143,52 +3204,55 @@ TEST_F(DBBloomFilterTest, DynamicBloomFilterOptions) {
 }
 
 TEST_F(DBBloomFilterTest, SeekForPrevWithPartitionedFilters) {
-  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);
+  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);
 
-  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));
+    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());
+    }
   }
-  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,6 +21,7 @@
 #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"
@@ -92,6 +93,11 @@ 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;
 
@@ -117,7 +123,10 @@ Status DBImpl::GetSortedWalFiles(VectorWalPtr& files) {
     }
   }
 
-  Status s = wal_manager_.GetSortedWalFiles(files);
+  // 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);
 
   // DisableFileDeletions / EnableFileDeletions not supported in read-only DB
   if (deletions_disabled.ok()) {
@@ -206,7 +215,12 @@ 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.
-      s = GetSortedWalFiles(live_wal_files);
+      // 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);
 
       if (!s.ok()) {
         return s;
@@ -217,6 +231,7 @@ 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) {
@@ -428,7 +443,8 @@ Status DBImpl::GetLiveFilesStorageInfo(
   // WAL files.
   if (s.ok()) {
     // FIXME: avoid querying the filesystem for current WAL state
-    s = GetSortedWalFiles(live_wal_files);
+    s = GetSortedWalFilesImpl(live_wal_files,
+                              /* need_seqnos */ false);
   }
   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()) {
-          s = error_handler_.SetBGError(io_s,
-                                        BackgroundErrorReason::kManifestWrite);
+          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()) {
-          s = error_handler_.SetBGError(versions_->io_status(),
-                                        BackgroundErrorReason::kManifestWrite);
+          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()) {
-    status = error_handler_.SetBGError(versions_->io_status(),
-                                       BackgroundErrorReason::kManifestWrite);
+    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) {
-    TruncatedRangeDelIterator* mem_tombstone_iter = nullptr;
+    std::unique_ptr<TruncatedRangeDelIterator> mem_tombstone_iter;
     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 = new TruncatedRangeDelIterator(
+      mem_tombstone_iter = std::make_unique<TruncatedRangeDelIterator>(
           std::unique_ptr<FragmentedRangeTombstoneIterator>(range_del_iter),
           &cfd->ioptions()->internal_comparator, nullptr /* smallest */,
           nullptr /* largest */);
     }
-    merge_iter_builder.AddPointAndTombstoneIterator(mem_iter,
-                                                    mem_tombstone_iter);
+    merge_iter_builder.AddPointAndTombstoneIterator(
+        mem_iter, std::move(mem_tombstone_iter));
   } else {
     merge_iter_builder.AddIterator(mem_iter);
   }
@@ -5828,6 +5828,18 @@ 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,6 +508,8 @@ 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
@@ -2348,6 +2350,9 @@ 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,10 +1595,12 @@ 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
@@ -4161,7 +4163,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->GetInputTableProperties();
+  const auto& input_table_properties = c->GetOrInitInputTableProperties();
   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,6 +530,12 @@ 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
@@ -640,7 +646,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,6 +556,35 @@ 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;
@@ -639,34 +668,6 @@ 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
@@ -1204,8 +1205,7 @@ 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)
-        .PermitUncheckedError();
+    error_handler_.SetBGError(status, BackgroundErrorReason::kMemTable);
     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()) {
-        s = error_handler_.SetBGError(versions_->io_status(),
-                                      BackgroundErrorReason::kManifestWrite);
+        error_handler_.SetBGError(versions_->io_status(),
+                                  BackgroundErrorReason::kManifestWrite);
       }
       if (!s.ok()) {
         return s;

db/db_io_failure_test.cc

@@ -705,6 +705,7 @@ class DBIOCorruptionTest
   DBIOCorruptionTest() : DBIOFailureTest() {
     BlockBasedTableOptions bbto;
     options_ = CurrentOptions();
+    options_.statistics = CreateDBStatistics();
 
     base_env_ = env_;
     EXPECT_NE(base_env_, nullptr);
@@ -727,6 +728,8 @@ 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_;
@@ -749,8 +752,12 @@ 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);
   }
 }
 
@@ -773,8 +780,12 @@ 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;
 }
@@ -799,9 +810,13 @@ 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);
   }
 }
 
@@ -818,6 +833,9 @@ 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;
@@ -826,6 +844,7 @@ TEST_P(DBIOCorruptionTest, CompactionReadCorruptionRetry) {
     ASSERT_EQ(val, "val1");
   } else {
     ASSERT_TRUE(s.IsCorruption());
+    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_COUNT), 0);
   }
 }
 
@@ -838,6 +857,9 @@ 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;
@@ -846,6 +868,7 @@ TEST_P(DBIOCorruptionTest, FlushReadCorruptionRetry) {
     ASSERT_EQ(val, "val1");
   } else {
     ASSERT_NOK(s);
+    ASSERT_EQ(stats()->getTickerCount(FILE_READ_CORRUPTION_RETRY_COUNT), 0);
   }
 }
 
@@ -862,8 +885,12 @@ 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,7 +245,13 @@ 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(1024 * 1024);
+      options.sst_file_manager->SetDeleteRateBytesPerSecond(1);
     }
     ASSERT_OK(Put("Key2", DummyString(1024, v)));
     ASSERT_OK(Put("Key3", DummyString(1024, v)));

db/db_test_util.cc

@@ -565,6 +565,11 @@ 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,6 +1041,7 @@ class DBTestBase : public testing::Test {
     kPartitionedFilterWithNewTableReaderForCompactions,
     kUniversalSubcompactions,
     kUnorderedWrite,
+    kBlockBasedTableWithBinarySearchWithFirstKeyIndex,
     // This must be the last line
     kEnd,
   };

db/error_handler.cc

@@ -250,8 +250,6 @@ 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 -
@@ -270,11 +268,11 @@ STATIC_AVOID_DESTRUCTION(const Status, kOkStatus){Status::OK()};
 // 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
-const Status& ErrorHandler::HandleKnownErrors(const Status& bg_err,
-                                              BackgroundErrorReason reason) {
+void ErrorHandler::HandleKnownErrors(const Status& bg_err,
+                                     BackgroundErrorReason reason) {
   db_mutex_->AssertHeld();
   if (bg_err.ok()) {
-    return kOkStatus;
+    return;
   }
 
   ROCKS_LOG_INFO(db_options_.info_log,
@@ -339,7 +337,7 @@ const Status& ErrorHandler::HandleKnownErrors(const Status& bg_err,
     } else {
       // This error is less severe than previously encountered error. Don't
       // take any further action
-      return bg_error_;
+      return;
     }
   }
 
@@ -356,7 +354,6 @@ const Status& 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
@@ -383,14 +380,14 @@ const Status& 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.
-const Status& ErrorHandler::SetBGError(const Status& bg_status,
-                                       BackgroundErrorReason reason) {
+void 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 kOkStatus;
+    return;
   }
   ROCKS_LOG_WARN(db_options_.info_log, "Background IO error %s",
                  bg_io_err.ToString().c_str());
@@ -413,11 +410,11 @@ const Status& ErrorHandler::SetBGError(const Status& bg_status,
     EventHelpers::NotifyOnBackgroundError(db_options_.listeners, reason,
                                           &bg_err, db_mutex_, &auto_recovery);
     recover_context_ = context;
-    return bg_error_;
-  } else if (bg_io_err.subcode() != IOStatus::SubCode::kNoSpace &&
-             (bg_io_err.GetScope() ==
-                  IOStatus::IOErrorScope::kIOErrorScopeFile ||
-              bg_io_err.GetRetryable())) {
+    return;
+  }
+  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
@@ -426,10 +423,6 @@ const Status& 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 */);
@@ -445,9 +438,13 @@ const Status& 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 bg_error_;
+      return;
     }
 
     Status::Severity severity;
@@ -469,10 +466,14 @@ const Status& ErrorHandler::SetBGError(const Status& bg_status,
     Status bg_err(new_bg_io_err, severity);
     CheckAndSetRecoveryAndBGError(bg_err);
     recover_context_ = context;
-    return StartRecoverFromRetryableBGIOError(bg_io_err);
-  } else {
-    return HandleKnownErrors(new_bg_io_err, reason);
+    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;
   }
+  HandleKnownErrors(new_bg_io_err, reason);
 }
 
 void ErrorHandler::AddFilesToQuarantine(
@@ -620,23 +621,23 @@ Status ErrorHandler::RecoverFromBGError(bool is_manual) {
   return s;
 }
 
-const Status& ErrorHandler::StartRecoverFromRetryableBGIOError(
+void ErrorHandler::StartRecoverFromRetryableBGIOError(
     const IOStatus& io_error) {
   db_mutex_->AssertHeld();
-  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_) {
+  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_) {
     // Can temporarily release db mutex
     EventHelpers::NotifyOnErrorRecoveryEnd(db_options_.listeners, bg_error_,
                                            Status::ShutdownInProgress(),
                                            db_mutex_);
     db_mutex_->AssertHeld();
-    return bg_error_;
+    return;
   }
   RecordStats({ERROR_HANDLER_AUTORESUME_COUNT}, {} /* int_histograms */);
   ROCKS_LOG_INFO(
@@ -664,12 +665,6 @@ const Status& 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);
 
-  const Status& SetBGError(const Status& bg_err, BackgroundErrorReason reason);
+  void SetBGError(const Status& bg_err, BackgroundErrorReason reason);
 
   Status GetBGError() const { return bg_error_; }
 
@@ -135,11 +135,10 @@ class ErrorHandler {
   // unsorted.
   autovector<uint64_t> files_to_quarantine_;
 
-  const Status& HandleKnownErrors(const Status& bg_err,
-                                  BackgroundErrorReason reason);
+  void HandleKnownErrors(const Status& bg_err, BackgroundErrorReason reason);
   Status OverrideNoSpaceError(const Status& bg_error, bool* auto_recovery);
   void RecoverFromNoSpace();
-  const Status& StartRecoverFromRetryableBGIOError(const IOStatus& io_error);
+  void 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,13 +851,17 @@ TEST_F(DBErrorHandlingFSTest, DoubleManifestWriteError) {
       });
   SyncPoint::GetInstance()->EnableProcessing();
   s = Flush();
-  ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kHardError);
+  ASSERT_TRUE(s.IsNoSpace());
+  ASSERT_EQ(dbfull()->TEST_GetBGError().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_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kHardError);
+  ASSERT_TRUE(s.IsNoSpace());
+  ASSERT_EQ(dbfull()->TEST_GetBGError().severity(),
+            ROCKSDB_NAMESPACE::Status::Severity::kHardError);
   ASSERT_FALSE(dbfull()->TEST_GetFilesToQuarantine().empty());
   fault_fs_->SetFilesystemActive(true);
   SyncPoint::GetInstance()->ClearAllCallBacks();

db/event_helpers.cc

@@ -228,15 +228,18 @@ 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;
-      info.new_bg_error = new_bg_error;
-      listener->OnErrorRecoveryCompleted(old_bg_error);
+      info.old_bg_error = old_bg_error_cp;
+      info.new_bg_error = new_bg_error_cp;
+      listener->OnErrorRecoveryCompleted(old_bg_error_cp);
       listener->OnErrorRecoveryEnd(info);
       info.old_bg_error.PermitUncheckedError();
       info.new_bg_error.PermitUncheckedError();

db/external_sst_file_ingestion_job.cc

@@ -44,9 +44,12 @@ 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()) {
+        file_to_ingest.cf_id != cfd_->GetID() &&
+        !ingestion_options_.allow_db_generated_files) {
       return Status::InvalidArgument(
           "External file column family id don't match");
     }
@@ -111,6 +114,7 @@ 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()) {
@@ -704,9 +708,16 @@ Status ExternalSstFileIngestionJob::SanityCheckTableProperties(
   // Get table version
   auto version_iter = uprops.find(ExternalSstFilePropertyNames::kVersion);
   if (version_iter == uprops.end()) {
-    return Status::Corruption("External file version not found");
+    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());
   }
-  file_to_ingest->version = DecodeFixed32(version_iter->second.c_str());
 
   auto seqno_iter = uprops.find(ExternalSstFilePropertyNames::kGlobalSeqno);
   if (file_to_ingest->version == 2) {
@@ -733,8 +744,15 @@ 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 is not supported");
+    return Status::InvalidArgument("External file version " +
+                                   std::to_string(file_to_ingest->version) +
+                                   " is not supported");
   }
 
   file_to_ingest->cf_id = static_cast<uint32_t>(props->column_family_id);
@@ -896,6 +914,25 @@ 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));
@@ -911,6 +948,11 @@ 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();
@@ -1045,11 +1087,18 @@ 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,10 +3719,7 @@ TEST_F(ExternalSSTFileWithTimestampTest, TimestampsNotPersistedBasic) {
 }
 
 INSTANTIATE_TEST_CASE_P(ExternalSSTFileTest, ExternalSSTFileTest,
-                        testing::Values(std::make_tuple(false, false),
-                                        std::make_tuple(false, true),
-                                        std::make_tuple(true, false),
-                                        std::make_tuple(true, true)));
+                        testing::Combine(testing::Bool(), testing::Bool()));
 
 INSTANTIATE_TEST_CASE_P(ExternSSTFileLinkFailFallbackTest,
                         ExternSSTFileLinkFailFallbackTest,
@@ -3730,6 +3727,339 @@ 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,34 +567,44 @@ 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());
 
-  uint32_t len = Random::GetTLSInstance()->Uniform(10000) + 1;
-  Random rnd(len);
+  // 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;
 
-  // Create partially synced file
-  std::string f = test::PerThreadDBPath("read_unsynced.data");
-  std::string data = rnd.RandomString(len);
-  {
+    std::string f =
+        test::PerThreadDBPath("read_unsynced." + std::to_string(seed));
+    std::string data = rnd.RandomString(len);
+
+    // Create partially synced file
     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));
-    ASSERT_OK(w->Sync({}, nullptr));
+    if (synced_len > 0) {
+      ASSERT_OK(w->Sync({}, nullptr));
+    }
     ASSERT_OK(w->Append(Slice(data.data() + synced_len, len - synced_len), {},
                         nullptr));
-    // 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.
-  {
+
+    // 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.
     std::unique_ptr<FSSequentialFile> r;
     ASSERT_OK(fault_fs->NewSequentialFile(f, {}, &r, nullptr));
     uint32_t first_read_len = rnd.Uniform(len + 1);
@@ -603,12 +613,36 @@ 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)));
-    uint32_t second_read_len = len - 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);
     scratch.reset(new char[second_read_len]);
     ASSERT_OK(r->Read(second_read_len, {}, &sl, scratch.get(), nullptr));
-    ASSERT_EQ(second_read_len, sl.size());
-    ASSERT_EQ(0,
-              sl.compare(Slice(data.data() + first_read_len, second_read_len)));
+    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())));
   }
 }
 }  // namespace ROCKSDB_NAMESPACE

db/memtable_list.cc

@@ -235,19 +235,19 @@ void MemTableListVersion::AddIterators(
       SequenceNumber read_seq = options.snapshot != nullptr
                                     ? options.snapshot->GetSequenceNumber()
                                     : kMaxSequenceNumber;
-      TruncatedRangeDelIterator* mem_tombstone_iter = nullptr;
+      std::unique_ptr<TruncatedRangeDelIterator> mem_tombstone_iter;
       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 = new TruncatedRangeDelIterator(
+        mem_tombstone_iter = std::make_unique<TruncatedRangeDelIterator>(
             std::unique_ptr<FragmentedRangeTombstoneIterator>(range_del_iter),
             &m->GetInternalKeyComparator(), nullptr /* smallest */,
             nullptr /* largest */);
       }
-      merge_iter_builder->AddPointAndTombstoneIterator(mem_iter,
-                                                       mem_tombstone_iter);
+      merge_iter_builder->AddPointAndTombstoneIterator(
+          mem_iter, std::move(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,
-    TruncatedRangeDelIterator** range_del_iter) {
+    std::unique_ptr<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 = new TruncatedRangeDelIterator(
+        *range_del_iter = std::make_unique<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,
-      TruncatedRangeDelIterator** range_del_iter = nullptr);
+      std::unique_ptr<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,7 +255,6 @@ 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,7 +976,8 @@ class LevelIterator final : public InternalIterator {
       const std::vector<AtomicCompactionUnitBoundary>* compaction_boundaries =
           nullptr,
       bool allow_unprepared_value = false,
-      TruncatedRangeDelIterator**** range_tombstone_iter_ptr_ = nullptr)
+      std::unique_ptr<TruncatedRangeDelIterator>*** range_tombstone_iter_ptr_ =
+          nullptr)
       : table_cache_(table_cache),
         read_options_(read_options),
         file_options_(file_options),
@@ -1116,9 +1117,8 @@ class LevelIterator final : public InternalIterator {
   }
 
   void ClearRangeTombstoneIter() {
-    if (range_tombstone_iter_ && *range_tombstone_iter_) {
-      delete *range_tombstone_iter_;
-      *range_tombstone_iter_ = nullptr;
+    if (range_tombstone_iter_) {
+      range_tombstone_iter_->reset();
     }
   }
 
@@ -1201,7 +1201,7 @@ class LevelIterator final : public InternalIterator {
   // iterator end).
   //
   // *range_tombstone_iter_ points to range tombstones of the current SST file
-  TruncatedRangeDelIterator** range_tombstone_iter_;
+  std::unique_ptr<TruncatedRangeDelIterator>* range_tombstone_iter_;
 
   // The sentinel key to be returned
   Slice sentinel_;
@@ -1280,11 +1280,9 @@ 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) ||
-           user_comparator_.CompareWithoutTimestamp(
-               prefix_extractor_->Transform(target_user_key_without_ts), false,
-               prefix_extractor_->Transform(
-                   next_file_first_user_key_without_ts),
-               false) != 0)) {
+           prefix_extractor_->Transform(target_user_key_without_ts)
+                   .compare(prefix_extractor_->Transform(
+                       next_file_first_user_key_without_ts)) != 0)) {
         // SkipEmptyFileForward() will not advance to next file when this flag
         // is set for reason detailed below.
         //
@@ -1929,7 +1927,7 @@ InternalIterator* Version::TEST_GetLevelIterator(
     int level, bool allow_unprepared_value) {
   auto* arena = merge_iter_builder->GetArena();
   auto* mem = arena->AllocateAligned(sizeof(LevelIterator));
-  TruncatedRangeDelIterator*** tombstone_iter_ptr = nullptr;
+  std::unique_ptr<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),
@@ -2029,7 +2027,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
-    TruncatedRangeDelIterator* tombstone_iter = nullptr;
+    std::unique_ptr<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(
@@ -2046,8 +2044,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,
-                                                         tombstone_iter);
+        merge_iter_builder->AddPointAndTombstoneIterator(
+            table_iter, std::move(tombstone_iter));
       }
     }
     if (should_sample) {
@@ -2064,7 +2062,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));
-    TruncatedRangeDelIterator*** tombstone_iter_ptr = nullptr;
+    std::unique_ptr<TruncatedRangeDelIterator>** tombstone_iter_ptr = nullptr;
     auto level_iter = new (mem) LevelIterator(
         cfd_->table_cache(), read_options, soptions,
         cfd_->internal_comparator(), &storage_info_.LevelFilesBrief(level),
@@ -5510,6 +5508,7 @@ 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();
@@ -5538,6 +5537,7 @@ 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,12 +5559,14 @@ Status VersionSet::ProcessManifestWrites(
             db_options_->listeners, nullptr,
             tmp_set.Contains(FileType::kDescriptorFile),
             tmp_set.Contains(FileType::kDescriptorFile)));
-        descriptor_log_.reset(
+        new_desc_log_ptr.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, descriptor_log_.get(),
-                                        io_s);
-      } else {
+                                        wal_additions, raw_desc_log_ptr, io_s);
+        assert(s == io_s);
+      }
+      if (!io_s.ok()) {
         manifest_io_status = io_s;
         s = io_s;
       }
@@ -5607,7 +5609,7 @@ Status VersionSet::ProcessManifestWrites(
         }
         ++idx;
 #endif /* !NDEBUG */
-        io_s = descriptor_log_->AddRecord(write_options, record);
+        io_s = raw_desc_log_ptr->AddRecord(write_options, record);
         if (!io_s.ok()) {
           s = io_s;
           manifest_io_status = io_s;
@@ -5617,7 +5619,7 @@ Status VersionSet::ProcessManifestWrites(
 
       if (s.ok()) {
         io_s =
-            SyncManifest(db_options_, write_options, descriptor_log_->file());
+            SyncManifest(db_options_, write_options, raw_desc_log_ptr->file());
         manifest_io_status = io_s;
         TEST_SYNC_POINT_CALLBACK(
             "VersionSet::ProcessManifestWrites:AfterSyncManifest", &io_s);
@@ -5650,7 +5652,7 @@ Status VersionSet::ProcessManifestWrites(
 
     if (s.ok()) {
       // find offset in manifest file where this version is stored.
-      new_manifest_file_size = descriptor_log_->file()->GetFileSize();
+      new_manifest_file_size = raw_desc_log_ptr->file()->GetFileSize();
     }
 
     if (first_writer.edit_list.front()->IsColumnFamilyDrop()) {
@@ -5696,6 +5698,7 @@ 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_));
   }
@@ -5773,6 +5776,7 @@ 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.
@@ -7078,8 +7082,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<TruncatedRangeDelIterator*, TruncatedRangeDelIterator***>>
+  std::vector<std::pair<std::unique_ptr<TruncatedRangeDelIterator>,
+                        std::unique_ptr<TruncatedRangeDelIterator>**>>
       range_tombstones;
   size_t num = 0;
   for (size_t which = 0; which < c->num_input_levels(); which++) {
@@ -7101,7 +7105,8 @@ InternalIterator* VersionSet::MakeInputIterator(
                   *end, fmd.smallest.user_key()) < 0) {
             continue;
           }
-          TruncatedRangeDelIterator* range_tombstone_iter = nullptr;
+          std::unique_ptr<TruncatedRangeDelIterator> range_tombstone_iter =
+              nullptr;
           list[num++] = cfd->table_cache()->NewIterator(
               read_options, file_options_compactions,
               cfd->internal_comparator(), fmd, range_del_agg,
@@ -7118,11 +7123,13 @@ 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(range_tombstone_iter, nullptr);
+          range_tombstones.emplace_back(std::move(range_tombstone_iter),
+                                        nullptr);
         }
       } else {
         // Create concatenating iterator for the files from this level
-        TruncatedRangeDelIterator*** tombstone_iter_ptr = nullptr;
+        std::unique_ptr<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,16 +43,17 @@ Status WalManager::DeleteFile(const std::string& fname, uint64_t number) {
   }
   return s;
 }
-
-Status WalManager::GetSortedWalFiles(VectorWalPtr& files) {
+Status WalManager::GetSortedWalFiles(VectorWalPtr& files, bool need_seqnos,
+                                     bool include_archived) {
   // 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);
-  if (!s.ok()) {
+  s = GetSortedWalsOfType(wal_dir_, logs, kAliveLogFile, need_seqnos);
+
+  if (!include_archived || !s.ok()) {
     return s;
   }
 
@@ -67,7 +68,7 @@ Status WalManager::GetSortedWalFiles(VectorWalPtr& files) {
   std::string archivedir = ArchivalDirectory(wal_dir_);
   Status exists = env_->FileExists(archivedir);
   if (exists.ok()) {
-    s = GetSortedWalsOfType(archivedir, files, kArchivedLogFile);
+    s = GetSortedWalsOfType(archivedir, files, kArchivedLogFile, need_seqnos);
     if (!s.ok()) {
       return s;
     }
@@ -249,7 +250,8 @@ void WalManager::PurgeObsoleteWALFiles() {
 
   size_t files_del_num = log_files_num - files_keep_num;
   VectorWalPtr archived_logs;
-  s = GetSortedWalsOfType(archival_dir, archived_logs, kArchivedLogFile);
+  s = GetSortedWalsOfType(archival_dir, archived_logs, kArchivedLogFile,
+                          /*need_seqno=*/false);
   if (!s.ok()) {
     ROCKS_LOG_WARN(db_options_.info_log,
                    "Unable to get archived WALs from: %s: %s",
@@ -284,6 +286,9 @@ 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());
@@ -291,7 +296,7 @@ void WalManager::ArchiveWALFile(const std::string& fname, uint64_t number) {
 
 Status WalManager::GetSortedWalsOfType(const std::string& path,
                                        VectorWalPtr& log_files,
-                                       WalFileType log_type) {
+                                       WalFileType log_type, bool need_seqnos) {
   std::vector<std::string> all_files;
   const Status status = env_->GetChildren(path, &all_files);
   if (!status.ok()) {
@@ -303,13 +308,17 @@ Status WalManager::GetSortedWalsOfType(const std::string& path,
     FileType type;
     if (ParseFileName(f, &number, &type) && type == kWalFile) {
       SequenceNumber sequence;
-      Status s = ReadFirstRecord(log_type, number, &sequence);
-      if (!s.ok()) {
-        return s;
-      }
-      if (sequence == 0) {
-        // empty file
-        continue;
+      if (need_seqnos) {
+        Status s = ReadFirstRecord(log_type, number, &sequence);
+        if (!s.ok()) {
+          return s;
+        }
+        if (sequence == 0) {
+          // empty file
+          continue;
+        }
+      } else {
+        sequence = 0;
       }
 
       // Reproduce the race condition where a log file is moved
@@ -319,7 +328,7 @@ Status WalManager::GetSortedWalsOfType(const std::string& path,
       TEST_SYNC_POINT("WalManager::GetSortedWalsOfType:2");
 
       uint64_t size_bytes;
-      s = env_->GetFileSize(LogFileName(path, number), &size_bytes);
+      Status 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,7 +49,8 @@ class WalManager {
         wal_in_db_path_(db_options_.IsWalDirSameAsDBPath()),
         io_tracer_(io_tracer) {}
 
-  Status GetSortedWalFiles(VectorWalPtr& files);
+  Status GetSortedWalFiles(VectorWalPtr& files, bool need_seqnos = true,
+                           bool include_archived = true);
 
   // Allow user to tail transaction log to find all recent changes to the
   // database that are newer than `seq_number`.
@@ -78,7 +79,7 @@ class WalManager {
 
  private:
   Status GetSortedWalsOfType(const std::string& path, VectorWalPtr& log_files,
-                             WalFileType type);
+                             WalFileType type, bool need_seqnos);
   // 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,16 +73,13 @@ class CfConsistencyStressTest : public StressTest {
       status = db_->Write(write_opts, &batch);
     }
 
-    if (IsRetryableInjectedError(status)) {
-      fprintf(stdout, "multi put or merge error: %s\n",
-              status.ToString().c_str());
-    } else if (!status.ok()) {
+    if (status.ok()) {
+      auto num = static_cast<long>(rand_column_families.size());
+      thread->stats.AddBytesForWrites(num, (sz + 1) * num);
+    } else if (!IsErrorInjectedAndRetryable(status)) {
       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;
@@ -99,13 +96,11 @@ class CfConsistencyStressTest : public StressTest {
       batch.Delete(cfh, key);
     }
     Status s = db_->Write(write_opts, &batch);
-    if (IsRetryableInjectedError(s)) {
-      fprintf(stdout, "multidel error: %s\n", s.ToString().c_str());
-    } else if (!s.ok()) {
+    if (s.ok()) {
+      thread->stats.AddDeletes(static_cast<long>(rand_column_families.size()));
+    } else if (!IsErrorInjectedAndRetryable(s)) {
       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;
   }
@@ -130,14 +125,12 @@ class CfConsistencyStressTest : public StressTest {
       batch.DeleteRange(cfh, key, end_key);
     }
     Status s = db_->Write(write_opts, &batch);
-    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 {
+    if (s.ok()) {
       thread->stats.AddRangeDeletions(
           static_cast<long>(rand_column_families.size()));
+    } else if (!IsErrorInjectedAndRetryable(s)) {
+      fprintf(stderr, "multi del range error: %s\n", s.ToString().c_str());
+      thread->stats.AddErrors(1);
     }
     return s;
   }
@@ -248,9 +241,7 @@ class CfConsistencyStressTest : public StressTest {
       thread->stats.AddGets(1, 1);
     } else if (s.IsNotFound()) {
       thread->stats.AddGets(1, 0);
-    } else if (IsRetryableInjectedError(s)) {
-      fprintf(stdout, "TestGet error: %s\n", s.ToString().c_str());
-    } else {
+    } else if (!IsErrorInjectedAndRetryable(s)) {
       fprintf(stderr, "TestGet error: %s\n", s.ToString().c_str());
       thread->stats.AddErrors(1);
     }
@@ -286,9 +277,7 @@ class CfConsistencyStressTest : public StressTest {
       } else if (s.IsNotFound()) {
         // not found case
         thread->stats.AddGets(1, 0);
-      } else if (IsRetryableInjectedError(s)) {
-        fprintf(stdout, "MultiGet error: %s\n", s.ToString().c_str());
-      } else {
+      } else if (!IsErrorInjectedAndRetryable(s)) {
         // errors case
         fprintf(stderr, "MultiGet error: %s\n", s.ToString().c_str());
         thread->stats.AddErrors(1);
@@ -324,12 +313,7 @@ class CfConsistencyStressTest : public StressTest {
       PinnableWideColumns result;
       s = db_->GetEntity(read_opts, cfh, key, &result);
 
-      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 (s.ok()) {
         if (!VerifyWideColumns(result.columns())) {
           fprintf(
               stderr,
@@ -359,10 +343,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();
 
@@ -517,9 +501,7 @@ class CfConsistencyStressTest : public StressTest {
       thread->stats.AddGets(1, 1);
     } else if (s.IsNotFound()) {
       thread->stats.AddGets(1, 0);
-    } else if (IsRetryableInjectedError(s)) {
-      fprintf(stdout, "TestGetEntity error: %s\n", s.ToString().c_str());
-    } else {
+    } else if (!IsErrorInjectedAndRetryable(s)) {
       fprintf(stderr, "TestGetEntity error: %s\n", s.ToString().c_str());
       thread->stats.AddErrors(1);
     }
@@ -590,9 +572,7 @@ 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 (IsRetryableInjectedError(s)) {
-            fprintf(stdout, "TestMultiGetEntity (AttributeGroup) error: %s\n",
-                    s.ToString().c_str());
+          if (!s.ok() && IsErrorInjectedAndRetryable(s)) {
             break;
           } else if (!s.ok() && !s.IsNotFound()) {
             fprintf(stderr, "TestMultiGetEntity (AttributeGroup) error: %s\n",
@@ -699,9 +679,7 @@ class CfConsistencyStressTest : public StressTest {
           const Status& s = statuses[j];
           const WideColumns& columns = results[j].columns();
 
-          if (IsRetryableInjectedError(s)) {
-            fprintf(stdout, "TestMultiGetEntity error: %s\n",
-                    s.ToString().c_str());
+          if (!s.ok() && IsErrorInjectedAndRetryable(s)) {
             break;
           } else if (!s.ok() && !s.IsNotFound()) {
             fprintf(stderr, "TestMultiGetEntity error: %s\n",
@@ -838,9 +816,7 @@ class CfConsistencyStressTest : public StressTest {
       s = iter->status();
     }
 
-    if (IsRetryableInjectedError(s)) {
-      fprintf(stdout, "TestPrefixScan error: %s\n", s.ToString().c_str());
-    } else if (!s.ok()) {
+    if (!s.ok() && !IsErrorInjectedAndRetryable(s)) {
       fprintf(stderr, "TestPrefixScan error: %s\n", s.ToString().c_str());
       thread->stats.AddErrors(1);
 

db_stress_tool/db_stress_common.cc

@@ -19,8 +19,6 @@
 
 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,6 +13,20 @@
 #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(
@@ -83,144 +97,67 @@ class DbStressWritableFileWrapper : public FSWritableFileOwnerWrapper {
 
   IOStatus Append(const Slice& data, const IOOptions& options,
                   IODebugContext* dbg) override {
-#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
+    CheckIOActivity(options);
     return target()->Append(data, options, dbg);
   }
   IOStatus Append(const Slice& data, const IOOptions& options,
                   const DataVerificationInfo& verification_info,
                   IODebugContext* dbg) override {
-#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
+    CheckIOActivity(options);
     return target()->Append(data, options, verification_info, dbg);
   }
   IOStatus PositionedAppend(const Slice& data, uint64_t offset,
                             const IOOptions& options,
                             IODebugContext* dbg) override {
-#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
+    CheckIOActivity(options);
     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 {
-#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
+    CheckIOActivity(options);
     return target()->PositionedAppend(data, offset, options, verification_info,
                                       dbg);
   }
 
   IOStatus Truncate(uint64_t size, const IOOptions& options,
                     IODebugContext* dbg) override {
-#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
+    CheckIOActivity(options);
     return target()->Truncate(size, options, dbg);
   }
 
   IOStatus Close(const IOOptions& options, IODebugContext* dbg) override {
-#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
+    CheckIOActivity(options);
     return target()->Close(options, dbg);
   }
 
   IOStatus Flush(const IOOptions& options, IODebugContext* dbg) override {
-#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
+    CheckIOActivity(options);
     return target()->Flush(options, dbg);
   }
 
   IOStatus Sync(const IOOptions& options, IODebugContext* dbg) override {
-#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
+    CheckIOActivity(options);
     return target()->Sync(options, dbg);
   }
 
   IOStatus Fsync(const IOOptions& options, IODebugContext* dbg) override {
-#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
+    CheckIOActivity(options);
     return target()->Fsync(options, dbg);
   }
 
 #ifdef ROCKSDB_FALLOCATE_PRESENT
   IOStatus Allocate(uint64_t offset, uint64_t len, const IOOptions& options,
                     IODebugContext* dbg) override {
-#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
+    CheckIOActivity(options);
     return target()->Allocate(offset, len, options, dbg);
   }
 #endif
 
   IOStatus RangeSync(uint64_t offset, uint64_t nbytes, const IOOptions& options,
                      IODebugContext* dbg) override {
-#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
+    CheckIOActivity(options);
     return target()->RangeSync(offset, nbytes, options, dbg);
   }
 };

db_stress_tool/db_stress_gflags.cc

@@ -1095,6 +1095,9 @@ 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,12 +284,13 @@ 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->SetIOActivtiesExemptedFromFaultInjection(
+      fault_fs_guard->SetIOActivtiesExcludedFromFaultInjection(
           {Env::IOActivity::kFlush});
     }
   }
 
-  void OnErrorRecoveryCompleted(Status /* old_bg_error */) override {
+  void OnErrorRecoveryEnd(
+      const BackgroundErrorRecoveryInfo& /*info*/) override {
     RandomSleep();
     if (FLAGS_error_recovery_with_no_fault_injection && fault_fs_guard) {
       fault_fs_guard->EnableThreadLocalErrorInjection(
@@ -300,7 +301,7 @@ class DbStressListener : public EventListener {
           FaultInjectionIOType::kMetadataRead);
       fault_fs_guard->EnableThreadLocalErrorInjection(
           FaultInjectionIOType::kMetadataWrite);
-      fault_fs_guard->SetIOActivtiesExemptedFromFaultInjection({});
+      fault_fs_guard->SetIOActivtiesExcludedFromFaultInjection({});
     }
   }
 

db_stress_tool/db_stress_shared_state.h

@@ -37,6 +37,7 @@ 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,12 +103,16 @@ StressTest::StressTest()
   }
 }
 
-StressTest::~StressTest() {
+void StressTest::CleanUp() {
   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;
@@ -286,19 +290,12 @@ 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),
@@ -343,6 +340,35 @@ 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",
@@ -436,7 +462,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 (MightHaveDataLoss() && IsStateTracked()) {
+  if (MightHaveUnsyncedDataLoss() && IsStateTracked()) {
     Status s = shared->SaveAtAndAfter(db_);
     if (!s.ok()) {
       fprintf(stderr, "Error enabling history tracing: %s\n",
@@ -516,9 +542,7 @@ void StressTest::ProcessStatus(SharedState* shared, std::string opname,
   if (s.ok()) {
     return;
   }
-  if (ignore_injected_error && IsRetryableInjectedError(s)) {
-    fprintf(stdout, "%s failed: %s\n", opname.c_str(), s.ToString().c_str());
-  } else {
+  if (!ignore_injected_error || !IsErrorInjectedAndRetryable(s)) {
     std::ostringstream oss;
     oss << opname << " failed: " << s.ToString();
     VerificationAbort(shared, oss.str());
@@ -1018,10 +1042,8 @@ 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 (IsRetryableInjectedError(s)) {
-          fprintf(stdout, "FlushWAL(sync=%s) failed: %s\n",
-                  (sync ? "true" : "false"), s.ToString().c_str());
-        } else if (!s.ok() && !(sync && s.IsNotSupported())) {
+        if (!s.ok() && !IsErrorInjectedAndRetryable(s) &&
+            !(sync && s.IsNotSupported())) {
           fprintf(stderr, "FlushWAL(sync=%s) failed: %s\n",
                   (sync ? "true" : "false"), s.ToString().c_str());
         }
@@ -1029,17 +1051,19 @@ void StressTest::OperateDb(ThreadState* thread) {
 
       if (thread->rand.OneInOpt(FLAGS_lock_wal_one_in)) {
         Status s = db_->LockWAL();
-        if (IsRetryableInjectedError(s)) {
-          fprintf(stdout, "LockWAL() failed: %s\n", s.ToString().c_str());
-        } else if (!s.ok()) {
+        if (!s.ok() && !IsErrorInjectedAndRetryable(s)) {
           fprintf(stderr, "LockWAL() failed: %s\n", s.ToString().c_str());
-        } else {
+        } else if (s.ok()) {
           //  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
@@ -1098,13 +1122,17 @@ 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() && !IsRetryableInjectedError(s)) {
+        if (!s.ok() && !s.IsNotSupported() && !IsErrorInjectedAndRetryable(s)) {
           fprintf(stderr, "SyncWAL() failed: %s\n", s.ToString().c_str());
         }
       }
@@ -1144,8 +1172,12 @@ void StressTest::OperateDb(ThreadState* thread) {
         ProcessStatus(shared, "GetLiveFiles", s_1);
         Status s_2 = TestGetLiveFilesMetaData();
         ProcessStatus(shared, "GetLiveFilesMetaData", s_2);
-        Status s_3 = TestGetLiveFilesStorageInfo();
-        ProcessStatus(shared, "GetLiveFilesStorageInfo", s_3);
+        // 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);
+        }
       }
 
       if (thread->rand.OneInOpt(FLAGS_get_all_column_family_metadata_one_in)) {
@@ -1202,7 +1234,32 @@ 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)) {
@@ -1229,17 +1286,29 @@ void StressTest::OperateDb(ThreadState* thread) {
         }
 
         if (total_size <= FLAGS_backup_max_size) {
-          // TODO(hx235): enable metadata error injection with
+          // TODO(hx235): enable error injection with
           // backup/restore after fixing the various issues it surfaces
-          fault_fs_guard->DisableThreadLocalErrorInjection(
-              FaultInjectionIOType::kMetadataRead);
-          fault_fs_guard->DisableThreadLocalErrorInjection(
-              FaultInjectionIOType::kMetadataWrite);
+          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);
+          }
           Status s = TestBackupRestore(thread, rand_column_families, rand_keys);
-          fault_fs_guard->EnableThreadLocalErrorInjection(
-              FaultInjectionIOType::kMetadataWrite);
-          fault_fs_guard->EnableThreadLocalErrorInjection(
-              FaultInjectionIOType::kMetadataRead);
+          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);
+          }
           ProcessStatus(shared, "Backup/restore", s);
         }
       }
@@ -1674,7 +1743,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() && IsRetryableInjectedError(s)) {
+      if (!s.ok() && IsErrorInjectedAndRetryable(s)) {
         return s;
       }
       assert(s.ok());
@@ -1704,10 +1773,13 @@ Status StressTest::TestIterateImpl(ThreadState* thread,
       op_logs += "S " + key.ToString(true) + " ";
     }
 
-    if (IsRetryableInjectedError(iter->status()) ||
-        IsRetryableInjectedError(cmp_iter->status())) {
-      return !iter->status().ok() ? iter->status() : cmp_iter->status();
+    if (!iter->status().ok() && IsErrorInjectedAndRetryable(iter->status())) {
+      return iter->status();
+    } else if (!cmp_iter->status().ok() &&
+               IsErrorInjectedAndRetryable(cmp_iter->status())) {
+      return cmp_iter->status();
     }
+
     VerifyIterator(thread, cmp_cfh, ro, iter.get(), cmp_iter.get(), last_op,
                    key, op_logs, verify_func, &diverged);
 
@@ -1732,10 +1804,13 @@ Status StressTest::TestIterateImpl(ThreadState* thread,
 
       last_op = kLastOpNextOrPrev;
 
-      if (IsRetryableInjectedError(iter->status()) ||
-          IsRetryableInjectedError(cmp_iter->status())) {
-        return !iter->status().ok() ? iter->status() : cmp_iter->status();
+      if (!iter->status().ok() && IsErrorInjectedAndRetryable(iter->status())) {
+        return iter->status();
+      } else if (!cmp_iter->status().ok() &&
+                 IsErrorInjectedAndRetryable(cmp_iter->status())) {
+        return cmp_iter->status();
       }
+
       VerifyIterator(thread, cmp_cfh, ro, iter.get(), cmp_iter.get(), last_op,
                      key, op_logs, verify_func, &diverged);
     }
@@ -2279,7 +2354,7 @@ Status StressTest::TestBackupRestore(
         FaultInjectionIOType::kMetadataWrite);
   }
 
-  if (s.ok() || IsRetryableInjectedError(s)) {
+  if (s.ok() || IsErrorInjectedAndRetryable(s)) {
     // Preserve directories on failure, or allowed persistent backup
     if (!allow_persistent) {
       s = DestroyDir(db_stress_env, backup_dir);
@@ -2289,7 +2364,7 @@ Status StressTest::TestBackupRestore(
     }
   }
 
-  if (s.ok() || IsRetryableInjectedError(s)) {
+  if (s.ok() || IsErrorInjectedAndRetryable(s)) {
     s = DestroyDir(db_stress_env, restore_dir);
     if (!s.ok()) {
       from = "Destroy restore dir";
@@ -2308,7 +2383,7 @@ Status StressTest::TestBackupRestore(
         FaultInjectionIOType::kMetadataWrite);
   }
 
-  if (!s.ok() && !IsRetryableInjectedError(s)) {
+  if (!s.ok() && !IsErrorInjectedAndRetryable(s)) {
     fprintf(stderr,
             "Failure in %s with: %s under specified BackupEngineOptions: %s, "
             "CreateBackupOptions: %s, RestoreOptions: %s (Empty string or "
@@ -2470,23 +2545,32 @@ Status StressTest::TestCheckpoint(ThreadState* thread,
   Status s = Checkpoint::Create(db_, &checkpoint);
   if (s.ok()) {
     s = checkpoint->CreateCheckpoint(checkpoint_dir);
-    if (!s.ok() && !IsRetryableInjectedError(s)) {
+    if (!s.ok() && !IsErrorInjectedAndRetryable(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
-      fault_fs_guard->DisableThreadLocalErrorInjection(
-          FaultInjectionIOType::kMetadataRead);
+      if (fault_fs_guard) {
+        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
-      fault_fs_guard->EnableThreadLocalErrorInjection(
-          FaultInjectionIOType::kMetadataRead);
-      assert(my_s.ok());
-
-      for (const auto& f : files) {
-        fprintf(stderr, " %s\n", f.c_str());
+      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());
+        }
       }
     }
   }
@@ -2573,7 +2657,7 @@ Status StressTest::TestCheckpoint(ThreadState* thread,
         FaultInjectionIOType::kMetadataWrite);
   }
 
-  if (!s.ok() && !IsRetryableInjectedError(s)) {
+  if (!s.ok() && !IsErrorInjectedAndRetryable(s)) {
     fprintf(stderr, "A checkpoint operation failed with: %s\n",
             s.ToString().c_str());
   } else {
@@ -2724,18 +2808,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() || (IsRetryableInjectedError(s)) ||
+            s.IsManualCompactionPaused() || IsErrorInjectedAndRetryable(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 {
@@ -2759,11 +2843,12 @@ 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();
     }
   }
@@ -2852,10 +2937,8 @@ 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 (IsRetryableInjectedError(status_at)) {
-    fprintf(stdout,
-            "Unable to perform Get() during TestAcquireSnapshot(): %s)\n",
-            status_at.ToString().c_str());
+  if (!status_at.ok() && IsErrorInjectedAndRetryable(status_at)) {
+    db_->ReleaseSnapshot(snapshot);
     return;
   }
   std::vector<bool>* key_vec = nullptr;
@@ -2964,19 +3047,33 @@ 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
-    fault_fs_guard->EnableThreadLocalErrorInjection(
-        FaultInjectionIOType::kMetadataRead);
-    fault_fs_guard->EnableThreadLocalErrorInjection(
-        FaultInjectionIOType::kRead);
+    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);
+    }
   }
   std::ostringstream compact_range_opt_oss;
   compact_range_opt_oss << "exclusive_manual_compaction: "
@@ -2996,15 +3093,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() || 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());
+        status.IsManualCompactionPaused() ||
+        IsErrorInjectedAndRetryable(status) || status.IsAborted() ||
+        status.IsInvalidArgument() || status.IsNotSupported();
     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();
     }
@@ -3012,11 +3109,16 @@ void StressTest::TestCompactRange(ThreadState* thread, int64_t rand_key,
 
   if (pre_snapshot != nullptr) {
     // Temporarily disable error injection for validation
-    // Declaring a snapshot and compare the data before and after the compaction
-    fault_fs_guard->DisableThreadLocalErrorInjection(
-        FaultInjectionIOType::kMetadataRead);
-    fault_fs_guard->DisableThreadLocalErrorInjection(
-        FaultInjectionIOType::kRead);
+    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);
+    }
     uint32_t post_hash =
         GetRangeHash(thread, pre_snapshot, column_family, start_key, end_key);
     if (pre_hash != post_hash) {
@@ -3032,11 +3134,17 @@ void StressTest::TestCompactRange(ThreadState* thread, int64_t rand_key,
       thread->shared->SetVerificationFailure();
     }
     db_->ReleaseSnapshot(pre_snapshot);
-    // Enable back error injection disabled for validation
-    fault_fs_guard->EnableThreadLocalErrorInjection(
-        FaultInjectionIOType::kMetadataRead);
-    fault_fs_guard->EnableThreadLocalErrorInjection(
-        FaultInjectionIOType::kRead);
+    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);
+    }
   }
 }
 
@@ -3426,17 +3534,23 @@ 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_open_meta_read_error || inject_open_meta_write_error ||
-           inject_open_read_error || inject_open_write_error) &&
+      if ((inject_sync_fault || 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),
@@ -3460,7 +3574,6 @@ 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 */,
@@ -3495,11 +3608,11 @@ void StressTest::Open(SharedState* shared, bool reopen) {
           }
         }
 
-        if (inject_open_meta_read_error || inject_open_meta_write_error ||
-            inject_open_read_error || inject_open_write_error) {
+        if (inject_sync_fault || 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(
@@ -3522,9 +3635,10 @@ void StressTest::Open(SharedState* shared, bool reopen) {
             }
           }
           if (!s.ok()) {
-            // 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.
+            // 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;
             inject_open_meta_read_error = false;
             inject_open_meta_write_error = false;
             inject_open_read_error = false;
@@ -3684,7 +3798,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) {
+  if (!FLAGS_disable_wal && !FLAGS_avoid_flush_during_shutdown) {
     Status s;
     if (FLAGS_manual_wal_flush_one_in > 0) {
       s = db_->FlushWAL(/*sync=*/true);
@@ -3721,7 +3835,7 @@ void StressTest::Reopen(ThreadState* thread) {
           clock_->TimeToString(now / 1000000).c_str(), num_times_reopened_);
   Open(thread->shared, /*reopen=*/true);
 
-  if (thread->shared->GetStressTest()->MightHaveDataLoss() &&
+  if (thread->shared->GetStressTest()->MightHaveUnsyncedDataLoss() &&
       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,12 +44,13 @@ class StressTest {
   virtual void VerifyDb(ThreadState* thread) const = 0;
   virtual void ContinuouslyVerifyDb(ThreadState* /*thread*/) const = 0;
   void PrintStatistics();
-  bool MightHaveDataLoss() {
-    return FLAGS_sync_fault_injection || FLAGS_write_fault_one_in > 0 ||
-           FLAGS_metadata_write_fault_one_in > 0 || FLAGS_disable_wal ||
+  bool MightHaveUnsyncedDataLoss() {
+    return FLAGS_sync_fault_injection || 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) {
@@ -280,9 +281,10 @@ class StressTest {
     return Status::NotSupported("TestCustomOperations() must be overridden");
   }
 
-  bool IsRetryableInjectedError(const Status& s) const {
-    return s.getState() && std::strstr(s.getState(), "inject") &&
-           !status_to_io_status(Status(s)).GetDataLoss();
+  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();
   }
 
   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 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.
+    // 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.
     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()));
-  if (RunStressTest(shared.get())) {
-    return 0;
-  } else {
-    return 1;
-  }
+  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;
 }
 
 }  // namespace ROCKSDB_NAMESPACE

db_stress_tool/no_batched_ops_stress.cc

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

env/composite_env_wrapper.h

@@ -18,6 +18,13 @@
 
 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
@@ -250,6 +257,20 @@ 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,6 +355,12 @@ 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,13 +233,17 @@ class WritableFileWriter {
   WritableFileWriter& operator=(const WritableFileWriter&) = delete;
 
   ~WritableFileWriter() {
-    ThreadStatus::OperationType cur_op_type =
+    IOOptions io_options;
+#ifndef NDEBUG
+    // This is needed to pass the IOActivity related checks in stress test.
+    // See DbStressWritableFileWrapper.
+    ThreadStatus::OperationType op_type =
         ThreadStatusUtil::GetThreadOperation();
-    ThreadStatusUtil::SetThreadOperation(
-        ThreadStatus::OperationType::OP_UNKNOWN);
-    auto s = Close(IOOptions());
+    io_options.io_activity =
+        ThreadStatusUtil::TEST_GetExpectedIOActivity(op_type);
+#endif
+    auto s = Close(io_options);
     s.PermitUncheckedError();
-    ThreadStatusUtil::SetThreadOperation(cur_op_type);
   }
 
   std::string file_name() const { return file_name_; }
@@ -304,6 +308,9 @@ 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,6 +650,19 @@ 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,
@@ -850,6 +863,13 @@ 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(
@@ -1205,6 +1225,11 @@ 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,9 +1334,10 @@ 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), fills
-  // "*value" with its current value and returns true.  Otherwise, returns
-  // false.
+  // 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.
   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,6 +145,11 @@ 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,9 +645,7 @@ 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. 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)
+  // e.g. to read/write files, schedule background work, etc.
   // Default: Env::Default()
   Env* env = Env::Default();
 
@@ -2181,6 +2179,24 @@ 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,6 +529,11 @@ 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,8 +32,10 @@ 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
-  // 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 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
   // 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 4
-#define ROCKSDB_PATCH 1
+#define ROCKSDB_MINOR 5
+#define ROCKSDB_PATCH 2
 
 // 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,6 +5269,10 @@ 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
@@ -5726,6 +5730,11 @@ 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,000ms should work for most cases.
+   *     overhead. The default of 100,000us 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,000ms should work for most cases.
+   *     overhead. The default of 100,000us 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,000ms should work for most cases.
+   *     overhead. The default of 100,000us 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,000ms should work for most cases.
+   *     overhead. The default of 100,000us 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,6 +878,10 @@ 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,6 +266,10 @@ 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,17 +98,20 @@ 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 `throw()` while clang's
+// glibc's `posix_memalign()` declaration specifies `noexcept` 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 `throw()` when built with clang.
+// declaration did not specify `noexcept` when built with clang.
 #include <mm_malloc.h>
 #endif
 

port/win/port_win.cc

@@ -101,8 +101,9 @@ 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
-#if defined(_MSC_VER) && \
-    (!defined(_MSVC_STL_UPDATE) || _MSVC_STL_UPDATE < 202008L)
+  // 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 (relTimeUs == std::chrono::microseconds::zero()) {
     lk.unlock();
     lk.lock();

table/block_based/block_based_table_iterator.cc

@@ -823,6 +823,12 @@ 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;
     }
 
@@ -852,6 +858,15 @@ 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,6 +478,7 @@ 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) {
@@ -500,12 +501,17 @@ Status GetGlobalSequenceNumber(const TableProperties& table_properties,
   }
 
   uint32_t version = DecodeFixed32(version_pos->second.c_str());
-  if (version < 2) {
-    if (seqno_pos != props.end() || version != 1) {
-      std::array<char, 200> msg_buf;
+  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()) {
       // 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 have global seqno "
+               "An external sst file with version %u has global seqno "
                "property with value %s",
                version, seqno_pos->second.c_str());
       return Status::Corruption(msg_buf.data());
@@ -687,6 +693,10 @@ 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,13 +223,16 @@ 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, BLOCK_CHECKSUM_MISMATCH_COUNT);
+          RecordTick(ioptions.stats, FILE_READ_CORRUPTION_RETRY_COUNT);
 
           // Repeat the read for this particular block using the regular
           // synchronous Read API. We can use the same chunk of memory
@@ -246,6 +249,10 @@ 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,7 +20,8 @@ namespace ROCKSDB_NAMESPACE {
 FullFilterBlockBuilder::FullFilterBlockBuilder(
     const SliceTransform* _prefix_extractor, bool whole_key_filtering,
     FilterBitsBuilder* filter_bits_builder)
-    : prefix_extractor_(_prefix_extractor),
+    : need_last_prefix_(whole_key_filtering && _prefix_extractor != nullptr),
+      prefix_extractor_(_prefix_extractor),
       whole_key_filtering_(whole_key_filtering),
       last_whole_key_recorded_(false),
       last_prefix_recorded_(false),
@@ -38,7 +39,7 @@ void FullFilterBlockBuilder::Add(const Slice& key_without_ts) {
   const bool add_prefix =
       prefix_extractor_ && prefix_extractor_->InDomain(key_without_ts);
 
-  if (!last_prefix_recorded_ && last_key_in_domain_) {
+  if (need_last_prefix_ && !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.
@@ -82,7 +83,15 @@ 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 (whole_key_filtering_) {
+  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 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,6 +69,7 @@ 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,6 +45,9 @@ 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) {
@@ -86,7 +89,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,7 +84,9 @@ 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() && retry_corrupt_read_)) {
+        } 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.
           return true;
         }
       }
@@ -334,9 +336,15 @@ 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();
@@ -355,7 +363,11 @@ 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<TruncatedRangeDelIterator*, TruncatedRangeDelIterator***>>
+      std::vector<std::pair<std::unique_ptr<TruncatedRangeDelIterator>,
+                            std::unique_ptr<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(p.first);
+      range_tombstone_iters_.push_back(std::move(p.first));
     }
     pinned_heap_item_.resize(n);
     for (int i = 0; i < n; ++i) {
@@ -47,10 +47,7 @@ class CompactionMergingIterator : public InternalIterator {
   }
 
   ~CompactionMergingIterator() override {
-    // TODO: use unique_ptr for range_tombstone_iters_
-    for (auto child : range_tombstone_iters_) {
-      delete child;
-    }
+    range_tombstone_iters_.clear();
 
     for (auto& child : children_) {
       child.iter.DeleteIter(is_arena_mode_);
@@ -197,7 +194,8 @@ 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<TruncatedRangeDelIterator*> range_tombstone_iters_;
+  std::vector<std::unique_ptr<TruncatedRangeDelIterator>>
+      range_tombstone_iters_;
   // Used as value for range tombstone keys
   std::string dummy_tombstone_val{};
 
@@ -349,8 +347,9 @@ void CompactionMergingIterator::AddToMinHeapOrCheckStatus(HeapItem* child) {
 
 InternalIterator* NewCompactionMergingIterator(
     const InternalKeyComparator* comparator, InternalIterator** children, int n,
-    std::vector<std::pair<TruncatedRangeDelIterator*,
-                          TruncatedRangeDelIterator***>>& range_tombstone_iters,
+    std::vector<std::pair<std::unique_ptr<TruncatedRangeDelIterator>,
+                          std::unique_ptr<TruncatedRangeDelIterator>**>>&
+        range_tombstone_iters,
     Arena* arena) {
   assert(n >= 0);
   if (n == 0) {

table/compaction_merging_iterator.h

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

table/merging_iterator.cc

@@ -96,8 +96,9 @@ 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(TruncatedRangeDelIterator* iter) {
-    range_tombstone_iters_.emplace_back(iter);
+  void AddRangeTombstoneIterator(
+      std::unique_ptr<TruncatedRangeDelIterator>&& iter) {
+    range_tombstone_iters_.emplace_back(std::move(iter));
   }
 
   // Called by MergingIteratorBuilder when all point iterators and range
@@ -125,9 +126,7 @@ class MergingIterator : public InternalIterator {
   }
 
   ~MergingIterator() override {
-    for (auto child : range_tombstone_iters_) {
-      delete child;
-    }
+    range_tombstone_iters_.clear();
 
     for (auto& child : children_) {
       child.iter.DeleteIter(is_arena_mode_);
@@ -624,7 +623,8 @@ 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<TruncatedRangeDelIterator*> range_tombstone_iters_;
+  std::vector<std::unique_ptr<TruncatedRangeDelIterator>>
+      range_tombstone_iters_;
 
   // Levels (indices into range_tombstone_iters_/children_ ) that currently have
   // "active" range tombstones. See comments above MergingIterator for meaning
@@ -841,7 +841,8 @@ void MergingIterator::SeekImpl(const Slice& target, size_t starting_level,
         prefetched_target.emplace_back(
             level, current_search_key.GetInternalKey().ToString());
       }
-      auto range_tombstone_iter = range_tombstone_iters_[level];
+      UnownedPtr<TruncatedRangeDelIterator> range_tombstone_iter =
+          range_tombstone_iters_[level].get();
       if (range_tombstone_iter) {
         range_tombstone_iter->SeekInternalKey(
             current_search_key.GetInternalKey());
@@ -1125,7 +1126,8 @@ void MergingIterator::SeekForPrevImpl(const Slice& target,
         prefetched_target.emplace_back(
             level, current_search_key.GetInternalKey().ToString());
       }
-      auto range_tombstone_iter = range_tombstone_iters_[level];
+      UnownedPtr<TruncatedRangeDelIterator> range_tombstone_iter =
+          range_tombstone_iters_[level].get();
       if (range_tombstone_iter) {
         range_tombstone_iter->SeekForPrev(current_search_key.GetUserKey());
         if (range_tombstone_iter->Valid()) {
@@ -1349,7 +1351,8 @@ void MergingIterator::SwitchToForward() {
     ParseInternalKey(target, &pik, false /* log_err_key */)
         .PermitUncheckedError();
     for (size_t i = 0; i < range_tombstone_iters_.size(); ++i) {
-      auto iter = range_tombstone_iters_[i];
+      UnownedPtr<TruncatedRangeDelIterator> iter =
+          range_tombstone_iters_[i].get();
       if (iter) {
         iter->Seek(pik.user_key);
         // The while loop is needed as the Seek() call above is only for user
@@ -1395,7 +1398,8 @@ void MergingIterator::SwitchToBackward() {
   ParseInternalKey(target, &pik, false /* log_err_key */)
       .PermitUncheckedError();
   for (size_t i = 0; i < range_tombstone_iters_.size(); ++i) {
-    auto iter = range_tombstone_iters_[i];
+    UnownedPtr<TruncatedRangeDelIterator> iter =
+        range_tombstone_iters_[i].get();
     if (iter) {
       iter->SeekForPrev(pik.user_key);
       // Since the SeekForPrev() call above is only for user key,
@@ -1690,8 +1694,9 @@ void MergeIteratorBuilder::AddIterator(InternalIterator* iter) {
 }
 
 void MergeIteratorBuilder::AddPointAndTombstoneIterator(
-    InternalIterator* point_iter, TruncatedRangeDelIterator* tombstone_iter,
-    TruncatedRangeDelIterator*** tombstone_iter_ptr) {
+    InternalIterator* point_iter,
+    std::unique_ptr<TruncatedRangeDelIterator>&& tombstone_iter,
+    std::unique_ptr<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() ||
@@ -1711,7 +1716,7 @@ void MergeIteratorBuilder::AddPointAndTombstoneIterator(
              merge_iter->children_.size() - 1) {
         merge_iter->AddRangeTombstoneIterator(nullptr);
       }
-      merge_iter->AddRangeTombstoneIterator(tombstone_iter);
+      merge_iter->AddRangeTombstoneIterator(std::move(tombstone_iter));
     }
 
     if (tombstone_iter_ptr) {

table/merging_iterator.h

@@ -70,8 +70,10 @@ 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, TruncatedRangeDelIterator* tombstone_iter,
-      TruncatedRangeDelIterator*** tombstone_iter_ptr = nullptr);
+      InternalIterator* point_iter,
+      std::unique_ptr<TruncatedRangeDelIterator>&& tombstone_iter,
+      std::unique_ptr<TruncatedRangeDelIterator>** tombstone_iter_ptr =
+          nullptr);
 
   // Get arena used to build the merging iterator. It is called one a child
   // iterator needs to be allocated.
@@ -91,7 +93,7 @@ class MergeIteratorBuilder {
   Arena* arena;
   // Used to set LevelIterator.range_tombstone_iter_.
   // See AddRangeTombstoneIterator() implementation for more detail.
-  std::vector<std::pair<size_t, TruncatedRangeDelIterator***>>
+  std::vector<std::pair<size_t, std::unique_ptr<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")
+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")
 # 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,13 +48,15 @@ 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"]
     ),
@@ -65,13 +67,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,
@@ -147,10 +149,16 @@ 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),
@@ -160,7 +168,9 @@ 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,
@@ -173,13 +183,7 @@ 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]),
@@ -196,8 +200,7 @@ 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]
     ),
@@ -222,13 +225,14 @@ 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]
@@ -265,14 +269,16 @@ 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]),
@@ -304,24 +310,34 @@ 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
@@ -507,6 +523,8 @@ 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
@@ -520,6 +538,8 @@ 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 = {
@@ -527,7 +547,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 = {
@@ -623,10 +643,11 @@ multiops_txn_default_params = {
     "create_timestamped_snapshot_one_in": 50,
     "sync_fault_injection": 0,
     "metadata_write_fault_one_in": 0,
-    "manual_wal_flush": 0,
+    "manual_wal_flush_one_in": 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,
@@ -666,6 +687,7 @@ 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():
@@ -727,23 +749,19 @@ 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["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
+        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
     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
     ):
@@ -759,6 +777,14 @@ 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.
@@ -813,8 +839,6 @@ 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
@@ -822,10 +846,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
@@ -833,15 +857,21 @@ 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
@@ -851,10 +881,12 @@ 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
@@ -874,22 +906,36 @@ 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
@@ -905,11 +951,14 @@ 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 = {}
 
@@ -984,7 +1033,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
         ]
@@ -1022,6 +1071,7 @@ 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:
@@ -1031,6 +1081,7 @@ 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):
@@ -1222,7 +1273,9 @@ 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")
@@ -1257,7 +1310,6 @@ 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) {}
+      : Logger(log_level), log_prefix(nullptr), log_prefix_len(0) {}
   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)   { (XXH3_state_ptr)->seed = 0; }
+#define XXH3_INITSTATE(XXH3_state_ptr)   do { (XXH3_state_ptr)->seed = 0; } while (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 */, kFlushAll);
+           (i + 1) * kNumKeysPerBackup /* to */);
 
     // 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,6 +13,7 @@
 #ifndef OS_WIN
 #include <unistd.h>
 #endif
+#include <cstdlib>
 #include <iostream>
 #include <thread>
 #include <utility>
@@ -994,6 +995,43 @@ 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,9 +148,11 @@ TestFSWritableFile::TestFSWritableFile(const std::string& fname,
       file_opts_(file_opts),
       target_(std::move(f)),
       writable_file_opened_(true),
-      fs_(fs) {
+      fs_(fs),
+      unsync_data_loss_(fs_->InjectUnsyncedDataLoss()) {
   assert(target_ != nullptr);
-  state_.pos_at_last_append_ = 0;
+  assert(state_.pos_at_last_append_ == 0);
+  assert(state_.pos_at_last_sync_ == 0);
 }
 
 TestFSWritableFile::~TestFSWritableFile() {
@@ -166,13 +168,13 @@ IOStatus TestFSWritableFile::Append(const Slice& data, const IOOptions& options,
     return fs_->GetError();
   }
 
-  IOStatus s =
-      fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite, options);
+  IOStatus s = fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite,
+                                                options, state_.filename_);
   if (!s.ok()) {
     return s;
   }
 
-  if (target_->use_direct_io()) {
+  if (target_->use_direct_io() || !unsync_data_loss_) {
     // TODO(hx235): buffer data for direct IO write to simulate data loss like
     // non-direct IO write
     s = target_->Append(data, options, dbg);
@@ -201,8 +203,8 @@ IOStatus TestFSWritableFile::Append(
     return IOStatus::Corruption("Data is corrupted!");
   }
 
-  IOStatus s =
-      fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite, options);
+  IOStatus s = fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite,
+                                                options, state_.filename_);
   if (!s.ok()) {
     return s;
   }
@@ -220,7 +222,7 @@ IOStatus TestFSWritableFile::Append(
     return IOStatus::Corruption(msg);
   }
 
-  if (target_->use_direct_io()) {
+  if (target_->use_direct_io() || !unsync_data_loss_) {
     // TODO(hx235): buffer data for direct IO write to simulate data loss like
     // non-direct IO write
     s = target_->Append(data, options, dbg);
@@ -240,8 +242,8 @@ IOStatus TestFSWritableFile::Truncate(uint64_t size, const IOOptions& options,
   if (!fs_->IsFilesystemActive()) {
     return fs_->GetError();
   }
-  IOStatus s =
-      fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite, options);
+  IOStatus s = fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite,
+                                                options, state_.filename_);
   if (!s.ok()) {
     return s;
   }
@@ -264,8 +266,8 @@ IOStatus TestFSWritableFile::PositionedAppend(const Slice& data,
   if (fs_->ShouldDataCorruptionBeforeWrite()) {
     return IOStatus::Corruption("Data is corrupted!");
   }
-  IOStatus s =
-      fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite, options);
+  IOStatus s = fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite,
+                                                options, state_.filename_);
   if (!s.ok()) {
     return s;
   }
@@ -290,8 +292,8 @@ IOStatus TestFSWritableFile::PositionedAppend(
   if (fs_->ShouldDataCorruptionBeforeWrite()) {
     return IOStatus::Corruption("Data is corrupted!");
   }
-  IOStatus s =
-      fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite, options);
+  IOStatus s = fs_->MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite,
+                                                options, state_.filename_);
   if (!s.ok()) {
     return s;
   }
@@ -376,15 +378,13 @@ 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 < buf_begin) {
+  if (sync_limit < state_.pos_at_last_sync_) {
     return io_s;
   }
   uint64_t num_to_sync = std::min(static_cast<uint64_t>(state_.buffer_.size()),
-                                  sync_limit - buf_begin);
+                                  sync_limit - state_.pos_at_last_sync_);
   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,26 +485,35 @@ 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 s;
+  IOStatus res_status;
   FSReadRequest res;
+  IOStatus s;
   if (!fs_->IsFilesystemActive()) {
-    s = fs_->GetError();
+    res_status = fs_->GetError();
   }
-  if (s.ok()) {
-    s = fs_->MaybeInjectThreadLocalError(
-        FaultInjectionIOType::kRead, opts,
+  if (res_status.ok()) {
+    res_status = fs_->MaybeInjectThreadLocalError(
+        FaultInjectionIOType::kRead, opts, "",
         FaultInjectionTestFS::ErrorOperation::kRead, &res.result,
         use_direct_io(), req.scratch, /*need_count_increase=*/true,
         /*fault_injected=*/nullptr);
   }
-  if (s.ok()) {
+  if (res_status.ok()) {
     s = target_->ReadAsync(req, opts, cb, cb_arg, io_handle, del_fn, nullptr);
     // TODO (low priority): fs_->ReadUnsyncedData()
-  }
-  if (!s.ok()) {
-    res.status = s;
+  } 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;
     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;
 }
 
@@ -524,7 +533,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,
@@ -533,7 +542,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);
@@ -549,38 +558,44 @@ size_t TestFSRandomAccessFile::GetUniqueId(char* id, size_t max_size) const {
   }
 }
 
-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();
+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"
 
   MutexLock l(&mutex_);
   auto it = db_file_state_.find(fname);
   if (it != db_file_state_.end()) {
     auto& st = it->second;
-    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);
-      }
+    *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);
     }
   }
 }
@@ -589,20 +604,146 @@ 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;
   }
 
-  s = target()->Read(n, options, result, scratch, dbg);
-  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 = {};
   }
 
   if (fs_->ReadUnsyncedData() && result->size() < n) {
-    fs_->AddUnsyncedToRead(fname_, read_pos_, n, result, scratch);
+    // 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());
+    }
   }
   read_pos_ += result->size();
 
@@ -614,7 +755,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()) {
@@ -698,12 +839,12 @@ IOStatus FaultInjectionTestFS::NewWritableFile(
     return GetError();
   }
 
-  if (ShouldUseDiretWritable(fname)) {
+  if (IsFilesystemDirectWritable()) {
     return target()->NewWritableFile(fname, file_opts, result, dbg);
   }
 
-  IOStatus io_s = MaybeInjectThreadLocalError(
-      FaultInjectionIOType::kMetadataWrite, file_opts.io_options);
+  IOStatus io_s = MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite,
+                                              file_opts.io_options, fname);
   if (!io_s.ok()) {
     return io_s;
   }
@@ -735,11 +876,11 @@ IOStatus FaultInjectionTestFS::ReopenWritableFile(
   if (!IsFilesystemActive()) {
     return GetError();
   }
-  if (ShouldUseDiretWritable(fname)) {
+  if (IsFilesystemDirectWritable()) {
     return target()->ReopenWritableFile(fname, file_opts, result, dbg);
   }
-  IOStatus io_s = MaybeInjectThreadLocalError(
-      FaultInjectionIOType::kMetadataWrite, file_opts.io_options);
+  IOStatus io_s = MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite,
+                                              file_opts.io_options, fname);
   if (!io_s.ok()) {
     return io_s;
   }
@@ -811,11 +952,11 @@ IOStatus FaultInjectionTestFS::NewRandomRWFile(
   if (!IsFilesystemActive()) {
     return GetError();
   }
-  if (ShouldUseDiretWritable(fname)) {
+  if (IsFilesystemDirectWritable()) {
     return target()->NewRandomRWFile(fname, file_opts, result, dbg);
   }
-  IOStatus io_s = MaybeInjectThreadLocalError(
-      FaultInjectionIOType::kMetadataWrite, file_opts.io_options);
+  IOStatus io_s = MaybeInjectThreadLocalError(FaultInjectionIOType::kWrite,
+                                              file_opts.io_options, fname);
   if (!io_s.ok()) {
     return io_s;
   }
@@ -847,7 +988,10 @@ IOStatus FaultInjectionTestFS::NewRandomAccessFile(
     return GetError();
   }
   IOStatus io_s = MaybeInjectThreadLocalError(
-      FaultInjectionIOType::kMetadataWrite, file_opts.io_options);
+      FaultInjectionIOType::kRead, file_opts.io_options, fname,
+      ErrorOperation::kOpen, nullptr /* result */, false /* direct_io */,
+      nullptr /* scratch */, true /*need_count_increase*/,
+      nullptr /*fault_injected*/);
   if (!io_s.ok()) {
     return io_s;
   }
@@ -867,7 +1011,10 @@ IOStatus FaultInjectionTestFS::NewSequentialFile(
     return GetError();
   }
   IOStatus io_s = MaybeInjectThreadLocalError(
-      FaultInjectionIOType::kMetadataWrite, file_opts.io_options);
+      FaultInjectionIOType::kRead, file_opts.io_options, fname,
+      ErrorOperation::kOpen, nullptr /* result */, false /* direct_io */,
+      nullptr /* scratch */, true /*need_count_increase*/,
+      nullptr /*fault_injected*/);
   if (!io_s.ok()) {
     return io_s;
   }
@@ -985,7 +1132,6 @@ 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;
       }
     }
@@ -1228,8 +1374,7 @@ IOStatus FaultInjectionTestFS::MaybeInjectThreadLocalReadError(
   ErrorContext* ctx =
       static_cast<ErrorContext*>(injected_thread_local_read_error_.Get());
   if (ctx == nullptr || !ctx->enable_error_injection || !ctx->one_in ||
-      io_activties_exempted_from_fault_injection.find(io_options.io_activity) !=
-          io_activties_exempted_from_fault_injection.end()) {
+      ShouldIOActivtiesExcludedFromFaultInjection(io_options.io_activity)) {
     return IOStatus::OK();
   }
 
@@ -1295,8 +1440,9 @@ bool FaultInjectionTestFS::TryParseFileName(const std::string& file_name,
 }
 
 IOStatus FaultInjectionTestFS::MaybeInjectThreadLocalError(
-    FaultInjectionIOType type, const IOOptions& io_options, ErrorOperation op,
-    Slice* result, bool direct_io, char* scratch, bool need_count_increase,
+    FaultInjectionIOType type, const IOOptions& io_options,
+    const std::string& file_name, 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,
@@ -1306,8 +1452,9 @@ IOStatus FaultInjectionTestFS::MaybeInjectThreadLocalError(
 
   ErrorContext* ctx = GetErrorContextFromFaultInjectionIOType(type);
   if (ctx == nullptr || !ctx->enable_error_injection || !ctx->one_in ||
-      io_activties_exempted_from_fault_injection.find(io_options.io_activity) !=
-          io_activties_exempted_from_fault_injection.end()) {
+      ShouldIOActivtiesExcludedFromFaultInjection(io_options.io_activity) ||
+      (type == FaultInjectionIOType::kWrite &&
+       ShouldExcludeFromWriteFaultInjection(file_name))) {
     return IOStatus::OK();
   }
 

utilities/fault_injection_fs.h

@@ -41,17 +41,15 @@ enum class FaultInjectionIOType {
 
 struct FSFileState {
   std::string filename_;
-  ssize_t pos_at_last_append_;
-  ssize_t pos_at_last_sync_;
+  uint64_t pos_at_last_append_ = 0;
+  uint64_t pos_at_last_sync_ = 0;
   std::string buffer_;
 
-  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) {}
+  explicit FSFileState(const std::string& filename = {})
+      : filename_(filename) {}
 
   bool IsFullySynced() const {
-    return pos_at_last_append_ <= 0 || pos_at_last_append_ == pos_at_last_sync_;
+    return pos_at_last_append_ == pos_at_last_sync_;
   }
 
   IOStatus DropUnsyncedData();
@@ -105,6 +103,7 @@ class TestFSWritableFile : public FSWritableFile {
   bool writable_file_opened_;
   FaultInjectionTestFS* fs_;
   port::Mutex mutex_;
+  const bool unsync_data_loss_;
 };
 
 // A wrapper around WritableFileWriter* file
@@ -177,7 +176,8 @@ class TestFSSequentialFile : public FSSequentialFileOwnerWrapper {
  private:
   FaultInjectionTestFS* fs_;
   std::string fname_;
-  size_t read_pos_ = 0;
+  uint64_t read_pos_ = 0;
+  uint64_t target_read_pos_ = 0;
 };
 
 class TestFSDirectory : public FSDirectory {
@@ -207,6 +207,7 @@ 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),
@@ -359,19 +360,6 @@ 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();
@@ -391,6 +379,18 @@ 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,15 +414,10 @@ class FaultInjectionTestFS : public FileSystemWrapper {
     allow_link_open_file_ = allow_link_open_file;
   }
 
-  void SetDirectWritableTypes(const std::set<FileType>& types) {
+  bool ShouldIOActivtiesExcludedFromFaultInjection(Env::IOActivity io_activty) {
     MutexLock l(&mutex_);
-    direct_writable_types_ = types;
-  }
-
-  void SetIOActivtiesExemptedFromFaultInjection(
-      const std::set<Env::IOActivity>& io_activties) {
-    MutexLock l(&mutex_);
-    io_activties_exempted_from_fault_injection = io_activties;
+    return io_activties_excluded_from_fault_injection.find(io_activty) !=
+           io_activties_excluded_from_fault_injection.end();
   }
 
   void AssertNoOpenFile() { assert(open_managed_files_.empty()); }
@@ -499,8 +494,8 @@ class FaultInjectionTestFS : public FileSystemWrapper {
 
   IOStatus MaybeInjectThreadLocalError(
       FaultInjectionIOType type, const IOOptions& io_options,
-      ErrorOperation op = kUnknown, Slice* slice = nullptr,
-      bool direct_io = false, char* scratch = nullptr,
+      const std::string& file_name = "", 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) {
@@ -513,6 +508,29 @@ 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) {
@@ -529,8 +547,14 @@ class FaultInjectionTestFS : public FileSystemWrapper {
 
   void PrintInjectedThreadLocalErrorBacktrace(FaultInjectionIOType type);
 
-  void AddUnsyncedToRead(const std::string& fname, size_t offset, size_t n,
-                         Slice* result, char* scratch);
+  // 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);
 
  private:
   port::Mutex mutex_;
@@ -545,6 +569,7 @@ 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_;
@@ -582,8 +607,8 @@ class FaultInjectionTestFS : public FileSystemWrapper {
     }
   };
 
-  std::set<FileType> direct_writable_types_;
-  std::set<Env::IOActivity> io_activties_exempted_from_fault_injection;
+  std::set<FileType> file_types_excluded_from_write_fault_injection_;
+  std::set<Env::IOActivity> io_activties_excluded_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,8 +139,6 @@ 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,14 +1227,15 @@ 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;
-      txn_db_impl_->RegisterTransaction(this);
+      s = txn_db_impl_->RegisterTransaction(this);
+      if (!s.ok()) {
+        name_.clear();
+      }
     }
   } else {
     s = Status::InvalidArgument("Transaction is beyond state for naming.");

utilities/transactions/pessimistic_transaction_db.cc

@@ -723,6 +723,11 @@ 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;
@@ -755,13 +760,15 @@ void PessimisticTransactionDB::SetDeadlockInfoBufferSize(uint32_t target_size) {
   lock_manager_->Resize(target_size);
 }
 
-void PessimisticTransactionDB::RegisterTransaction(Transaction* txn) {
+Status 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_);
-  transactions_[txn->GetName()] = txn;
+  if (!transactions_.insert({txn->GetName(), txn}).second) {
+    return Status::InvalidArgument("Duplicate txn name " + txn->GetName());
+  }
+  return Status::OK();
 }
 
 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;
 
-  void RegisterTransaction(Transaction* txn);
+  Status RegisterTransaction(Transaction* txn);
   void UnregisterTransaction(Transaction* txn);
 
   // not thread safe. current use case is during recovery (single thread)
@@ -239,6 +239,7 @@ 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,11 +832,13 @@ Status TransactionBaseImpl::RebuildFromWriteBatch(WriteBatch* src_batch) {
     }
 
     Status PutCF(uint32_t cf, const Slice& key, const Slice& val) override {
-      return txn_->Put(db_->GetColumnFamilyHandle(cf), key, val);
+      Slice user_key = GetUserKey(cf, key);
+      return txn_->Put(db_->GetColumnFamilyHandle(cf), user_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 =
@@ -845,19 +847,22 @@ Status TransactionBaseImpl::RebuildFromWriteBatch(WriteBatch* src_batch) {
         return s;
       }
 
-      return txn_->PutEntity(db_->GetColumnFamilyHandle(cf), key, columns);
+      return txn_->PutEntity(db_->GetColumnFamilyHandle(cf), user_key, columns);
     }
 
     Status DeleteCF(uint32_t cf, const Slice& key) override {
-      return txn_->Delete(db_->GetColumnFamilyHandle(cf), key);
+      Slice user_key = GetUserKey(cf, key);
+      return txn_->Delete(db_->GetColumnFamilyHandle(cf), user_key);
     }
 
     Status SingleDeleteCF(uint32_t cf, const Slice& key) override {
-      return txn_->SingleDelete(db_->GetColumnFamilyHandle(cf), key);
+      Slice user_key = GetUserKey(cf, key);
+      return txn_->SingleDelete(db_->GetColumnFamilyHandle(cf), user_key);
     }
 
     Status MergeCF(uint32_t cf, const Slice& key, const Slice& val) override {
-      return txn_->Merge(db_->GetColumnFamilyHandle(cf), key, val);
+      Slice user_key = GetUserKey(cf, key);
+      return txn_->Merge(db_->GetColumnFamilyHandle(cf), user_key, val);
     }
 
     // this is used for reconstructing prepared transactions upon
@@ -880,6 +885,21 @@ 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,12 +288,25 @@ 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_TRUE(s.IsNotFound());
+    ASSERT_OK(s);
+    ASSERT_EQ("foo_value", value);
 
     s = db->Get(ReadOptions(), handles_[0], "bar", &value);
     ASSERT_OK(s);
@@ -314,6 +327,9 @@ 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());
   }
 }