revision-id: 24ca9e111197ba779a007e5b13d14ed4f4e54b4f (v5.8-2697-g24ca9e111) parent(s): c91814aeffe1043714481723bdffb8b38034ec8a author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2020-08-10 18:03:07 +0300 message: Range Locking: Cleanup and more test coverage - Cleanup in test code - Add a test that makes locking calls similar to MyRocks' calls for UPDATE t WHERE range_cond --- TARGETS | 4 +- utilities/transactions/pessimistic_transaction.cc | 20 ++++---- utilities/transactions/range_locking_test.cc | 56 +++++++++++++++-------- utilities/transactions/transaction_lock_mgr.cc | 1 + 4 files changed, 51 insertions(+), 30 deletions(-) diff --git a/TARGETS b/TARGETS index 42ac1c695..8eeb5edd9 100644 --- a/TARGETS +++ b/TARGETS @@ -1407,7 +1407,9 @@ ROCKS_TESTS = [ [ "range_locking_test", "utilities/transactions/range_locking_test.cc", - "serial", + "parallel", + [], + [], ], [ "rate_limiter_test", diff --git a/utilities/transactions/pessimistic_transaction.cc b/utilities/transactions/pessimistic_transaction.cc index f6fd3a504..865e4ba4d 100644 --- a/utilities/transactions/pessimistic_transaction.cc +++ b/utilities/transactions/pessimistic_transaction.cc @@ -567,7 +567,8 @@ Status PessimisticTransaction::TryLock(ColumnFamilyHandle* column_family, previously_locked = status.locked; lock_upgrade = previously_locked && exclusive && !status.exclusive; } else { - previously_locked = false; + // If the record is tracked, we can assume it was locked, too. + previously_locked = assume_tracked; status.locked = false; lock_upgrade = false; } @@ -589,7 +590,8 @@ Status PessimisticTransaction::TryLock(ColumnFamilyHandle* column_family, SequenceNumber tracked_at_seq = status.locked ? status.seq : kMaxSequenceNumber; if (!do_validate || snapshot_ == nullptr) { - if (assume_tracked && !previously_locked && tracked_locks_->IsPointLockSupported()) { + if (assume_tracked && !previously_locked && + tracked_locks_->IsPointLockSupported()) { s = Status::InvalidArgument( "assume_tracked is set but it is not tracked yet"); } @@ -646,13 +648,13 @@ Status PessimisticTransaction::TryLock(ColumnFamilyHandle* column_family, TrackKey(cfh_id, key_str, tracked_at_seq, read_only, exclusive); } else { #ifndef NDEBUG -#if 0 // psergey-todo: this will fail with range lock manager! - PointLockStatus lock_status = - tracked_locks_->GetPointLockStatus(cfh_id, key_str); - assert(lock_status.locked); - assert(lock_status.seq <= tracked_at_seq); - assert(lock_status.exclusive == exclusive); -#endif + if (tracked_locks_->IsPointLockSupported()) { + PointLockStatus lock_status = + tracked_locks_->GetPointLockStatus(cfh_id, key_str); + assert(lock_status.locked); + assert(lock_status.seq <= tracked_at_seq); + assert(lock_status.exclusive == exclusive); + } #endif } } diff --git a/utilities/transactions/range_locking_test.cc b/utilities/transactions/range_locking_test.cc index 07914df6c..8a930ec4d 100644 --- a/utilities/transactions/range_locking_test.cc +++ b/utilities/transactions/range_locking_test.cc @@ -17,16 +17,6 @@ #include "rocksdb/perf_context.h" #include "rocksdb/utilities/transaction.h" #include "rocksdb/utilities/transaction_db.h" -#include "table/mock_table.h" -// #include "test_util/fault_injection_test_env.h" -#include "util/random.h" -#include "util/string_util.h" -#include "test_util/sync_point.h" -#include "test_util/testharness.h" -#include "test_util/testutil.h" -#include "test_util/transaction_test_util.h" -#include "utilities/merge_operators.h" -#include "utilities/merge_operators/string_append/stringappend.h" #include "utilities/transactions/pessimistic_transaction_db.h" #include "port/port.h" @@ -85,40 +75,36 @@ TEST_F(RangeLockingTest, BasicRangeLocking) { TransactionOptions txn_options; std::string value; ReadOptions read_options; + auto cf = db->DefaultColumnFamily(); Transaction* txn0 = db->BeginTransaction(write_options, txn_options); Transaction* txn1 = db->BeginTransaction(write_options, txn_options); // Get a range lock { - auto s= txn0->GetRangeLock(db->DefaultColumnFamily(), - Endpoint("a"), Endpoint("c")); + auto s= txn0->GetRangeLock(cf, Endpoint("a"), Endpoint("c")); ASSERT_EQ(s, Status::OK()); } // Check that range Lock inhibits an overlapping range lock { - auto s= txn1->GetRangeLock(db->DefaultColumnFamily(), - Endpoint("b"), Endpoint("z")); + auto s= txn1->GetRangeLock(cf, Endpoint("b"), Endpoint("z")); ASSERT_TRUE(s.IsTimedOut()); } // Check that range Lock inhibits an overlapping point lock { - auto s= txn1->GetForUpdate(read_options, db->DefaultColumnFamily(), - Slice("b"), &value); + auto s= txn1->GetForUpdate(read_options, cf, Slice("b"), &value); ASSERT_TRUE(s.IsTimedOut()); } // Get a point lock, check that it inhibits range locks { - auto s= txn0->Put(db->DefaultColumnFamily(), - Slice("d"), Slice("value")); + auto s= txn0->Put(cf, Slice("n"), Slice("value")); ASSERT_EQ(s, Status::OK()); - auto s2= txn1->GetRangeLock(db->DefaultColumnFamily(), - Endpoint("c"), Endpoint("e")); + auto s2= txn1->GetRangeLock(cf, Endpoint("m"), Endpoint("p")); ASSERT_TRUE(s2.IsTimedOut()); } @@ -129,6 +115,36 @@ TEST_F(RangeLockingTest, BasicRangeLocking) { delete txn1; } +TEST_F(RangeLockingTest, MyRocksLikeUpdate) { + WriteOptions write_options; + TransactionOptions txn_options; + Transaction* txn0 = db->BeginTransaction(write_options, txn_options); + auto cf= db->DefaultColumnFamily(); + Status s; + + // Get a range lock for the range we are about to update + s = txn0->GetRangeLock(cf, Endpoint("a"), Endpoint("c")); + ASSERT_EQ(s, Status::OK()); + + bool try_range_lock_called = false; + + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack( + "RangeLockMgr::TryRangeLock:enter", + [&](void* /*arg*/) { try_range_lock_called = true; }); + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing(); + + // For performance reasons, the following must NOT call lock_mgr->TryLock(): + // We verify that by checking the value of try_range_lock_called. + s = txn0->Put(cf, Slice("b"), Slice("value"), /*assume_tracked=*/true); + ASSERT_EQ(s, Status::OK()); + + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing(); + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->ClearAllCallBacks(); + ASSERT_EQ(try_range_lock_called, false); + + txn0->Rollback(); +} + TEST_F(RangeLockingTest, SnapshotValidation) { Status s; Slice key_slice= Slice("k"); diff --git a/utilities/transactions/transaction_lock_mgr.cc b/utilities/transactions/transaction_lock_mgr.cc index aa1d6ac75..340bbdb5d 100644 --- a/utilities/transactions/transaction_lock_mgr.cc +++ b/utilities/transactions/transaction_lock_mgr.cc @@ -779,6 +779,7 @@ Status RangeLockMgr::TryRangeLock(PessimisticTransaction* txn, request.create(mutex_factory_); DBT start_key_dbt, end_key_dbt; + TEST_SYNC_POINT("RangeLockMgr::TryRangeLock:enter"); std::string start_key; std::string end_key; serialize_endpoint(start_endp, &start_key);