revision-id: 3181c0c421a08bda45ccaffbeff547312588068f (v5.8-1901-g3181c0c42) parent(s): 2d97925f35970781a649ab312a7330fdb4446312 author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2019-12-09 18:47:10 +0300 message: Remove RocksDB-side implementation of SeekForUpdate (It is implemented at MyRocks level now. The RocksDB patch has ended up in rocksdb-with-range-locking branch due to a rebase error) --- include/rocksdb/utilities/transaction.h | 5 - include/rocksdb/utilities/transaction_db.h | 5 +- utilities/transactions/pessimistic_transaction.cc | 240 --------------------- utilities/transactions/pessimistic_transaction.h | 3 - .../transactions/pessimistic_transaction_db.cc | 2 +- .../transactions/pessimistic_transaction_db.h | 1 - utilities/transactions/range_locking_test.cc | 163 +------------- utilities/transactions/transaction_lock_mgr.h | 3 + 8 files changed, 7 insertions(+), 415 deletions(-) diff --git a/include/rocksdb/utilities/transaction.h b/include/rocksdb/utilities/transaction.h index 53bca8848..f3a987c2d 100644 --- a/include/rocksdb/utilities/transaction.h +++ b/include/rocksdb/utilities/transaction.h @@ -386,11 +386,6 @@ class Transaction { virtual Iterator* GetIterator(const ReadOptions& read_options, ColumnFamilyHandle* column_family) = 0; - virtual Iterator* GetLockingIterator(const ReadOptions&, - ColumnFamilyHandle*) { - return nullptr; // not supported - }; - // Put, Merge, Delete, and SingleDelete behave similarly to the corresponding // functions in WriteBatch, but will also do conflict checking on the // keys being written. diff --git a/include/rocksdb/utilities/transaction_db.h b/include/rocksdb/utilities/transaction_db.h index bb13bba07..e6cc110c6 100644 --- a/include/rocksdb/utilities/transaction_db.h +++ b/include/rocksdb/utilities/transaction_db.h @@ -45,9 +45,8 @@ class BaseLockMgr; // will be used to perform locking. class LockManagerHandle { public: - // dynamic_cast from this class to BaseLockMgr should work, this is how - // PessimisticTransactionDB will get the Lock Manager it's going to use. - + // Get the underlying LockManager. To be used by RocksDB Internals + virtual BaseLockMgr* GetManager()=0; virtual ~LockManagerHandle(){}; }; diff --git a/utilities/transactions/pessimistic_transaction.cc b/utilities/transactions/pessimistic_transaction.cc index d4b167aee..2b67cb3f4 100644 --- a/utilities/transactions/pessimistic_transaction.cc +++ b/utilities/transactions/pessimistic_transaction.cc @@ -58,246 +58,6 @@ PessimisticTransaction::PessimisticTransaction( } } -////////////////////////////////////////////////////////////////////////////// -// Locking iterator -////////////////////////////////////////////////////////////////////////////// - -// -// LockingIterator is an iterator that locks the rows before returning, as well -// as scanned gaps between the rows. -// -// Example: -// lock_iter= trx->GetLockingIterator(); -// lock_iter->Seek('abc'); -// lock_iter->Valid()==true && lock_iter->key() == 'bcd'; -// -// After the above, the returned record 'bcd' is locked by transaction trx. -// Also, the range between ['abc'..'bcd'] is empty and locked by trx. -// -// lock_iter->Next(); -// lock_iter->Valid()==true && lock_iter->key() == 'efg' -// -// Now, the range ['bcd'.. 'efg'] (bounds incluive) is also locked, and there are no -// records between 'bcd' and 'efg'. -// -class LockingIterator : public Iterator { - - ColumnFamilyHandle* cfh_; - PessimisticTransaction *txn_; - Iterator *iter_; - Status status_; - - public: - LockingIterator(Iterator *iter, ColumnFamilyHandle *cfh, - PessimisticTransaction *txn) : - cfh_(cfh), txn_(txn), iter_(iter), status_(Status::InvalidArgument()) {} - - ~LockingIterator() { delete iter_; } - // An iterator is either positioned at a key/value pair, or - // not valid. This method returns true iff the iterator is valid. - // Always returns false if !status().ok(). - virtual bool Valid() const override { return status_.ok() && iter_->Valid(); } - - // Note: MyRocks doesn't ever call these: - virtual void SeekToFirst() override; - virtual void SeekToLast() override; - - virtual void Seek(const Slice& target) override; - - // Position at the last key in the source that at or before target. - // The iterator is Valid() after this call iff the source contains - // an entry that comes at or before target. - virtual void SeekForPrev(const Slice& target) override; - - virtual void Next() override; - virtual void Prev() override; - - virtual Slice key() const override { - assert(Valid()); - return iter_->key(); - } - - virtual Slice value() const override { - assert(Valid()); - return iter_->value(); - } - - virtual Status status() const override { - return status_; - } - - private: - - template <bool forward> void Scan(const Slice& target, bool call_next) { - if (!iter_->Valid()) { - status_ = iter_->status(); - return; - } - - while (1) { - /* - note: the underlying iterator checks iterator bounds, so we don't need - to check them here - */ - auto end_key = iter_->key(); - if (forward) - status_ = txn_->GetRangeLock(cfh_, Endpoint(target), Endpoint(end_key)); - else - status_ = txn_->GetRangeLock(cfh_, Endpoint(end_key), Endpoint(target)); - - if (!status_.ok()) { - // Failed to get a lock (most likely lock wait timeout) - return; - } - - //Ok, now we have a lock which is inhibiting modifications in the range - // Somebody might have done external modifications, though: - // - removed the key we've found - // - added a key before that key. - if (forward) - iter_->Seek(target); - else - iter_->SeekForPrev(target); - - if (call_next && iter_->Valid()) { - if (forward) - iter_->Next(); - else - iter_->Prev(); - } - - if (iter_->Valid()) { - int invert= forward? 1 : -1; - if (cfh_->GetComparator()->Compare(iter_->key(), end_key) * invert <= 0) { - // Ok, the key is within the range. - status_ = Status::OK(); - break; - } else { - // We've got a row but it is outside the range we've locked. - // Re-try the lock-and-read step. - continue; - } - } else { - // There's no row (within the iterator bounds perhaps). Exit now. - // (we might already have locked a range in this function but there's - // nothing we can do about it) - status_ = iter_->status(); - break; - } - } - } - - inline void ScanForward(const Slice& target, bool call_next) { - Scan<true>(target, call_next); - } - - inline void ScanBackward(const Slice& target, bool call_next) { - Scan<false>(target, call_next); - } -}; - - -Iterator* PessimisticTransaction::GetLockingIterator( - const ReadOptions& read_options, - ColumnFamilyHandle* column_family) { - - if (!txn_db_impl_->UsesRangeLocking()) { - return nullptr; - } - auto iter= GetIterator(read_options, column_family); - - if (iter) - return new LockingIterator(iter, column_family, this); - else - return nullptr; -} - -/* - @brief - Seek to the first key K that is equal or greater than target, - locking the range [target; K]. -*/ - -void LockingIterator::Seek(const Slice& target) { - iter_->Seek(target); - ScanForward(target, false); -} - -void LockingIterator::SeekForPrev(const Slice& target) { - iter_->SeekForPrev(target); - ScanBackward(target, false); -} - -/* - @brief - Move the iterator to the next key, locking the range between the current - and the next key. - - @detail - Implementation is similar to Seek(next_key). Since we don't know what the - next_key is, we reach it by calling { Seek(current_key); Next(); } -*/ -void LockingIterator::Next() { - assert(Valid()); - // Save the current key value. We need it as the left endpoint - // of the range lock we're going to acquire - std::string current_key = iter_->key().ToString(); - - iter_->Next(); - ScanForward(Slice(current_key), true); -} - -/* - @brief - Move the iterator to the previous key, locking the range between the current - and the previous key. -*/ - -void LockingIterator::Prev() { - assert(Valid()); - - std::string current_key = iter_->key().ToString(); - iter_->Prev(); - ScanBackward(Slice(current_key), true); -} - - -/* - @detail - Ideally, this function should - - find the first key $first_key - - lock the range [-inf; $first_key] - - return, the iterator is positioned on $first_key - - The problem here is that we cannot have "-infinity" bound. - - Note: we don't have a practical use for this function - MyRocks always - searches within one index_name.table_name, which means we are only looking - at the keys with index_number as the prefix. -*/ - -void LockingIterator::SeekToFirst() { - iter_->SeekToFirst(); - if (!iter_->Valid()) { - status_ = iter_->status(); - return; - } - - std::string current_key = iter_->key().ToString(); - ScanForward(Slice(current_key), true); -} - -/* - @detail - See SeekToFirst. -*/ - -void LockingIterator::SeekToLast() { - status_ = Status::NotSupported("Not implemented"); -} - -///////////////////////////////////////////////////////////////////////////// - void PessimisticTransaction::Initialize(const TransactionOptions& txn_options) { txn_id_ = GenTxnID(); diff --git a/utilities/transactions/pessimistic_transaction.h b/utilities/transactions/pessimistic_transaction.h index d6302b7c9..eb79066d6 100644 --- a/utilities/transactions/pessimistic_transaction.h +++ b/utilities/transactions/pessimistic_transaction.h @@ -61,9 +61,6 @@ class PessimisticTransaction : public TransactionBaseImpl { Status SetName(const TransactionName& name) override; - virtual Iterator* GetLockingIterator(const ReadOptions& read_options, - ColumnFamilyHandle* column_family) override; - // Generate a new unique transaction identifier static TransactionID GenTxnID(); diff --git a/utilities/transactions/pessimistic_transaction_db.cc b/utilities/transactions/pessimistic_transaction_db.cc index e225b2aae..7704796cb 100644 --- a/utilities/transactions/pessimistic_transaction_db.cc +++ b/utilities/transactions/pessimistic_transaction_db.cc @@ -40,7 +40,7 @@ void PessimisticTransactionDB::init_lock_manager() { if (txn_db_options_.lock_mgr_handle) { // A custom lock manager was provided in options - lock_mgr_ = std::dynamic_pointer_cast<BaseLockMgr>(txn_db_options_.lock_mgr_handle); + lock_mgr_.reset(txn_db_options_.lock_mgr_handle->GetManager()); range_lock_mgr_ = dynamic_cast<RangeLockMgr*>(lock_mgr_.get()); } else { // Use point lock manager by default diff --git a/utilities/transactions/pessimistic_transaction_db.h b/utilities/transactions/pessimistic_transaction_db.h index 7e3d6febd..269e787f4 100644 --- a/utilities/transactions/pessimistic_transaction_db.h +++ b/utilities/transactions/pessimistic_transaction_db.h @@ -150,7 +150,6 @@ class PessimisticTransactionDB : public TransactionDB { // Key Tracking should be done only with point lock manager. bool ShouldDoKeyTracking() const { return range_lock_mgr_ == nullptr; } - bool UsesRangeLocking() const { return range_lock_mgr_ != nullptr; } protected: DBImpl* db_impl_; std::shared_ptr<Logger> info_log_; diff --git a/utilities/transactions/range_locking_test.cc b/utilities/transactions/range_locking_test.cc index 83dc7c058..a93ec7a83 100644 --- a/utilities/transactions/range_locking_test.cc +++ b/utilities/transactions/range_locking_test.cc @@ -54,16 +54,13 @@ class RangeLockingTest : public ::testing::Test { Status s; range_lock_mgr.reset(rocksdb::NewRangeLockManager(nullptr)); - txn_db_options.lock_mgr_handle = range_lock_mgr; + txn_db_options.range_lock_mgr = range_lock_mgr; s = TransactionDB::Open(options, txn_db_options, dbname, &db); assert(s.ok()); } - void VerifyLocks(ColumnFamilyHandle *cf, Transaction* txn, - const char *json, const std::string& ignore_lock); - ~RangeLockingTest() { delete db; db = nullptr; @@ -85,9 +82,6 @@ TEST_F(RangeLockingTest, BasicRangeLocking) { Transaction* txn0 = db->BeginTransaction(write_options, txn_options); Transaction* txn1 = db->BeginTransaction(write_options, txn_options); - txn0->SetLockTimeout(50); - txn1->SetLockTimeout(50); - // Get a range lock { auto s= txn0->GetRangeLock(db->DefaultColumnFamily(), @@ -128,161 +122,6 @@ TEST_F(RangeLockingTest, BasicRangeLocking) { delete txn1; } -// Print a string into stream, with non-printable character escaped -void escape_print_str(std::ostringstream& oss, const std::string &s) { - const char *hexstr= "0123456789ABCDEF"; - for (char c : s) { - if (isprint(c)) - oss << c; - else - oss << "\\x" << hexstr[((c >> 8) & 0xF)] << hexstr[c & 0xF]; - } -} - - -using AcquiredLockMap = std::unordered_multimap<uint32_t, KeyLockInfo>; - -// Dump the output of db-GetLockStatusData() into a siccint JSON-like format -// -// @param default_cf Assume normally locks are from this CF and don't -// mention it in the output -// @param default_trx Assume locks are from this TRX and don't mention it -// in the output -// @param ignore_lock Don't print the lock on this key - -std::string DumpLocksToJson(AcquiredLockMap& lock_data, uint32_t default_cf, - TransactionID default_trx, - const std::string& ignore_lock) { - std::ostringstream oss; - bool first= true; - for (auto iter: lock_data) { - - assert(iter.second.ids.size() > 0); - if (iter.second.key == ignore_lock) - continue; - - oss << "lock:{"; - if (iter.first != default_cf) - oss << "cf:" << iter.first << ","; - if (iter.second.has_key2) { - oss << "start:'"; - escape_print_str(oss, iter.second.key); - oss << "',end:'"; - escape_print_str(oss, iter.second.key2); - oss << "'"; - } else { - oss << "key:'"; - escape_print_str(oss, iter.second.key); - oss << "'"; - } - if (iter.second.ids.size() != 1 || - iter.second.ids.front() != default_trx) { - oss << ",owners["; - bool first_owner= true; - for (auto o: iter.second.ids) { - if (first_owner) - first_owner= false; - else - oss << ","; - oss << o; - } - oss << "]"; - } - oss << "}"; - if (first) - first= false; - else - oss << std::endl; - } - return oss.str(); -} - -// Verify that locks match the specified JSON-like description -void RangeLockingTest::VerifyLocks(ColumnFamilyHandle *cf, Transaction* txn, - const char *json, - const std::string& ignore_lock) { - AcquiredLockMap lock_data = db->GetLockStatusData(); - std::string s = DumpLocksToJson(lock_data, cf->GetID(), txn->GetID(), - ignore_lock); - if (s != json) { - std::cerr << "FAILURE: expected " << json << " ,got " << s << std::endl; - assert(0); - } -} - -// Test for LockingIterator (aka SeekForUpdate) feature -TEST_F(RangeLockingTest, SeekForUpdate) { - - WriteOptions write_options; - TransactionOptions txn_options; - auto cf= db->DefaultColumnFamily(); - - Transaction* txn0 = db->BeginTransaction(write_options, txn_options); - // Insert some initial contents into the database - txn0->Put(cf, "00", "value"); - txn0->Put(cf, "02", "value"); - txn0->Put(cf, "04", "value"); - txn0->Put(cf, "06", "value"); - txn0->Put(cf, "08", "value"); - txn0->Put(cf, "10", "value"); - - txn0->Commit(); - delete txn0; - txn0 = db->BeginTransaction(write_options, txn_options); - txn0->Put(cf, "FF", "value"); - std::string ignored_key; - ignored_key.push_back(0); - ignored_key.append("FF"); - - Transaction* txn1 = db->BeginTransaction(write_options, txn_options); - - // TODO: Does Locking + using snapshot make any sense? If not, should we - // disallow it? - ReadOptions read_options; - Iterator *it = txn1->GetLockingIterator(read_options, cf); - - it->Seek("03"); - ASSERT_TRUE(it->Valid()); - ASSERT_EQ(it->key(), "04"); - VerifyLocks(cf, txn1, "lock:{start:'\\x0003',end:'\\x0004'}", ignored_key); - - it->Next(); - ASSERT_TRUE(it->Valid()); - ASSERT_EQ(it->key(), "06"); - VerifyLocks(cf, txn1, "lock:{start:'\\x0003',end:'\\x0006'}", ignored_key); - - it->Next(); - ASSERT_TRUE(it->Valid()); - ASSERT_EQ(it->key(), "08"); - VerifyLocks(cf, txn1, "lock:{start:'\\x0003',end:'\\x0008'}", ignored_key); - - // Try seeking where we won't find any data: - it->Seek("AA"); - ASSERT_FALSE(it->Valid()); - VerifyLocks(cf, txn1, "lock:{start:'\\x0003',end:'\\x0008'}", ignored_key); - - delete it; - txn1->Rollback(); - delete txn1; - - // Restart the transaction. - txn1 = db->BeginTransaction(write_options, txn_options); - it = txn1->GetLockingIterator(ReadOptions(), cf); - - - // Now, try the backward scans. - it->SeekForPrev("03"); - ASSERT_TRUE(it->Valid()); - ASSERT_EQ(it->key(), "02"); - VerifyLocks(cf, txn1, "lock:{start:'\\x0002',end:'\\x0003'}", ignored_key); - - delete it; - txn1->Rollback(); - delete txn1; - - txn0->Rollback(); - delete txn0; -} } // namespace rocksdb int main(int argc, char** argv) { diff --git a/utilities/transactions/transaction_lock_mgr.h b/utilities/transactions/transaction_lock_mgr.h index 4bb66febf..24be61814 100644 --- a/utilities/transactions/transaction_lock_mgr.h +++ b/utilities/transactions/transaction_lock_mgr.h @@ -248,6 +248,9 @@ class RangeLockMgr : LockStatusData GetLockStatusData() override; + BaseLockMgr* GetManager() override { + return this; + } private: toku::locktree_manager ltm_;