From cd227d74ba8430041a5168ed7ffb5758a9e00173 Mon Sep 17 00:00:00 2001 From: Maysam Yabandeh Date: Tue, 8 Jan 2019 11:23:10 -0800 Subject: [PATCH] WritePrepared: improve IsInSnapshotEmptyMapTest (#4853) Summary: IsInSnapshotEmptyMapTest tests that IsInSnapshot returns correct value for existing data after a recovery, where max is not zero and yet commit cache is empty. The existing test was preliminary which is improved in this patch. It also increases the db sequence after recovery so that there the snapshot immediately taken after recovery would have a sequence number different than that of max_evicted_seq. This simplifies the logic in IsInSnapshot by not having to consider the special case that an old snapshot might be equal to max_evicted_seq and yet not present in old_commit_map. Pull Request resolved: https://github.com/facebook/rocksdb/pull/4853 Differential Revision: D13595223 Pulled By: maysamyabandeh fbshipit-source-id: 77c12ca8a3f61a47479a93bef2038ff502dc3322 --- .../write_prepared_transaction_test.cc | 72 +++++++++++++++---- .../transactions/write_prepared_txn_db.cc | 8 +++ 2 files changed, 67 insertions(+), 13 deletions(-) diff --git a/utilities/transactions/write_prepared_transaction_test.cc b/utilities/transactions/write_prepared_transaction_test.cc index 1d645d237..51110292a 100644 --- a/utilities/transactions/write_prepared_transaction_test.cc +++ b/utilities/transactions/write_prepared_transaction_test.cc @@ -1267,12 +1267,12 @@ TEST_P(SeqAdvanceConcurrentTest, SeqAdvanceConcurrentTest) { assert(db != nullptr); db_impl = reinterpret_cast(db->GetRootDB()); seq = db_impl->TEST_GetLastVisibleSequence(); - ASSERT_EQ(exp_seq, seq); + ASSERT_LE(exp_seq, seq); // Check if flush preserves the last sequence number db_impl->Flush(fopt); seq = db_impl->GetLatestSequenceNumber(); - ASSERT_EQ(exp_seq, seq); + ASSERT_LE(exp_seq, seq); // Check if recovery after flush preserves the last sequence number db_impl->FlushWAL(true); @@ -1280,7 +1280,7 @@ TEST_P(SeqAdvanceConcurrentTest, SeqAdvanceConcurrentTest) { assert(db != nullptr); db_impl = reinterpret_cast(db->GetRootDB()); seq = db_impl->GetLatestSequenceNumber(); - ASSERT_EQ(exp_seq, seq); + ASSERT_LE(exp_seq, seq); } } @@ -1425,17 +1425,63 @@ TEST_P(WritePreparedTransactionTest, BasicRecoveryTest) { } // After recovery the commit map is empty while the max is set. The code would -// go through a different path which requires a separate test. +// go through a different path which requires a separate test. Test that the +// committed data before the restart is visible to all snapshots. TEST_P(WritePreparedTransactionTest, IsInSnapshotEmptyMapTest) { - WritePreparedTxnDB* wp_db = dynamic_cast(db); - wp_db->max_evicted_seq_ = 100; - ASSERT_FALSE(wp_db->IsInSnapshot(50, 40)); - ASSERT_TRUE(wp_db->IsInSnapshot(50, 50)); - ASSERT_TRUE(wp_db->IsInSnapshot(50, 100)); - ASSERT_TRUE(wp_db->IsInSnapshot(50, 150)); - ASSERT_FALSE(wp_db->IsInSnapshot(100, 80)); - ASSERT_TRUE(wp_db->IsInSnapshot(100, 100)); - ASSERT_TRUE(wp_db->IsInSnapshot(100, 150)); + for (bool end_with_prepare : {false, true}) { + ReOpen(); + WriteOptions woptions; + ASSERT_OK(db->Put(woptions, "key", "value")); + ASSERT_OK(db->Put(woptions, "key", "value")); + ASSERT_OK(db->Put(woptions, "key", "value")); + SequenceNumber prepare_seq = kMaxSequenceNumber; + if (end_with_prepare) { + TransactionOptions txn_options; + Transaction* txn = db->BeginTransaction(woptions, txn_options); + ASSERT_OK(txn->SetName("xid0")); + ASSERT_OK(txn->Prepare()); + prepare_seq = txn->GetId(); + delete txn; + } + dynamic_cast(db)->TEST_Crash(); + auto db_impl = reinterpret_cast(db->GetRootDB()); + db_impl->FlushWAL(true); + ReOpenNoDelete(); + WritePreparedTxnDB* wp_db = dynamic_cast(db); + ASSERT_GT(wp_db->max_evicted_seq_, 0); // max after recovery + // Take a snapshot right after recovery + const Snapshot* snap = db->GetSnapshot(); + auto snap_seq = snap->GetSequenceNumber(); + ASSERT_GT(snap_seq, 0); + + for (SequenceNumber seq = 0; + seq <= wp_db->max_evicted_seq_ && seq != prepare_seq; seq++) { + ASSERT_TRUE(wp_db->IsInSnapshot(seq, snap_seq)); + } + if (end_with_prepare) { + ASSERT_FALSE(wp_db->IsInSnapshot(prepare_seq, snap_seq)); + } + // trivial check + ASSERT_FALSE(wp_db->IsInSnapshot(snap_seq + 1, snap_seq)); + + db->ReleaseSnapshot(snap); + + ASSERT_OK(db->Put(woptions, "key", "value")); + // Take a snapshot after some writes + snap = db->GetSnapshot(); + snap_seq = snap->GetSequenceNumber(); + for (SequenceNumber seq = 0; + seq <= wp_db->max_evicted_seq_ && seq != prepare_seq; seq++) { + ASSERT_TRUE(wp_db->IsInSnapshot(seq, snap_seq)); + } + if (end_with_prepare) { + ASSERT_FALSE(wp_db->IsInSnapshot(prepare_seq, snap_seq)); + } + // trivial check + ASSERT_FALSE(wp_db->IsInSnapshot(snap_seq + 1, snap_seq)); + + db->ReleaseSnapshot(snap); + } } // Test WritePreparedTxnDB's IsInSnapshot against different ordering of diff --git a/utilities/transactions/write_prepared_txn_db.cc b/utilities/transactions/write_prepared_txn_db.cc index ca728d507..fba2af79f 100644 --- a/utilities/transactions/write_prepared_txn_db.cc +++ b/utilities/transactions/write_prepared_txn_db.cc @@ -49,6 +49,14 @@ Status WritePreparedTxnDB::Initialize( SequenceNumber prev_max = max_evicted_seq_; SequenceNumber last_seq = db_impl_->GetLatestSequenceNumber(); AdvanceMaxEvictedSeq(prev_max, last_seq); + // Create a gap between max and the next snapshot. This simplifies the logic + // in IsInSnapshot by not having to consider the special case of max == + // snapshot after recovery. This is tested in IsInSnapshotEmptyMapTest. + if (last_seq) { + db_impl_->versions_->SetLastAllocatedSequence(last_seq + 1); + db_impl_->versions_->SetLastSequence(last_seq + 1); + db_impl_->versions_->SetLastPublishedSequence(last_seq + 1); + } db_impl_->SetSnapshotChecker(new WritePreparedSnapshotChecker(this)); // A callback to commit a single sub-batch