From 00007a98d2b74c00d2c9e761e2058178a789ede4 Mon Sep 17 00:00:00 2001 From: sdong Date: Fri, 27 Sep 2019 11:09:06 -0700 Subject: [PATCH] crash_test to do some verification for prefix extractor and iterator bounds. (#5846) Summary: For now, crash_test is not able to report any failure for the logic related to iterator upper, lower bounds or iterators, or reseek. These are features prone to errors. Improve db_stress in several ways: (1) For each iterator run, reseek up to 3 times. (2) For every iterator, create control iterator with upper or lower bound, with total order seek. Compare the results with the iterator. (3) Make simple crash test to avoid prefix size to have more coverage. (4) make prefix_size = 0 a valid size and -1 to indicate disabling prefix extractor. Pull Request resolved: https://github.com/facebook/rocksdb/pull/5846 Test Plan: Manually hack the code to create wrong results and see they are caught by the tool. Differential Revision: D17631760 fbshipit-source-id: acd460a177bd2124a5ffd7fff490702dba63030b --- tools/db_crashtest.py | 5 +- tools/db_stress.cc | 187 ++++++++++++++++++++++++++++++++++++------ 2 files changed, 167 insertions(+), 25 deletions(-) diff --git a/tools/db_crashtest.py b/tools/db_crashtest.py index 4013910ab2c..75cf95b2f7d 100644 --- a/tools/db_crashtest.py +++ b/tools/db_crashtest.py @@ -124,8 +124,9 @@ def is_direct_io_supported(dbname): "max_background_compactions": 1, "max_bytes_for_level_base": 67108864, "memtablerep": "skip_list", - "prefixpercent": 25, - "readpercent": 25, + "prefixpercent": 0, + "readpercent": 50, + "prefix_size" : -1, "target_file_size_base": 16777216, "target_file_size_multiplier": 1, "test_batches_snapshots": 0, diff --git a/tools/db_stress.cc b/tools/db_stress.cc index cb8f96765ac..30a62e7f2f7 100644 --- a/tools/db_stress.cc +++ b/tools/db_stress.cc @@ -693,14 +693,16 @@ static enum RepFactory FLAGS_rep_factory; DEFINE_string(memtablerep, "prefix_hash", ""); static bool ValidatePrefixSize(const char* flagname, int32_t value) { - if (value < 0 || value > 8) { - fprintf(stderr, "Invalid value for --%s: %d. 0 <= PrefixSize <= 8\n", + if (value < -1 || value > 8) { + fprintf(stderr, "Invalid value for --%s: %d. -1 <= PrefixSize <= 8\n", flagname, value); return false; } return true; } -DEFINE_int32(prefix_size, 7, "Control the prefix size for HashSkipListRep"); +DEFINE_int32(prefix_size, 7, + "Control the prefix size for HashSkipListRep. " + "-1 is disabled."); static const bool FLAGS_prefix_size_dummy __attribute__((__unused__)) = RegisterFlagValidator(&FLAGS_prefix_size, &ValidatePrefixSize); @@ -2348,6 +2350,11 @@ class StressTest { lock); } else { // OPERATION iterate + int num_seeks = static_cast( + std::min(static_cast(thread->rand.Uniform(4)), + FLAGS_ops_per_thread - i - 1)); + rand_keys = GenerateNKeys(thread, num_seeks, i); + i += num_seeks - 1; TestIterate(thread, read_opts, rand_column_families, rand_keys); } thread->stats.FinishedSingleOp(); @@ -2445,7 +2452,7 @@ class StressTest { std::string lower_bound_str; Slice lower_bound; if (thread->rand.OneIn(16)) { - // in 1/16 chance, set a iterator lower bound + // in 1/16 chance, enable iterator lower bound int64_t rand_lower_key = GenerateOneKey(thread, FLAGS_ops_per_thread); lower_bound_str = Key(rand_lower_key); lower_bound = Slice(lower_bound_str); @@ -2457,21 +2464,61 @@ class StressTest { auto cfh = column_families_[rand_column_families[0]]; std::unique_ptr iter(db_->NewIterator(readoptionscopy, cfh)); - std::string key_str = Key(rand_keys[0]); - Slice key = key_str; - iter->Seek(key); - for (uint64_t i = 0; i < FLAGS_num_iterations && iter->Valid(); i++) { - if (thread->rand.OneIn(2)) { - iter->Next(); - } else { - iter->Prev(); + for (int64_t rkey : rand_keys) { + std::string key_str = Key(rkey); + Slice key = key_str; + + if (readoptionscopy.iterate_upper_bound != nullptr && + thread->rand.OneIn(2)) { + // 1/2 chance, change the upper bound. + // It is possible that it is changed without first use, but there is no + // problem with that. + int64_t rand_upper_key = GenerateOneKey(thread, FLAGS_ops_per_thread); + upper_bound_str = Key(rand_upper_key); + upper_bound = Slice(upper_bound_str); } - } - if (s.ok()) { - thread->stats.AddIterations(1); - } else { - thread->stats.AddErrors(1); + // Set up an iterator and does the same without bounds and with total + // order seek and compare the results. This is to identify bugs related + // to bounds, prefix extractor or reseeking. Sometimes we are comparing + // iterators with the same set-up, and it doesn't hurt to check them + // to be equal. + ReadOptions cmp_ro; + cmp_ro.snapshot = snapshot; + cmp_ro.total_order_seek = true; + std::unique_ptr cmp_iter( + db_->NewIterator(readoptionscopy, cfh)); + bool diverged = false; + + iter->Seek(key); + cmp_iter->Seek(key); + VerifyIterator(thread, readoptionscopy, iter.get(), cmp_iter.get(), key, + &diverged); + + for (uint64_t i = 0; i < FLAGS_num_iterations && iter->Valid(); i++) { + if (thread->rand.OneIn(2)) { + iter->Next(); + if (!diverged) { + assert(cmp_iter->Valid()); + cmp_iter->Next(); + } + } else { + iter->Prev(); + if (!diverged) { + assert(cmp_iter->Valid()); + cmp_iter->Prev(); + } + } + VerifyIterator(thread, readoptionscopy, iter.get(), cmp_iter.get(), key, + &diverged); + } + + if (s.ok()) { + thread->stats.AddIterations(1); + } else { + thread->stats.AddErrors(1); + break; + } } db_->ReleaseSnapshot(snapshot); @@ -2479,6 +2526,98 @@ class StressTest { return s; } + // Compare the two iterator, iter and cmp_iter are in the same position, + // unless iter might be made invalidate or undefined because of + // upper or lower bounds, or prefix extractor. + // Will flag failure if the verification fails. + // diverged = true if the two iterator is already diverged. + // True if verification passed, false if not. + void VerifyIterator(ThreadState* thread, const ReadOptions& ro, + Iterator* iter, Iterator* cmp_iter, const Slice& seek_key, + bool* diverged) { + if (*diverged) { + return; + } + + if (iter->Valid() && !cmp_iter->Valid()) { + fprintf(stderr, + "Control interator is invalid but iterator has value %s seek key " + "%s\n", + iter->key().ToString(true).c_str(), + seek_key.ToString(true).c_str()); + + *diverged = true; + } else if (cmp_iter->Valid()) { + // Iterator is not valid. It can be legimate if it has already been + // out of upper or lower bound, or filtered out by prefix iterator. + const Slice& total_order_key = cmp_iter->key(); + const SliceTransform* pe = options_.prefix_extractor.get(); + const Comparator* cmp = options_.comparator; + + if (pe != nullptr) { + if (!pe->InDomain(seek_key)) { + // Prefix seek a non-in-domain key is undefined. Skip checking for + // this scenario. + *diverged = true; + return; + } + + if (!pe->InDomain(total_order_key) || + pe->Transform(total_order_key) != pe->Transform(seek_key)) { + // If the prefix is exhausted, the only thing needs to check + // is the iterator isn't return a position in prefix. + // Either way, checking can stop from here. + *diverged = true; + if (!iter->Valid() || !pe->InDomain(iter->key()) || + pe->Transform(iter->key()) != pe->Transform(seek_key)) { + return; + } + fprintf(stderr, + "Iterator stays in prefix bug contol doesn't" + " seek key %s iterator key %s control iterator key %s\n", + seek_key.ToString(true).c_str(), + iter->key().ToString(true).c_str(), + cmp_iter->key().ToString(true).c_str()); + } + } + // Check upper or lower bounds. + if (!*diverged) { + if (!iter->Valid() || + (iter->key() != cmp_iter->key() && + (ro.iterate_upper_bound == nullptr || + cmp->Compare(total_order_key, *ro.iterate_upper_bound) < 0) && + (ro.iterate_lower_bound == nullptr || + cmp->Compare(total_order_key, *ro.iterate_lower_bound) > 0))) { + fprintf(stderr, + "Iterator diverged from control iterator which" + " has value %s seek key %s\n", + total_order_key.ToString(true).c_str(), + seek_key.ToString(true).c_str()); + if (iter->Valid()) { + fprintf(stderr, "iterator has value %s\n", + iter->key().ToString(true).c_str()); + } else { + fprintf(stderr, "iterator is not valid\n"); + } + if (ro.iterate_upper_bound != nullptr) { + fprintf(stderr, "upper bound %s\n", + ro.iterate_upper_bound->ToString(true).c_str()); + } + if (ro.iterate_lower_bound != nullptr) { + fprintf(stderr, "lower bound %s\n", + ro.iterate_lower_bound->ToString(true).c_str()); + } + *diverged = true; + } + } + } + if (*diverged) { + thread->stats.AddErrors(1); + // Fail fast to preserve the DB state. + thread->shared->SetVerificationFailure(); + } + } + #ifdef ROCKSDB_LITE virtual Status TestBackupRestore( ThreadState* /* thread */, @@ -2802,8 +2941,10 @@ class StressTest { options_.max_background_flushes = FLAGS_max_background_flushes; options_.compaction_style = static_cast(FLAGS_compaction_style); - options_.prefix_extractor.reset( - NewFixedPrefixTransform(FLAGS_prefix_size)); + if (FLAGS_prefix_size >= 0) { + options_.prefix_extractor.reset( + NewFixedPrefixTransform(FLAGS_prefix_size)); + } options_.max_open_files = FLAGS_open_files; options_.statistics = dbstats; options_.env = FLAGS_env; @@ -3814,8 +3955,8 @@ class BatchedOpsStressTest : public StressTest { fprintf(stderr, "error : inconsistent values for key %s: %s, %s\n", key.ToString(true).c_str(), StringToHex(values[0]).c_str(), StringToHex(values[i]).c_str()); - // we continue after error rather than exiting so that we can - // find more errors if any + // we continue after error rather than exiting so that we can + // find more errors if any } } @@ -4425,7 +4566,7 @@ int main(int argc, char** argv) { FLAGS_env->SetBackgroundThreads(FLAGS_max_background_compactions); FLAGS_env->SetBackgroundThreads(FLAGS_num_bottom_pri_threads, rocksdb::Env::Priority::BOTTOM); - if (FLAGS_prefixpercent > 0 && FLAGS_prefix_size <= 0) { + if (FLAGS_prefixpercent > 0 && FLAGS_prefix_size < 0) { fprintf(stderr, "Error: prefixpercent is non-zero while prefix_size is " "not positive!\n"); @@ -4437,7 +4578,7 @@ int main(int argc, char** argv) { "test_batches_snapshots test!\n"); exit(1); } - if (FLAGS_memtable_prefix_bloom_size_ratio > 0.0 && FLAGS_prefix_size <= 0) { + if (FLAGS_memtable_prefix_bloom_size_ratio > 0.0 && FLAGS_prefix_size < 0) { fprintf(stderr, "Error: please specify positive prefix_size in order to use " "memtable_prefix_bloom_size_ratio\n");