From a1e92bd9561ab89bc777ce31f4121d6eb6855297 Mon Sep 17 00:00:00 2001 From: Karim TAAM Date: Thu, 26 Jan 2023 17:38:59 -0800 Subject: [PATCH] use verify checksum option in block based table reader Open() (#11099) Summary: ## Description In this issue https://github.com/facebook/rocksdb/issues/11002 we found that when we use rocksdb with the `verify checksum` read_option to false the verification is done anyway By analyzing the code along the stacktrace I saw that at the level of https://github.com/facebook/rocksdb/compare/main...matkt:feature/use-verify-checksum-in-block-based-table-reader?expand=1#diff-57ed8c49db2bdd4db7618646a177397674bbf25beacacecb104070071d30129f we are not keeping all the options and we forget the `verify_checksum` the comment in this class suggests that it should be managed https://github.com/facebook/rocksdb/compare/main...matkt:feature/use-verify-checksum-in-block-based-table-reader?expand=1#diff-57ed8c49db2bdd4db7618646a177397674bbf25beacacecb104070071d30129fL581 204511641-86ab4b9b-45e5-4a2b-a13d-81fa26435d38 this PR just adds the line to manage the `verify checksum` ## Tests - Running unit tests - Test without setting `verify checksum` and verifying that we are calling the checksum code - Test by setting `verify checksum` to true and verifying that we are calling the checksum code - Test by setting `verify checksum` to false and verifying that we are **not** calling the checksum code Pull Request resolved: https://github.com/facebook/rocksdb/pull/11099 Reviewed By: cbi42 Differential Revision: D42679881 Pulled By: ajkr fbshipit-source-id: c7dd10768282fd0699f7e1bf397ceb7adbea4ab6 --- HISTORY.md | 4 ++++ db/corruption_test.cc | 8 ++++++-- table/block_based/block_based_table_reader.cc | 8 +++----- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 89babeb6d..74cd5d24a 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,5 +1,9 @@ # Rocksdb Change Log ## Unreleased + +### Behavior changes +* `ReadOptions::verify_checksums=false` disables checksum verification for more reads of non-`CacheEntryRole::kDataBlock` blocks. + ### Bug Fixes * Fixed a data race on `ColumnFamilyData::flush_reason` caused by concurrent flushes. diff --git a/db/corruption_test.cc b/db/corruption_test.cc index 6c909f39a..a23cffb4c 100644 --- a/db/corruption_test.cc +++ b/db/corruption_test.cc @@ -168,6 +168,10 @@ class CorruptionTest : public testing::Test { void Build(int n, int flush_every = 0) { Build(n, 0, flush_every); } void Check(int min_expected, int max_expected) { + Check(min_expected, max_expected, ReadOptions(false, true)); + } + + void Check(int min_expected, int max_expected, ReadOptions read_options) { uint64_t next_expected = 0; uint64_t missed = 0; int bad_keys = 0; @@ -179,7 +183,7 @@ class CorruptionTest : public testing::Test { // Instead, we want the reads to be successful and this test // will detect whether the appropriate corruptions have // occurred. - Iterator* iter = db_->NewIterator(ReadOptions(false, true)); + Iterator* iter = db_->NewIterator(read_options); for (iter->SeekToFirst(); iter->Valid(); iter->Next()) { ASSERT_OK(iter->status()); uint64_t key; @@ -515,7 +519,7 @@ TEST_F(CorruptionTest, TableFileIndexData) { dbi = static_cast_with_check(db_); // one full file may be readable, since only one was corrupted // the other file should be fully non-readable, since index was corrupted - Check(0, 5000); + Check(0, 5000, ReadOptions(true, true)); ASSERT_NOK(dbi->VerifyChecksum()); // In paranoid mode, the db cannot be opened due to the corrupted file. diff --git a/table/block_based/block_based_table_reader.cc b/table/block_based/block_based_table_reader.cc index f5421a829..029a67eec 100644 --- a/table/block_based/block_based_table_reader.cc +++ b/table/block_based/block_based_table_reader.cc @@ -576,15 +576,13 @@ Status BlockBasedTable::Open( Footer footer; std::unique_ptr prefetch_buffer; - // From read_options, retain deadline, io_timeout, and rate_limiter_priority. - // In future, we may retain more - // options. Specifically, we ignore verify_checksums and default to - // checksum verification anyway when creating the index and filter - // readers. + // From read_options, retain deadline, io_timeout, rate_limiter_priority, and + // verify_checksums. In future, we may retain more options. ReadOptions ro; ro.deadline = read_options.deadline; ro.io_timeout = read_options.io_timeout; ro.rate_limiter_priority = read_options.rate_limiter_priority; + ro.verify_checksums = read_options.verify_checksums; // prefetch both index and filters, down to all partitions const bool prefetch_all = prefetch_index_and_filter_in_cache || level == 0;