- commits - lists.mariadb.org

[Commits] 1c271443c83: MDEV-21243: Join buffer: condition is checked in wrong place for range access
by psergey 09 Dec '19

09 Dec '19

revision-id: 1c271443c83783af2ef99f0a2cd5bdaf7a44b289 (mariadb-10.3.10-1-g1c271443c83) parent(s): bad2f1569da57c4a81cc84ec2f4a79924df9c8d6 author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2019-12-09 23:25:13 +0300 message: MDEV-21243: Join buffer: condition is checked in wrong place for range access In this scenario: - There is a possible range access for table T - And there is a ref access on the same index which uses fewer key parts - The join optimizer picks the ref access (because it is cheaper) - make_join_select applies this heuristic to switch to range: /* Range uses longer key; Use this instead of ref on key */ Join buffer will be used without having called JOIN_TAB::make_scan_filter(). This means, conditions that should be checked when reading table T will be checked after T is joined with the contents of the join buffer, instead. Fixed this by adding a make_scan_filter() check. (updated patch after backport to 10.3) (Backported to 10.3.10) --- mysql-test/main/join_cache.result | 72 +++++++++++++++++++++++++++++++++++++++ mysql-test/main/join_cache.test | 39 +++++++++++++++++++++ sql/sql_select.cc | 10 ++++++ 3 files changed, 121 insertions(+) diff --git a/mysql-test/main/join_cache.result b/mysql-test/main/join_cache.result index fde6e0fec6b..4f9616d5235 100644 --- a/mysql-test/main/join_cache.result +++ b/mysql-test/main/join_cache.result @@ -6049,4 +6049,76 @@ select f2 from t2,t1 where f2 = 0; f2 drop table t1, t2; set join_buffer_size = default; +# +# MDEV-21243: Join buffer: condition is checked in wrong place for range access +# +create table t1(a int primary key); +insert into t1 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); +create table t2 (a int); +insert into t2 select A.a + 10*B.a from t1 A, t1 B; +create table t3 ( +kp1 int, +kp2 int, +col1 int, +col2 int, +key (kp1, kp2) +); +insert into t3 +select +A.a, +B.a, +A.a + 100*B.a, +A.a + 100*B.a +from +t2 A, t2 B; +analyze table t3; +Table Op Msg_type Msg_text +test.t3 analyze status Table is already up to date +# The following must have "B.col1 + 1 < 33333" attached to table B +# and not to the block-nl-join node: +explain format=json +select * +from t1 A, t3 B +where +B.kp1=A.a and +B.kp1 <= 10 and +B.kp2 <= 10 and +B.col1 +1 < 33333; +EXPLAIN +{ + "query_block": { + "select_id": 1, + "table": { + "table_name": "A", + "access_type": "index", + "possible_keys": ["PRIMARY"], + "key": "PRIMARY", + "key_length": "4", + "used_key_parts": ["a"], + "rows": 10, + "filtered": 100, + "attached_condition": "A.a <= 10", + "using_index": true + }, + "block-nl-join": { + "table": { + "table_name": "B", + "access_type": "range", + "possible_keys": ["kp1"], + "key": "kp1", + "key_length": "10", + "used_key_parts": ["kp1", "kp2"], + "rows": 836, + "filtered": 11.962, + "index_condition": "B.kp2 <= 10", + "attached_condition": "B.kp2 <= 10 and B.col1 + 1 < 33333" + }, + "buffer_type": "flat", + "buffer_size": "256Kb", + "join_type": "BNL", + "attached_condition": "B.kp1 = A.a" + } + } +} +drop table t1,t2,t3; set @@optimizer_switch=@save_optimizer_switch; diff --git a/mysql-test/main/join_cache.test b/mysql-test/main/join_cache.test index df89fc30dee..a3d16552fc4 100644 --- a/mysql-test/main/join_cache.test +++ b/mysql-test/main/join_cache.test @@ -3988,5 +3988,44 @@ select f2 from t2,t1 where f2 = 0; drop table t1, t2; set join_buffer_size = default; +--echo # +--echo # MDEV-21243: Join buffer: condition is checked in wrong place for range access +--echo # +create table t1(a int primary key); +insert into t1 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); +create table t2 (a int); +insert into t2 select A.a + 10*B.a from t1 A, t1 B; + +create table t3 ( + kp1 int, + kp2 int, + col1 int, + col2 int, + key (kp1, kp2) +); + +insert into t3 +select + A.a, + B.a, + A.a + 100*B.a, + A.a + 100*B.a +from + t2 A, t2 B; +analyze table t3; + +--echo # The following must have "B.col1 + 1 < 33333" attached to table B +--echo # and not to the block-nl-join node: +explain format=json +select * +from t1 A, t3 B +where + B.kp1=A.a and + B.kp1 <= 10 and + B.kp2 <= 10 and + B.col1 +1 < 33333; + +drop table t1,t2,t3; + # The following command must be the last one the file set @@optimizer_switch=@save_optimizer_switch; diff --git a/sql/sql_select.cc b/sql/sql_select.cc index cd8f3d07824..491e684f7a0 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -10734,6 +10734,13 @@ make_join_select(JOIN *join,SQL_SELECT *select,COND *cond) if (i != join->const_tables && tab->use_quick != 2 && !tab->first_inner) { /* Read with cache */ + /* + TODO: the execution also gets here when we will not be using + join buffer. Review these cases and perhaps, remove this call. + (The final decision whether to use join buffer is made in + check_join_cache_usage, so we should only call make_scan_filter() + there, too). + */ if (tab->make_scan_filter()) DBUG_RETURN(1); } @@ -11673,6 +11680,9 @@ uint check_join_cache_usage(JOIN_TAB *tab, if ((tab->cache= new (root) JOIN_CACHE_BNL(join, tab, prev_cache))) { tab->icp_other_tables_ok= FALSE; + /* If make_join_select() hasn't called make_scan_filter(), do it now */ + if (!tab->cache_select && tab->make_scan_filter()) + goto no_join_cache; return (2 - MY_TEST(!prev_cache)); } goto no_join_cache;

1 0

[Commits] 301a5e487bb: MDEV-19901: Wrong window function calculation
by Varun 09 Dec '19

09 Dec '19

revision-id: 301a5e487bb625b41543f11626ab524ad348d0ba (mariadb-10.2.29-49-g301a5e487bb) parent(s): 425748f1b5890a4bb3c7dcadf8f81f567f9a26fe author: Varun Gupta committer: Varun Gupta timestamp: 2019-12-10 01:43:57 +0530 message: MDEV-19901: Wrong window function calculation The function compare_window_funcs_by_window_specs() does not correctly compare the window functions and this leads to skipping sorting for some window functions as it assumes that the sort order has not changed. --- mysql-test/r/win.result | 27 +++++++++++++++++++++++++++ mysql-test/t/win.test | 22 ++++++++++++++++++++++ sql/sql_window.cc | 2 +- 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/mysql-test/r/win.result b/mysql-test/r/win.result index 13d452f3ef2..b543863bc50 100644 --- a/mysql-test/r/win.result +++ b/mysql-test/r/win.result @@ -3634,5 +3634,32 @@ rank() over (partition by 'abc' order by 'xyz') 1 drop table t1; # +# MDEV-19901: Wrong window function calculation +# +create table t1 (a int, b int); +insert into t1 values (1, 1), (2, 1), (3, 2), (4, 2); +SELECT +SUM(a) OVER (ORDER BY a) AS x, +SUM(a) OVER (PARTITION BY b) AS y, +SUM(a) OVER () AS x +FROM t1 +ORDER BY a; +x y x +1 3 10 +3 3 10 +6 7 10 +10 7 10 +SELECT +SUM(a) OVER (ORDER BY a) AS x, +SUM(a) OVER (PARTITION BY b) AS y, +SUM(a) OVER () AS z +FROM t1; +x y z +1 3 10 +3 3 10 +6 7 10 +10 7 10 +drop table t1; +# # End of 10.2 tests # diff --git a/mysql-test/t/win.test b/mysql-test/t/win.test index a4d42ce2b91..79a357b3b34 100644 --- a/mysql-test/t/win.test +++ b/mysql-test/t/win.test @@ -2341,6 +2341,28 @@ select rank() over (partition by 'abc' order by 'xyz') from t1; select rank() over (partition by 'abc' order by 'xyz') from t1; drop table t1; +--echo # +--echo # MDEV-19901: Wrong window function calculation +--echo # + +create table t1 (a int, b int); +insert into t1 values (1, 1), (2, 1), (3, 2), (4, 2); + +SELECT + SUM(a) OVER (ORDER BY a) AS x, + SUM(a) OVER (PARTITION BY b) AS y, + SUM(a) OVER () AS x +FROM t1 +ORDER BY a; + +SELECT + SUM(a) OVER (ORDER BY a) AS x, + SUM(a) OVER (PARTITION BY b) AS y, + SUM(a) OVER () AS z +FROM t1; + +drop table t1; + --echo # --echo # End of 10.2 tests --echo # diff --git a/sql/sql_window.cc b/sql/sql_window.cc index b258b8f56c9..f3f1915c6e4 100644 --- a/sql/sql_window.cc +++ b/sql/sql_window.cc @@ -521,7 +521,7 @@ int compare_window_funcs_by_window_specs(Item_window_func *win_func1, if (CMP_LT_C <= cmp && cmp <= CMP_GT_C) cmp= win_spec1->partition_list->elements < - win_spec2->partition_list->elements ? CMP_GT_C : CMP_LT_C; + win_spec2->partition_list->elements ? CMP_LT_C : CMP_GT_C; return cmp; }

1 0

[Commits] 1f738064ee9: Update RocksDB revision
by Sergei Petrunia 09 Dec '19

09 Dec '19

revision-id: 1f738064ee9d3e3d04491ab01b6a6afcd6085136 (fb-prod201903-270-g1f738064ee9) parent(s): 01e20a6d0f137ea4e59029699fb426e2b2da2a73 author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2019-12-09 19:16:30 +0300 message: Update RocksDB revision --- rocksdb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rocksdb b/rocksdb index 2d97925f359..1c9d747b0e4 160000 --- a/rocksdb +++ b/rocksdb @@ -1 +1 @@ -Subproject commit 2d97925f35970781a649ab312a7330fdb4446312 +Subproject commit 1c9d747b0e426df3dff4bc70f1ff7ba0271621b0

1 0

[Commits] 1c9d747b0: Remove RocksDB-side implementation of SeekForUpdate
by Sergei Petrunia 09 Dec '19

09 Dec '19

revision-id: 1c9d747b0e426df3dff4bc70f1ff7ba0271621b0 (v5.8-1901-g1c9d747b0) parent(s): 2d97925f35970781a649ab312a7330fdb4446312 author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2019-12-09 19:15:34 +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 | 6 +- utilities/transactions/pessimistic_transaction.cc | 240 --------------------- utilities/transactions/pessimistic_transaction.h | 3 - .../transactions/pessimistic_transaction_db.h | 1 - utilities/transactions/range_locking_test.cc | 161 -------------- 6 files changed, 3 insertions(+), 413 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..8cf2c06f9 100644 --- a/include/rocksdb/utilities/transaction_db.h +++ b/include/rocksdb/utilities/transaction_db.h @@ -45,10 +45,10 @@ 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. + // dynamic_cast from this class to BaseLockMgr should work, this is how + // PessimisticTransactionDB will get the Lock Manager it's going to use. - virtual ~LockManagerHandle(){}; + virtual ~LockManagerHandle(){}; }; // A handle to control RangeLockMgr (Range-based lock manager) from outside 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.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..ff1ca5aae 100644 --- a/utilities/transactions/range_locking_test.cc +++ b/utilities/transactions/range_locking_test.cc @@ -61,9 +61,6 @@ class RangeLockingTest : public ::testing::Test { } - 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) {

1 0

[Commits] 1814adf51b5: Update RocksDB revision
by Sergei Petrunia 09 Dec '19

09 Dec '19

revision-id: 1814adf51b563f39bc070d085c13630ea5b16228 (fb-prod201903-270-g1814adf51b5) parent(s): 01e20a6d0f137ea4e59029699fb426e2b2da2a73 author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2019-12-09 18:51:04 +0300 message: Update RocksDB revision --- rocksdb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rocksdb b/rocksdb index 2d97925f359..3181c0c421a 160000 --- a/rocksdb +++ b/rocksdb @@ -1 +1 @@ -Subproject commit 2d97925f35970781a649ab312a7330fdb4446312 +Subproject commit 3181c0c421a08bda45ccaffbeff547312588068f

1 0

[Commits] 3181c0c42: Remove RocksDB-side implementation of SeekForUpdate
by Sergei Petrunia 09 Dec '19

09 Dec '19

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_;

1 0

[Commits] 01e20a6d0f1: Range Locking: Release the iterator when taking a lock on a range
by Sergei Petrunia 09 Dec '19

09 Dec '19

revision-id: 01e20a6d0f137ea4e59029699fb426e2b2da2a73 (fb-prod201903-269-g01e20a6d0f1) parent(s): 7da2037ab2d6940ac11d4befcf248fd83ad23213 author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2019-12-09 18:38:27 +0300 message: Range Locking: Release the iterator when taking a lock on a range This will cause the iterator to be refreshed so that it returns the latest committed data, and not the data from a snapshot taken at the point the iterator was created. Iterator::Refresh() is not usable for us because this method is not implemented in rocksdb::BaseDeltaIterator that Transaction::GetIterator() returns. --- .../rocksdb/r/range_locking_refresh_iter.result | 50 ++++++++++++++++ .../rocksdb/t/range_locking_refresh_iter.test | 69 ++++++++++++++++++++++ storage/rocksdb/ha_rocksdb.cc | 21 ++++++- storage/rocksdb/rdb_locking_iter.cc | 1 + storage/rocksdb/rdb_locking_iter.h | 20 ++++--- 5 files changed, 153 insertions(+), 8 deletions(-) diff --git a/mysql-test/suite/rocksdb/r/range_locking_refresh_iter.result b/mysql-test/suite/rocksdb/r/range_locking_refresh_iter.result new file mode 100644 index 00000000000..f96d5f5f45c --- /dev/null +++ b/mysql-test/suite/rocksdb/r/range_locking_refresh_iter.result @@ -0,0 +1,50 @@ +select @@rocksdb_use_range_locking; +@@rocksdb_use_range_locking +1 +set debug_sync='RESET'; +create table ten(a int primary key); +insert into ten values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); +create table one_k(a int primary key); +insert into one_k select A.a + B.a* 10 + C.a * 100 from ten A, ten B, ten C; +create table t1 ( +pk int primary key, +a int +) engine=rocksdb; +insert into t1 select a,a from ten; +insert into t1 select a+40, a+40 from ten; +insert into t1 select a+100, a+100 from one_k; +delete from t1 where pk=44; +set global rocksdb_force_flush_memtable_and_lzero_now=1; +begin; +set debug_sync='rocksdb.check_flags_rmi SIGNAL con1_stopped WAIT_FOR con1_cont'; +update t1 set a=a+100 where pk < 3 or pk between 10 and 50; +set debug_sync='now WAIT_FOR con1_stopped'; +insert into t1 values (44,5000); +delete from t1 where pk= 42; +update t1 set a=5000 where pk between 40 and 45; +set global rocksdb_force_flush_memtable_and_lzero_now=1; +set debug_sync='now SIGNAL con1_cont'; +select * from t1 where pk<100; +pk a +0 100 +1 101 +2 102 +3 3 +4 4 +5 5 +6 6 +7 7 +8 8 +9 9 +40 5100 +41 5100 +43 5100 +44 5100 +45 5100 +46 146 +47 147 +48 148 +49 149 +commit; +set debug_sync='RESET'; +drop table t1, ten, one_k; diff --git a/mysql-test/suite/rocksdb/t/range_locking_refresh_iter.test b/mysql-test/suite/rocksdb/t/range_locking_refresh_iter.test new file mode 100644 index 00000000000..a9e3de29aac --- /dev/null +++ b/mysql-test/suite/rocksdb/t/range_locking_refresh_iter.test @@ -0,0 +1,69 @@ +--source include/have_rocksdb.inc +--source suite/rocksdb/include/have_range_locking.inc + +select @@rocksdb_use_range_locking; + +--disable_warnings +set debug_sync='RESET'; +--enable_warnings +# +# Testcase for iterator snapshot refresh +# +create table ten(a int primary key); +insert into ten values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); + +create table one_k(a int primary key); +insert into one_k select A.a + B.a* 10 + C.a * 100 from ten A, ten B, ten C; + +create table t1 ( + pk int primary key, + a int +) engine=rocksdb; + +insert into t1 select a,a from ten; +insert into t1 select a+40, a+40 from ten; +insert into t1 select a+100, a+100 from one_k; +delete from t1 where pk=44; +set global rocksdb_force_flush_memtable_and_lzero_now=1; + +# Ok, now the table has these PK ranges: +# 0..9 40..49 100...1000 +# and all rows have pk=a +connect (con1,localhost,root,,); +connect (con2,localhost,root,,); + +connection con1; +begin; +set debug_sync='rocksdb.check_flags_rmi SIGNAL con1_stopped WAIT_FOR con1_cont'; +send +update t1 set a=a+100 where pk < 3 or pk between 10 and 50; + +# The query is how stuck at the start of the second range. + + +## con2> +connection con2; +set debug_sync='now WAIT_FOR con1_stopped'; + +# Make some changes to check if the iterator is reading current data or +# snapshot +insert into t1 values (44,5000); +delete from t1 where pk= 42; +update t1 set a=5000 where pk between 40 and 45; +set global rocksdb_force_flush_memtable_and_lzero_now=1; + +set debug_sync='now SIGNAL con1_cont'; + +connection con1; +#--error ER_GET_ERRMSG +reap; +select * from t1 where pk<100; + +commit; +disconnect con1; +disconnect con2; +connection default; +set debug_sync='RESET'; + +drop table t1, ten, one_k; + diff --git a/storage/rocksdb/ha_rocksdb.cc b/storage/rocksdb/ha_rocksdb.cc index 262af4816f1..2e161dbfb4b 100644 --- a/storage/rocksdb/ha_rocksdb.cc +++ b/storage/rocksdb/ha_rocksdb.cc @@ -8604,6 +8604,25 @@ int ha_rocksdb::set_range_lock(Rdb_transaction *tx, return 0; } + /* + RocksDB's iterator is reading the snapshot of the data that was taken at + the time the iterator was created. + + After we've got a lock on the range, we'll need to refresh the iterator + to read the latest contents. (If we use the iterator created before the + lock_range() call, we may miss the changes that were made/committed after + the iterator was created but before the lock_range() call was made). + + RocksDB has Iterator::Refresh() method, but alas, it is not implemented for + the iterator returned by Transaction object (Transaction object returns + BaseDeltaIterator which allows one to see the transactions's own changes). + + Our solution to this is to release the iterator and create the new one. + We release it here, it will be created as soon as there's a need to read + records. + */ + release_scan_iterator(); + auto s= tx->lock_range(kd.get_cf(), start_endp, end_endp); if (!s.ok()) { return (tx->set_status_error(table->in_use, s, kd, m_tbl_def, @@ -10704,7 +10723,7 @@ void ha_rocksdb::setup_scan_iterator(const Rdb_key_def &kd, and re-create Iterator. */ - // psergey-todo: create Locking Iterator here.. + if (m_scan_it_skips_bloom != skip_bloom || use_locking_iterator) { release_scan_iterator(); } diff --git a/storage/rocksdb/rdb_locking_iter.cc b/storage/rocksdb/rdb_locking_iter.cc index bd2ff90fc83..7f5e241d906 100644 --- a/storage/rocksdb/rdb_locking_iter.cc +++ b/storage/rocksdb/rdb_locking_iter.cc @@ -44,6 +44,7 @@ void LockingIterator::SeekForPrev(const rocksdb::Slice& target) { next_key is, we reach it by calling { Seek(current_key); Next(); } */ void LockingIterator::Next() { + DEBUG_SYNC(current_thd, "rocksdb.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 diff --git a/storage/rocksdb/rdb_locking_iter.h b/storage/rocksdb/rdb_locking_iter.h index e2373648d7e..855f3f2cc0a 100644 --- a/storage/rocksdb/rdb_locking_iter.h +++ b/storage/rocksdb/rdb_locking_iter.h @@ -86,7 +86,8 @@ class LockingIterator : public rocksdb::Iterator { } private: - template <bool forward> void Scan(const rocksdb::Slice& target, bool call_next) { + template <bool forward> void Scan(const rocksdb::Slice& target, + bool call_next) { if (!iter_->Valid()) { status_ = iter_->status(); valid_ = false; @@ -101,10 +102,15 @@ class LockingIterator : public rocksdb::Iterator { DEBUG_SYNC(current_thd, "rocksdb.locking_iter_scan"); auto end_key = iter_->key(); bool endp_arg= m_is_rev_cf; - if (forward) - status_ = txn_->GetRangeLock(cfh_, rocksdb::Endpoint(target, endp_arg), rocksdb::Endpoint(end_key, endp_arg)); - else - status_ = txn_->GetRangeLock(cfh_, rocksdb::Endpoint(end_key, endp_arg), rocksdb::Endpoint(target, endp_arg)); + if (forward) { + status_ = txn_->GetRangeLock(cfh_, + rocksdb::Endpoint(target, endp_arg), + rocksdb::Endpoint(end_key, endp_arg)); + } else { + status_ = txn_->GetRangeLock(cfh_, + rocksdb::Endpoint(end_key, endp_arg), + rocksdb::Endpoint(target, endp_arg)); + } if (!status_.ok()) { // Failed to get a lock (most likely lock wait timeout) @@ -138,8 +144,8 @@ class LockingIterator : public rocksdb::Iterator { } if (iter_->Valid()) { - int invert= forward? 1 : -1; - if (cmp->Compare(iter_->key(), rocksdb::Slice(end_key_copy)) * invert <= 0) { + int inv = forward ? 1 : -1; + if (cmp->Compare(iter_->key(), rocksdb::Slice(end_key_copy))*inv <= 0) { // Ok, the found key is within the range. status_ = rocksdb::Status::OK(); valid_= true;

1 0

[Commits] 8b312ab6f5b: MDEV-21243: Join buffer: condition is checked in wrong place for range access
by psergey 07 Dec '19

07 Dec '19

revision-id: 8b312ab6f5b18f812114a7c23eee12b0ef7d559d (mariadb-10.3.20-49-g8b312ab6f5b) parent(s): e5e5877740f248de848219ee3a1d2881cd5c5b82 author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2019-12-08 00:07:31 +0300 message: MDEV-21243: Join buffer: condition is checked in wrong place for range access In this scenario: - There is a possible range access for table T - And there is a ref access on the same index which uses fewer key parts - The join optimizer picks the ref access (because it is cheaper) - make_join_select applies this heuristic to switch to range: /* Range uses longer key; Use this instead of ref on key */ Join buffer will be used without having called JOIN_TAB::make_scan_filter(). This means, conditions that should be checked when reading table T will be checked after T is joined with the contents of the join buffer, instead. Fixed this by adding a make_scan_filter() check. (updated patch after backport to 10.3) --- mysql-test/main/join_cache.result | 72 +++++++++++++++++++++++++++++++++++++++ mysql-test/main/join_cache.test | 40 ++++++++++++++++++++++ sql/sql_select.cc | 10 ++++++ 3 files changed, 122 insertions(+) diff --git a/mysql-test/main/join_cache.result b/mysql-test/main/join_cache.result index bab32395413..3074d3b3cf4 100644 --- a/mysql-test/main/join_cache.result +++ b/mysql-test/main/join_cache.result @@ -6056,4 +6056,76 @@ select f2 from t2,t1 where f2 = 0; f2 drop table t1, t2; set join_buffer_size=@save_join_buffer_size; +# +# MDEV-21243: Join buffer: condition is checked in wrong place for range access +# +create table t1(a int primary key); +insert into t1 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); +create table t2 (a int); +insert into t2 select A.a + 10*B.a from t1 A, t1 B; +create table t3 ( +kp1 int, +kp2 int, +col1 int, +col2 int, +key (kp1, kp2) +); +insert into t3 +select +A.a, +B.a, +A.a + 100*B.a, +A.a + 100*B.a +from +t2 A, t2 B; +analyze table t3; +Table Op Msg_type Msg_text +test.t3 analyze status Table is already up to date +# The following must have "B.col1 + 1 < 33333" attached to table B +# and not to the block-nl-join node: +explain format=json +select * +from t1 A, t3 B +where +B.kp1=A.a and +B.kp1 <= 10 and +B.kp2 <= 10 and +B.col1 +1 < 33333; +EXPLAIN +{ + "query_block": { + "select_id": 1, + "table": { + "table_name": "A", + "access_type": "index", + "possible_keys": ["PRIMARY"], + "key": "PRIMARY", + "key_length": "4", + "used_key_parts": ["a"], + "rows": 10, + "filtered": 100, + "attached_condition": "A.a <= 10", + "using_index": true + }, + "block-nl-join": { + "table": { + "table_name": "B", + "access_type": "range", + "possible_keys": ["kp1"], + "key": "kp1", + "key_length": "10", + "used_key_parts": ["kp1", "kp2"], + "rows": 836, + "filtered": 11.962, + "index_condition": "B.kp2 <= 10", + "attached_condition": "B.kp2 <= 10 and B.col1 + 1 < 33333" + }, + "buffer_type": "flat", + "buffer_size": "256Kb", + "join_type": "BNL", + "attached_condition": "B.kp1 = A.a" + } + } +} +drop table t1,t2,t3; set @@optimizer_switch=@save_optimizer_switch; diff --git a/mysql-test/main/join_cache.test b/mysql-test/main/join_cache.test index 9576d598125..248b8dbddf5 100644 --- a/mysql-test/main/join_cache.test +++ b/mysql-test/main/join_cache.test @@ -4014,5 +4014,45 @@ select f2 from t2,t1 where f2 = 0; drop table t1, t2; set join_buffer_size=@save_join_buffer_size; + +--echo # +--echo # MDEV-21243: Join buffer: condition is checked in wrong place for range access +--echo # +create table t1(a int primary key); +insert into t1 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); +create table t2 (a int); +insert into t2 select A.a + 10*B.a from t1 A, t1 B; + +create table t3 ( + kp1 int, + kp2 int, + col1 int, + col2 int, + key (kp1, kp2) +); + +insert into t3 +select + A.a, + B.a, + A.a + 100*B.a, + A.a + 100*B.a +from + t2 A, t2 B; +analyze table t3; + +--echo # The following must have "B.col1 + 1 < 33333" attached to table B +--echo # and not to the block-nl-join node: +explain format=json +select * +from t1 A, t3 B +where + B.kp1=A.a and + B.kp1 <= 10 and + B.kp2 <= 10 and + B.col1 +1 < 33333; + +drop table t1,t2,t3; + # The following command must be the last one in the file set @@optimizer_switch=@save_optimizer_switch; diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 275c46141cd..fbce8708c79 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -10960,6 +10960,13 @@ make_join_select(JOIN *join,SQL_SELECT *select,COND *cond) if (i != join->const_tables && tab->use_quick != 2 && !tab->first_inner) { /* Read with cache */ + /* + TODO: the execution also gets here when we will not be using + join buffer. Review these cases and perhaps, remove this call. + (The final decision whether to use join buffer is made in + check_join_cache_usage, so we should only call make_scan_filter() + there, too). + */ if (tab->make_scan_filter()) DBUG_RETURN(1); } @@ -11922,6 +11929,9 @@ uint check_join_cache_usage(JOIN_TAB *tab, if ((tab->cache= new (root) JOIN_CACHE_BNL(join, tab, prev_cache))) { tab->icp_other_tables_ok= FALSE; + /* If make_join_select() hasn't called make_scan_filter(), do it now */ + if (!tab->cache_select && tab->make_scan_filter()) + goto no_join_cache; return (2 - MY_TEST(!prev_cache)); } goto no_join_cache;

1 0

[Commits] ad01508d2f4: MDEV-21243: Join buffer: condition is checked in wrong place for range access
by psergey 07 Dec '19

07 Dec '19

revision-id: ad01508d2f44d1b5b28231a5100b34cfb5831cfc (mariadb-10.3.20-49-gad01508d2f4) parent(s): e5e5877740f248de848219ee3a1d2881cd5c5b82 author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2019-12-07 18:21:36 +0300 message: MDEV-21243: Join buffer: condition is checked in wrong place for range access In this scenario: - There is a possible range access for table T - And there is a ref access on the same index which uses fewer key parts - The join optimizer picks the ref access (because it is cheaper) - make_join_select applies this heuristic to switch to range: /* Range uses longer key; Use this instead of ref on key */ Join buffer will be used without having called JOIN_TAB::make_scan_filter(). This means, conditions that should be checked when reading table T will be checked after T is joined with the contents of the join buffer, instead. Fixed this by adding a make_scan_filter() check. --- mysql-test/main/join_cache.result | 73 +++++++++++++++++++++++++++++++++++++++ mysql-test/main/join_cache.test | 40 +++++++++++++++++++++ sql/sql_select.cc | 10 ++++++ 3 files changed, 123 insertions(+) diff --git a/mysql-test/main/join_cache.result b/mysql-test/main/join_cache.result index bab32395413..6e871f43b76 100644 --- a/mysql-test/main/join_cache.result +++ b/mysql-test/main/join_cache.result @@ -6056,4 +6056,77 @@ select f2 from t2,t1 where f2 = 0; f2 drop table t1, t2; set join_buffer_size=@save_join_buffer_size; +# +# MDEV-21243: Join buffer: condition is checked in wrong place for range access +# +create table t1(a int primary key); +insert into t1 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); +create table t2 (a int); +insert into t2 select A.a + 10*B.a from t1 A, t1 B; +create table t3 ( +kp1 int, +kp2 int, +col1 int, +col2 int, +key (kp1, kp2) +); +insert into t3 +select +A.a, +B.a, +A.a + 100*B.a, +A.a + 100*B.a +from +t2 A, t2 B; +analyze table t3; +Table Op Msg_type Msg_text +test.t3 analyze status Engine-independent statistics collected +test.t3 analyze status Table is already up to date +# The following must have "B.col1 + 1 < 33333" attached to table B +# and not to the block-nl-join node: +explain format=json +select * +from t1 A, t3 B +where +B.kp1=A.a and +B.kp1 <= 10 and +B.kp2 <= 10 and +B.col1 +1 < 33333; +EXPLAIN +{ + "query_block": { + "select_id": 1, + "table": { + "table_name": "A", + "access_type": "index", + "possible_keys": ["PRIMARY"], + "key": "PRIMARY", + "key_length": "4", + "used_key_parts": ["a"], + "rows": 10, + "filtered": 100, + "attached_condition": "A.a <= 10", + "using_index": true + }, + "block-nl-join": { + "table": { + "table_name": "B", + "access_type": "range", + "possible_keys": ["kp1"], + "key": "kp1", + "key_length": "10", + "used_key_parts": ["kp1", "kp2"], + "rows": 836, + "filtered": 76.434, + "index_condition": "B.kp2 <= 10", + "attached_condition": "B.kp2 <= 10 and B.col1 + 1 < 33333" + }, + "buffer_type": "flat", + "buffer_size": "54", + "join_type": "BNL", + "attached_condition": "B.kp1 = A.a" + } + } +} +drop table t1,t2,t3; set @@optimizer_switch=@save_optimizer_switch; diff --git a/mysql-test/main/join_cache.test b/mysql-test/main/join_cache.test index 9576d598125..248b8dbddf5 100644 --- a/mysql-test/main/join_cache.test +++ b/mysql-test/main/join_cache.test @@ -4014,5 +4014,45 @@ select f2 from t2,t1 where f2 = 0; drop table t1, t2; set join_buffer_size=@save_join_buffer_size; + +--echo # +--echo # MDEV-21243: Join buffer: condition is checked in wrong place for range access +--echo # +create table t1(a int primary key); +insert into t1 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); +create table t2 (a int); +insert into t2 select A.a + 10*B.a from t1 A, t1 B; + +create table t3 ( + kp1 int, + kp2 int, + col1 int, + col2 int, + key (kp1, kp2) +); + +insert into t3 +select + A.a, + B.a, + A.a + 100*B.a, + A.a + 100*B.a +from + t2 A, t2 B; +analyze table t3; + +--echo # The following must have "B.col1 + 1 < 33333" attached to table B +--echo # and not to the block-nl-join node: +explain format=json +select * +from t1 A, t3 B +where + B.kp1=A.a and + B.kp1 <= 10 and + B.kp2 <= 10 and + B.col1 +1 < 33333; + +drop table t1,t2,t3; + # The following command must be the last one in the file set @@optimizer_switch=@save_optimizer_switch; diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 275c46141cd..fbce8708c79 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -10960,6 +10960,13 @@ make_join_select(JOIN *join,SQL_SELECT *select,COND *cond) if (i != join->const_tables && tab->use_quick != 2 && !tab->first_inner) { /* Read with cache */ + /* + TODO: the execution also gets here when we will not be using + join buffer. Review these cases and perhaps, remove this call. + (The final decision whether to use join buffer is made in + check_join_cache_usage, so we should only call make_scan_filter() + there, too). + */ if (tab->make_scan_filter()) DBUG_RETURN(1); } @@ -11922,6 +11929,9 @@ uint check_join_cache_usage(JOIN_TAB *tab, if ((tab->cache= new (root) JOIN_CACHE_BNL(join, tab, prev_cache))) { tab->icp_other_tables_ok= FALSE; + /* If make_join_select() hasn't called make_scan_filter(), do it now */ + if (!tab->cache_select && tab->make_scan_filter()) + goto no_join_cache; return (2 - MY_TEST(!prev_cache)); } goto no_join_cache;

1 0

[Commits] e949b2d4aba: MDEV-20959: binlog.binlog_parallel_replication_marks_row fails in buildbot with wrong result
by sujatha 06 Dec '19

06 Dec '19

revision-id: e949b2d4abac99ffbc68bfd8c39a74e30caa3902 (mariadb-10.1.43-29-ge949b2d4aba) parent(s): 42bad56aab25db4a05c676ba99290949ad4e1c34 author: Sujatha committer: Sujatha timestamp: 2019-12-06 19:58:12 +0530 message: MDEV-20959: binlog.binlog_parallel_replication_marks_row fails in buildbot with wrong result Problem: ======= Test "binlog.binlog_parallel_replication_marks_row" fails sporadically due to result length mismatch. Analysis: ========= Test generates a binary log and it looks for certain words within the binary log file and prints them. For example word like "GTID,BEGIN,COMMIT ...". Binary log output contains base64 encoded characters. Occasionally the encoded characters match with the above words and results in test failure. +XwoFWxMBAAAALgAAAGEDAAAAAB8AAAAAAAEABHRlc3QAAnQxAAIDAwACFGTIDQ== +AAAAAAAAAAAEEwQADQgICAoKCgGTIDw9 Fix: === Improve the regular expression to match exact words. --- mysql-test/include/binlog_parallel_replication_marks.test | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mysql-test/include/binlog_parallel_replication_marks.test b/mysql-test/include/binlog_parallel_replication_marks.test index 4e673bd30c3..6e22c7dd75e 100644 --- a/mysql-test/include/binlog_parallel_replication_marks.test +++ b/mysql-test/include/binlog_parallel_replication_marks.test @@ -79,7 +79,7 @@ while (<F>) { s/end_log_pos \d+/end_log_pos #/; s/table id \d+/table id #/; s/mapped to number \d+/mapped to number #/; - print if /GTID|BEGIN|COMMIT|Table_map|Write_rows|Update_rows|Delete_rows|generated by server|40005 TEMPORARY/; + print if /\b(GTID|BEGIN|COMMIT|Table_map|Write_rows|Update_rows|Delete_rows|generated by server|40005 TEMPORARY)\b/; } close F; EOF

1 0