From 37f490834dde7877db0282a733dd42fdbefc0a77 Mon Sep 17 00:00:00 2001
From: Changyu Bi <changyubi@fb.com>
Date: Mon, 2 May 2022 10:22:08 -0700
Subject: [PATCH] Specify largest_seqno in VerifyChecksum (#9919)

Summary:
`VerifyChecksum()` does not specify `largest_seqno` when creating a `TableReader`. As a result, the `TableReader` uses the `TableReaderOptions` default value (0) for `largest_seqno`. This causes the following error when the file has a nonzero global seqno in its properties:
```
Corruption: An external sst file with version 2 have global seqno property with value , while largest seqno in the file is 0
```
This PR fixes this by specifying `largest_seqno` in `VerifyChecksumInternal` with `largest_seqno` from the file metadata.

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

Test Plan: `make check`

Reviewed By: ajkr

Differential Revision: D36028824

Pulled By: cbi42

fbshipit-source-id: 428d028a79386f46ef97bb6b6051dc76c83e1f2b
---
 db/convenience.cc                  | 14 ++++++++------
 db/db_impl/db_impl.cc              |  4 ++--
 db/external_sst_file_basic_test.cc | 22 ++++++++++++++++++++++
 include/rocksdb/convenience.h      |  3 ++-
 4 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/db/convenience.cc b/db/convenience.cc
index 713ca8da9..81389112d 100644
--- a/db/convenience.cc
+++ b/db/convenience.cc
@@ -40,7 +40,8 @@ Status VerifySstFileChecksum(const Options& options,
 Status VerifySstFileChecksum(const Options& options,
                              const EnvOptions& env_options,
                              const ReadOptions& read_options,
-                             const std::string& file_path) {
+                             const std::string& file_path,
+                             const SequenceNumber& largest_seqno) {
   std::unique_ptr<FSRandomAccessFile> file;
   uint64_t file_size;
   InternalKeyComparator internal_comparator(options.comparator);
@@ -61,12 +62,13 @@ Status VerifySstFileChecksum(const Options& options,
           nullptr /* stats */, 0 /* hist_type */, nullptr /* file_read_hist */,
           ioptions.rate_limiter.get()));
   const bool kImmortal = true;
+  auto reader_options = TableReaderOptions(
+      ioptions, options.prefix_extractor, env_options, internal_comparator,
+      false /* skip_filters */, !kImmortal, false /* force_direct_prefetch */,
+      -1 /* level */);
+  reader_options.largest_seqno = largest_seqno;
   s = ioptions.table_factory->NewTableReader(
-      TableReaderOptions(ioptions, options.prefix_extractor, env_options,
-                         internal_comparator, false /* skip_filters */,
-                         !kImmortal, false /* force_direct_prefetch */,
-                         -1 /* level */),
-      std::move(file_reader), file_size, &table_reader,
+      reader_options, std::move(file_reader), file_size, &table_reader,
       false /* prefetch_index_and_filter_in_cache */);
   if (!s.ok()) {
     return s;
diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc
index dff9fce50..c44612100 100644
--- a/db/db_impl/db_impl.cc
+++ b/db/db_impl/db_impl.cc
@@ -5123,8 +5123,8 @@ Status DBImpl::VerifyChecksumInternal(const ReadOptions& read_options,
                                      fmeta->file_checksum_func_name, fname,
                                      read_options);
         } else {
-          s = ROCKSDB_NAMESPACE::VerifySstFileChecksum(opts, file_options_,
-                                                       read_options, fname);
+          s = ROCKSDB_NAMESPACE::VerifySstFileChecksum(
+              opts, file_options_, read_options, fname, fd.largest_seqno);
         }
         RecordTick(stats_, VERIFY_CHECKSUM_READ_BYTES,
                    IOSTATS(bytes_read) - prev_bytes_read);
diff --git a/db/external_sst_file_basic_test.cc b/db/external_sst_file_basic_test.cc
index b6f05a846..8aa4b6cf5 100644
--- a/db/external_sst_file_basic_test.cc
+++ b/db/external_sst_file_basic_test.cc
@@ -1844,6 +1844,28 @@ TEST_F(ExternalSSTFileBasicTest, FailIfNotBottommostLevel) {
   }
 }
 
+TEST_F(ExternalSSTFileBasicTest, VerifyChecksum) {
+  const std::string kPutVal = "put_val";
+  const std::string kIngestedVal = "ingested_val";
+
+  ASSERT_OK(Put("k", kPutVal, WriteOptions()));
+  ASSERT_OK(Flush());
+
+  std::string external_file = sst_files_dir_ + "/file_to_ingest.sst";
+  {
+    SstFileWriter sst_file_writer{EnvOptions(), CurrentOptions()};
+
+    ASSERT_OK(sst_file_writer.Open(external_file));
+    ASSERT_OK(sst_file_writer.Put("k", kIngestedVal));
+    ASSERT_OK(sst_file_writer.Finish());
+  }
+
+  ASSERT_OK(db_->IngestExternalFile(db_->DefaultColumnFamily(), {external_file},
+                                    IngestExternalFileOptions()));
+
+  ASSERT_OK(db_->VerifyChecksum());
+}
+
 INSTANTIATE_TEST_CASE_P(ExternalSSTFileBasicTest, ExternalSSTFileBasicTest,
                         testing::Values(std::make_tuple(true, true),
                                         std::make_tuple(true, false),
diff --git a/include/rocksdb/convenience.h b/include/rocksdb/convenience.h
index dc9bdc162..921ec221b 100644
--- a/include/rocksdb/convenience.h
+++ b/include/rocksdb/convenience.h
@@ -518,7 +518,8 @@ Status VerifySstFileChecksum(const Options& options,
 Status VerifySstFileChecksum(const Options& options,
                              const EnvOptions& env_options,
                              const ReadOptions& read_options,
-                             const std::string& file_path);
+                             const std::string& file_path,
+                             const SequenceNumber& largest_seqno = 0);
 #endif  // ROCKSDB_LITE
 
 }  // namespace ROCKSDB_NAMESPACE