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

@@ -124,8 +124,9 @@ simple_default_params = {
     "max_background_compactions": 1,
     "max_bytes_for_level_base": 67108864,
     "memtablerep": "skip_list",
-    "prefixpercent": 25,
+    "prefixpercent": 0,
-    "readpercent": 25,
+    "readpercent": 50,
+    "prefix_size" : -1,
     "target_file_size_base": 16777216,
     "target_file_size_multiplier": 1,
     "test_batches_snapshots": 0,

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) {
+  if (value < -1 || value > 8) {
-    fprintf(stderr, "Invalid value for --%s: %d. 0 <= PrefixSize <= 8\n",
+    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<int>(
+            std::min(static_cast<uint64_t>(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<Iterator> iter(db_->NewIterator(readoptionscopy, cfh));
 
-    std::string key_str = Key(rand_keys[0]);
+    for (int64_t rkey : rand_keys) {
-    Slice key = key_str;
+      std::string key_str = Key(rkey);
-    iter->Seek(key);
+      Slice key = key_str;
-    for (uint64_t i = 0; i < FLAGS_num_iterations && iter->Valid(); i++) {
+
-      if (thread->rand.OneIn(2)) {
+      if (readoptionscopy.iterate_upper_bound != nullptr &&
-        iter->Next();
+          thread->rand.OneIn(2)) {
-      } else {
+        // 1/2 chance, change the upper bound.
-        iter->Prev();
+        // 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()) {
+      // Set up an iterator and does the same without bounds and with total
-      thread->stats.AddIterations(1);
+      // order seek and compare the results. This is to identify bugs related
-    } else {
+      // to bounds, prefix extractor or reseeking. Sometimes we are comparing
-      thread->stats.AddErrors(1);
+      // 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<Iterator> 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<rocksdb::CompactionStyle>(FLAGS_compaction_style);
-      options_.prefix_extractor.reset(
+      if (FLAGS_prefix_size >= 0) {
-          NewFixedPrefixTransform(FLAGS_prefix_size));
+        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
+        // we continue after error rather than exiting so that we can
-      // find more errors if any
+        // 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");