From 8a678a50ba2048f1b65247fca80037c390a2ff38 Mon Sep 17 00:00:00 2001
From: Manuel Ung <mung@fb.com>
Date: Tue, 13 Aug 2019 13:08:48 -0700
Subject: [PATCH] WriteUnPrepared: Relax restriction on iterators and writes
 with no snapshot (#5697)

Summary:
Currently, if a write is done without a snapshot, then `largest_validated_seq_` is set to `kMaxSequenceNumber`. This is too aggressive, because an iterator with a snapshot created after this write should be valid.

Set `largest_validated_seq_` to `GetLastPublishedSequence` instead. The variable means that no keys in the current tracked key set has changed by other transactions since `largest_validated_seq_`.

Also, do some extra cleanup in Clear() for safety.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/5697

Differential Revision: D16788613

Pulled By: lth

fbshipit-source-id: f2aa40b8b12e0c0cf9e38c940fecc8f1cc0d2385
---
 .../write_unprepared_transaction_test.cc      | 27 +++++++++++++++++++
 .../transactions/write_unprepared_txn.cc      | 10 ++++++-
 utilities/transactions/write_unprepared_txn.h |  6 ++---
 3 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/utilities/transactions/write_unprepared_transaction_test.cc b/utilities/transactions/write_unprepared_transaction_test.cc
index 88b638975..48a07fa12 100644
--- a/utilities/transactions/write_unprepared_transaction_test.cc
+++ b/utilities/transactions/write_unprepared_transaction_test.cc
@@ -537,6 +537,33 @@ TEST_P(WriteUnpreparedTransactionTest, MarkLogWithPrepSection) {
   }
 }
 
+TEST_P(WriteUnpreparedTransactionTest, NoSnapshotWrite) {
+  WriteOptions woptions;
+  TransactionOptions txn_options;
+  txn_options.write_batch_flush_threshold = 1;
+
+  Transaction* txn = db->BeginTransaction(woptions, txn_options);
+
+  // Do some writes with no snapshot
+  ASSERT_OK(txn->Put("a", "a"));
+  ASSERT_OK(txn->Put("b", "b"));
+  ASSERT_OK(txn->Put("c", "c"));
+
+  // Test that it is still possible to create iterators after writes with no
+  // snapshot, if iterator snapshot is fresh enough.
+  ReadOptions roptions;
+  auto iter = txn->GetIterator(roptions);
+  int keys = 0;
+  for (iter->SeekToLast(); iter->Valid(); iter->Prev(), keys++) {
+    ASSERT_OK(iter->status());
+    ASSERT_EQ(iter->key().ToString(), iter->value().ToString());
+  }
+  ASSERT_EQ(keys, 3);
+
+  delete iter;
+  delete txn;
+}
+
 }  // namespace rocksdb
 
 int main(int argc, char** argv) {
diff --git a/utilities/transactions/write_unprepared_txn.cc b/utilities/transactions/write_unprepared_txn.cc
index 761da30af..af39680ac 100644
--- a/utilities/transactions/write_unprepared_txn.cc
+++ b/utilities/transactions/write_unprepared_txn.cc
@@ -106,7 +106,10 @@ Status WriteUnpreparedTxn::HandleWrite(std::function<Status()> do_write) {
       largest_validated_seq_ =
           std::max(largest_validated_seq_, snapshot_->GetSequenceNumber());
     } else {
-      largest_validated_seq_ = kMaxSequenceNumber;
+      // TODO(lth): We should use the same number as tracked_at_seq in TryLock,
+      // because what is actually being tracked is the sequence number at which
+      // this key was locked at.
+      largest_validated_seq_ = db_impl_->GetLastPublishedSequence();
     }
   }
   return s;
@@ -680,6 +683,11 @@ void WriteUnpreparedTxn::Clear() {
   if (!recovered_txn_) {
     txn_db_impl_->UnLock(this, &GetTrackedKeys());
   }
+  unprep_seqs_.clear();
+  flushed_save_points_.reset(nullptr);
+  unflushed_save_points_.reset(nullptr);
+  recovered_txn_ = false;
+  largest_validated_seq_ = 0;
   TransactionBaseImpl::Clear();
 }
 
diff --git a/utilities/transactions/write_unprepared_txn.h b/utilities/transactions/write_unprepared_txn.h
index cfa18d6ee..692578f61 100644
--- a/utilities/transactions/write_unprepared_txn.h
+++ b/utilities/transactions/write_unprepared_txn.h
@@ -241,9 +241,9 @@ class WriteUnpreparedTxn : public WritePreparedTxn {
 
   // Track the largest sequence number at which we performed snapshot
   // validation. If snapshot validation was skipped because no snapshot was set,
-  // then this is set to kMaxSequenceNumber. This value is useful because it
-  // means that for keys that have unprepared seqnos, we can guarantee that no
-  // committed keys by other transactions can exist between
+  // then this is set to GetLastPublishedSequence. This value is useful because
+  // it means that for keys that have unprepared seqnos, we can guarantee that
+  // no committed keys by other transactions can exist between
   // largest_validated_seq_ and max_unprep_seq. See
   // WriteUnpreparedTxnDB::NewIterator for an explanation for why this is
   // necessary for iterator Prev().
-- 
GitLab