Fix a nuisance compiler warning from clang (#12144)

Summary:
Example:

```
cache/clock_cache.cc:56:7: error: fallthrough annotation in unreachable code [-Werror,-Wimplicit-fallthrough]
      FALLTHROUGH_INTENDED;
      ^
./port/lang.h:10:30: note: expanded from macro 'FALLTHROUGH_INTENDED'
                             ^
```

In clang < 14, this is annoyingly generated from -Wimplicit-fallthrough, but was changed to -Wunreachable-code-fallthrough (implied by -Wunreachable-code) in clang 14. See https://reviews.llvm.org/D107933 for how this nuisance pattern generated false positives similar to ours in the Linux kernel.

Just to underscore the ridiculousness of this warning, here an error is reported on the annotation, not the call to do_something(), depending on the constexpr value (https://godbolt.org/z/EvxqdPTdr):

```
#include <atomic>
void do_something();
void test(int v) {
    switch (v) {
        case 1:
            if constexpr (std::atomic<long>::is_always_lock_free) {
                return;
            } else {
                do_something();
                [[fallthrough]];
            }
        case 2:
            return;
    }
}
```

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

Test Plan: Added the warning to our Makefile for USE_CLANG, which reproduced the warning-as-error as shown above, but is now fixed.

Reviewed By: jaykorean

Differential Revision: D52139615

Pulled By: pdillinger

fbshipit-source-id: ba967ae700c0916d1a478bc465cf917633e337d9

HISTORY.md

@@ -1,6 +1,38 @@
 # Rocksdb Change Log
 > NOTE: Entries for next release do not go here. Follow instructions in `unreleased_history/README.txt`
 
+## 8.9.2 (12/12/2023)
+### Bug Fixes
+* A lookup by MultiGet in a TieredCache that goes to the local flash cache and finishes with very low latency, i.e before the subsequent call to WaitAll, is ignored, resulting in a false negative and a memory leak.
+* Sanitize the cache_opts->secondary_cache field in TieredCacheOptions. It can be mistakenly set by the user, which would conflict with nvm_sec_cache in TieredCacheOptions if not sanitized to nullptr.
+
+## 8.9.1 (12/8/2023)
+### Bug Fixes
+* Avoid destroying the periodic task scheduler's default timer in order to prevent static destruction order issues.
+
+## 8.9.0 (11/17/2023)
+### New Features
+* Add GetEntity() and PutEntity() API implementation for Attribute Group support. Through the use of Column Families, AttributeGroup enables users to logically group wide-column entities.
+
+### Public API Changes
+* Added rocksdb_ratelimiter_create_auto_tuned API to create an auto-tuned GenericRateLimiter.
+* Added clipColumnFamily() to the Java API to clip the entries in the CF according to the range [begin_key, end_key).
+* Make the `EnableFileDeletion` API not default to force enabling. For users that rely on this default behavior and still
+want to continue to use force enabling, they need to explicitly pass a `true` to `EnableFileDeletion`.
+* Add new Cache APIs GetSecondaryCacheCapacity() and GetSecondaryCachePinnedUsage() to return the configured capacity, and cache reservation charged to the secondary cache.
+
+### Behavior Changes
+* During off-peak hours defined by `daily_offpeak_time_utc`, the compaction picker will select a larger number of files for periodic compaction. This selection will include files that are projected to expire by the next off-peak start time, ensuring that these files are not chosen for periodic compaction outside of off-peak hours.
+* If an error occurs when writing to a trace file after `DB::StartTrace()`, the subsequent trace writes are skipped to avoid writing to a file that has previously seen error. In this case, `DB::EndTrace()` will also return a non-ok status with info about the error occured previously in its status message.
+* Deleting stale files upon recovery are delegated to SstFileManger if available so they can be rate limited.
+* Make RocksDB only call `TablePropertiesCollector::Finish()` once.
+* When `WAL_ttl_seconds > 0`, we now process archived WALs for deletion at least every `WAL_ttl_seconds / 2` seconds. Previously it could be less frequent in case of small `WAL_ttl_seconds` values when size-based expiration (`WAL_size_limit_MB > 0 `) was simultaneously enabled.
+
+### Bug Fixes
+* Fixed a crash or assertion failure bug in experimental new HyperClockCache variant, especially when running with a SecondaryCache.
+* Fix a race between flush error recovery and db destruction that can lead to db crashing.
+* Fixed some bugs in the index builder/reader path for user-defined timestamps in Memtable only feature.
+
 ## 8.8.0 (10/23/2023)
 ### New Features
 * Introduce AttributeGroup by adding the first AttributeGroup support API, MultiGetEntity(). Through the use of Column Families, AttributeGroup enables users to logically group wide-column entities. More APIs to support AttributeGroup will come soon, including GetEntity, PutEntity, and others.

Makefile

@@ -539,7 +539,7 @@ endif
 
 ifdef USE_CLANG
 	# Used by some teams in Facebook
-	WARNING_FLAGS += -Wshift-sign-overflow -Wambiguous-reversed-operator
+	WARNING_FLAGS += -Wshift-sign-overflow -Wambiguous-reversed-operator -Wimplicit-fallthrough
 endif
 
 ifeq ($(PLATFORM), OS_OPENBSD)

cache/clock_cache.cc

@@ -51,14 +51,15 @@ inline uint64_t GetInitialCountdown(Cache::Priority priority) {
   switch (priority) {
     case Cache::Priority::HIGH:
       return ClockHandle::kHighCountdown;
-    default:
-      assert(false);
-      FALLTHROUGH_INTENDED;
     case Cache::Priority::LOW:
       return ClockHandle::kLowCountdown;
     case Cache::Priority::BOTTOM:
       return ClockHandle::kBottomCountdown;
   }
+  // Switch should have been exhaustive.
+  assert(false);
+  // For release build, fall back on something reasonable.
+  return ClockHandle::kLowCountdown;
 }
 
 inline void MarkEmpty(ClockHandle& h) {

cache/secondary_cache_adapter.cc

@@ -683,12 +683,14 @@ std::shared_ptr<Cache> NewTieredCache(const TieredCacheOptions& _opts) {
         *(static_cast_with_check<LRUCacheOptions, ShardedCacheOptions>(
             opts.cache_opts));
     cache_opts.capacity = opts.total_capacity;
+    cache_opts.secondary_cache = nullptr;
     cache = cache_opts.MakeSharedCache();
   } else if (opts.cache_type == PrimaryCacheType::kCacheTypeHCC) {
     HyperClockCacheOptions cache_opts =
         *(static_cast_with_check<HyperClockCacheOptions, ShardedCacheOptions>(
             opts.cache_opts));
     cache_opts.capacity = opts.total_capacity;
+    cache_opts.secondary_cache = nullptr;
     cache = cache_opts.MakeSharedCache();
   } else {
     return nullptr;

cache/tiered_secondary_cache.cc

@@ -109,10 +109,8 @@ void TieredSecondaryCache::WaitAll(
   }
   nvm_sec_cache_->WaitAll(nvm_handles);
   for (auto handle : my_handles) {
-    assert(handle->IsReady());
-    auto nvm_handle = handle->inner_handle();
-    handle->SetSize(nvm_handle->Size());
-    handle->SetValue(nvm_handle->Value());
+    assert(handle->inner_handle()->IsReady());
+    handle->Complete();
   }
 }
 

cache/tiered_secondary_cache.h

@@ -79,7 +79,10 @@ class TieredSecondaryCache : public SecondaryCacheWrapper {
     ~ResultHandle() override {}
 
     bool IsReady() override {
-      return !inner_handle_ || inner_handle_->IsReady();
+      if (inner_handle_ && inner_handle_->IsReady()) {
+        Complete();
+      }
+      return ready_;
     }
 
     void Wait() override {
@@ -92,10 +95,10 @@ class TieredSecondaryCache : public SecondaryCacheWrapper {
     Cache::ObjectPtr Value() override { return value_; }
 
     void Complete() {
-      assert(IsReady());
       size_ = inner_handle_->Size();
       value_ = inner_handle_->Value();
       inner_handle_.reset();
+      ready_ = true;
     }
 
     void SetInnerHandle(std::unique_ptr<SecondaryCacheResultHandle>&& handle) {
@@ -115,6 +118,7 @@ class TieredSecondaryCache : public SecondaryCacheWrapper {
     CreateContext ctx_;
     size_t size_;
     Cache::ObjectPtr value_;
+    bool ready_ = false;
   };
 
   static void NoopDelete(Cache::ObjectPtr /*obj*/,

cache/tiered_secondary_cache_test.cc

@@ -15,10 +15,11 @@ namespace ROCKSDB_NAMESPACE {
 
 class TestSecondaryCache : public SecondaryCache {
  public:
-  explicit TestSecondaryCache(size_t capacity)
+  explicit TestSecondaryCache(size_t capacity, bool ready_before_wait)
       : cache_(NewLRUCache(capacity, 0, false, 0.5 /* high_pri_pool_ratio */,
                            nullptr, kDefaultToAdaptiveMutex,
                            kDontChargeCacheMetadata)),
+        ready_before_wait_(ready_before_wait),
         num_insert_saved_(0),
         num_hits_(0),
         num_misses_(0) {}
@@ -88,7 +89,8 @@ class TestSecondaryCache : public SecondaryCache {
                             /*alloc*/ nullptr, &value, &charge);
       if (s.ok()) {
         secondary_handle.reset(new TestSecondaryCacheResultHandle(
-            cache_.get(), handle, value, charge, /*ready=*/wait));
+            cache_.get(), handle, value, charge,
+            /*ready=*/wait || ready_before_wait_));
         kept_in_sec_cache = true;
       } else {
         cache_.Release(handle);
@@ -168,6 +170,7 @@ class TestSecondaryCache : public SecondaryCache {
       BasicTypedSharedCacheInterface<char[], CacheEntryRole::kMisc>;
   using TypedHandle = SharedCache::TypedHandle;
   SharedCache cache_;
+  bool ready_before_wait_;
   uint32_t num_insert_saved_;
   uint32_t num_hits_;
   uint32_t num_misses_;
@@ -179,11 +182,10 @@ class DBTieredSecondaryCacheTest : public DBTestBase {
   DBTieredSecondaryCacheTest()
       : DBTestBase("db_tiered_secondary_cache_test", /*env_do_fsync=*/true) {}
 
-  std::shared_ptr<Cache> NewCache(size_t pri_capacity,
-                                  size_t compressed_capacity,
-                                  size_t nvm_capacity,
-                                  TieredAdmissionPolicy adm_policy =
-                                      TieredAdmissionPolicy::kAdmPolicyAuto) {
+  std::shared_ptr<Cache> NewCache(
+      size_t pri_capacity, size_t compressed_capacity, size_t nvm_capacity,
+      TieredAdmissionPolicy adm_policy = TieredAdmissionPolicy::kAdmPolicyAuto,
+      bool ready_before_wait = false) {
     LRUCacheOptions lru_opts;
     TieredCacheOptions opts;
     lru_opts.capacity = 0;
@@ -194,10 +196,11 @@ class DBTieredSecondaryCacheTest : public DBTestBase {
     opts.comp_cache_opts.capacity = 0;
     opts.comp_cache_opts.num_shard_bits = 0;
     opts.total_capacity = pri_capacity + compressed_capacity;
-    opts.compressed_secondary_ratio =
+    opts.compressed_secondary_ratio = compressed_secondary_ratio_ =
         (double)compressed_capacity / opts.total_capacity;
     if (nvm_capacity > 0) {
-      nvm_sec_cache_.reset(new TestSecondaryCache(nvm_capacity));
+      nvm_sec_cache_.reset(
+          new TestSecondaryCache(nvm_capacity, ready_before_wait));
       opts.nvm_sec_cache = nvm_sec_cache_;
     }
     opts.adm_policy = adm_policy;
@@ -207,6 +210,12 @@ class DBTieredSecondaryCacheTest : public DBTestBase {
     return cache_;
   }
 
+  void ClearPrimaryCache() {
+    ASSERT_EQ(UpdateTieredCache(cache_, -1, 1.0), Status::OK());
+    ASSERT_EQ(UpdateTieredCache(cache_, -1, compressed_secondary_ratio_),
+              Status::OK());
+  }
+
   TestSecondaryCache* nvm_sec_cache() { return nvm_sec_cache_.get(); }
 
   CompressedSecondaryCache* compressed_secondary_cache() {
@@ -218,6 +227,7 @@ class DBTieredSecondaryCacheTest : public DBTestBase {
  private:
   std::shared_ptr<Cache> cache_;
   std::shared_ptr<TestSecondaryCache> nvm_sec_cache_;
+  double compressed_secondary_ratio_;
 };
 
 // In this test, the block size is set to 4096. Each value is 1007 bytes, so
@@ -582,6 +592,116 @@ TEST_F(DBTieredSecondaryCacheTest, WaitAllTest) {
   Destroy(options);
 }
 
+TEST_F(DBTieredSecondaryCacheTest, ReadyBeforeWaitAllTest) {
+  if (!LZ4_Supported()) {
+    ROCKSDB_GTEST_SKIP("This test requires LZ4 support.");
+    return;
+  }
+
+  BlockBasedTableOptions table_options;
+  table_options.block_cache = NewCache(250 * 1024, 20 * 1024, 256 * 1024,
+                                       TieredAdmissionPolicy::kAdmPolicyAuto,
+                                       /*ready_before_wait=*/true);
+  table_options.block_size = 4 * 1024;
+  table_options.cache_index_and_filter_blocks = false;
+  Options options = GetDefaultOptions();
+  options.create_if_missing = true;
+  options.table_factory.reset(NewBlockBasedTableFactory(table_options));
+  options.statistics = CreateDBStatistics();
+
+  options.paranoid_file_checks = false;
+  DestroyAndReopen(options);
+  Random rnd(301);
+  const int N = 256;
+  for (int i = 0; i < N; i++) {
+    std::string p_v;
+    test::CompressibleString(&rnd, 0.5, 1007, &p_v);
+    ASSERT_OK(Put(Key(i), p_v));
+  }
+
+  ASSERT_OK(Flush());
+
+  std::vector<std::string> keys;
+  std::vector<std::string> values;
+
+  keys.push_back(Key(0));
+  keys.push_back(Key(4));
+  keys.push_back(Key(8));
+  values = MultiGet(keys, /*snapshot=*/nullptr, /*async=*/true);
+  ASSERT_EQ(values.size(), keys.size());
+  for (auto value : values) {
+    ASSERT_EQ(1007, value.size());
+  }
+  ASSERT_EQ(nvm_sec_cache()->num_insert_saved(), 3u);
+  ASSERT_EQ(nvm_sec_cache()->num_misses(), 3u);
+  ASSERT_EQ(nvm_sec_cache()->num_hits(), 0u);
+  ASSERT_EQ(options.statistics->getTickerCount(BLOCK_CACHE_MISS), 3u);
+
+  keys.clear();
+  values.clear();
+  keys.push_back(Key(12));
+  keys.push_back(Key(16));
+  keys.push_back(Key(20));
+  values = MultiGet(keys, /*snapshot=*/nullptr, /*async=*/true);
+  ASSERT_EQ(values.size(), keys.size());
+  for (auto value : values) {
+    ASSERT_EQ(1007, value.size());
+  }
+  ASSERT_EQ(nvm_sec_cache()->num_insert_saved(), 6u);
+  ASSERT_EQ(nvm_sec_cache()->num_misses(), 6u);
+  ASSERT_EQ(nvm_sec_cache()->num_hits(), 0u);
+  ASSERT_EQ(options.statistics->getTickerCount(BLOCK_CACHE_MISS), 6u);
+
+  keys.clear();
+  values.clear();
+  keys.push_back(Key(0));
+  keys.push_back(Key(4));
+  keys.push_back(Key(8));
+  values = MultiGet(keys, /*snapshot=*/nullptr, /*async=*/true);
+  ASSERT_EQ(values.size(), keys.size());
+  for (auto value : values) {
+    ASSERT_EQ(1007, value.size());
+  }
+  ASSERT_EQ(nvm_sec_cache()->num_insert_saved(), 6u);
+  ASSERT_EQ(nvm_sec_cache()->num_misses(), 6u);
+  ASSERT_EQ(nvm_sec_cache()->num_hits(), 3u);
+  ASSERT_EQ(options.statistics->getTickerCount(BLOCK_CACHE_MISS), 6u);
+
+  ClearPrimaryCache();
+
+  keys.clear();
+  values.clear();
+  keys.push_back(Key(0));
+  keys.push_back(Key(32));
+  keys.push_back(Key(36));
+  values = MultiGet(keys, /*snapshot=*/nullptr, /*async=*/true);
+  ASSERT_EQ(values.size(), keys.size());
+  for (auto value : values) {
+    ASSERT_EQ(1007, value.size());
+  }
+  ASSERT_EQ(nvm_sec_cache()->num_insert_saved(), 8u);
+  ASSERT_EQ(nvm_sec_cache()->num_misses(), 8u);
+  ASSERT_EQ(nvm_sec_cache()->num_hits(), 4u);
+  ASSERT_EQ(options.statistics->getTickerCount(BLOCK_CACHE_MISS), 8u);
+
+  keys.clear();
+  values.clear();
+  keys.push_back(Key(0));
+  keys.push_back(Key(32));
+  keys.push_back(Key(36));
+  values = MultiGet(keys, /*snapshot=*/nullptr, /*async=*/true);
+  ASSERT_EQ(values.size(), keys.size());
+  for (auto value : values) {
+    ASSERT_EQ(1007, value.size());
+  }
+  ASSERT_EQ(nvm_sec_cache()->num_insert_saved(), 8u);
+  ASSERT_EQ(nvm_sec_cache()->num_misses(), 8u);
+  ASSERT_EQ(nvm_sec_cache()->num_hits(), 4u);
+  ASSERT_EQ(options.statistics->getTickerCount(BLOCK_CACHE_MISS), 8u);
+
+  Destroy(options);
+}
+
 // This test is for iteration. It iterates through a set of keys in two
 // passes. First pass loads the compressed blocks into the nvm tier, and
 // the second pass should hit all of those blocks.

db/periodic_task_scheduler.cc

@@ -94,7 +94,7 @@ Status PeriodicTaskScheduler::Unregister(PeriodicTaskType task_type) {
 }
 
 Timer* PeriodicTaskScheduler::Default() {
-  static Timer timer(SystemClock::Default().get());
+  STATIC_AVOID_DESTRUCTION(Timer, timer)(SystemClock::Default().get());
   return &timer;
 }
 
@@ -108,4 +108,3 @@ void PeriodicTaskScheduler::TEST_OverrideTimer(SystemClock* clock) {
 #endif  // NDEBUG
 
 }  // namespace ROCKSDB_NAMESPACE
-

include/rocksdb/cache.h

@@ -499,6 +499,10 @@ enum TieredAdmissionPolicy {
 // allocations costed to the block cache, will be distributed
 // proportionally across both the primary and secondary.
 struct TieredCacheOptions {
+  // This should point to an instance of either LRUCacheOptions or
+  // HyperClockCacheOptions, depending on the cache_type. In either
+  // case, the capacity and secondary_cache fields in those options
+  // should not be set. If set, they will be ignored by NewTieredCache.
   ShardedCacheOptions* cache_opts = nullptr;
   PrimaryCacheType cache_type = PrimaryCacheType::kCacheTypeLRU;
   TieredAdmissionPolicy adm_policy = TieredAdmissionPolicy::kAdmPolicyAuto;

include/rocksdb/version.h

@@ -13,7 +13,7 @@
 // minor or major version number planned for release.
 #define ROCKSDB_MAJOR 8
 #define ROCKSDB_MINOR 9
-#define ROCKSDB_PATCH 0
+#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

tools/check_format_compatible.sh

@@ -125,7 +125,7 @@ EOF
 
 # To check for DB forward compatibility with loading options (old version
 # reading data from new), as well as backward compatibility
-declare -a db_forward_with_options_refs=("6.27.fb" "6.28.fb" "6.29.fb" "7.0.fb" "7.1.fb" "7.2.fb" "7.3.fb" "7.4.fb" "7.5.fb" "7.6.fb" "7.7.fb" "7.8.fb" "7.9.fb" "7.10.fb" "8.0.fb" "8.1.fb" "8.2.fb" "8.3.fb" "8.4.fb" "8.5.fb" "8.6.fb" "8.7.fb" "8.8.fb")
+declare -a db_forward_with_options_refs=("6.27.fb" "6.28.fb" "6.29.fb" "7.0.fb" "7.1.fb" "7.2.fb" "7.3.fb" "7.4.fb" "7.5.fb" "7.6.fb" "7.7.fb" "7.8.fb" "7.9.fb" "7.10.fb" "8.0.fb" "8.1.fb" "8.2.fb" "8.3.fb" "8.4.fb" "8.5.fb" "8.6.fb" "8.7.fb" "8.8.fb" "8.9.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

unreleased_history/behavior_changes/WAL_ttl_seconds_expiration.md

@@ -1 +0,0 @@
-* When `WAL_ttl_seconds > 0`, we now process archived WALs for deletion at least every `WAL_ttl_seconds / 2` seconds. Previously it could be less frequent in case of small `WAL_ttl_seconds` values when size-based expiration (`WAL_size_limit_MB > 0 `) was simultaneously enabled.

unreleased_history/behavior_changes/periodic_compaction_during_offpeak.md

@@ -1 +0,0 @@
-During off-peak hours defined by `daily_offpeak_time_utc`, the compaction picker will select a larger number of files for periodic compaction. This selection will include files that are projected to expire by the next off-peak start time, ensuring that these files are not chosen for periodic compaction outside of off-peak hours.

unreleased_history/behavior_changes/skip_trace_write.md

@@ -1 +0,0 @@
-If an error occurs when writing to a trace file after `DB::StartTrace()`, the subsequent trace writes are skipped to avoid writing to a file that has previously seen error. In this case, `DB::EndTrace()` will also return a non-ok status with info about the error occured previously in its status message.

unreleased_history/behavior_changes/stale_file_deletion_on_recovery_rate_limited.md

@@ -1 +0,0 @@
-Deleting stale files upon recovery are delegated to SstFileManger if available so they can be rate limited.

unreleased_history/behavior_changes/table_prop_collector_api_call.md

@@ -1 +0,0 @@
-Make RocksDB only call `TablePropertiesCollector::Finish()` once.

unreleased_history/bug_fixes/exp_autohcc_fix.md

@@ -1 +0,0 @@
-Fixed a crash or assertion failure bug in experimental new HyperClockCache variant, especially when running with a SecondaryCache.

unreleased_history/bug_fixes/flush_recovery_db_destructor_race.md

@@ -1 +0,0 @@
-Fix a race between flush error recovery and db destruction that can lead to db crashing.

unreleased_history/bug_fixes/index_bug_fix_for_udt_in_memtable_only.md

@@ -1 +0,0 @@
-Fixed some bugs in the index builder/reader path for user-defined timestamps in Memtable only feature.

unreleased_history/new_features/attribute_group_support.md

@@ -1 +0,0 @@
-Add GetEntity() and PutEntity() API implementation for Attribute Group support. Through the use of Column Families, AttributeGroup enables users to logically group wide-column entities.

unreleased_history/public_api_changes/add_auto_tuned_rate_limiter_to_c_api.md

@@ -1 +0,0 @@
-Added rocksdb_ratelimiter_create_auto_tuned API to create an auto-tuned GenericRateLimiter.

unreleased_history/public_api_changes/add_clip_column_family_to_java_api.md

@@ -1 +0,0 @@
-Added clipColumnFamily() to the Java API to clip the entries in the CF according to the range [begin_key, end_key).

unreleased_history/public_api_changes/enable_file_deletion_not_default_force.md

@@ -1,2 +0,0 @@
-Make the `EnableFileDeletion` API not default to force enabling. For users that rely on this default behavior and still
-want to continue to use force enabling, they need to explicitly pass a `true` to `EnableFileDeletion`.

unreleased_history/public_api_changes/tiered_cache_capacity_and_usage.md

@@ -1 +0,0 @@
-Add new Cache APIs GetSecondaryCacheCapacity() and GetSecondaryCachePinnedUsage() to return the configured capacity, and cache reservation charged to the secondary cache.