From 4d5ebad971c775af7e9a3242928f81f31e021af7 Mon Sep 17 00:00:00 2001
From: Andrew Kryczka <andrew.kryczka2@gmail.com>
Date: Fri, 15 Mar 2024 14:41:58 -0700
Subject: [PATCH] Fix kBlockCacheTier read with table cache miss (#12443)

Summary:
Thanks ltamasi for pointing out this bug.

We were incorrectly overwriting `Status::Incomplete` with `Status::OK` after a table cache miss failed to open the file due to the read being memory-only (`kBlockCacheTier`). The fix is to simply stop overwriting the status.

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

Reviewed By: cbi42

Differential Revision: D54930128

Pulled By: ajkr

fbshipit-source-id: 52f912a2e93b46e71d79fc5968f8ca35b299213d
---
 db/db_test2.cc                                | 21 +++++++++++++++++++
 db/table_cache.cc                             |  5 ++---
 .../blockcachetier_tablecachemiss.md          |  1 +
 3 files changed, 24 insertions(+), 3 deletions(-)
 create mode 100644 unreleased_history/bug_fixes/blockcachetier_tablecachemiss.md

diff --git a/db/db_test2.cc b/db/db_test2.cc
index 8d7f37f6d3..2da4b563d0 100644
--- a/db/db_test2.cc
+++ b/db/db_test2.cc
@@ -7808,6 +7808,27 @@ TEST_F(DBTest2, ZSTDChecksum) {
 }
 #endif
 
+TEST_F(DBTest2, TableCacheMissDuringReadFromBlockCacheTier) {
+  Options options = CurrentOptions();
+  options.statistics = ROCKSDB_NAMESPACE::CreateDBStatistics();
+  Reopen(options);
+
+  // Give table cache zero capacity to prevent preloading tables. That way,
+  // `kBlockCacheTier` reads will fail due to table cache misses.
+  dbfull()->TEST_table_cache()->SetCapacity(0);
+  ASSERT_OK(Put("foo", "bar"));
+  ASSERT_OK(Flush());
+
+  uint64_t orig_num_file_opens = TestGetTickerCount(options, NO_FILE_OPENS);
+
+  ReadOptions non_blocking_opts;
+  non_blocking_opts.read_tier = kBlockCacheTier;
+  std::string value;
+  ASSERT_TRUE(db_->Get(non_blocking_opts, "foo", &value).IsIncomplete());
+
+  ASSERT_EQ(orig_num_file_opens, TestGetTickerCount(options, NO_FILE_OPENS));
+}
+
 }  // namespace ROCKSDB_NAMESPACE
 
 int main(int argc, char** argv) {
diff --git a/db/table_cache.cc b/db/table_cache.cc
index 2c0092e7d4..13cbbe5841 100644
--- a/db/table_cache.cc
+++ b/db/table_cache.cc
@@ -489,9 +489,8 @@ Status TableCache::Get(
       s = t->Get(options, k, get_context, prefix_extractor.get(), skip_filters);
       get_context->SetReplayLog(nullptr);
     } else if (options.read_tier == kBlockCacheTier && s.IsIncomplete()) {
-      // Couldn't find Table in cache but treat as kFound if no_io set
+      // Couldn't find table in cache and couldn't open it because of no_io.
       get_context->MarkKeyMayExist();
-      s = Status::OK();
       done = true;
     }
   }
@@ -729,4 +728,4 @@ uint64_t TableCache::ApproximateSize(
 
   return result;
 }
-}  // namespace ROCKSDB_NAMESPACE
\ No newline at end of file
+}  // namespace ROCKSDB_NAMESPACE
diff --git a/unreleased_history/bug_fixes/blockcachetier_tablecachemiss.md b/unreleased_history/bug_fixes/blockcachetier_tablecachemiss.md
new file mode 100644
index 0000000000..298b69b1ca
--- /dev/null
+++ b/unreleased_history/bug_fixes/blockcachetier_tablecachemiss.md
@@ -0,0 +1 @@
+* Fixed `kBlockCacheTier` reads to return `Status::Incomplete` on table cache miss rather than incorrectly returning an empty value.