From c79e43d0dc9caabbae94d6d400655185059a54ce Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Tue, 21 Jun 2022 16:23:58 -0700 Subject: [PATCH] Add data block hash index to crash test, fix MultiGet issue (#10220) Summary: There was a bug in the MultiGet enhancement in https://github.com/facebook/rocksdb/issues/9899 with data block hash index, which was not caught because data block hash index was never added to stress tests. This change fixes both issues. Fixes https://github.com/facebook/rocksdb/issues/10186 I intend to pick this into the 7.4.0 release candidate Pull Request resolved: https://github.com/facebook/rocksdb/pull/10220 Test Plan: Failure quickly reproduces in crash test with kDataBlockBinaryAndHash, and does not seem to with the fix. Reproducing the failure with a unit test I believe would be too tricky and fragile to be worthwhile. Reviewed By: anand1976 Differential Revision: D37315647 Pulled By: pdillinger fbshipit-source-id: 9f648265bba867275edc752f7a56611a59401cba --- HISTORY.md | 1 + db_stress_tool/db_stress_common.h | 1 + db_stress_tool/db_stress_gflags.cc | 8 +++++++- db_stress_tool/db_stress_test_base.cc | 3 +++ .../block_based_table_reader_sync_and_async.h | 18 +++++++++--------- tools/db_crashtest.py | 1 + 6 files changed, 22 insertions(+), 10 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 1dfdbe2d9c6..36654240c04 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -13,6 +13,7 @@ * Avoid a crash if the IDENTITY file is accidentally truncated to empty. A new DB ID will be written and generated on Open. * Fixed a possible corruption for users of `manual_wal_flush` and/or `FlushWAL(true /* sync */)`, together with `track_and_verify_wals_in_manifest == true`. For those users, losing unsynced data (e.g., due to power loss) could make future DB opens fail with a `Status::Corruption` complaining about missing WAL data. * Fixed a bug in `WriteBatchInternal::Append()` where WAL termination point in write batch was not considered and the function appends an incorrect number of checksums. +* Fixed a crash bug introduced in 7.3.0 affecting users of MultiGet with `kDataBlockBinaryAndHash`. ### Public API changes * Add new API GetUnixTime in Snapshot class which returns the unix time at which Snapshot is taken. diff --git a/db_stress_tool/db_stress_common.h b/db_stress_tool/db_stress_common.h index 1dd99884fc3..c00a69d881b 100644 --- a/db_stress_tool/db_stress_common.h +++ b/db_stress_tool/db_stress_common.h @@ -157,6 +157,7 @@ DECLARE_bool(partition_filters); DECLARE_bool(optimize_filters_for_memory); DECLARE_bool(detect_filter_construct_corruption); DECLARE_int32(index_type); +DECLARE_int32(data_block_index_type); DECLARE_string(db); DECLARE_string(secondaries_base); DECLARE_bool(test_secondary); diff --git a/db_stress_tool/db_stress_gflags.cc b/db_stress_tool/db_stress_gflags.cc index b689028120e..26e8c1ebb53 100644 --- a/db_stress_tool/db_stress_gflags.cc +++ b/db_stress_tool/db_stress_gflags.cc @@ -493,9 +493,15 @@ DEFINE_bool( DEFINE_int32( index_type, static_cast( - ROCKSDB_NAMESPACE::BlockBasedTableOptions::kBinarySearch), + ROCKSDB_NAMESPACE::BlockBasedTableOptions().index_type), "Type of block-based table index (see `enum IndexType` in table.h)"); +DEFINE_int32( + data_block_index_type, + static_cast( + ROCKSDB_NAMESPACE::BlockBasedTableOptions().data_block_index_type), + "Index type for data blocks (see `enum DataBlockIndexType` in table.h)"); + DEFINE_string(db, "", "Use the db with the following name."); DEFINE_string(secondaries_base, "", diff --git a/db_stress_tool/db_stress_test_base.cc b/db_stress_tool/db_stress_test_base.cc index 91036dc22c7..97e4b4cb0e5 100644 --- a/db_stress_tool/db_stress_test_base.cc +++ b/db_stress_tool/db_stress_test_base.cc @@ -2775,6 +2775,9 @@ void InitializeOptionsFromFlags( FLAGS_detect_filter_construct_corruption; block_based_options.index_type = static_cast(FLAGS_index_type); + block_based_options.data_block_index_type = + static_cast( + FLAGS_data_block_index_type); block_based_options.prepopulate_block_cache = static_cast( FLAGS_prepopulate_block_cache); diff --git a/table/block_based/block_based_table_reader_sync_and_async.h b/table/block_based/block_based_table_reader_sync_and_async.h index 450979dd7f4..208a0783b9b 100644 --- a/table/block_based/block_based_table_reader_sync_and_async.h +++ b/table/block_based/block_based_table_reader_sync_and_async.h @@ -610,15 +610,6 @@ DEFINE_SYNC_AND_ASYNC(void, BlockBasedTable::MultiGet) break; } - bool may_exist = biter->SeekForGet(key); - if (!may_exist) { - // HashSeek cannot find the key this block and the the iter is not - // the end of the block, i.e. cannot be in the following blocks - // either. In this case, the seek_key cannot be found, so we break - // from the top level for-loop. - break; - } - // Reusing blocks complicates pinning/Cleanable, because the cache // entry referenced by biter can only be released once all returned // pinned values are released. This code previously did an extra @@ -661,6 +652,15 @@ DEFINE_SYNC_AND_ASYNC(void, BlockBasedTable::MultiGet) value_pinner = nullptr; } + bool may_exist = biter->SeekForGet(key); + if (!may_exist) { + // HashSeek cannot find the key this block and the the iter is not + // the end of the block, i.e. cannot be in the following blocks + // either. In this case, the seek_key cannot be found, so we break + // from the top level for-loop. + break; + } + // Call the *saver function on each entry/block until it returns false for (; biter->Valid(); biter->Next()) { ParsedInternalKey parsed_key; diff --git a/tools/db_crashtest.py b/tools/db_crashtest.py index 579885fd582..4d174b9fcaa 100644 --- a/tools/db_crashtest.py +++ b/tools/db_crashtest.py @@ -63,6 +63,7 @@ "clear_column_family_one_in": 0, "compact_files_one_in": 1000000, "compact_range_one_in": 1000000, + "data_block_index_type": lambda: random.choice([0, 1]), "delpercent": 4, "delrangepercent": 1, "destroy_db_initially": 0,