WritePrepared: followup fix for snapshot double release issue (#4734)

Summary:
The fix in #4727 for double snapshot release was incomplete since it does not properly remove the duplicate entires in the snapshot list after finding that a snapshot is still valid. The patch does that and also improves the unit test to show the issue.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/4734

Differential Revision: D13266260

Pulled By: maysamyabandeh

fbshipit-source-id: 351e2c40cca45a87b757774c11af74182314911e

utilities/transactions/write_prepared_transaction_test.cc

@@ -747,7 +747,8 @@ TEST_P(WritePreparedTransactionTest, DoubleSnapshot) {
   ASSERT_OK(txn->SetName("txn"));
   ASSERT_OK(txn->Put("key", "value2"));
   ASSERT_OK(txn->Prepare());
-  // Two snapshots with the same seq number
+  // Three snapshots with the same seq number
+  const Snapshot* snapshot0 = wp_db->GetSnapshot();
   const Snapshot* snapshot1 = wp_db->GetSnapshot();
   const Snapshot* snapshot2 = wp_db->GetSnapshot();
   ASSERT_OK(txn->Commit());
@@ -755,7 +756,11 @@ TEST_P(WritePreparedTransactionTest, DoubleSnapshot) {
   SequenceNumber overlap_seq = txn->GetId() + cache_size;
   delete txn;
 
+  // 4th snapshot with a larger seq
+  const Snapshot* snapshot3 = wp_db->GetSnapshot();
   // Cause an eviction to advance max evicted seq number
+  // This also fetches the 4 snapshots from db since their seq is lower than the
+  // new max
   wp_db->AddCommitted(overlap_seq, overlap_seq);
 
   ReadOptions ropt;
@@ -774,7 +779,21 @@ TEST_P(WritePreparedTransactionTest, DoubleSnapshot) {
   ASSERT_OK(s);
   ASSERT_TRUE(pinnable_val == "value1");
   pinnable_val.Reset();
+
+  // Cause an eviction to advance max evicted seq number and trigger updating
+  // the snapshot list
+  overlap_seq += cache_size;
+  wp_db->AddCommitted(overlap_seq, overlap_seq);
+
+  // It should still see the value before commit
+  s = wp_db->Get(ropt, wp_db->DefaultColumnFamily(), "key", &pinnable_val);
+  ASSERT_OK(s);
+  ASSERT_TRUE(pinnable_val == "value1");
+  pinnable_val.Reset();
+
+  wp_db->ReleaseSnapshot(snapshot0);
   wp_db->ReleaseSnapshot(snapshot2);
+  wp_db->ReleaseSnapshot(snapshot3);
 }
 
 // Test that the entries in old_commit_map_ get garbage collected properly

utilities/transactions/write_prepared_txn_db.cc

@@ -592,8 +592,13 @@ void WritePreparedTxnDB::CleanupReleasedSnapshots(
   for (; newi != new_snapshots.end() && oldi != old_snapshots.end();) {
     assert(*newi >= *oldi);  // cannot have new snapshots with lower seq
     if (*newi == *oldi) {    // still not released
-      newi++;
+      auto value = *newi;
-      oldi++;
+      while (newi != new_snapshots.end() && *newi == value) {
+        newi++;
+      }
+      while (oldi != old_snapshots.end() && *oldi == value) {
+        oldi++;
+      }
     } else {
       assert(*newi > *oldi);  // *oldi is released
       ReleaseSnapshotInternal(*oldi);