Use do_validate flag to control timestamp based validation in WriteCommittedTxn::GetForUpdate (#12369)

Summary:
When PR https://github.com/facebook/rocksdb/issues/9629 introduced user-defined timestamp support for `WriteCommittedTxn`, it adds this usage mandate for API `GetForUpdate` when UDT is enabled. The `do_validate` flag has to be true, and user should have already called `Transaction::SetReadTimestampForValidation` to set a read timestamp for validation. The rationale behind this mandate is this:
1) with do_vaildate = true, `GetForUpdate` could verify this relationships: let's denote the user-defined timestamp in db for the key as  `Ts_db` and the read timestamp user set via `Transaction::SetReadTimestampForValidation` as `Ts_read`. UDT based validation will only pass if `Ts_db <= Ts_read`.
5950907a82/utilities/transactions/transaction_util.cc (L141)

2)  Let's denote the committed timestamp set via `Transaction::SetCommitTimestamp` to be `Ts_cmt`. Later `WriteCommitedTxn::Commit` would only pass if this condition is met: `Ts_read < Ts_cmt`. 5950907a82/utilities/transactions/pessimistic_transaction.cc (L431)

Together these two checks can ensure `Ts_db < Ts_cmt` to meet the user-defined timestamp invariant that newer timestamp should have newer sequence number.

The `do_validate` flag was originally intended to make snapshot based validation optional. If it's true, `GetForUpdate` checks no entry is written after the snapshot. If it's false, it will skip this snapshot based validation. In this PR, we are making the UDT based validation configurable too based on this flag instead of mandating it for below reasons: 1) in some cases the users themselves can enforce aformentioned invariant on their side independently, without RocksDB help, for example, if they are managing a monotonically increasing timestamp, and their transactions are only committed in a single thread. So they don't need this UDT based validation and wants to skip it, 2) It also could be expensive or not practical for users to come up with such a read timestamp that is exactly in between their commit timestamp and the db's timestamp. For example, in aformentioned case where a monotonically increasing timestamp is managed, the users would need to access this timestamp both for setting the read timestamp and for setting the commit timestamp. So it's preferable to skip this check too.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/12369

Test Plan: added unit tests

Reviewed By: ltamasi

Differential Revision: D54268920

Pulled By: jowlyzhang

fbshipit-source-id: ca7693796f9bb11f376a2059d91841e51c89435a

unreleased_history/public_api_changes/optional_skip_udt_based_validation_write_committed.md

@@ -0,0 +1 @@
+*For API `WriteCommittedTransaction::GetForUpdate`, if the column family enables user-defined timestamp, it was mandated that argument `do_validate` cannot be false, and UDT based validation has to be done with a user set read timestamp. It's updated to make the UDT based validation optional if user sets `do_validate` to false and does not set a read timestamp. With this, `GetForUpdate` skips UDT based validation and it's users' responsibility to enforce the UDT invariant. SO DO NOT skip this UDT-based validation if users do not have ways to enforce the UDT invariant. Ways to enforce the invariant on the users side include manage a monotonically increasing timestamp, commit transactions in a single thread etc.

utilities/transactions/pessimistic_transaction.cc

@@ -190,11 +190,11 @@ inline Status WriteCommittedTxn::GetForUpdateImpl(
     }
   }
 
-  if (!do_validate) {
+  if (!do_validate && kMaxTxnTimestamp != read_timestamp_) {
     return Status::InvalidArgument(
         "If do_validate is false then GetForUpdate with read_timestamp is not "
         "defined.");
-  } else if (kMaxTxnTimestamp == read_timestamp_) {
+  } else if (do_validate && kMaxTxnTimestamp == read_timestamp_) {
     return Status::InvalidArgument("read_timestamp must be set for validation");
   }
 

utilities/transactions/write_committed_transaction_ts_test.cc

@@ -3,12 +3,12 @@
 //  COPYING file in the root directory) and Apache 2.0 License
 //  (found in the LICENSE.Apache file in the root directory).
 
+#include "rocksdb/comparator.h"
 #include "rocksdb/db.h"
 #include "rocksdb/options.h"
 #include "rocksdb/utilities/transaction_db.h"
-#include "utilities/merge_operators.h"
-
 #include "test_util/testutil.h"
+#include "utilities/merge_operators.h"
 #include "utilities/transactions/transaction_test.h"
 
 namespace ROCKSDB_NAMESPACE {
@@ -453,13 +453,17 @@ TEST_P(WriteCommittedTxnWithTsTest, GetForUpdate) {
   std::unique_ptr<Transaction> txn0(
       NewTxn(WriteOptions(), TransactionOptions()));
 
+  // Not set read timestamp, use blind write
   std::unique_ptr<Transaction> txn1(
       NewTxn(WriteOptions(), TransactionOptions()));
   ASSERT_OK(txn1->Put(handles_[1], "key", "value1"));
+  ASSERT_OK(txn1->Put(handles_[1], "foo", "value1"));
   ASSERT_OK(txn1->SetCommitTimestamp(24));
   ASSERT_OK(txn1->Commit());
   txn1.reset();
 
+  // Set read timestamp, use it for validation in GetForUpdate, validation fail
+  // with conflict: timestamp from db(24) > read timestamp(23)
   std::string value;
   ASSERT_OK(txn0->SetReadTimestampForValidation(23));
   ASSERT_TRUE(
@@ -467,13 +471,71 @@ TEST_P(WriteCommittedTxnWithTsTest, GetForUpdate) {
   ASSERT_OK(txn0->Rollback());
   txn0.reset();
 
+  // Set read timestamp, use it for validation in GetForUpdate, validation pass
+  // with no conflict: timestamp from db(24) < read timestamp (25)
   std::unique_ptr<Transaction> txn2(
       NewTxn(WriteOptions(), TransactionOptions()));
   ASSERT_OK(txn2->SetReadTimestampForValidation(25));
   ASSERT_OK(txn2->GetForUpdate(ReadOptions(), handles_[1], "key", &value));
+  // Use a different read timestamp in ReadOptions while doing validation is
+  // invalid.
+  ReadOptions read_options;
+  std::string read_timestamp;
+  Slice diff_read_ts = EncodeU64Ts(24, &read_timestamp);
+  read_options.timestamp = &diff_read_ts;
+  ASSERT_TRUE(txn2->GetForUpdate(read_options, handles_[1], "foo", &value)
+                  .IsInvalidArgument());
   ASSERT_OK(txn2->SetCommitTimestamp(26));
   ASSERT_OK(txn2->Commit());
   txn2.reset();
+
+  // Set read timestamp, call GetForUpdate without validation, invalid
+  std::unique_ptr<Transaction> txn3(
+      NewTxn(WriteOptions(), TransactionOptions()));
+  ASSERT_OK(txn3->SetReadTimestampForValidation(27));
+  ASSERT_TRUE(txn3->GetForUpdate(ReadOptions(), handles_[1], "key", &value,
+                                 /*exclusive=*/true, /*do_validate=*/false)
+                  .IsInvalidArgument());
+  ASSERT_OK(txn3->Rollback());
+  txn3.reset();
+
+  // Not set read timestamp, call GetForUpdate with validation, invalid
+  std::unique_ptr<Transaction> txn4(
+      NewTxn(WriteOptions(), TransactionOptions()));
+  // ReadOptions.timestamp is not set, invalid
+  ASSERT_TRUE(txn4->GetForUpdate(ReadOptions(), handles_[1], "key", &value)
+                  .IsInvalidArgument());
+  // ReadOptions.timestamp is set, also invalid.
+  // `SetReadTimestampForValidation` must have been called with the same
+  // timestamp as in ReadOptions.timestamp for validation.
+  Slice read_ts = EncodeU64Ts(27, &read_timestamp);
+  read_options.timestamp = &read_ts;
+  ASSERT_TRUE(txn4->GetForUpdate(read_options, handles_[1], "key", &value)
+                  .IsInvalidArgument());
+  ASSERT_OK(txn4->Rollback());
+  txn4.reset();
+
+  // Not set read timestamp, call GetForUpdate without validation, pass
+  std::unique_ptr<Transaction> txn5(
+      NewTxn(WriteOptions(), TransactionOptions()));
+  // ReadOptions.timestamp is not set, pass
+  ASSERT_OK(txn5->GetForUpdate(ReadOptions(), handles_[1], "key", &value,
+                               /*exclusive=*/true, /*do_validate=*/false));
+  // ReadOptions.timestamp explicitly set to max timestamp, pass
+  Slice max_ts = MaxU64Ts();
+  read_options.timestamp = &max_ts;
+  ASSERT_OK(txn5->GetForUpdate(read_options, handles_[1], "foo", &value,
+                               /*exclusive=*/true, /*do_validate=*/false));
+  // NOTE: this commit timestamp is smaller than the db's timestamp (26), but
+  // this commit can still go through, that breaks the user-defined timestamp
+  // invariant: newer user-defined timestamp should have newer sequence number.
+  // So be aware of skipping UDT based validation. Unless users have their own
+  // ways to ensure the UDT invariant is met, DO NOT skip it. Ways to ensure
+  // the UDT invariant include: manage a monotonically increasing timestamp,
+  // commit transactions in a single thread etc.
+  ASSERT_OK(txn5->SetCommitTimestamp(3));
+  ASSERT_OK(txn5->Commit());
+  txn5.reset();
 }
 
 TEST_P(WriteCommittedTxnWithTsTest, BlindWrite) {