From ef0c3eda275837e9b566bd089f3b1cd111d9c48e Mon Sep 17 00:00:00 2001 From: Cheng Chang Date: Thu, 30 Apr 2020 16:21:05 -0700 Subject: [PATCH] Make users explicitly be aware of prepare before commit (#6775) Summary: In current commit protocol of pessimistic transaction, if the transaction is not prepared before commit, the commit protocol implicitly assumes that the user wants to commit without prepare. This PR adds TransactionOptions::skip_prepare, the default value is `true` because if set to `false`, all existing users who commit without prepare need to update their code to set skip_prepare to true. Although this does not force the user to explicitly express their intention of skip_prepare, it at least lets the user be aware of the assumption of being able to commit without prepare. Pull Request resolved: https://github.com/facebook/rocksdb/pull/6775 Test Plan: added a new unit test TransactionTest::CommitWithoutPrepare Reviewed By: lth Differential Revision: D21313270 Pulled By: cheng-chang fbshipit-source-id: 3d95b7c9b2d6cdddc09bdd66c561bc4fae8c3251 --- include/rocksdb/status.h | 13 +++++++++++ include/rocksdb/utilities/transaction.h | 4 +++- include/rocksdb/utilities/transaction_db.h | 4 ++++ .../transactions/pessimistic_transaction.cc | 10 +++++---- .../transactions/pessimistic_transaction.h | 3 +++ utilities/transactions/transaction_test.cc | 22 +++++++++++++++++++ 6 files changed, 51 insertions(+), 5 deletions(-) diff --git a/include/rocksdb/status.h b/include/rocksdb/status.h index b0d7dbef0..ddddee949 100644 --- a/include/rocksdb/status.h +++ b/include/rocksdb/status.h @@ -79,6 +79,7 @@ class Status { KMergeOperandsInsufficientCapacity = 10, kManualCompactionPaused = 11, kOverwritten = 12, + kTxnNotPrepared = 13, kMaxSubCode }; @@ -224,6 +225,13 @@ class Status { return Status(kIOError, kPathNotFound, msg, msg2); } + static Status TxnNotPrepared() { + return Status(kInvalidArgument, kTxnNotPrepared); + } + static Status TxnNotPrepared(const Slice& msg, const Slice& msg2 = Slice()) { + return Status(kInvalidArgument, kTxnNotPrepared, msg, msg2); + } + // Returns true iff the status indicates success. bool ok() const { return code() == kOk; } @@ -315,6 +323,11 @@ class Status { return (code() == kIncomplete) && (subcode() == kManualCompactionPaused); } + // Returns true iff the status indicates a TxnNotPrepared error. + bool IsTxnNotPrepared() const { + return (code() == kInvalidArgument) && (subcode() == kTxnNotPrepared); + } + // Return a string representation of this status suitable for printing. // Returns the string "OK" for success. std::string ToString() const; diff --git a/include/rocksdb/utilities/transaction.h b/include/rocksdb/utilities/transaction.h index 18e27844c..6ebdbcc40 100644 --- a/include/rocksdb/utilities/transaction.h +++ b/include/rocksdb/utilities/transaction.h @@ -139,7 +139,9 @@ class Transaction { // // If this transaction was created by a TransactionDB(), Status::Expired() // may be returned if this transaction has lived for longer than - // TransactionOptions.expiration. + // TransactionOptions.expiration. Status::TxnNotPrepared() may be returned if + // TransactionOptions.skip_prepare is false and Prepare is not called on this + // transaction before Commit. virtual Status Commit() = 0; // Discard all batched writes in this transaction. diff --git a/include/rocksdb/utilities/transaction_db.h b/include/rocksdb/utilities/transaction_db.h index 73b7416a0..8967b7eef 100644 --- a/include/rocksdb/utilities/transaction_db.h +++ b/include/rocksdb/utilities/transaction_db.h @@ -172,6 +172,10 @@ struct TransactionOptions { // Default: false bool skip_concurrency_control = false; + // In pessimistic transaction, if this is true, then you can skip Prepare + // before Commit, otherwise, you must Prepare before Commit. + bool skip_prepare = true; + // See TransactionDBOptions::default_write_batch_flush_threshold for // description. If a negative value is specified, then the default value from // TransactionDBOptions is used. diff --git a/utilities/transactions/pessimistic_transaction.cc b/utilities/transactions/pessimistic_transaction.cc index 722e6321d..a33e5df51 100644 --- a/utilities/transactions/pessimistic_transaction.cc +++ b/utilities/transactions/pessimistic_transaction.cc @@ -87,6 +87,7 @@ void PessimisticTransaction::Initialize(const TransactionOptions& txn_options) { } use_only_the_last_commit_time_batch_for_recovery_ = txn_options.use_only_the_last_commit_time_batch_for_recovery; + skip_prepare_ = txn_options.skip_prepare; } PessimisticTransaction::~PessimisticTransaction() { @@ -283,10 +284,11 @@ Status PessimisticTransaction::Commit() { commit_prepared = true; } else if (txn_state_ == STARTED) { // expiration and lock stealing is not a concern - commit_without_prepare = true; - // TODO(myabandeh): what if the user mistakenly forgets prepare? We should - // add an option so that the user explictly express the intention of - // skipping the prepare phase. + if (skip_prepare_) { + commit_without_prepare = true; + } else { + return Status::TxnNotPrepared(); + } } if (commit_without_prepare) { diff --git a/utilities/transactions/pessimistic_transaction.h b/utilities/transactions/pessimistic_transaction.h index 8f2c84405..f81405bc3 100644 --- a/utilities/transactions/pessimistic_transaction.h +++ b/utilities/transactions/pessimistic_transaction.h @@ -120,6 +120,9 @@ class PessimisticTransaction : public TransactionBaseImpl { // Refer to // TransactionOptions::use_only_the_last_commit_time_batch_for_recovery bool use_only_the_last_commit_time_batch_for_recovery_ = false; + // Refer to + // TransactionOptions::skip_prepare + bool skip_prepare_ = false; virtual Status PrepareInternal() = 0; diff --git a/utilities/transactions/transaction_test.cc b/utilities/transactions/transaction_test.cc index 06a88c30c..9668d447c 100644 --- a/utilities/transactions/transaction_test.cc +++ b/utilities/transactions/transaction_test.cc @@ -6205,6 +6205,28 @@ TEST_P(TransactionTest, DoubleCrashInRecovery) { } } +TEST_P(TransactionTest, CommitWithoutPrepare) { + { + // skip_prepare = false. + WriteOptions write_options; + TransactionOptions txn_options; + txn_options.skip_prepare = false; + Transaction* txn = db->BeginTransaction(write_options, txn_options); + ASSERT_TRUE(txn->Commit().IsTxnNotPrepared()); + delete txn; + } + + { + // skip_prepare = true. + WriteOptions write_options; + TransactionOptions txn_options; + txn_options.skip_prepare = true; + Transaction* txn = db->BeginTransaction(write_options, txn_options); + ASSERT_OK(txn->Commit()); + delete txn; + } +} + } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) {