From e5c614e1df5a81ec675b18536e4c48c80caddbea Mon Sep 17 00:00:00 2001
From: Victor Tyutyunov <vat@fb.com>
Date: Tue, 12 Apr 2016 10:35:15 -0700
Subject: [PATCH] Fixing snapshot 0 assertion

Summary:
Solution is not to change db sequence number to start from 1 because 0 value is used in multiple other places.
Fix covers only compact_iterator::findEarliestVisibleSnapshot with updated logic to support snapshot's numbering starting from 0.

Test Plan:
run:
  make all check

it should pass all tests

Reviewers: IslamAbdelRahman, sdong

Reviewed By: sdong

Subscribers: lgalanis, mgalushka, andrewkr, dhruba

Differential Revision: https://reviews.facebook.net/D56601
---
 db/compaction_iterator.cc |  8 ++++----
 db/db_test2.cc            | 24 ++++++------------------
 2 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/db/compaction_iterator.cc b/db/compaction_iterator.cc
index a649a24a6..bb4c87649 100644
--- a/db/compaction_iterator.cc
+++ b/db/compaction_iterator.cc
@@ -423,15 +423,15 @@ void CompactionIterator::PrepareOutput() {
 inline SequenceNumber CompactionIterator::findEarliestVisibleSnapshot(
     SequenceNumber in, SequenceNumber* prev_snapshot) {
   assert(snapshots_->size());
-  SequenceNumber prev __attribute__((unused)) = 0;
+  SequenceNumber prev __attribute__((__unused__)) = kMaxSequenceNumber;
   for (const auto cur : *snapshots_) {
-    assert(prev <= cur);
+    assert(prev == kMaxSequenceNumber || prev <= cur);
     if (cur >= in) {
-      *prev_snapshot = prev;
+      *prev_snapshot = prev == kMaxSequenceNumber ? 0 : prev;
       return cur;
     }
     prev = cur;
-    assert(prev);
+    assert(prev < kMaxSequenceNumber);
   }
   *prev_snapshot = prev;
   return kMaxSequenceNumber;
diff --git a/db/db_test2.cc b/db/db_test2.cc
index 17150104d..7c02a6ffe 100644
--- a/db/db_test2.cc
+++ b/db/db_test2.cc
@@ -452,8 +452,7 @@ TEST_F(DBTest2, WalFilterTestWithChangeBatch) {
 
   for (size_t i = 0; i < batch_keys.size(); i++) {
     for (size_t j = 0; j < batch_keys[i].size(); j++) {
-      if (i >= change_records_from_index && j >= 
-        num_keys_to_add_in_new_batch) {
+      if (i >= change_records_from_index && j >= num_keys_to_add_in_new_batch) {
         keys_must_not_exist.push_back(Slice(batch_keys[i][j]));
       }
       else {
@@ -528,8 +527,7 @@ TEST_F(DBTest2, WalFilterTestWithChangeBatchExtraKeys) {
   // Reopen database with option to use WAL filter
   options = OptionsForLogIterTest();
   options.wal_filter = &test_wal_filter_extra_keys;
-  Status status = TryReopenWithColumnFamilies({ "default", "pikachu" }, 
-    options);
+  Status status = TryReopenWithColumnFamilies({"default", "pikachu"}, options);
   ASSERT_TRUE(status.IsNotSupported());
 
   // Reopen without filter, now reopen should succeed - previous
@@ -612,8 +610,7 @@ TEST_F(DBTest2, WalFilterTestWithColumnFamilies) {
       return "WalFilterTestWithColumnFamilies";
     }
 
-    const std::map<uint32_t, std::vector<std::string>> & 
-    GetColumnFamilyKeys() {
+    const std::map<uint32_t, std::vector<std::string>>& GetColumnFamilyKeys() {
       return cf_wal_keys_;
     }
 
@@ -684,8 +681,7 @@ TEST_F(DBTest2, WalFilterTestWithColumnFamilies) {
   // verify that handles_[0] only has post_flush keys
   // while handles_[1] has pre and post flush keys
   auto cf_wal_keys = test_wal_filter_column_families.GetColumnFamilyKeys();
-  auto name_id_map = 
-    test_wal_filter_column_families.GetColumnFamilyNameIdMap();
+  auto name_id_map = test_wal_filter_column_families.GetColumnFamilyNameIdMap();
   size_t index = 0;
   auto keys_cf = cf_wal_keys[name_id_map[kDefaultColumnFamilyName]];
   //default column-family, only post_flush keys are expected
@@ -720,21 +716,13 @@ TEST_F(DBTest2, WalFilterTestWithColumnFamilies) {
 }
 #endif  // ROCKSDB_LITE
 
-TEST_F(DBTest2, DISABLED_FirstSnapshotTest) {
+TEST_F(DBTest2, FirstSnapshotTest) {
   Options options;
   options.write_buffer_size = 100000;  // Small write buffer
   options = CurrentOptions(options);
   CreateAndReopenWithCF({"pikachu"}, options);
 
-  // This snapshot will have sequence number 0.  When compaction encounters
-  // this snapshot, CompactionIterator::findEarliestVisibleSnapshot() will
-  // assert as it expects non-zero snapshots.
-  //
-  // One fix would be to simply remove this assert.  However, a better fix
-  // might
-  // be to always have db sequence numbers start from 1 so that no code is
-  // ever
-  // confused by 0.
+  // This snapshot will have sequence number 0 what is expected behaviour.
   const Snapshot* s1 = db_->GetSnapshot();
 
   Put(1, "k1", std::string(100000, 'x'));  // Fill memtable
-- 
GitLab