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

<img width="1724" alt="204511641-86ab4b9b-45e5-4a2b-a13d-81fa26435d38" src="https://user-images.githubusercontent.com/26581503/213152802-c46bc1c7-a3a2-4a6f-9bb1-bf92ee93af7a.png">

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
oxigraph-8.1.1
Karim TAAM 2 years ago committed by Facebook GitHub Bot
parent b44cbbf709
commit a1e92bd956
  1. 4
      HISTORY.md
  2. 8
      db/corruption_test.cc
  3. 8
      table/block_based/block_based_table_reader.cc

@ -1,5 +1,9 @@
# Rocksdb Change Log # Rocksdb Change Log
## Unreleased ## Unreleased
### Behavior changes
* `ReadOptions::verify_checksums=false` disables checksum verification for more reads of non-`CacheEntryRole::kDataBlock` blocks.
### Bug Fixes ### Bug Fixes
* Fixed a data race on `ColumnFamilyData::flush_reason` caused by concurrent flushes. * Fixed a data race on `ColumnFamilyData::flush_reason` caused by concurrent flushes.

@ -168,6 +168,10 @@ class CorruptionTest : public testing::Test {
void Build(int n, int flush_every = 0) { Build(n, 0, flush_every); } void Build(int n, int flush_every = 0) { Build(n, 0, flush_every); }
void Check(int min_expected, int max_expected) { 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 next_expected = 0;
uint64_t missed = 0; uint64_t missed = 0;
int bad_keys = 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 // Instead, we want the reads to be successful and this test
// will detect whether the appropriate corruptions have // will detect whether the appropriate corruptions have
// occurred. // occurred.
Iterator* iter = db_->NewIterator(ReadOptions(false, true)); Iterator* iter = db_->NewIterator(read_options);
for (iter->SeekToFirst(); iter->Valid(); iter->Next()) { for (iter->SeekToFirst(); iter->Valid(); iter->Next()) {
ASSERT_OK(iter->status()); ASSERT_OK(iter->status());
uint64_t key; uint64_t key;
@ -515,7 +519,7 @@ TEST_F(CorruptionTest, TableFileIndexData) {
dbi = static_cast_with_check<DBImpl>(db_); dbi = static_cast_with_check<DBImpl>(db_);
// one full file may be readable, since only one was corrupted // one full file may be readable, since only one was corrupted
// the other file should be fully non-readable, since index 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()); ASSERT_NOK(dbi->VerifyChecksum());
// In paranoid mode, the db cannot be opened due to the corrupted file. // In paranoid mode, the db cannot be opened due to the corrupted file.

@ -576,15 +576,13 @@ Status BlockBasedTable::Open(
Footer footer; Footer footer;
std::unique_ptr<FilePrefetchBuffer> prefetch_buffer; std::unique_ptr<FilePrefetchBuffer> prefetch_buffer;
// From read_options, retain deadline, io_timeout, and rate_limiter_priority. // From read_options, retain deadline, io_timeout, rate_limiter_priority, and
// In future, we may retain more // verify_checksums. In future, we may retain more options.
// options. Specifically, we ignore verify_checksums and default to
// checksum verification anyway when creating the index and filter
// readers.
ReadOptions ro; ReadOptions ro;
ro.deadline = read_options.deadline; ro.deadline = read_options.deadline;
ro.io_timeout = read_options.io_timeout; ro.io_timeout = read_options.io_timeout;
ro.rate_limiter_priority = read_options.rate_limiter_priority; 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 // prefetch both index and filters, down to all partitions
const bool prefetch_all = prefetch_index_and_filter_in_cache || level == 0; const bool prefetch_all = prefetch_index_and_filter_in_cache || level == 0;

Loading…
Cancel
Save