From 1f7e8fc1cee79808cb53f977322bc31671eaed0f Mon Sep 17 00:00:00 2001 From: myabandeh Date: Mon, 4 Feb 2019 11:01:22 -0800 Subject: [PATCH 1/2] WritePrepared: release snapshot equal to max --- .../write_prepared_transaction_test.cc | 23 +++++++++++++++++++ .../transactions/write_prepared_txn_db.cc | 6 ++--- .../transactions/write_prepared_txn_db.h | 1 + 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/utilities/transactions/write_prepared_transaction_test.cc b/utilities/transactions/write_prepared_transaction_test.cc index e4badf79318..989105fdf16 100644 --- a/utilities/transactions/write_prepared_transaction_test.cc +++ b/utilities/transactions/write_prepared_transaction_test.cc @@ -1239,6 +1239,29 @@ TEST_P(WritePreparedTransactionTest, MaxCatchupWithNewSnapshot) { db->ReleaseSnapshot(snap); } +TEST_P(WritePreparedTransactionTest, CleanupSnapshotEqualToMax) { + const size_t snapshot_cache_bits = 7; // same as default + const size_t commit_cache_bits = 0; // only 1 entry => frequent eviction + DestroyAndReopenWithExtraOptions(snapshot_cache_bits, commit_cache_bits); + WriteOptions woptions; + WritePreparedTxnDB* wp_db = dynamic_cast(db); + ASSERT_OK(db->Put(woptions, "key", "value")); + auto snap = db->GetSnapshot(); + auto snap_seq = snap->GetSequenceNumber(); + ASSERT_OK(db->Put(woptions, "key", "value")); + ASSERT_EQ(snap_seq, wp_db->max_evicted_seq_); + ASSERT_EQ(1, wp_db->snapshots_total_); + ASSERT_EQ(1, wp_db->old_commit_map_.size()); + + db->ReleaseSnapshot(snap); + + ASSERT_OK(db->Put(woptions, "key", "value")); + + ASSERT_EQ(0, wp_db->snapshots_total_); + ASSERT_TRUE(wp_db->snapshots_all_.empty()); + ASSERT_EQ(0, wp_db->old_commit_map_.size()); +} + TEST_P(WritePreparedTransactionTest, AdvanceSeqByOne) { auto snap = db->GetSnapshot(); auto seq1 = snap->GetSequenceNumber(); diff --git a/utilities/transactions/write_prepared_txn_db.cc b/utilities/transactions/write_prepared_txn_db.cc index c0cf4255025..a86e3b00421 100644 --- a/utilities/transactions/write_prepared_txn_db.cc +++ b/utilities/transactions/write_prepared_txn_db.cc @@ -710,9 +710,9 @@ const std::vector WritePreparedTxnDB::GetSnapshotListFromDB( void WritePreparedTxnDB::ReleaseSnapshotInternal( const SequenceNumber snap_seq) { - // relax is enough since max increases monotonically, i.e., if snap_seq < - // old_max => snap_seq < new_max as well. - if (snap_seq < max_evicted_seq_.load(std::memory_order_relaxed)) { + // TODO(myabandeh): relax should enough since the synchronizatin is already + // done by snapshots_mutex_ under which this function is called. + if (snap_seq <= max_evicted_seq_.load(std::memory_order_acquire)) { // Then this is a rare case that transaction did not finish before max // advances. It is expected for a few read-only backup snapshots. For such // snapshots we might have kept around a couple of entries in the diff --git a/utilities/transactions/write_prepared_txn_db.h b/utilities/transactions/write_prepared_txn_db.h index d053b80a8d2..60fa28f627f 100644 --- a/utilities/transactions/write_prepared_txn_db.h +++ b/utilities/transactions/write_prepared_txn_db.h @@ -414,6 +414,7 @@ class WritePreparedTxnDB : public PessimisticTransactionDB { WritePreparedTransactionTest_AdvanceMaxEvictedSeqWithDuplicatesTest_Test; friend class WritePreparedTransactionTest_AdvanceSeqByOne_Test; friend class WritePreparedTransactionTest_BasicRecoveryTest_Test; + friend class WritePreparedTransactionTest_CleanupSnapshotEqualToMax_Test; friend class WritePreparedTransactionTest_DoubleSnapshot_Test; friend class WritePreparedTransactionTest_IsInSnapshotEmptyMapTest_Test; friend class WritePreparedTransactionTest_IsInSnapshotReleased_Test; From 97615ab61852c53cfaab4ae139ebc931bb4d251f Mon Sep 17 00:00:00 2001 From: myabandeh Date: Mon, 4 Feb 2019 11:18:08 -0800 Subject: [PATCH 2/2] add comments --- utilities/transactions/write_prepared_transaction_test.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/utilities/transactions/write_prepared_transaction_test.cc b/utilities/transactions/write_prepared_transaction_test.cc index 989105fdf16..6af9ac08588 100644 --- a/utilities/transactions/write_prepared_transaction_test.cc +++ b/utilities/transactions/write_prepared_transaction_test.cc @@ -1239,24 +1239,32 @@ TEST_P(WritePreparedTransactionTest, MaxCatchupWithNewSnapshot) { db->ReleaseSnapshot(snap); } +// Check that old_commit_map_ cleanup works correctly if the snapshot equals +// max_evicted_seq_. TEST_P(WritePreparedTransactionTest, CleanupSnapshotEqualToMax) { const size_t snapshot_cache_bits = 7; // same as default const size_t commit_cache_bits = 0; // only 1 entry => frequent eviction DestroyAndReopenWithExtraOptions(snapshot_cache_bits, commit_cache_bits); WriteOptions woptions; WritePreparedTxnDB* wp_db = dynamic_cast(db); + // Insert something to increase seq ASSERT_OK(db->Put(woptions, "key", "value")); auto snap = db->GetSnapshot(); auto snap_seq = snap->GetSequenceNumber(); + // Another insert should trigger eviction + load snapshot from db ASSERT_OK(db->Put(woptions, "key", "value")); + // This is the scenario that we check agaisnt ASSERT_EQ(snap_seq, wp_db->max_evicted_seq_); + // old_commit_map_ now has some data that needs gc ASSERT_EQ(1, wp_db->snapshots_total_); ASSERT_EQ(1, wp_db->old_commit_map_.size()); db->ReleaseSnapshot(snap); + // Another insert should trigger eviction + load snapshot from db ASSERT_OK(db->Put(woptions, "key", "value")); + // the snapshot and related metadata must be properly garbage collected ASSERT_EQ(0, wp_db->snapshots_total_); ASSERT_TRUE(wp_db->snapshots_all_.empty()); ASSERT_EQ(0, wp_db->old_commit_map_.size());