diff --git a/utilities/transactions/write_prepared_transaction_test.cc b/utilities/transactions/write_prepared_transaction_test.cc index e4badf793..6af9ac085 100644 --- a/utilities/transactions/write_prepared_transaction_test.cc +++ b/utilities/transactions/write_prepared_transaction_test.cc @@ -1239,6 +1239,37 @@ 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()); +} + 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 c0cf42550..a86e3b004 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 d053b80a8..60fa28f62 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;