From db74b1a21905336e2c178ff1f2ffd12c7852b7b8 Mon Sep 17 00:00:00 2001
From: Aaron Gao <gzh@fb.com>
Date: Tue, 30 Aug 2016 22:19:42 -0700
Subject: [PATCH] fix bug in merge_iterator when data race happens

Summary:
core dump when run
`./db_stress --max_background_compactions=1 --max_write_buffer_number=3 --sync=0 --reopen=20 --write_buffer_size=33554432 --delpercent=5 --log2_keys_per_lock=10 --block_size=16384 --allow_concurrent_memtable_write=1 --test_batches_snapshots=0 --max_bytes_for_level_base=67108864 --progress_reports=0 --mmap_read=1 --kill_prefix_blacklist=WritableFileWriter::Append,WritableFileWriter::WriteBuffered --writepercent=35 --disable_data_sync=0 --readpercent=50 --subcompactions=3 --ops_per_thread=20000000 --memtablerep=skip_list --prefix_size=0 --target_file_size_multiplier=1 --column_families=1 --db=/dev/shm/rocksdb/rocksdb_crashtest_whitebox --threads=32 --disable_wal=0 --open_files=500000 --destroy_db_initially=0 --target_file_size_base=16777216 --nooverwritepercent=1 --iterpercent=10 --max_key=100000000 --prefixpercent=0 --use_clock_cache=false --kill_random_test=189 --cache_size=1048576 --verify_checksum=1`
Actually the relevant flag is `--threads`, data race when --thread > 1 cause problem.
It is possible that multiple
threads read/write memtable simultaneously. After one thread
calls Prev(), another thread may insert a new key just between
the current key and the key next, which may cause the
assert(current_ == CurrentForward()) failure when the first
thread calls Next() again if in prefix seek mode

Test Plan: rerun db_stress with >1 thread / make all check -j64

Reviewers: sdong, andrewkr, IslamAbdelRahman

Reviewed By: IslamAbdelRahman

Subscribers: andrewkr, dhruba, leveldb

Differential Revision: https://reviews.facebook.net/D62979
---
 table/merger.cc | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/table/merger.cc b/table/merger.cc
index 5733d279f..3d506a4fb 100644
--- a/table/merger.cc
+++ b/table/merger.cc
@@ -141,6 +141,9 @@ class MergingIterator : public InternalIterator {
         if (&child != current_) {
           if (prefix_extractor_ == nullptr) {
             child.Seek(key());
+            if (child.Valid() && comparator_->Equal(key(), child.key())) {
+              child.Next();
+            }
           } else {
             // only for prefix_seek_mode
             // we should not call Seek() here
@@ -150,7 +153,14 @@ class MergingIterator : public InternalIterator {
               child.SeekToFirst();
             }
           }
-          if (child.Valid() && comparator_->Equal(key(), child.key())) {
+          // This condition is needed because it is possible that multiple
+          // threads read/write memtable simultaneously. After one thread
+          // calls Prev(), another thread may insert a new key just between
+          // the current key and the key next, which may cause the
+          // assert(current_ == CurrentForward()) failure when the first
+          // thread calls Next() again if in prefix seek mode
+          while (child.Valid() &&
+                 comparator_->Compare(key(), child.key()) >= 0) {
             child.Next();
           }
         }
@@ -217,6 +227,10 @@ class MergingIterator : public InternalIterator {
             TEST_SYNC_POINT("MergeIterator::Prev:BeforeSeekToLast");
             child.SeekToLast();
           }
+          while (child.Valid() &&
+                 comparator_->Compare(key(), child.key()) <= 0) {
+            child.Prev();
+          }
         }
 
         if (child.Valid()) {
-- 
GitLab