- commits - lists.mariadb.org

[Commits] f69cc267577: MDEV-23596: Assertion `tab->ref.use_count' failed in join_read_key_unlock_row
by Varun 27 Aug '20

27 Aug '20

revision-id: f69cc26757733724254ee37aec5a092f520d230f (mariadb-10.1.43-266-gf69cc267577) parent(s): 62d1e3bf67a12eb6f48ac615bda119e2ed65cf16 author: Varun Gupta committer: Varun Gupta timestamp: 2020-08-27 17:58:13 +0530 message: MDEV-23596: Assertion `tab->ref.use_count' failed in join_read_key_unlock_row The issue here was that the query was using ORDER BY LIMIT optimzation where the access method was changed from EQ_REF access to an index scan (index that would resolve the ORDER BY clause). But the parameter READ_RECORD::unlock_row was not reset to rr_unlock_row, which is used when the access method is not EQ_REF access. --- mysql-test/r/order_by.result | 16 ++++++++++++++++ mysql-test/t/order_by.test | 15 +++++++++++++++ sql/sql_select.cc | 3 +++ 3 files changed, 34 insertions(+) diff --git a/mysql-test/r/order_by.result b/mysql-test/r/order_by.result index ffb37c9309f..f7427bcd30e 100644 --- a/mysql-test/r/order_by.result +++ b/mysql-test/r/order_by.result @@ -3356,3 +3356,19 @@ SET max_sort_length= @save_max_sort_length; SET sort_buffer_size= @save_sort_buffer_size; SET max_length_for_sort_data= @save_max_length_for_sort_data; DROP TABLE t1; +# +# MDEV-23596: Assertion `tab->ref.use_count' failed in join_read_key_unlock_row +# +CREATE TABLE t1 (a INT PRIMARY KEY, b INT, KEY(b)); +INSERT INTO t1 VALUES (0, 1),(1, 2); +CREATE TABLE t2 SELECT * FROM t1; +EXPLAIN SELECT (SELECT 1 FROM t1 WHERE t1.a=t2.b ORDER BY t1.b LIMIT 1) AS c FROM t2; +id select_type table type possible_keys key key_len ref rows Extra +1 PRIMARY t2 ALL NULL NULL NULL NULL 2 +2 DEPENDENT SUBQUERY t1 index PRIMARY b 5 NULL 1 Using where +SELECT (SELECT 1 FROM t1 WHERE t1.a=t2.b ORDER BY t1.b LIMIT 1) AS c FROM t2; +c +1 +NULL +DROP TABLE t1,t2; +# end of 10.1 tests diff --git a/mysql-test/t/order_by.test b/mysql-test/t/order_by.test index 3a30e0b6c76..3e614ed3316 100644 --- a/mysql-test/t/order_by.test +++ b/mysql-test/t/order_by.test @@ -2196,3 +2196,18 @@ SET max_sort_length= @save_max_sort_length; SET sort_buffer_size= @save_sort_buffer_size; SET max_length_for_sort_data= @save_max_length_for_sort_data; DROP TABLE t1; + +--echo # +--echo # MDEV-23596: Assertion `tab->ref.use_count' failed in join_read_key_unlock_row +--echo # + +CREATE TABLE t1 (a INT PRIMARY KEY, b INT, KEY(b)); +INSERT INTO t1 VALUES (0, 1),(1, 2); +CREATE TABLE t2 SELECT * FROM t1; + +EXPLAIN SELECT (SELECT 1 FROM t1 WHERE t1.a=t2.b ORDER BY t1.b LIMIT 1) AS c FROM t2; +SELECT (SELECT 1 FROM t1 WHERE t1.a=t2.b ORDER BY t1.b LIMIT 1) AS c FROM t2; + +DROP TABLE t1,t2; + +--echo # end of 10.1 tests diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 4c6e87e4f27..e2e87bb1a86 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -21601,6 +21601,9 @@ test_if_skip_sort_order(JOIN_TAB *tab,ORDER *order,ha_rows select_limit, else if (select && select->quick) select->quick->need_sorted_output(); + tab->read_record.unlock_row= (tab->type == JT_EQ_REF) ? + join_read_key_unlock_row : rr_unlock_row; + } // QEP has been modified /*

1 0

[Commits] 2f859962b03: MDEV-23534: SIGSEGV in sf_malloc_usable_size/my_free on SET GLOBAL REPLICATE_DO_TABLE
by sujatha 27 Aug '20

27 Aug '20

revision-id: 2f859962b032cc75176e05df8d704eec413cdb17 (mariadb-10.1.43-266-g2f859962b03) parent(s): 62d1e3bf67a12eb6f48ac615bda119e2ed65cf16 author: Sujatha committer: Sujatha timestamp: 2020-08-26 16:25:28 +0530 message: MDEV-23534: SIGSEGV in sf_malloc_usable_size/my_free on SET GLOBAL REPLICATE_DO_TABLE Backporting fixes for: MDEV-22317: SIGSEGV in my_free/delete_dynamic in optimized builds (ARIA) MDEV-22059: MSAN report at replicate_ignore_table_grant --- .../suite/rpl/r/rpl_filter_tables_dynamic.result | 2 + .../rpl/r/rpl_filter_wild_tables_dynamic.result | 2 + .../suite/rpl/t/rpl_filter_tables_dynamic.test | 2 + .../rpl/t/rpl_filter_wild_tables_dynamic.test | 2 + sql/rpl_filter.cc | 52 ++++++++++++++++------ 5 files changed, 46 insertions(+), 14 deletions(-) diff --git a/mysql-test/suite/rpl/r/rpl_filter_tables_dynamic.result b/mysql-test/suite/rpl/r/rpl_filter_tables_dynamic.result index 5a746c88458..9709e24fbde 100644 --- a/mysql-test/suite/rpl/r/rpl_filter_tables_dynamic.result +++ b/mysql-test/suite/rpl/r/rpl_filter_tables_dynamic.result @@ -5,6 +5,8 @@ ERROR HY000: This operation cannot be performed as you have a running slave ''; SET @@GLOBAL.replicate_ignore_table="test.t4,test.t5,test.t6"; ERROR HY000: This operation cannot be performed as you have a running slave ''; run STOP SLAVE '' first include/stop_slave.inc +SET @@GLOBAL.replicate_do_table=""; +SET @@GLOBAL.replicate_ignore_table=""; SET @@GLOBAL.replicate_do_table="test.t1,test.t2,test.t3"; SET @@GLOBAL.replicate_ignore_table="test.t4,test.t5,test.t6"; include/start_slave.inc diff --git a/mysql-test/suite/rpl/r/rpl_filter_wild_tables_dynamic.result b/mysql-test/suite/rpl/r/rpl_filter_wild_tables_dynamic.result index 19d8e513e6f..338f4b3bbcf 100644 --- a/mysql-test/suite/rpl/r/rpl_filter_wild_tables_dynamic.result +++ b/mysql-test/suite/rpl/r/rpl_filter_wild_tables_dynamic.result @@ -5,6 +5,8 @@ ERROR HY000: This operation cannot be performed as you have a running slave ''; SET @@GLOBAL.replicate_wild_ignore_table="test.b%"; ERROR HY000: This operation cannot be performed as you have a running slave ''; run STOP SLAVE '' first include/stop_slave.inc +SET @@GLOBAL.replicate_wild_do_table=""; +SET @@GLOBAL.replicate_wild_ignore_table=""; SET @@GLOBAL.replicate_wild_do_table="test.a%"; SET @@GLOBAL.replicate_wild_ignore_table="test.b%"; include/start_slave.inc diff --git a/mysql-test/suite/rpl/t/rpl_filter_tables_dynamic.test b/mysql-test/suite/rpl/t/rpl_filter_tables_dynamic.test index 97ecc167356..ebededc36b3 100644 --- a/mysql-test/suite/rpl/t/rpl_filter_tables_dynamic.test +++ b/mysql-test/suite/rpl/t/rpl_filter_tables_dynamic.test @@ -51,6 +51,8 @@ SET @@GLOBAL.replicate_ignore_table="test.t4,test.t5,test.t6"; connection slave; source include/stop_slave.inc; +SET @@GLOBAL.replicate_do_table=""; +SET @@GLOBAL.replicate_ignore_table=""; SET @@GLOBAL.replicate_do_table="test.t1,test.t2,test.t3"; SET @@GLOBAL.replicate_ignore_table="test.t4,test.t5,test.t6"; source include/start_slave.inc; diff --git a/mysql-test/suite/rpl/t/rpl_filter_wild_tables_dynamic.test b/mysql-test/suite/rpl/t/rpl_filter_wild_tables_dynamic.test index c822c81f270..09db91aa4d3 100644 --- a/mysql-test/suite/rpl/t/rpl_filter_wild_tables_dynamic.test +++ b/mysql-test/suite/rpl/t/rpl_filter_wild_tables_dynamic.test @@ -13,6 +13,8 @@ SET @@GLOBAL.replicate_wild_ignore_table="test.b%"; connection slave; source include/stop_slave.inc; +SET @@GLOBAL.replicate_wild_do_table=""; +SET @@GLOBAL.replicate_wild_ignore_table=""; SET @@GLOBAL.replicate_wild_do_table="test.a%"; SET @@GLOBAL.replicate_wild_ignore_table="test.b%"; source include/start_slave.inc; diff --git a/sql/rpl_filter.cc b/sql/rpl_filter.cc index 366902c1f26..0d5d9ffeea8 100644 --- a/sql/rpl_filter.cc +++ b/sql/rpl_filter.cc @@ -349,14 +349,20 @@ Rpl_filter::set_do_table(const char* table_spec) int status; if (do_table_inited) - my_hash_reset(&do_table); + { + my_hash_free(&do_table); + do_table_inited= 0; + } status= parse_filter_rule(table_spec, &Rpl_filter::add_do_table); - if (!do_table.records) + if (do_table_inited && status) { - my_hash_free(&do_table); - do_table_inited= 0; + if (!do_table.records) + { + my_hash_free(&do_table); + do_table_inited= 0; + } } return status; @@ -369,14 +375,20 @@ Rpl_filter::set_ignore_table(const char* table_spec) int status; if (ignore_table_inited) - my_hash_reset(&ignore_table); + { + my_hash_free(&ignore_table); + ignore_table_inited= 0; + } status= parse_filter_rule(table_spec, &Rpl_filter::add_ignore_table); - if (!ignore_table.records) + if (ignore_table_inited && status) { - my_hash_free(&ignore_table); - ignore_table_inited= 0; + if (!ignore_table.records) + { + my_hash_free(&ignore_table); + ignore_table_inited= 0; + } } return status; @@ -411,14 +423,20 @@ Rpl_filter::set_wild_do_table(const char* table_spec) int status; if (wild_do_table_inited) + { free_string_array(&wild_do_table); + wild_do_table_inited= 0; + } status= parse_filter_rule(table_spec, &Rpl_filter::add_wild_do_table); - if (!wild_do_table.elements) + if (wild_do_table_inited && status) { - delete_dynamic(&wild_do_table); - wild_do_table_inited= 0; + if (!wild_do_table.elements) + { + delete_dynamic(&wild_do_table); + wild_do_table_inited= 0; + } } return status; @@ -431,14 +449,20 @@ Rpl_filter::set_wild_ignore_table(const char* table_spec) int status; if (wild_ignore_table_inited) + { free_string_array(&wild_ignore_table); + wild_ignore_table_inited= 0; + } status= parse_filter_rule(table_spec, &Rpl_filter::add_wild_ignore_table); - if (!wild_ignore_table.elements) + if (wild_ignore_table_inited && status) { - delete_dynamic(&wild_ignore_table); - wild_ignore_table_inited= 0; + if (!wild_ignore_table.elements) + { + delete_dynamic(&wild_ignore_table); + wild_ignore_table_inited= 0; + } } return status;

1 0

[Commits] 9dfa6234f20: Post-rebase fixes part 2: use DBUG_RETURN, not return
by Sergei Petrunia 24 Aug '20

24 Aug '20

revision-id: 9dfa6234f2092c3e65f54823731b65b2a6360b52 (fb-prod202002-128-g9dfa6234f20) parent(s): b211e08c0b717a7a12b239c941d2be891a03c52a author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2020-08-24 18:26:15 +0300 message: Post-rebase fixes part 2: use DBUG_RETURN, not return --- storage/rocksdb/ha_rocksdb.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/storage/rocksdb/ha_rocksdb.cc b/storage/rocksdb/ha_rocksdb.cc index 053fe4a99f6..a2e5900373f 100644 --- a/storage/rocksdb/ha_rocksdb.cc +++ b/storage/rocksdb/ha_rocksdb.cc @@ -11337,11 +11337,13 @@ int ha_rocksdb::delete_row(const uchar *const buf) { /* For point locking, Deleting on secondary key doesn't need any locks. Range locking must set locks + (TODO: don't get the lock here if we've got it in key_info->flags & + HA_NOSAME branch above?) */ if (rocksdb_use_range_locking) { auto s= tx->lock_singlepoint_range(kd.get_cf(), secondary_key_slice); if (!s.ok()) { - return (tx->set_status_error(table->in_use, s, kd, m_tbl_def, + DBUG_RETURN(tx->set_status_error(table->in_use, s, kd, m_tbl_def, m_table_handler)); } }

1 0

[Commits] b211e08c0b7: More Post-rebase fixes, update RocksDB to RangeLocking+LockTracker
by Sergei Petrunia 24 Aug '20

24 Aug '20

revision-id: b211e08c0b717a7a12b239c941d2be891a03c52a (fb-prod202002-127-gb211e08c0b7) parent(s): 6618bee92d82872739fac5c115e70077da238fb4 author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2020-08-24 16:59:31 +0300 message: More Post-rebase fixes, update RocksDB to RangeLocking+LockTracker --- mysql-test/suite/rocksdb/t/range_locking.test | 3 ++- rocksdb | 2 +- storage/rocksdb/CMakeLists.txt | 9 +++++---- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/mysql-test/suite/rocksdb/t/range_locking.test b/mysql-test/suite/rocksdb/t/range_locking.test index 9ce2ac34fd8..5c599238a0a 100644 --- a/mysql-test/suite/rocksdb/t/range_locking.test +++ b/mysql-test/suite/rocksdb/t/range_locking.test @@ -2,4 +2,5 @@ --let pk_cf=default --let sk_cf=default ---source range_locking.inc \ No newline at end of file +--source range_locking.inc + diff --git a/rocksdb b/rocksdb index 515a27941a4..402758a0f68 160000 --- a/rocksdb +++ b/rocksdb @@ -1 +1 @@ -Subproject commit 515a27941a42018531354414eca661113da92957 +Subproject commit 402758a0f6899ec200860f13b5b606654cada766 diff --git a/storage/rocksdb/CMakeLists.txt b/storage/rocksdb/CMakeLists.txt index fb36bebe9cc..3cffa064130 100644 --- a/storage/rocksdb/CMakeLists.txt +++ b/storage/rocksdb/CMakeLists.txt @@ -40,8 +40,9 @@ ELSE() # Statically include all RocksDB source SET(ROCKSDB_SOURCES - ${CMAKE_SOURCE_DIR}/rocksdb/utilities/transactions/range_locking - ${CMAKE_SOURCE_DIR}/rocksdb/utilities/transactions/range_locking/portability + ${ROCKSDB_LIB_SOURCES} + ${CMAKE_SOURCE_DIR}/rocksdb/utilities/transactions/range_locking + ${CMAKE_SOURCE_DIR}/rocksdb/utilities/transactions/range_locking/portability ) ENDIF() @@ -49,6 +50,8 @@ INCLUDE_DIRECTORIES( ${ROCKSDB_ROOT} ${ROCKSDB_ROOT}/include ${ROCKSDB_ROOT}/third-party/gtest-1.8.1/fused-src + ${CMAKE_SOURCE_DIR}/rocksdb/utilities/transactions/range_locking + ${CMAKE_SOURCE_DIR}/rocksdb/utilities/transactions/range_locking/portability ) IF(UNIX) @@ -81,8 +84,6 @@ else() "MyRocks requires that feature.") endif() - - SET(ROCKSDB_SOURCES ${ROCKSDB_SOURCES} ha_rocksdb.cc ha_rocksdb.h ha_rocksdb_proto.h

1 0

[Commits] 6618bee92d8: Post-rebase fixes: apply the patch lost during rebase:
by Sergei Petrunia 24 Aug '20

24 Aug '20

revision-id: 6618bee92d82872739fac5c115e70077da238fb4 (fb-prod202002-126-g6618bee92d8) parent(s): cf38ba95e9a5c4fda9351cf981b5bd55f42db61c author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2020-08-24 16:58:09 +0300 message: Post-rebase fixes: apply the patch lost during rebase: commit 43613926c1b82427c83a02759fc6785282fbfce2 Author: Sergei Petrunia <psergey(a)askmonty.org> Date: Mon Jan 7 19:52:48 2019 +0300 Range Locking: Make range locks on secondary indexes inhibit DMLs --- mysql-test/suite/rocksdb/r/range_locking.result | 45 +++++++++++++++++++ storage/rocksdb/ha_rocksdb.cc | 57 +++++++++++++++++++------ storage/rocksdb/ha_rocksdb.h | 4 +- 3 files changed, 91 insertions(+), 15 deletions(-) diff --git a/mysql-test/suite/rocksdb/r/range_locking.result b/mysql-test/suite/rocksdb/r/range_locking.result index dfac4ef5123..3641fb1943e 100644 --- a/mysql-test/suite/rocksdb/r/range_locking.result +++ b/mysql-test/suite/rocksdb/r/range_locking.result @@ -161,6 +161,51 @@ connection default; disconnect con1; drop table t0,t1; # +# Test that locks on ranges on non-unique secondary keys inhibit +# modifications of the contents of these ranges +# +create table t0(a int); +insert into t0 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); +create table t1 ( +kp1 int not null, +kp2 int not null, +a int, +key(kp1, kp2) comment 'default' +) engine=rocksdb; +insert into t1 select 1, a, 1234 from t0; +insert into t1 values (2, 3, 1234); +insert into t1 values (2, 5, 1234); +insert into t1 values (2, 7, 1234); +insert into t1 select 3, a, 1234 from t0; +connect con1,localhost,root,,; +connection con1; +begin; +explain +select * from t1 where kp1=2 for update; +id select_type table type possible_keys key key_len ref rows Extra +1 SIMPLE t1 ref kp1 kp1 4 const # NULL +select * from t1 where kp1=2 for update; +kp1 kp2 a +2 3 1234 +2 5 1234 +2 7 1234 +connection default; +begin; +insert into t1 values (2, 9, 9999); +ERROR HY000: Lock wait timeout exceeded; try restarting transaction: Timeout on index: test.t1.kp1 +delete from t1 where kp1=2 and kp2=5; +ERROR HY000: Lock wait timeout exceeded; try restarting transaction: Timeout on index: test.t1.kp1 +update t1 set kp1=333 where kp1=2 and kp2=3; +ERROR HY000: Lock wait timeout exceeded; try restarting transaction: Timeout on index: test.t1.kp1 +update t1 set kp1=2 where kp1=1 and kp2=8; +ERROR HY000: Lock wait timeout exceeded; try restarting transaction: Timeout on index: test.t1.kp1 +rollback; +connection con1; +rollback; +disconnect con1; +connection default; +drop table t0,t1; +# # Transaction isolation test # create table t1 (pk int primary key, a int) engine=rocksdb; diff --git a/storage/rocksdb/ha_rocksdb.cc b/storage/rocksdb/ha_rocksdb.cc index cd02ee1f8a3..053fe4a99f6 100644 --- a/storage/rocksdb/ha_rocksdb.cc +++ b/storage/rocksdb/ha_rocksdb.cc @@ -2580,6 +2580,7 @@ class Rdb_transaction { bool m_is_delayed_snapshot = false; bool m_is_two_phase = false; + std::unordered_set<Rdb_tbl_def*> modified_tables; private: @@ -2614,7 +2615,6 @@ class Rdb_transaction { virtual rocksdb::WriteBatchBase *get_write_batch() = 0; virtual bool commit_no_binlog() = 0; - /* @detail This function takes in the WriteBatch of the transaction to add @@ -2866,10 +2866,7 @@ class Rdb_transaction { return lock_range(cf, endp, endp); } - virtual bool prepare(const rocksdb::TransactionName &name) = 0; - - - + virtual bool prepare() = 0; bool commit_or_rollback() { bool res; @@ -3488,7 +3485,7 @@ class Rdb_transaction_impl : public Rdb_transaction { virtual bool is_writebatch_trx() const override { return false; } /* - Both start and end endpoint may may be prefixes. + Both start and end endpoint may be prefixes. Both bounds are inclusive. */ rocksdb::Status lock_range(rocksdb::ColumnFamilyHandle *const cf, @@ -3496,8 +3493,6 @@ class Rdb_transaction_impl : public Rdb_transaction { const rocksdb::Endpoint &end_endp) override { ++m_lock_count; return m_rocksdb_tx->GetRangeLock(cf, start_endp, end_endp); - ) override - { } private: void release_tx(void) { @@ -3968,7 +3963,6 @@ class Rdb_writebatch_impl : public Rdb_transaction { rocksdb::Status lock_range(rocksdb::ColumnFamilyHandle *const cf, const rocksdb::Endpoint &left_endp, const rocksdb::Endpoint &right_endp) override { - { return rocksdb::Status::OK(); } @@ -4088,8 +4082,8 @@ class Rdb_writebatch_impl : public Rdb_transaction { } void set_name() override {} - void start_stmt(bool is_dml_statement) override {} + void start_stmt(bool is_dml_statement) override {} void rollback_stmt() override { if (m_batch) rollback_to_stmt_savepoint(); @@ -8589,7 +8583,7 @@ int ha_rocksdb::index_read_map(uchar *const buf, const uchar *const key, } -void ha_rocksdb::set_range_lock(Rdb_transaction *tx, +int ha_rocksdb::set_range_lock(Rdb_transaction *tx, const Rdb_key_def &kd, const enum ha_rkey_function &find_flag, const rocksdb::Slice &slice_arg, @@ -8605,7 +8599,7 @@ void ha_rocksdb::set_range_lock(Rdb_transaction *tx, *use_locking_iterator= false; if (m_lock_rows == RDB_LOCK_NONE || !rocksdb_use_range_locking) { - return; + return 0; } bool no_end_endpoint= false; @@ -10662,6 +10656,15 @@ int ha_rocksdb::update_write_sk(const TABLE *const table_arg, old_key_slice = rocksdb::Slice( reinterpret_cast<const char *>(m_sk_packed_tuple_old), old_packed_size); + /* Range locking: lock the index tuple being deleted */ + if (rocksdb_use_range_locking) { + auto s= row_info.tx->lock_singlepoint_range(kd.get_cf(), old_key_slice); + if (!s.ok()) { + return (row_info.tx->set_status_error(table->in_use, s, kd, + m_tbl_def, m_table_handler)); + } + } + row_info.tx->get_indexed_write_batch()->SingleDelete(kd.get_cf(), old_key_slice); @@ -10677,6 +10680,14 @@ int ha_rocksdb::update_write_sk(const TABLE *const table_arg, if (bulk_load_sk && row_info.old_data == nullptr) { rc = bulk_load_key(row_info.tx, kd, new_key_slice, new_value_slice, true); } else { + /* Range locking: lock the index tuple being inserted */ + if (rocksdb_use_range_locking) { + auto s= row_info.tx->lock_singlepoint_range(kd.get_cf(), new_key_slice); + if (!s.ok()) { + return (row_info.tx->set_status_error(table->in_use, s, kd, + m_tbl_def, m_table_handler)); + } + } row_info.tx->get_indexed_write_batch()->Put(kd.get_cf(), new_key_slice, new_value_slice); } @@ -10774,6 +10785,10 @@ int ha_rocksdb::update_write_row(const uchar *const old_data, DBUG_RETURN(rc); } + // Range Locking: do we have a lock on the old PK value here? + // - we have read the row we are about to update, right? (except for some + // RBR mode? (in which we won't want to acquire locks anyway?)) + /* For UPDATEs, if the key has changed, we need to obtain a lock. INSERTs always require locking. @@ -10879,6 +10894,7 @@ void ha_rocksdb::setup_scan_iterator(const Rdb_key_def &kd, Rdb_transaction *const tx = get_or_create_tx(table->in_use); bool skip_bloom = true; + const rocksdb::Slice eq_cond(slice->data(), eq_cond_len); // The size of m_scan_it_lower_bound (and upper) is technically // max_packed_sk_len as calculated in ha_rocksdb::alloc_key_buffers. Rather @@ -10913,7 +10929,6 @@ void ha_rocksdb::setup_scan_iterator(const Rdb_key_def &kd, and re-create Iterator. */ - if (m_scan_it_skips_bloom != skip_bloom || use_locking_iterator) { release_scan_iterator(); } @@ -11262,6 +11277,9 @@ int ha_rocksdb::delete_row(const uchar *const buf) { Rdb_transaction *const tx = get_or_create_tx(table->in_use); ulonglong bytes_written = 0; + // Range Locking: we are certain that the PK record is already locked here, + // right? + const uint index = pk_index(table, m_tbl_def); rocksdb::Status s = delete_or_singledelete(index, tx, m_pk_descr->get_cf(), key_slice); @@ -11315,6 +11333,19 @@ int ha_rocksdb::delete_row(const uchar *const buf) { nullptr, false, hidden_pk_id); rocksdb::Slice secondary_key_slice( reinterpret_cast<const char *>(m_sk_packed_tuple), packed_size); + + /* + For point locking, Deleting on secondary key doesn't need any locks. + Range locking must set locks + */ + if (rocksdb_use_range_locking) { + auto s= tx->lock_singlepoint_range(kd.get_cf(), secondary_key_slice); + if (!s.ok()) { + return (tx->set_status_error(table->in_use, s, kd, m_tbl_def, + m_table_handler)); + } + } + tx->get_indexed_write_batch()->SingleDelete(kd.get_cf(), secondary_key_slice); bytes_written += secondary_key_slice.size(); diff --git a/storage/rocksdb/ha_rocksdb.h b/storage/rocksdb/ha_rocksdb.h index dbc7920efff..364492cda89 100644 --- a/storage/rocksdb/ha_rocksdb.h +++ b/storage/rocksdb/ha_rocksdb.h @@ -329,8 +329,8 @@ class ha_rocksdb : public my_core::handler { const bool use_all_keys, const uint eq_cond_len, bool use_locking_iterator) MY_ATTRIBUTE((__nonnull__)); - //psergey: - void set_range_lock(Rdb_transaction *tx, + + int set_range_lock(Rdb_transaction *tx, const Rdb_key_def &kd, const enum ha_rkey_function &find_flag, const rocksdb::Slice &slice,

1 0

[Commits] 5a8a7aefe77: MDEV-18335: Assertion `!error || error == 137' failed in subselect_rowid_merge_engine::init
by Varun 24 Aug '20

24 Aug '20

revision-id: 5a8a7aefe773f3a9a11e1574ba5b3c132302f8b3 (mariadb-10.3.21-216-g5a8a7aefe77) parent(s): c277bcd591821b9956bf0d0c0a71ceb3e230f060 author: Varun Gupta committer: Varun Gupta timestamp: 2020-08-24 17:51:18 +0530 message: MDEV-18335: Assertion `!error || error == 137' failed in subselect_rowid_merge_engine::init When duplicates are removed from a table using a hash, if the record is a duplicate it is marked as deleted. The handler API check if the record is deleted and send an error flag HA_ERR_RECORD_DELETED. When we scan over the table if the thread is not killed then we skip the records marked as HA_ERR_RECORD_DELETED. The issue here is when a query is aborted by a user (this is happening when the LIMIT for ROWS EXAMINED is exceeded), the scan over the table does not skip the records for which HA_ERR_RECORD_DELETED is sent. It just returns an error flag HA_ERR_ABORTED_BY_USER. This error flag is not checked at the upper level and hence we hit the assert. If the query is aborted by the user we should just skip reading rows and return control to the upper levels of execution. --- mysql-test/main/subselect4.result | 31 +++++++++++++++++++++++++++++++ mysql-test/main/subselect4.test | 36 ++++++++++++++++++++++++++++++++++++ sql/item_subselect.cc | 3 +++ 3 files changed, 70 insertions(+) diff --git a/mysql-test/main/subselect4.result b/mysql-test/main/subselect4.result index d1d3b514549..020a7064291 100644 --- a/mysql-test/main/subselect4.result +++ b/mysql-test/main/subselect4.result @@ -2686,4 +2686,35 @@ SELECT * FROM t2; f bar DROP TABLE t1, t2; +# +# MDEV-18335: Assertion `!error || error == 137' failed in subselect_rowid_merge_engine::init +# +CREATE TABLE t1 (i1 int,v1 varchar(1),KEY (v1,i1)); +INSERT INTO t1 VALUES +(9,'y'),(4,'q'),(0,null),(0,'p'),(null,'j'); +CREATE TABLE t2 (pk int); +INSERT INTO t2 VALUES (1),(2); +CREATE TABLE t3 (v2 varchar(1)); +INSERT INTO t3 VALUES +('p'),('j'),('y'),('q'); +CREATE TABLE t4 (v2 varchar(1)); +INSERT INTO t4 VALUES +('a'),('a'),('b'),('b'),('c'),('c'), +('d'),('d'),('e'),('e'),('f'),('f'), +('g'),('g'),('h'),('h'),('i'),('i'), +('j'),('j'),('k'),('k'),('l'),('l'), +('m'),('m'),('n'),('n'),('o'),('o'), +('p'),('p'),('q'),('q'),('r'),('r'), +('s'),('s'),('t'),('t'),('u'),('u'),('v'),('v'), +('w'),('w'),('x'),('x'), (NULL),(NULL); +SET @save_join_cache_level=@@join_cache_level; +SET join_cache_level=0; +select 1 +from t2 join t1 on +('i','w') not in (select t1.v1,t4.v2 from t4,t1,t3 where t3.v2 = t1.v1) LIMIT ROWS EXAMINED 500; +1 +Warnings: +Warning 1931 Query execution was interrupted. The query examined at least 3020 rows, which exceeds LIMIT ROWS EXAMINED (500). The query result may be incomplete +SET join_cache_level= @save_join_cache_level; +DROP TABLE t1,t2,t3,t4; # End of 10.2 tests diff --git a/mysql-test/main/subselect4.test b/mysql-test/main/subselect4.test index 6eada9b27d9..6f5eb1f2985 100644 --- a/mysql-test/main/subselect4.test +++ b/mysql-test/main/subselect4.test @@ -2201,4 +2201,40 @@ SELECT * FROM t2; DROP TABLE t1, t2; +--echo # +--echo # MDEV-18335: Assertion `!error || error == 137' failed in subselect_rowid_merge_engine::init +--echo # + +CREATE TABLE t1 (i1 int,v1 varchar(1),KEY (v1,i1)); +INSERT INTO t1 VALUES +(9,'y'),(4,'q'),(0,null),(0,'p'),(null,'j'); + +CREATE TABLE t2 (pk int); +INSERT INTO t2 VALUES (1),(2); + +CREATE TABLE t3 (v2 varchar(1)); +INSERT INTO t3 VALUES +('p'),('j'),('y'),('q'); + +CREATE TABLE t4 (v2 varchar(1)); +INSERT INTO t4 VALUES +('a'),('a'),('b'),('b'),('c'),('c'), +('d'),('d'),('e'),('e'),('f'),('f'), +('g'),('g'),('h'),('h'),('i'),('i'), +('j'),('j'),('k'),('k'),('l'),('l'), +('m'),('m'),('n'),('n'),('o'),('o'), +('p'),('p'),('q'),('q'),('r'),('r'), +('s'),('s'),('t'),('t'),('u'),('u'),('v'),('v'), +('w'),('w'),('x'),('x'), (NULL),(NULL); + +SET @save_join_cache_level=@@join_cache_level; +SET join_cache_level=0; + +select 1 +from t2 join t1 on +('i','w') not in (select t1.v1,t4.v2 from t4,t1,t3 where t3.v2 = t1.v1) LIMIT ROWS EXAMINED 500; +SET join_cache_level= @save_join_cache_level; + +DROP TABLE t1,t2,t3,t4; + --echo # End of 10.2 tests diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc index c32cd5ef84a..bc5111667ff 100644 --- a/sql/item_subselect.cc +++ b/sql/item_subselect.cc @@ -6281,6 +6281,9 @@ subselect_rowid_merge_engine::init(MY_BITMAP *non_null_key_parts, while (TRUE) { error= tmp_table->file->ha_rnd_next(tmp_table->record[0]); + + if (error == HA_ERR_ABORTED_BY_USER) + break; /* This is a temp table that we fully own, there should be no other cause to stop the iteration than EOF.

1 0

[Commits] 5dc2ebae474: MDEV-22330: mysqlbinlog stops with an error Don't know how to handle column type: 255 meta: 4 (0004)
by sujatha 17 Aug '20

17 Aug '20

revision-id: 5dc2ebae47481afe5b0769f0e0463d64b77143c0 (mariadb-10.1.43-260-g5dc2ebae474) parent(s): 101ce10d0ddda1e38806b8d2c491298482e7ab78 author: Sujatha committer: Sujatha timestamp: 2020-08-13 09:58:25 +0530 message: MDEV-22330: mysqlbinlog stops with an error Don't know how to handle column type: 255 meta: 4 (0004) Analysis: ======== "mysqlbinlog -v" option will reconstruct row events and display them as commented SQL statements. If this option is given twice, the output includes comments to indicate column data types and some metadata. `log_event_print_value` is the function responsible for printing values and their types. This function doesn't handle GEOMETRY type. Hence the above error gets printed. Fix: === Add support for GEOMETRY datatype. --- .../suite/binlog/r/binlog_mysqlbinlog_row.result | 56 +++++++++++++++++++++- .../suite/binlog/t/binlog_mysqlbinlog_row.test | 14 +++++- sql/log_event.cc | 17 +++++-- 3 files changed, 80 insertions(+), 7 deletions(-) diff --git a/mysql-test/suite/binlog/r/binlog_mysqlbinlog_row.result b/mysql-test/suite/binlog/r/binlog_mysqlbinlog_row.result index 9a3f4751165..0b05c4f449f 100644 --- a/mysql-test/suite/binlog/r/binlog_mysqlbinlog_row.result +++ b/mysql-test/suite/binlog/r/binlog_mysqlbinlog_row.result @@ -339,7 +339,11 @@ a 123.47 999.99 DROP TABLE t1dec102; -flush logs; +CREATE TABLE t1 (a GEOMETRY DEFAULT NULL); +INSERT INTO t1 VALUES (NULL); +INSERT INTO t1 VALUES (POINT(10,10)); +DROP TABLE t1; +FLUSH LOGS; /*!50530 SET @@SESSION.PSEUDO_SLAVE_MODE=1*/; /*!40019 SET @@session.max_insert_delayed_threads=0*/; /*!50003 SET @OLD_COMPLETION_TYPE=@@COMPLETION_TYPE,COMPLETION_TYPE=0*/; @@ -4623,6 +4627,56 @@ SET TIMESTAMP=1000000000/*!*/; DROP TABLE `t1dec102` /* generated by server */ /*!*/; # at # +#010909 4:46:40 server id 1 end_log_pos # GTID 0-1-321 ddl +/*!100001 SET @@session.gtid_seq_no=321*//*!*/; +# at # +#010909 4:46:40 server id 1 end_log_pos # Query thread_id=# exec_time=# error_code=0 +SET TIMESTAMP=1000000000/*!*/; +CREATE TABLE t1 (a GEOMETRY DEFAULT NULL) +/*!*/; +# at # +#010909 4:46:40 server id 1 end_log_pos # GTID 0-1-322 +/*!100001 SET @@session.gtid_seq_no=322*//*!*/; +BEGIN +/*!*/; +# at # +#010909 4:46:40 server id 1 end_log_pos # Table_map: `test`.`t1` mapped to number # +# at # +#010909 4:46:40 server id 1 end_log_pos # Write_rows: table id # flags: STMT_END_F +### INSERT INTO `test`.`t1` +### SET +### @1=NULL /* GEOMETRY meta=4 nullable=1 is_null=1 */ +# at # +#010909 4:46:40 server id 1 end_log_pos # Query thread_id=# exec_time=# error_code=0 +SET TIMESTAMP=1000000000/*!*/; +COMMIT +/*!*/; +# at # +#010909 4:46:40 server id 1 end_log_pos # GTID 0-1-323 +/*!100001 SET @@session.gtid_seq_no=323*//*!*/; +BEGIN +/*!*/; +# at # +#010909 4:46:40 server id 1 end_log_pos # Table_map: `test`.`t1` mapped to number # +# at # +#010909 4:46:40 server id 1 end_log_pos # Write_rows: table id # flags: STMT_END_F +### INSERT INTO `test`.`t1` +### SET +### @1='\x00\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00$@\x00\x00\x00\x00\x00\x00$@' /* GEOMETRY meta=4 nullable=1 is_null=0 */ +# at # +#010909 4:46:40 server id 1 end_log_pos # Query thread_id=# exec_time=# error_code=0 +SET TIMESTAMP=1000000000/*!*/; +COMMIT +/*!*/; +# at # +#010909 4:46:40 server id 1 end_log_pos # GTID 0-1-324 ddl +/*!100001 SET @@session.gtid_seq_no=324*//*!*/; +# at # +#010909 4:46:40 server id 1 end_log_pos # Query thread_id=# exec_time=# error_code=0 +SET TIMESTAMP=1000000000/*!*/; +DROP TABLE `t1` /* generated by server */ +/*!*/; +# at # #010909 4:46:40 server id 1 end_log_pos # Rotate to master-bin.000002 pos: 4 DELIMITER ; # End of log file diff --git a/mysql-test/suite/binlog/t/binlog_mysqlbinlog_row.test b/mysql-test/suite/binlog/t/binlog_mysqlbinlog_row.test index 0c94d968338..93c26cc57a2 100644 --- a/mysql-test/suite/binlog/t/binlog_mysqlbinlog_row.test +++ b/mysql-test/suite/binlog/t/binlog_mysqlbinlog_row.test @@ -450,7 +450,19 @@ INSERT INTO t1dec102 VALUES (999.99); SELECT * FROM t1dec102 ORDER BY a; DROP TABLE t1dec102; -flush logs; + +# +# MDEV-22330: mysqlbinlog stops with an error Don't know how to handle column +# type: 255 meta: 4 (0004) +# Check support for GEOMETRY type with verbose mode. +# +CREATE TABLE t1 (a GEOMETRY DEFAULT NULL); + +INSERT INTO t1 VALUES (NULL); +INSERT INTO t1 VALUES (POINT(10,10)); +DROP TABLE t1; + +FLUSH LOGS; --replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR --replace_regex /SQL_LOAD_MB-[0-9]-[0-9]/SQL_LOAD_MB-#-#/ /exec_time=[0-9]*/exec_time=#/ /end_log_pos [0-9]*/end_log_pos #/ /# at [0-9]*/# at #/ /thread_id=[0-9]*/thread_id=#/ /table id [0-9]*/table id #/ /mapped to number [0-9]*/mapped to number #/ /server v [^ ]*/server v #.##.##/ /((a)[0-9]*=[0-9]*[.][0-9]{1,3})[0-9e+-]*[^ ]*(.*(FLOAT|DOUBLE).*[*].)/\1...\2/ diff --git a/sql/log_event.cc b/sql/log_event.cc index a1a442df43f..e8121ba35ff 100644 --- a/sql/log_event.cc +++ b/sql/log_event.cc @@ -2575,13 +2575,20 @@ log_event_print_value(IO_CACHE *file, const uchar *ptr, "Not enough metadata to display the value. "); break; + case MYSQL_TYPE_GEOMETRY: + strmake(typestr, "GEOMETRY", typestr_length); + if (!ptr) + goto return_null; + + length= uint4korr(ptr); + my_b_write_quoted(file, ptr + meta, length); + return length + meta; + default: { - char tmp[5]; - my_snprintf(tmp, sizeof(tmp), "%04x", meta); - my_b_printf(file, - "!! Don't know how to handle column type=%d meta=%d (%s)", - type, meta, tmp); + fprintf(stderr, + "\nError: Don't know how to handle column type: %d meta: %d (%04x)\n", + type, meta, meta); } break; }

1 0

[Commits] ef985d3de92: MDEV-23449: alias do not exist and a query do not report an error
by Varun 12 Aug '20

12 Aug '20

revision-id: ef985d3de923334248a03732b042a18f18925e8a (mariadb-10.1.43-251-gef985d3de92) parent(s): ab578bdf453c3cb0e9ca561cf373f64c96b22fda author: Varun Gupta committer: Varun Gupta timestamp: 2020-08-12 02:19:17 +0530 message: MDEV-23449: alias do not exist and a query do not report an error For an IN/ANY/ALL subquery without an aggregate function and HAVING clause, the GROUP BY clause is removed. Due to the GROUP BY list being removed, the invalid reference in the GROUP BY clause was never resolved. Remove the GROUP BY list only when the all the items in the GROUP BY list are resolved. --- mysql-test/r/subselect4.result | 8 ++++++++ mysql-test/t/subselect4.test | 11 +++++++++++ sql/sql_select.cc | 32 ++++++++++++++++---------------- 3 files changed, 35 insertions(+), 16 deletions(-) diff --git a/mysql-test/r/subselect4.result b/mysql-test/r/subselect4.result index e7655131fcf..a4fd1123227 100644 --- a/mysql-test/r/subselect4.result +++ b/mysql-test/r/subselect4.result @@ -2648,4 +2648,12 @@ a 1 2 DROP TABLE t1,t2; +# +# MDEV-23449: alias do not exist and a query do not report an error +# +CREATE TABLE t1(a INT, b INT); +INSERT INTO t1 VALUES (1,1), (2,2), (3,3), (4,4), (5,5); +SELECT a, b FROM t1 WHERE a IN (SELECT A.a FROM t1 A GROUP BY s.id); +ERROR 42S22: Unknown column 's.id' in 'group statement' +DROP TABLE t1; # end of 10.1 tests diff --git a/mysql-test/t/subselect4.test b/mysql-test/t/subselect4.test index 8f1ad51ca50..03929517126 100644 --- a/mysql-test/t/subselect4.test +++ b/mysql-test/t/subselect4.test @@ -2162,4 +2162,15 @@ SELECT t1.a FROM t1 WHERE t1.a IN ( SELECT A.a FROM t1 A UNION ALL SELECT B.a FR DROP TABLE t1,t2; +--echo # +--echo # MDEV-23449: alias do not exist and a query do not report an error +--echo # + +CREATE TABLE t1(a INT, b INT); +INSERT INTO t1 VALUES (1,1), (2,2), (3,3), (4,4), (5,5); + +--error ER_BAD_FIELD_ERROR +SELECT a, b FROM t1 WHERE a IN (SELECT A.a FROM t1 A GROUP BY s.id); +DROP TABLE t1; + --echo # end of 10.1 tests diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 4c6e87e4f27..7a1a7baaa1c 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -731,22 +731,6 @@ JOIN::prepare(Item ***rref_pointer_array, tables_list, select_lex->leaf_tables, FALSE, SELECT_ACL, SELECT_ACL, FALSE)) DBUG_RETURN(-1); - - /* - Permanently remove redundant parts from the query if - 1) This is a subquery - 2) This is the first time this query is optimized (since the - transformation is permanent - 3) Not normalizing a view. Removal should take place when a - query involving a view is optimized, not when the view - is created - */ - if (select_lex->master_unit()->item && // 1) - select_lex->first_cond_optimization && // 2) - !thd->lex->is_view_context_analysis()) // 3) - { - remove_redundant_subquery_clauses(select_lex); - } /* TRUE if the SELECT list mixes elements with and without grouping, @@ -814,6 +798,22 @@ JOIN::prepare(Item ***rref_pointer_array, ref_pointer_array= *rref_pointer_array; + /* + Permanently remove redundant parts from the query if + 1) This is a subquery + 2) This is the first time this query is optimized (since the + transformation is permanent + 3) Not normalizing a view. Removal should take place when a + query involving a view is optimized, not when the view + is created + */ + if (select_lex->master_unit()->item && // 1) + select_lex->first_cond_optimization && // 2) + !thd->lex->is_view_context_analysis()) // 3) + { + remove_redundant_subquery_clauses(select_lex); + } + /* Resolve the ORDER BY that was skipped, then remove it. */ if (skip_order_by && select_lex != select_lex->master_unit()->global_parameters())

1 0

[Commits] 402758a0f: Apply coding style cleanup from make check-format
by Sergei Petrunia 10 Aug '20

10 Aug '20

revision-id: 402758a0f6899ec200860f13b5b606654cada766 (v5.8-2698-g402758a0f) parent(s): 24ca9e111197ba779a007e5b13d14ed4f4e54b4f author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2020-08-10 21:58:38 +0300 message: Apply coding style cleanup from make check-format --- include/rocksdb/utilities/transaction.h | 17 +- include/rocksdb/utilities/transaction_db.h | 9 +- utilities/transactions/lock/lock_tracker.h | 9 +- utilities/transactions/lock/point_lock_tracker.h | 5 +- utilities/transactions/lock/range_lock_tracker.cc | 14 +- utilities/transactions/lock/range_lock_tracker.h | 67 +++-- utilities/transactions/optimistic_transaction.cc | 4 +- utilities/transactions/pessimistic_transaction.cc | 23 +- utilities/transactions/pessimistic_transaction.h | 1 + .../transactions/pessimistic_transaction_db.cc | 35 ++- .../transactions/pessimistic_transaction_db.h | 19 +- utilities/transactions/range_locking_test.cc | 23 +- utilities/transactions/transaction_base.cc | 2 +- utilities/transactions/transaction_base.h | 24 +- utilities/transactions/transaction_lock_mgr.cc | 301 ++++++++++----------- utilities/transactions/transaction_lock_mgr.h | 87 +++--- .../transactions/transaction_lock_mgr_test.cc | 30 +- utilities/transactions/write_unprepared_txn.cc | 2 +- 18 files changed, 305 insertions(+), 367 deletions(-) diff --git a/include/rocksdb/utilities/transaction.h b/include/rocksdb/utilities/transaction.h index 1b0cfabcb..2f4804255 100644 --- a/include/rocksdb/utilities/transaction.h +++ b/include/rocksdb/utilities/transaction.h @@ -24,7 +24,6 @@ using TransactionName = std::string; using TransactionID = uint64_t; - /* class Endpoint allows to define prefix ranges. @@ -90,14 +89,14 @@ class Endpoint { */ bool inf_suffix; - Endpoint(const Slice &slice_arg, bool inf_suffix_arg=false) : - slice(slice_arg), inf_suffix(inf_suffix_arg) {} + Endpoint(const Slice& slice_arg, bool inf_suffix_arg = false) + : slice(slice_arg), inf_suffix(inf_suffix_arg) {} - Endpoint(const char* s, bool inf_suffix_arg=false) : - slice(s), inf_suffix(inf_suffix_arg) {} + Endpoint(const char* s, bool inf_suffix_arg = false) + : slice(s), inf_suffix(inf_suffix_arg) {} - Endpoint(const char* s, size_t size, bool inf_suffix_arg=false) : - slice(s, size), inf_suffix(inf_suffix_arg) {} + Endpoint(const char* s, size_t size, bool inf_suffix_arg = false) + : slice(s, size), inf_suffix(inf_suffix_arg) {} Endpoint() : inf_suffix(false) {} }; @@ -356,8 +355,8 @@ class Transaction { } // Get a range lock on [start_endpoint; end_endpoint]. - virtual Status GetRangeLock(ColumnFamilyHandle*, - const Endpoint&, const Endpoint&) { + virtual Status GetRangeLock(ColumnFamilyHandle*, const Endpoint&, + const Endpoint&) { return Status::NotSupported(); } diff --git a/include/rocksdb/utilities/transaction_db.h b/include/rocksdb/utilities/transaction_db.h index 34aed0df9..8d5db02ac 100644 --- a/include/rocksdb/utilities/transaction_db.h +++ b/include/rocksdb/utilities/transaction_db.h @@ -62,7 +62,7 @@ class RangeLockMgrHandle : public LockManagerHandle { }; virtual Counters GetStatus() = 0; - virtual ~RangeLockMgrHandle() {}; + virtual ~RangeLockMgrHandle(){}; }; // A factory function to create a Range Lock Manager. The created object should @@ -71,8 +71,7 @@ class RangeLockMgrHandle : public LockManagerHandle { // range-locking mode // 2. Used to control the lock manager when the DB is already open. RangeLockMgrHandle* NewRangeLockManager( - std::shared_ptr<TransactionDBMutexFactory> mutex_factory -); + std::shared_ptr<TransactionDBMutexFactory> mutex_factory); struct TransactionDBOptions { // Specifies the maximum number of keys that can be locked at the same time @@ -245,10 +244,10 @@ struct TransactionDBWriteOptimizations { struct KeyLockInfo { std::string key; - std::string key2; // Used when range locking is used + std::string key2; // Used when range locking is used std::vector<TransactionID> ids; bool exclusive; - bool has_key2 = false; // TRUE <=> key2 has a value + bool has_key2 = false; // TRUE <=> key2 has a value }; struct DeadlockInfo { diff --git a/utilities/transactions/lock/lock_tracker.h b/utilities/transactions/lock/lock_tracker.h index 924bfd98c..104b58f1b 100644 --- a/utilities/transactions/lock/lock_tracker.h +++ b/utilities/transactions/lock/lock_tracker.h @@ -10,7 +10,7 @@ #include "rocksdb/rocksdb_namespace.h" #include "rocksdb/status.h" #include "rocksdb/types.h" -#include "rocksdb/utilities/transaction_db.h" // for Endpoint +#include "rocksdb/utilities/transaction_db.h" // for Endpoint namespace ROCKSDB_NAMESPACE { @@ -199,17 +199,16 @@ class LockTracker { ColumnFamilyId /*column_family_id*/) const = 0; }; - -// An interface to LockTracker factory. LockTracker objects should only be +// An interface to LockTracker factory. LockTracker objects should only be // created through this interface's Create() method. -// +// // One can get the factory pointer e.g. from Lock Manager which overloads // BaseLockMgr::getLockTrackerFactory(). class LockTrackerFactory { public: // Caller owns the returned pointer. virtual LockTracker* Create() const = 0; - virtual ~LockTrackerFactory(){} + virtual ~LockTrackerFactory() {} }; } // namespace ROCKSDB_NAMESPACE diff --git a/utilities/transactions/lock/point_lock_tracker.h b/utilities/transactions/lock/point_lock_tracker.h index b38a83f26..faf6de121 100644 --- a/utilities/transactions/lock/point_lock_tracker.h +++ b/utilities/transactions/lock/point_lock_tracker.h @@ -81,9 +81,8 @@ class PointLockTracker : public LockTracker { TrackedKeys tracked_keys_; }; -class PointLockTrackerFactory : public LockTrackerFactory -{ -public: +class PointLockTrackerFactory : public LockTrackerFactory { + public: LockTracker* Create() const override { return new PointLockTracker; } static PointLockTrackerFactory instance; diff --git a/utilities/transactions/lock/range_lock_tracker.cc b/utilities/transactions/lock/range_lock_tracker.cc index 1bb23956b..001a28548 100644 --- a/utilities/transactions/lock/range_lock_tracker.cc +++ b/utilities/transactions/lock/range_lock_tracker.cc @@ -7,16 +7,14 @@ RangeLockTrackerFactory RangeLockTrackerFactory::instance; RangeLockList *RangeLockTracker::getOrCreateList() { RangeLockList *res; - if ((res = getList())) - return res; + if ((res = getList())) return res; // Doesn't exist, create range_list.reset(new RangeLockList()); return getList(); } - -void RangeLockTracker::Track(const PointLockRequest& lock_req) { +void RangeLockTracker::Track(const PointLockRequest &lock_req) { DBT key_dbt; std::string key; serialize_endpoint(Endpoint(lock_req.key, false), &key); @@ -25,12 +23,12 @@ void RangeLockTracker::Track(const PointLockRequest& lock_req) { rl->append(lock_req.column_family_id, &key_dbt, &key_dbt); } -void RangeLockTracker::Track(const RangeLockRequest& lock_req) { +void RangeLockTracker::Track(const RangeLockRequest &lock_req) { DBT start_dbt, end_dbt; std::string start_key, end_key; serialize_endpoint(lock_req.start_endp, &start_key); - serialize_endpoint(lock_req.end_endp, &end_key); + serialize_endpoint(lock_req.end_endp, &end_key); toku_fill_dbt(&start_dbt, start_key.data(), start_key.size()); toku_fill_dbt(&end_dbt, end_key.data(), end_key.size()); @@ -39,8 +37,8 @@ void RangeLockTracker::Track(const RangeLockRequest& lock_req) { rl->append(lock_req.column_family_id, &start_dbt, &end_dbt); } -PointLockStatus RangeLockTracker::GetPointLockStatus(ColumnFamilyId /*cf_id*/, - const std::string& /*key*/) const { +PointLockStatus RangeLockTracker::GetPointLockStatus( + ColumnFamilyId /*cf_id*/, const std::string & /*key*/) const { // TODO: what to do here if we are holding a range lock that is embedding the // point? diff --git a/utilities/transactions/lock/range_lock_tracker.h b/utilities/transactions/lock/range_lock_tracker.h index 70778e180..36787ce5e 100644 --- a/utilities/transactions/lock/range_lock_tracker.h +++ b/utilities/transactions/lock/range_lock_tracker.h @@ -9,18 +9,17 @@ #include <string> #include <unordered_map> -#include "utilities/transactions/lock/lock_tracker.h" #include "util/mutexlock.h" +#include "utilities/transactions/lock/lock_tracker.h" // Range Locking: -#include <locktree/locktree.h> #include <locktree/lock_request.h> +#include <locktree/locktree.h> namespace ROCKSDB_NAMESPACE { class RangeLockList; - /* Storage for locks that are currently held by a transaction. @@ -33,30 +32,30 @@ class RangeLockList; This property is currently harmless. */ class RangeLockList /*: public PessimisticTransaction::Lock--Storage */ { -public: - virtual ~RangeLockList() { - clear(); - } + public: + virtual ~RangeLockList() { clear(); } void clear() { - for(auto it : buffers_) { + for (auto it : buffers_) { it.second->destroy(); } buffers_.clear(); } - RangeLockList() : releasing_locks_(false) { - } + RangeLockList() : releasing_locks_(false) {} - void append(uint32_t cf_id, const DBT *left_key, const DBT *right_key) { + void append(uint32_t cf_id, const DBT* left_key, const DBT* right_key) { MutexLock l(&mutex_); // there's only one thread that calls this function. // the same thread will do lock release. assert(!releasing_locks_); - auto it= buffers_.find(cf_id); + auto it = buffers_.find(cf_id); if (it == buffers_.end()) { // create a new one - it= buffers_.emplace(cf_id, std::shared_ptr<toku::range_buffer>(new toku::range_buffer())).first; + it = buffers_ + .emplace(cf_id, std::shared_ptr<toku::range_buffer>( + new toku::range_buffer())) + .first; it->second->create(); } it->second->append(left_key, right_key); @@ -73,45 +72,43 @@ public: class RangeLockTracker : public LockTracker { public: - RangeLockTracker(): range_list(nullptr) {} + RangeLockTracker() : range_list(nullptr) {} RangeLockTracker(const RangeLockTracker&) = delete; RangeLockTracker& operator=(const RangeLockTracker&) = delete; - void Track(const PointLockRequest& ) override; - void Track(const RangeLockRequest& ) override ; - + void Track(const PointLockRequest&) override; + void Track(const RangeLockRequest&) override; + bool IsPointLockSupported() const override { return false; } bool IsRangeLockSupported() const override { return true; } // a Not-supported dummy implementation. - UntrackStatus Untrack( - const RangeLockRequest& /*lock_request*/) override { + UntrackStatus Untrack(const RangeLockRequest& /*lock_request*/) override { return UntrackStatus::NOT_TRACKED; } - UntrackStatus Untrack( - const PointLockRequest& /*lock_request*/) override { + UntrackStatus Untrack(const PointLockRequest& /*lock_request*/) override { return UntrackStatus::NOT_TRACKED; } // "If this method is not supported, leave it as a no-op." - void Merge(const LockTracker& ) override {} + void Merge(const LockTracker&) override {} // "If this method is not supported, leave it as a no-op." - void Subtract(const LockTracker& ) override {} - + void Subtract(const LockTracker&) override {} + void Clear() override; // "If this method is not supported, returns nullptr." virtual LockTracker* GetTrackedLocksSinceSavePoint( - const LockTracker& ) const override { + const LockTracker&) const override { return nullptr; } PointLockStatus GetPointLockStatus(ColumnFamilyId column_family_id, const std::string& key) const override; - + // The return value is only used for tests uint64_t GetNumPointLocks() const override { return 0; } @@ -119,23 +116,23 @@ class RangeLockTracker : public LockTracker { return nullptr; } - KeyIterator* GetKeyIterator(ColumnFamilyId /*column_family_id*/) const override { + KeyIterator* GetKeyIterator( + ColumnFamilyId /*column_family_id*/) const override { return nullptr; } - + // Non-override - RangeLockList *getList() { return range_list.get(); } - RangeLockList *getOrCreateList(); + RangeLockList* getList() { return range_list.get(); } + RangeLockList* getOrCreateList(); private: std::shared_ptr<RangeLockList> range_list; }; -class RangeLockTrackerFactory : public LockTrackerFactory -{ -public: - LockTracker* Create() const override { return new RangeLockTracker; } - +class RangeLockTrackerFactory : public LockTrackerFactory { + public: + LockTracker* Create() const override { return new RangeLockTracker; } + static RangeLockTrackerFactory instance; }; diff --git a/utilities/transactions/optimistic_transaction.cc b/utilities/transactions/optimistic_transaction.cc index 2c5191483..758373416 100644 --- a/utilities/transactions/optimistic_transaction.cc +++ b/utilities/transactions/optimistic_transaction.cc @@ -17,10 +17,10 @@ #include "rocksdb/utilities/optimistic_transaction_db.h" #include "util/cast_util.h" #include "util/string_util.h" -#include "utilities/transactions/transaction_util.h" +#include "utilities/transactions/lock/point_lock_tracker.h" #include "utilities/transactions/optimistic_transaction.h" #include "utilities/transactions/optimistic_transaction_db_impl.h" -#include "utilities/transactions/lock/point_lock_tracker.h" +#include "utilities/transactions/transaction_util.h" namespace ROCKSDB_NAMESPACE { diff --git a/utilities/transactions/pessimistic_transaction.cc b/utilities/transactions/pessimistic_transaction.cc index 865e4ba4d..3d676bfe9 100644 --- a/utilities/transactions/pessimistic_transaction.cc +++ b/utilities/transactions/pessimistic_transaction.cc @@ -38,9 +38,11 @@ TransactionID PessimisticTransaction::GenTxnID() { PessimisticTransaction::PessimisticTransaction( TransactionDB* txn_db, const WriteOptions& write_options, const TransactionOptions& txn_options, const bool init) - : TransactionBaseImpl(txn_db->GetRootDB(), write_options, - static_cast_with_check<PessimisticTransactionDB>(txn_db)-> - getLockMgr()->getLockTrackerFactory()), + : TransactionBaseImpl( + txn_db->GetRootDB(), write_options, + static_cast_with_check<PessimisticTransactionDB>(txn_db) + ->getLockMgr() + ->getLockTrackerFactory()), txn_db_impl_(nullptr), expiration_time_(0), txn_id_(0), @@ -94,7 +96,7 @@ void PessimisticTransaction::Initialize(const TransactionOptions& txn_options) { } PessimisticTransaction::~PessimisticTransaction() { - txn_db_impl_->UnLock(this, *tracked_locks_, /*all_keys_hint=*/true); + txn_db_impl_->UnLock(this, *tracked_locks_, /*all_keys_hint=*/true); if (expiration_time_ > 0) { txn_db_impl_->RemoveExpirableTransaction(txn_id_); } @@ -662,18 +664,17 @@ Status PessimisticTransaction::TryLock(ColumnFamilyHandle* column_family, return s; } -Status -PessimisticTransaction::GetRangeLock(ColumnFamilyHandle* column_family, - const Endpoint& start_endp, - const Endpoint& end_endp) { +Status PessimisticTransaction::GetRangeLock(ColumnFamilyHandle* column_family, + const Endpoint& start_endp, + const Endpoint& end_endp) { ColumnFamilyHandle* cfh = column_family ? column_family : db_impl_->DefaultColumnFamily(); - uint32_t cfh_id= GetColumnFamilyID(cfh); + uint32_t cfh_id = GetColumnFamilyID(cfh); - Status s= txn_db_impl_->TryRangeLock(this, cfh_id, start_endp, end_endp); + Status s = txn_db_impl_->TryRangeLock(this, cfh_id, start_endp, end_endp); if (s.ok()) { - RangeLockRequest req {cfh_id, start_endp, end_endp}; + RangeLockRequest req{cfh_id, start_endp, end_endp}; tracked_locks_->Track(req); } return s; diff --git a/utilities/transactions/pessimistic_transaction.h b/utilities/transactions/pessimistic_transaction.h index 824f7f074..aa52b0a0e 100644 --- a/utilities/transactions/pessimistic_transaction.h +++ b/utilities/transactions/pessimistic_transaction.h @@ -119,6 +119,7 @@ class PessimisticTransaction : public TransactionBaseImpl { virtual Status GetRangeLock(ColumnFamilyHandle* column_family, const Endpoint& start_key, const Endpoint& end_key); + protected: // Refer to // TransactionOptions::use_only_the_last_commit_time_batch_for_recovery diff --git a/utilities/transactions/pessimistic_transaction_db.cc b/utilities/transactions/pessimistic_transaction_db.cc index 9d779cb5f..e5ab97481 100644 --- a/utilities/transactions/pessimistic_transaction_db.cc +++ b/utilities/transactions/pessimistic_transaction_db.cc @@ -37,22 +37,21 @@ PessimisticTransactionDB::PessimisticTransactionDB( } 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_ = + std::dynamic_pointer_cast<BaseLockMgr>(txn_db_options_.lock_mgr_handle); range_lock_mgr_ = dynamic_cast<RangeLockMgr*>(lock_mgr_.get()); } else { // Use point lock manager by default std::shared_ptr<TransactionDBMutexFactory> mutex_factory = - txn_db_options_.custom_mutex_factory? - txn_db_options_.custom_mutex_factory : - std::shared_ptr<TransactionDBMutexFactory>( - new TransactionDBMutexFactoryImpl()); - auto lock_mgr = new TransactionLockMgr(this, txn_db_options_.num_stripes, - txn_db_options_.max_num_locks, - txn_db_options_.max_num_deadlocks, - mutex_factory); + txn_db_options_.custom_mutex_factory + ? txn_db_options_.custom_mutex_factory + : std::shared_ptr<TransactionDBMutexFactory>( + new TransactionDBMutexFactoryImpl()); + auto lock_mgr = new TransactionLockMgr( + this, txn_db_options_.num_stripes, txn_db_options_.max_num_locks, + txn_db_options_.max_num_deadlocks, mutex_factory); lock_mgr_.reset(lock_mgr); range_lock_mgr_ = nullptr; } @@ -414,16 +413,14 @@ Status PessimisticTransactionDB::TryLock(PessimisticTransaction* txn, return lock_mgr_->TryLock(txn, cfh_id, key, GetEnv(), exclusive); } -Status -PessimisticTransactionDB::TryRangeLock(PessimisticTransaction *txn, - uint32_t cfh_id, - const Endpoint& start_endp, - const Endpoint& end_endp) { +Status PessimisticTransactionDB::TryRangeLock(PessimisticTransaction* txn, + uint32_t cfh_id, + const Endpoint& start_endp, + const Endpoint& end_endp) { if (range_lock_mgr_) { - return range_lock_mgr_->TryRangeLock(txn, cfh_id, start_endp, - end_endp, /*exclusive=*/true); - } - else + return range_lock_mgr_->TryRangeLock(txn, cfh_id, start_endp, end_endp, + /*exclusive=*/true); + } else return Status::NotSupported(); } diff --git a/utilities/transactions/pessimistic_transaction_db.h b/utilities/transactions/pessimistic_transaction_db.h index 837862e52..099715440 100644 --- a/utilities/transactions/pessimistic_transaction_db.h +++ b/utilities/transactions/pessimistic_transaction_db.h @@ -98,13 +98,11 @@ class PessimisticTransactionDB : public TransactionDB { Status TryLock(PessimisticTransaction* txn, uint32_t cfh_id, const std::string& key, bool exclusive); - Status TryRangeLock(PessimisticTransaction* txn, - uint32_t cfh_id, - const Endpoint& start_endp, - const Endpoint& end_endp); + Status TryRangeLock(PessimisticTransaction* txn, uint32_t cfh_id, + const Endpoint& start_endp, const Endpoint& end_endp); void UnLock(PessimisticTransaction* txn, const LockTracker& keys, - bool all_keys_hint=false); + bool all_keys_hint = false); void UnLock(PessimisticTransaction* txn, uint32_t cfh_id, const std::string& key); @@ -147,7 +145,7 @@ class PessimisticTransactionDB : public TransactionDB { virtual void UpdateCFComparatorMap(const std::vector<ColumnFamilyHandle*>&) {} virtual void UpdateCFComparatorMap(ColumnFamilyHandle*) {} - BaseLockMgr *getLockMgr() const { return lock_mgr_.get(); } + BaseLockMgr* getLockMgr() const { return lock_mgr_.get(); } protected: DBImpl* db_impl_; @@ -174,14 +172,15 @@ class PessimisticTransactionDB : public TransactionDB { friend class WriteUnpreparedTransactionTest_RecoveryTest_Test; friend class WriteUnpreparedTransactionTest_MarkLogWithPrepSection_Test; - // Lock manager being used. This is either a TransactionLockMgr or a RangeLockMgr + // Lock manager being used. This is either a TransactionLockMgr or a + // RangeLockMgr std::shared_ptr<BaseLockMgr> lock_mgr_; // Non-null if we are using a lock manager that supports range locking. - RangeLockMgr *range_lock_mgr_ = nullptr; - + RangeLockMgr* range_lock_mgr_ = nullptr; + void init_lock_manager(); - + // Must be held when adding/dropping column families. InstrumentedMutex column_family_mutex_; Transaction* BeginInternalTransaction(const WriteOptions& options); diff --git a/utilities/transactions/range_locking_test.cc b/utilities/transactions/range_locking_test.cc index 8a930ec4d..443703cde 100644 --- a/utilities/transactions/range_locking_test.cc +++ b/utilities/transactions/range_locking_test.cc @@ -25,7 +25,6 @@ using std::string; namespace rocksdb { - class RangeLockingTest : public ::testing::Test { public: TransactionDB* db; @@ -35,8 +34,7 @@ class RangeLockingTest : public ::testing::Test { std::shared_ptr<RangeLockMgrHandle> range_lock_mgr; TransactionDBOptions txn_db_options; - RangeLockingTest() - : db(nullptr) { + RangeLockingTest() : db(nullptr) { options.create_if_missing = true; dbname = test::PerThreadDBPath("range_locking_testdb"); @@ -48,7 +46,6 @@ class RangeLockingTest : public ::testing::Test { s = TransactionDB::Open(options, txn_db_options, dbname, &db); assert(s.ok()); - } ~RangeLockingTest() { @@ -66,7 +63,6 @@ class RangeLockingTest : public ::testing::Test { Transaction* txn = db->BeginTransaction(WriteOptions(), txn_opt); return reinterpret_cast<PessimisticTransaction*>(txn); } - }; // TODO: set a smaller lock wait timeout so that the test runs faster. @@ -82,29 +78,28 @@ TEST_F(RangeLockingTest, BasicRangeLocking) { // Get a range lock { - auto s= txn0->GetRangeLock(cf, 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(cf, 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, cf, 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(cf, Slice("n"), Slice("value")); + auto s = txn0->Put(cf, Slice("n"), Slice("value")); ASSERT_EQ(s, Status::OK()); - auto s2= txn1->GetRangeLock(cf, Endpoint("m"), Endpoint("p")); + auto s2 = txn1->GetRangeLock(cf, Endpoint("m"), Endpoint("p")); ASSERT_TRUE(s2.IsTimedOut()); } @@ -119,7 +114,7 @@ TEST_F(RangeLockingTest, MyRocksLikeUpdate) { WriteOptions write_options; TransactionOptions txn_options; Transaction* txn0 = db->BeginTransaction(write_options, txn_options); - auto cf= db->DefaultColumnFamily(); + auto cf = db->DefaultColumnFamily(); Status s; // Get a range lock for the range we are about to update @@ -147,8 +142,8 @@ TEST_F(RangeLockingTest, MyRocksLikeUpdate) { TEST_F(RangeLockingTest, SnapshotValidation) { Status s; - Slice key_slice= Slice("k"); - ColumnFamilyHandle *cfh= db->DefaultColumnFamily(); + Slice key_slice = Slice("k"); + ColumnFamilyHandle* cfh = db->DefaultColumnFamily(); auto txn0 = NewTxn(); txn0->Put(key_slice, Slice("initial")); diff --git a/utilities/transactions/transaction_base.cc b/utilities/transactions/transaction_base.cc index 0053c0145..3303784f7 100644 --- a/utilities/transactions/transaction_base.cc +++ b/utilities/transactions/transaction_base.cc @@ -22,7 +22,7 @@ namespace ROCKSDB_NAMESPACE { TransactionBaseImpl::TransactionBaseImpl(DB* db, const WriteOptions& write_options, - const LockTrackerFactory *ltf) + const LockTrackerFactory* ltf) : db_(db), dbimpl_(static_cast_with_check<DBImpl>(db)), write_options_(write_options), diff --git a/utilities/transactions/transaction_base.h b/utilities/transactions/transaction_base.h index 4e3dd6dd4..3f5b48a5d 100644 --- a/utilities/transactions/transaction_base.h +++ b/utilities/transactions/transaction_base.h @@ -29,7 +29,7 @@ namespace ROCKSDB_NAMESPACE { class TransactionBaseImpl : public Transaction { public: TransactionBaseImpl(DB* db, const WriteOptions& write_options, - const LockTrackerFactory *ltf); + const LockTrackerFactory* ltf); virtual ~TransactionBaseImpl(); @@ -281,7 +281,7 @@ class TransactionBaseImpl : public Transaction { const Comparator* cmp_; - const LockTrackerFactory *ltf_; + const LockTrackerFactory* ltf_; // Stores that time the txn was constructed, in microseconds. uint64_t start_time_; @@ -308,7 +308,7 @@ class TransactionBaseImpl : public Transaction { SavePoint(std::shared_ptr<const Snapshot> snapshot, bool snapshot_needed, std::shared_ptr<TransactionNotifier> snapshot_notifier, uint64_t num_puts, uint64_t num_deletes, uint64_t num_merges, - const LockTrackerFactory *ltf) + const LockTrackerFactory* ltf) : snapshot_(snapshot), snapshot_needed_(snapshot_needed), snapshot_notifier_(snapshot_notifier), @@ -317,28 +317,28 @@ class TransactionBaseImpl : public Transaction { num_merges_(num_merges), new_locks_(ltf->Create()) {} - SavePoint(const LockTrackerFactory *ltf) : new_locks_(ltf->Create()) {} + SavePoint(const LockTrackerFactory* ltf) : new_locks_(ltf->Create()) {} - SavePoint(const SavePoint &s) {new_locks_ = s.new_locks_;} + SavePoint(const SavePoint& s) { new_locks_ = s.new_locks_; } }; // Records writes pending in this transaction WriteBatchWithIndex write_batch_; -public: + public: // For Pessimistic Transactions this is the set of acquired locks. // Optimistic Transactions will keep note the requested locks (not actually // locked), and do conflict checking until commit time based on the tracked // lock requests. -/* - psergey-merge: it's public because there are these users: - - RangeLockMgr::UnLockAll (probably solvable) - - RangeLockMgr::on_escalate -- HARDER! -*/ + /* + psergey-merge: it's public because there are these users: + - RangeLockMgr::UnLockAll (probably solvable) + - RangeLockMgr::on_escalate -- HARDER! + */ std::unique_ptr<LockTracker> tracked_locks_; -protected: + protected: // Stack of the Snapshot saved at each save point. Saved snapshots may be // nullptr if there was no snapshot at the time SetSavePoint() was called. std::unique_ptr<std::stack<TransactionBaseImpl::SavePoint, diff --git a/utilities/transactions/transaction_lock_mgr.cc b/utilities/transactions/transaction_lock_mgr.cc index 340bbdb5d..75e683b25 100644 --- a/utilities/transactions/transaction_lock_mgr.cc +++ b/utilities/transactions/transaction_lock_mgr.cc @@ -18,9 +18,9 @@ #include "util/cast_util.h" #include "util/hash.h" #include "util/thread_local.h" +#include "utilities/transactions/lock/range_lock_tracker.h" #include "utilities/transactions/pessimistic_transaction_db.h" #include "utilities/transactions/transaction_db_mutex_impl.h" -#include "utilities/transactions/lock/range_lock_tracker.h" namespace ROCKSDB_NAMESPACE { @@ -178,8 +178,8 @@ size_t LockMap::GetStripe(const std::string& key) const { return fastrange64(GetSliceNPHash64(key), num_stripes_); } -void TransactionLockMgr::AddColumnFamily(const ColumnFamilyHandle *cfh) { - uint32_t column_family_id= cfh->GetID(); +void TransactionLockMgr::AddColumnFamily(const ColumnFamilyHandle* cfh) { + uint32_t column_family_id = cfh->GetID(); InstrumentedMutexLock l(&lock_map_mutex_); if (lock_maps_.find(column_family_id) == lock_maps_.end()) { @@ -191,8 +191,8 @@ void TransactionLockMgr::AddColumnFamily(const ColumnFamilyHandle *cfh) { } } -void TransactionLockMgr::RemoveColumnFamily(const ColumnFamilyHandle *cfh) { - uint32_t column_family_id= cfh->GetID(); +void TransactionLockMgr::RemoveColumnFamily(const ColumnFamilyHandle* cfh) { + uint32_t column_family_id = cfh->GetID(); // Remove lock_map for this column family. Since the lock map is stored // as a shared ptr, concurrent transactions can still keep using it // until they release their references to it. @@ -742,13 +742,12 @@ void TransactionLockMgr::Resize(uint32_t target_size) { dlock_buffer_.Resize(target_size); } - ///////////////////////////////////////////////////////////////////////////// // RangeLockMgr - a lock manager that supports range locking ///////////////////////////////////////////////////////////////////////////// RangeLockMgrHandle* NewRangeLockManager( - std::shared_ptr<TransactionDBMutexFactory> mutex_factory) { + std::shared_ptr<TransactionDBMutexFactory> mutex_factory) { std::shared_ptr<TransactionDBMutexFactory> use_factory; if (mutex_factory) @@ -759,22 +758,19 @@ RangeLockMgrHandle* NewRangeLockManager( return new RangeLockMgr(use_factory); } +static const char SUFFIX_INFIMUM = 0x0; +static const char SUFFIX_SUPREMUM = 0x1; -static const char SUFFIX_INFIMUM= 0x0; -static const char SUFFIX_SUPREMUM= 0x1; - -void serialize_endpoint(const Endpoint &endp, std::string *buf) { +void serialize_endpoint(const Endpoint& endp, std::string* buf) { buf->push_back(endp.inf_suffix ? SUFFIX_SUPREMUM : SUFFIX_INFIMUM); buf->append(endp.slice.data(), endp.slice.size()); } - // Get a range lock on [start_key; end_key] range Status RangeLockMgr::TryRangeLock(PessimisticTransaction* txn, uint32_t column_family_id, - const Endpoint &start_endp, - const Endpoint &end_endp, - bool exclusive) { + const Endpoint& start_endp, + const Endpoint& end_endp, bool exclusive) { toku::lock_request request; request.create(mutex_factory_); DBT start_key_dbt, end_key_dbt; @@ -794,24 +790,22 @@ Status RangeLockMgr::TryRangeLock(PessimisticTransaction* txn, // locktree::kill_waiter call. Do we need this anymore? TransactionID wait_txn_id = txn->GetID(); - auto lt= get_locktree_by_cfid(column_family_id); + auto lt = get_locktree_by_cfid(column_family_id); request.set(lt, (TXNID)txn, &start_key_dbt, &end_key_dbt, - exclusive? toku::lock_request::WRITE: toku::lock_request::READ, - false /* not a big txn */, - (void*)wait_txn_id); - - uint64_t killed_time_msec = 0; // TODO: what should this have? + exclusive ? toku::lock_request::WRITE : toku::lock_request::READ, + false /* not a big txn */, (void*)wait_txn_id); + + uint64_t killed_time_msec = 0; // TODO: what should this have? uint64_t wait_time_msec = txn->GetLockTimeout(); // convert microseconds to milliseconds if (wait_time_msec != (uint64_t)-1) wait_time_msec = (wait_time_msec + 500) / 1000; std::vector<DeadlockInfo> di_path; - request.m_deadlock_cb = [&] (TXNID txnid, bool is_exclusive, - std::string key) { - di_path.push_back({((PessimisticTransaction*)txnid)->GetID(), - column_family_id, is_exclusive, key}); + request.m_deadlock_cb = [&](TXNID txnid, bool is_exclusive, std::string key) { + di_path.push_back({((PessimisticTransaction*)txnid)->GetID(), + column_family_id, is_exclusive, key}); }; request.start(); @@ -827,80 +821,77 @@ Status RangeLockMgr::TryRangeLock(PessimisticTransaction* txn, struct st_wait_info { PessimisticTransaction* txn; uint32_t column_family_id; - std::string *key_ptr; + std::string* key_ptr; autovector<TransactionID> wait_ids; - bool done= false; + bool done = false; - static void lock_wait_callback(void *cdata, TXNID /*waiter*/, TXNID waitee) { + static void lock_wait_callback(void* cdata, TXNID /*waiter*/, + TXNID waitee) { TEST_SYNC_POINT("RangeLockMgr::TryRangeLock:WaitingTxn"); - auto self= (struct st_wait_info*)cdata; + auto self = (struct st_wait_info*)cdata; // we know that the waiter is self->txn->GetID() (TODO: assert?) - if (!self->done) - { + if (!self->done) { self->wait_ids.push_back(waitee); self->txn->SetWaitingTxn(self->wait_ids, self->column_family_id, self->key_ptr); - self->done= true; + self->done = true; } } } wait_info; - wait_info.txn= txn; - wait_info.column_family_id= column_family_id; - wait_info.key_ptr= &key_str; - wait_info.done= false; + wait_info.txn = txn; + wait_info.column_family_id = column_family_id; + wait_info.key_ptr = &key_str; + wait_info.done = false; - const int r = request.wait(wait_time_msec, killed_time_msec, - nullptr, // killed_callback - st_wait_info::lock_wait_callback, - (void*)&wait_info); + const int r = + request.wait(wait_time_msec, killed_time_msec, + nullptr, // killed_callback + st_wait_info::lock_wait_callback, (void*)&wait_info); // Inform the txn that we are no longer waiting: txn->ClearWaitingTxn(); request.destroy(); switch (r) { - case 0: - break; /* fall through */ - case DB_LOCK_NOTGRANTED: - return Status::TimedOut(Status::SubCode::kLockTimeout); - case TOKUDB_OUT_OF_LOCKS: - return Status::Busy(Status::SubCode::kLockLimit); - case DB_LOCK_DEADLOCK: - { - std::reverse(di_path.begin(), di_path.end()); - dlock_buffer_.AddNewPath(DeadlockPath(di_path, request.get_start_time())); - return Status::Busy(Status::SubCode::kDeadlock); - } - default: + case 0: + break; /* fall through */ + case DB_LOCK_NOTGRANTED: + return Status::TimedOut(Status::SubCode::kLockTimeout); + case TOKUDB_OUT_OF_LOCKS: + return Status::Busy(Status::SubCode::kLockLimit); + case DB_LOCK_DEADLOCK: { + std::reverse(di_path.begin(), di_path.end()); + dlock_buffer_.AddNewPath(DeadlockPath(di_path, request.get_start_time())); + return Status::Busy(Status::SubCode::kDeadlock); + } + default: assert(0); return Status::Busy(Status::SubCode::kLockLimit); } // Don't: save the acquired lock in txn->owned_locks. // It is now responsibility of RangeLockMgr - // RangeLockList* range_list= ((RangeLockTracker*)txn->tracked_locks_.get())->getOrCreateList(); + // RangeLockList* range_list= + // ((RangeLockTracker*)txn->tracked_locks_.get())->getOrCreateList(); // range_list->append(column_family_id, &start_key_dbt, &end_key_dbt); return Status::OK(); } - // Get a singlepoint lock // (currently it is the same as getting a range lock) Status RangeLockMgr::TryLock(PessimisticTransaction* txn, - uint32_t column_family_id, - const std::string& key, Env*, - bool exclusive) { + uint32_t column_family_id, const std::string& key, + Env*, bool exclusive) { Endpoint endp(key.data(), key.size(), false); return TryRangeLock(txn, column_family_id, endp, endp, exclusive); } -static void -range_lock_mgr_release_lock_int(toku::locktree *lt, - const PessimisticTransaction* txn, - uint32_t /*column_family_id*/, - const std::string& key) { +static void range_lock_mgr_release_lock_int(toku::locktree* lt, + const PessimisticTransaction* txn, + uint32_t /*column_family_id*/, + const std::string& key) { DBT key_dbt; Endpoint endp(key.data(), key.size(), false); std::string endp_image; @@ -915,11 +906,12 @@ range_lock_mgr_release_lock_int(toku::locktree *lt, } void RangeLockMgr::UnLock(PessimisticTransaction* txn, - uint32_t column_family_id, - const std::string& key, Env*) { - auto lt= get_locktree_by_cfid(column_family_id); + uint32_t column_family_id, const std::string& key, + Env*) { + auto lt = get_locktree_by_cfid(column_family_id); range_lock_mgr_release_lock_int(lt, txn, column_family_id, key); - toku::lock_request::retry_all_lock_requests(lt, nullptr /* lock_wait_needed_callback */); + toku::lock_request::retry_all_lock_requests( + lt, nullptr /* lock_wait_needed_callback */); } void RangeLockMgr::UnLock(const PessimisticTransaction* /*txn*/, @@ -943,14 +935,12 @@ void RangeLockMgr::UnLock(const PessimisticTransaction* /*txn*/, } void RangeLockMgr::UnLockAll(const PessimisticTransaction* txn, Env*) { - // tracked_locks_->range_list may hold nullptr if the transaction has never // acquired any locks. - RangeLockList* range_list= - ((RangeLockTracker*)txn->tracked_locks_.get())->getList(); + RangeLockList* range_list = + ((RangeLockTracker*)txn->tracked_locks_.get())->getList(); - if (range_list) - { + if (range_list) { { MutexLock l(&range_list->mutex_); /* @@ -979,16 +969,16 @@ void RangeLockMgr::UnLockAll(const PessimisticTransaction* txn, Env*) { (the code in this function doesnt do that as there's only one thread that releases transaction's locks) */ - range_list->releasing_locks_= true; + range_list->releasing_locks_ = true; } // Don't try to call release_locks() if the buffer is empty! if we are - // not holding any locks, the lock tree might be in the STO-mode with - // another transaction, and our attempt to release an empty set of locks + // not holding any locks, the lock tree might be in the STO-mode with + // another transaction, and our attempt to release an empty set of locks // will cause an assertion failure. - for (auto it: range_list->buffers_) { + for (auto it : range_list->buffers_) { if (it.second->get_num_ranges()) { - toku::locktree *lt = get_locktree_by_cfid(it.first); + toku::locktree* lt = get_locktree_by_cfid(it.first); lt->release_locks((TXNID)txn, it.second.get(), true); it.second->destroy(); @@ -998,52 +988,42 @@ void RangeLockMgr::UnLockAll(const PessimisticTransaction* txn, Env*) { } } range_list->clear(); - range_list->releasing_locks_= false; + range_list->releasing_locks_ = false; } } +int RangeLockMgr::compare_dbt_endpoints(__toku_db*, void* arg, const DBT* a_key, + const DBT* b_key) { + const char* a = (const char*)a_key->data; + const char* b = (const char*)b_key->data; -int RangeLockMgr::compare_dbt_endpoints(__toku_db*, void *arg, - const DBT *a_key, - const DBT *b_key) { - const char *a= (const char*)a_key->data; - const char *b= (const char*)b_key->data; - - size_t a_len= a_key->size; - size_t b_len= b_key->size; + size_t a_len = a_key->size; + size_t b_len = b_key->size; - size_t min_len= std::min(a_len, b_len); + size_t min_len = std::min(a_len, b_len); // Compare the values. The first byte encodes the endpoint type, its value // is either SUFFIX_INFIMUM or SUFFIX_SUPREMUM. - Comparator *cmp = (Comparator*) arg; - int res= cmp->Compare(Slice(a+1, min_len-1), Slice(b+1, min_len-1)); - if (!res) - { - if (b_len > min_len) - { + Comparator* cmp = (Comparator*)arg; + int res = cmp->Compare(Slice(a + 1, min_len - 1), Slice(b + 1, min_len - 1)); + if (!res) { + if (b_len > min_len) { // a is shorter; if (a[0] == SUFFIX_INFIMUM) - return -1; //"a is smaller" - else - { + return -1; //"a is smaller" + else { // a is considered padded with 0xFF:FF:FF:FF... - return 1; // "a" is bigger + return 1; // "a" is bigger } - } - else if (a_len > min_len) - { + } else if (a_len > min_len) { // the opposite of the above: b is shorter. if (b[0] == SUFFIX_INFIMUM) - return 1; //"b is smaller" - else - { + return 1; //"b is smaller" + else { // b is considered padded with 0xFF:FF:FF:FF... - return -1; // "b" is bigger + return -1; // "b" is bigger } - } - else - { + } else { // the lengths are equal (and the key values, too) if (a[0] < b[0]) return -1; @@ -1052,15 +1032,15 @@ int RangeLockMgr::compare_dbt_endpoints(__toku_db*, void *arg, else return 0; } - } - else + } else return res; } -RangeLockMgr::RangeLockMgr(std::shared_ptr<TransactionDBMutexFactory> mutex_factory) : - mutex_factory_(mutex_factory), - ltree_lookup_cache_(new ThreadLocalPtr(nullptr)), - dlock_buffer_(10) { +RangeLockMgr::RangeLockMgr( + std::shared_ptr<TransactionDBMutexFactory> mutex_factory) + : mutex_factory_(mutex_factory), + ltree_lookup_cache_(new ThreadLocalPtr(nullptr)), + dlock_buffer_(10) { ltm_.create(on_create, on_destroy, on_escalate, NULL, mutex_factory_); } @@ -1076,18 +1056,19 @@ std::vector<DeadlockPath> RangeLockMgr::GetDeadlockInfoBuffer() { @brief Lock Escalation Callback function @param txnid Transaction whose locks got escalated - @param lt Lock Tree where escalation is happening (currently there is only one) + @param lt Lock Tree where escalation is happening (currently there is + only one) @param buffer Escalation result: list of locks that this transaction now owns in this lock tree. @param void* Callback context */ void RangeLockMgr::on_escalate(TXNID txnid, const locktree* lt, - const range_buffer &buffer, void *) { - auto txn= (PessimisticTransaction*)txnid; + const range_buffer& buffer, void*) { + auto txn = (PessimisticTransaction*)txnid; RangeLockList* trx_locks = - ((RangeLockTracker*)txn->tracked_locks_.get())->getList(); + ((RangeLockTracker*)txn->tracked_locks_.get())->getList(); MutexLock l(&trx_locks->mutex_); if (trx_locks->releasing_locks_) { @@ -1099,11 +1080,12 @@ void RangeLockMgr::on_escalate(TXNID txnid, const locktree* lt, return; } - // TODO: are we tracking this mem: lt->get_manager()->note_mem_released(trx_locks->ranges.buffer->total_memory_size()); + // TODO: are we tracking this mem: + // lt->get_manager()->note_mem_released(trx_locks->ranges.buffer->total_memory_size()); uint32_t cf_id = (uint32_t)lt->get_dict_id().dictid; - auto it= trx_locks->buffers_.find(cf_id); + auto it = trx_locks->buffers_.find(cf_id); it->second->destroy(); it->second->create(); @@ -1113,13 +1095,13 @@ void RangeLockMgr::on_escalate(TXNID txnid, const locktree* lt, it->second->append(rec.get_left_key(), rec.get_right_key()); iter.next(); } - // TODO: same as above: lt->get_manager()->note_mem_used(ranges.buffer->total_memory_size()); + // TODO: same as above: + // lt->get_manager()->note_mem_used(ranges.buffer->total_memory_size()); } - RangeLockMgr::~RangeLockMgr() { - //TODO: at this point, synchronization is not needed, right? - for (auto it: ltree_map_) { + // TODO: at this point, synchronization is not needed, right? + for (auto it : ltree_map_) { ltm_.release_lt(it.second); } ltm_.destroy(); @@ -1129,44 +1111,44 @@ RangeLockMgrHandle::Counters RangeLockMgr::GetStatus() { LTM_STATUS_S ltm_status_test; ltm_.get_status(&ltm_status_test); Counters res; - + // Searching status variable by its string name is how Toku's unit tests // do it (why didn't they make LTM_ESCALATION_COUNT constant visible?) // lookup keyname in status for (int i = 0; i < LTM_STATUS_S::LTM_STATUS_NUM_ROWS; i++) { - TOKU_ENGINE_STATUS_ROW status = &ltm_status_test.status[i]; - if (strcmp(status->keyname, "LTM_ESCALATION_COUNT") == 0) { - res.escalation_count = status->value.num; - continue; - } - if (strcmp(status->keyname, "LTM_SIZE_CURRENT") == 0) { - res.current_lock_memory = status->value.num; - } + TOKU_ENGINE_STATUS_ROW status = &ltm_status_test.status[i]; + if (strcmp(status->keyname, "LTM_ESCALATION_COUNT") == 0) { + res.escalation_count = status->value.num; + continue; + } + if (strcmp(status->keyname, "LTM_SIZE_CURRENT") == 0) { + res.current_lock_memory = status->value.num; + } } return res; } -void RangeLockMgr::AddColumnFamily(const ColumnFamilyHandle *cfh) { - uint32_t column_family_id= cfh->GetID(); +void RangeLockMgr::AddColumnFamily(const ColumnFamilyHandle* cfh) { + uint32_t column_family_id = cfh->GetID(); InstrumentedMutexLock l(&ltree_map_mutex_); if (ltree_map_.find(column_family_id) == ltree_map_.end()) { - DICTIONARY_ID dict_id = { .dictid = column_family_id }; + DICTIONARY_ID dict_id = {.dictid = column_family_id}; toku::comparator cmp; cmp.create(compare_dbt_endpoints, (void*)cfh->GetComparator(), NULL); - toku::locktree *ltree= ltm_.get_lt(dict_id, cmp, - /* on_create_extra*/nullptr); + toku::locktree* ltree = ltm_.get_lt(dict_id, cmp, + /* on_create_extra*/ nullptr); // This is ok to because get_lt has copied the comparator: cmp.destroy(); ltree_map_.emplace(column_family_id, ltree); } else { // column_family already exists in lock map - //assert(false); + // assert(false); } } -void RangeLockMgr::RemoveColumnFamily(const ColumnFamilyHandle *cfh) { - uint32_t column_family_id= cfh->GetID(); +void RangeLockMgr::RemoveColumnFamily(const ColumnFamilyHandle* cfh) { + uint32_t column_family_id = cfh->GetID(); // Remove lock_map for this column family. Since the lock map is stored // as a shared ptr, concurrent transactions can still keep using it // until they release their references to it. @@ -1185,7 +1167,7 @@ void RangeLockMgr::RemoveColumnFamily(const ColumnFamilyHandle *cfh) { ltree_map_.erase(lock_maps_iter); } // lock_map_mutex_ - //TODO: why do we delete first and clear the caches second? Shouldn't this be + // TODO: why do we delete first and clear the caches second? Shouldn't this be // done in the reverse order? (if we do it in the reverse order, how will we // prevent somebody from re-populating the cache?) @@ -1195,8 +1177,7 @@ void RangeLockMgr::RemoveColumnFamily(const ColumnFamilyHandle *cfh) { ltree_lookup_cache_->Scrape(&local_caches, nullptr); } -toku::locktree *RangeLockMgr::get_locktree_by_cfid(uint32_t column_family_id) { - +toku::locktree* RangeLockMgr::get_locktree_by_cfid(uint32_t column_family_id) { // First check thread-local cache if (ltree_lookup_cache_->Get() == nullptr) { ltree_lookup_cache_->Reset(new LockTreeMap()); @@ -1218,7 +1199,7 @@ toku::locktree *RangeLockMgr::get_locktree_by_cfid(uint32_t column_family_id) { return nullptr; } else { // Found lock map. Store in thread-local cache and return. - //std::shared_ptr<LockMap>& lock_map = lock_map_iter->second; + // std::shared_ptr<LockMap>& lock_map = lock_map_iter->second; ltree_map_cache->insert({column_family_id, it->second}); return it->second; } @@ -1227,48 +1208,44 @@ toku::locktree *RangeLockMgr::get_locktree_by_cfid(uint32_t column_family_id) { } struct LOCK_PRINT_CONTEXT { - BaseLockMgr::LockStatusData *data; // Save locks here - uint32_t cfh_id; // Column Family whose tree we are traversing + BaseLockMgr::LockStatusData* data; // Save locks here + uint32_t cfh_id; // Column Family whose tree we are traversing }; -static -void push_into_lock_status_data(void* param, - const DBT *left, const DBT *right, - TXNID txnid_arg, bool is_shared, - TxnidVector *owners) { - struct LOCK_PRINT_CONTEXT *ctx= (LOCK_PRINT_CONTEXT*)param; +static void push_into_lock_status_data(void* param, const DBT* left, + const DBT* right, TXNID txnid_arg, + bool is_shared, TxnidVector* owners) { + struct LOCK_PRINT_CONTEXT* ctx = (LOCK_PRINT_CONTEXT*)param; struct KeyLockInfo info; info.key.append((const char*)left->data, (size_t)left->size); - info.exclusive= !is_shared; + info.exclusive = !is_shared; - if (!(left->size == right->size && - !memcmp(left->data, right->data, left->size))) - { - // not a single-point lock - info.has_key2= true; + if (!(left->size == right->size && + !memcmp(left->data, right->data, left->size))) { + // not a single-point lock + info.has_key2 = true; info.key2.append((const char*)right->data, right->size); } if (txnid_arg != TXNID_SHARED) { - TXNID txnid= ((PessimisticTransaction*)txnid_arg)->GetID(); + TXNID txnid = ((PessimisticTransaction*)txnid_arg)->GetID(); info.ids.push_back(txnid); } else { for (auto it : *owners) { - TXNID real_id= ((PessimisticTransaction*)it)->GetID(); + TXNID real_id = ((PessimisticTransaction*)it)->GetID(); info.ids.push_back(real_id); } } ctx->data->insert({ctx->cfh_id, info}); } - BaseLockMgr::LockStatusData RangeLockMgr::GetLockStatusData() { LockStatusData data; { InstrumentedMutexLock l(&ltree_map_mutex_); for (auto it : ltree_map_) { - LOCK_PRINT_CONTEXT ctx = {&data, it.first }; + LOCK_PRINT_CONTEXT ctx = {&data, it.first}; it.second->dump_locks((void*)&ctx, push_into_lock_status_data); } } diff --git a/utilities/transactions/transaction_lock_mgr.h b/utilities/transactions/transaction_lock_mgr.h index 1b754ce8e..45fe228a3 100644 --- a/utilities/transactions/transaction_lock_mgr.h +++ b/utilities/transactions/transaction_lock_mgr.h @@ -17,14 +17,14 @@ #include "util/autovector.h" #include "util/hash_map.h" #include "util/thread_local.h" -#include "utilities/transactions/pessimistic_transaction.h" #include "utilities/transactions/lock/lock_tracker.h" #include "utilities/transactions/lock/point_lock_tracker.h" #include "utilities/transactions/lock/range_lock_tracker.h" +#include "utilities/transactions/pessimistic_transaction.h" // Range Locking: -#include <locktree/locktree.h> #include <locktree/lock_request.h> +#include <locktree/locktree.h> namespace ROCKSDB_NAMESPACE { @@ -63,34 +63,30 @@ class PessimisticTransactionDB; // class BaseLockMgr { public: - virtual LockTrackerFactory *getLockTrackerFactory() = 0; + virtual LockTrackerFactory* getLockTrackerFactory() = 0; - virtual void AddColumnFamily(const ColumnFamilyHandle *cfh) = 0; - virtual void RemoveColumnFamily(const ColumnFamilyHandle *cfh) = 0; + virtual void AddColumnFamily(const ColumnFamilyHandle* cfh) = 0; + virtual void RemoveColumnFamily(const ColumnFamilyHandle* cfh) = 0; - virtual - Status TryLock(PessimisticTransaction* txn, uint32_t column_family_id, - const std::string& key, Env* env, bool exclusive) = 0; - virtual - void UnLock(const PessimisticTransaction* txn, const LockTracker& tracker, - Env* env) = 0; - virtual - void UnLock(PessimisticTransaction* txn, uint32_t column_family_id, - const std::string& key, Env* env) = 0; + virtual Status TryLock(PessimisticTransaction* txn, uint32_t column_family_id, + const std::string& key, Env* env, bool exclusive) = 0; + virtual void UnLock(const PessimisticTransaction* txn, + const LockTracker& tracker, Env* env) = 0; + virtual void UnLock(PessimisticTransaction* txn, uint32_t column_family_id, + const std::string& key, Env* env) = 0; // Resize the deadlock info buffer - virtual void Resize(uint32_t)=0; - virtual std::vector<DeadlockPath> GetDeadlockInfoBuffer()= 0; + virtual void Resize(uint32_t) = 0; + virtual std::vector<DeadlockPath> GetDeadlockInfoBuffer() = 0; // TransactionDB will call this at start - virtual void init(TransactionDB*) {}; - virtual ~BaseLockMgr(){} + virtual void init(TransactionDB*){}; + virtual ~BaseLockMgr() {} using LockStatusData = std::unordered_multimap<uint32_t, KeyLockInfo>; - virtual LockStatusData GetLockStatusData()=0; + virtual LockStatusData GetLockStatusData() = 0; }; - // Point lock manager class TransactionLockMgr : public BaseLockMgr { public: @@ -109,11 +105,11 @@ class TransactionLockMgr : public BaseLockMgr { // Creates a new LockMap for this column family. Caller should guarantee // that this column family does not already exist. - void AddColumnFamily(const ColumnFamilyHandle *cfh); + void AddColumnFamily(const ColumnFamilyHandle* cfh); // Deletes the LockMap for this column family. Caller should guarantee that // this column family is no longer in use. - void RemoveColumnFamily(const ColumnFamilyHandle *cfh); + void RemoveColumnFamily(const ColumnFamilyHandle* cfh); // Attempt to lock key. If OK status is returned, the caller is responsible // for calling UnLock() on this key. @@ -132,7 +128,6 @@ class TransactionLockMgr : public BaseLockMgr { void Resize(uint32_t) override; private: - PessimisticTransactionDB* txn_db_impl_; // Default number of lock map stripes per column family @@ -198,25 +193,21 @@ class TransactionLockMgr : public BaseLockMgr { const autovector<TransactionID>& wait_ids); }; - using namespace toku; /* A lock manager that supports Range-based locking. */ -class RangeLockMgr : - public BaseLockMgr, - public RangeLockMgrHandle { +class RangeLockMgr : public BaseLockMgr, public RangeLockMgrHandle { public: - LockTrackerFactory* getLockTrackerFactory() override { return &RangeLockTrackerFactory::instance; } - void AddColumnFamily(const ColumnFamilyHandle *cfh) override; - void RemoveColumnFamily(const ColumnFamilyHandle *cfh) override; + void AddColumnFamily(const ColumnFamilyHandle* cfh) override; + void RemoveColumnFamily(const ColumnFamilyHandle* cfh) override; Status TryLock(PessimisticTransaction* txn, uint32_t column_family_id, - const std::string& key, Env* env, bool exclusive) override ; + const std::string& key, Env* env, bool exclusive) override; // Resize the deadlock-info buffer, does nothing currently void Resize(uint32_t) override; @@ -225,24 +216,20 @@ class RangeLockMgr : // Get a lock on a range // @note only exclusive locks are currently supported (requesting a // non-exclusive lock will get an exclusive one) - Status TryRangeLock(PessimisticTransaction* txn, - uint32_t column_family_id, - const Endpoint &start_endp, - const Endpoint &end_endp, + Status TryRangeLock(PessimisticTransaction* txn, uint32_t column_family_id, + const Endpoint& start_endp, const Endpoint& end_endp, bool exclusive); - + void UnLock(const PessimisticTransaction* txn, const LockTracker& tracker, - Env* env) override ; + Env* env) override; // Release all locks the transaction is holding void UnLockAll(const PessimisticTransaction* txn, Env* env); void UnLock(PessimisticTransaction* txn, uint32_t column_family_id, - const std::string& key, Env* env) override ; + const std::string& key, Env* env) override; RangeLockMgr(std::shared_ptr<TransactionDBMutexFactory> mutex_factory); - void init(TransactionDB *db_arg) override { - my_txn_db_ = db_arg; - } + void init(TransactionDB* db_arg) override { my_txn_db_ = db_arg; } ~RangeLockMgr(); @@ -250,9 +237,7 @@ class RangeLockMgr : return ltm_.set_max_lock_memory(max_lock_memory); } - size_t get_max_lock_memory() { - return ltm_.get_max_lock_memory(); - } + size_t get_max_lock_memory() { return ltm_.get_max_lock_memory(); } Counters GetStatus() override; @@ -278,19 +263,19 @@ class RangeLockMgr : DeadlockInfoBuffer dlock_buffer_; // Get the lock tree which stores locks for Column Family with given cf_id - toku::locktree *get_locktree_by_cfid(uint32_t cf_id); + toku::locktree* get_locktree_by_cfid(uint32_t cf_id); - static int compare_dbt_endpoints(__toku_db*, void *arg, const DBT *a_key, const DBT *b_key); + static int compare_dbt_endpoints(__toku_db*, void* arg, const DBT* a_key, + const DBT* b_key); // Callbacks - static int on_create(locktree*, void*) { return 0; /* no error */ } + static int on_create(locktree*, void*) { return 0; /* no error */ } static void on_destroy(locktree*) {} - static void on_escalate(TXNID txnid, const locktree *lt, - const range_buffer &buffer, void *extra); - + static void on_escalate(TXNID txnid, const locktree* lt, + const range_buffer& buffer, void* extra); }; -void serialize_endpoint(const Endpoint &endp, std::string *buf); +void serialize_endpoint(const Endpoint& endp, std::string* buf); } // namespace ROCKSDB_NAMESPACE #endif // ROCKSDB_LITE diff --git a/utilities/transactions/transaction_lock_mgr_test.cc b/utilities/transactions/transaction_lock_mgr_test.cc index 2d79f788a..9ebae4074 100644 --- a/utilities/transactions/transaction_lock_mgr_test.cc +++ b/utilities/transactions/transaction_lock_mgr_test.cc @@ -47,11 +47,9 @@ class TransactionLockMgrTest : public testing::Test { // If not using range locking, this test creates a separate lock manager // object (right, NOT the one TransactionDB is using!), and runs tests on // that. - locker_ = std::shared_ptr<TransactionLockMgr>( - new TransactionLockMgr(db_, txn_opt.num_stripes, - txn_opt.max_num_locks, - txn_opt.max_num_deadlocks, - mutex_factory_)); + locker_ = std::shared_ptr<TransactionLockMgr>(new TransactionLockMgr( + db_, txn_opt.num_stripes, txn_opt.max_num_locks, + txn_opt.max_num_deadlocks, mutex_factory_)); } } @@ -74,7 +72,7 @@ class TransactionLockMgrTest : public testing::Test { // With Range Locking, functions like GetLockStatusData() return range // endpoints, not the keys themselves. This function extracts the key. - std::string key_value(const std::string &s) { + std::string key_value(const std::string& s) { if (use_range_locking) return s.substr(1); else @@ -89,16 +87,14 @@ class TransactionLockMgrTest : public testing::Test { class AnyLockManagerTest : public TransactionLockMgrTest, public testing::WithParamInterface<bool> { -public: - AnyLockManagerTest() { - use_range_locking = GetParam(); - } + public: + AnyLockManagerTest() { use_range_locking = GetParam(); } }; - class MockColumnFamily : public ColumnFamilyHandle { uint32_t id_; std::string name; + public: ~MockColumnFamily() {} MockColumnFamily(uint32_t id_arg) : id_(id_arg) {} @@ -106,7 +102,7 @@ class MockColumnFamily : public ColumnFamilyHandle { const std::string& GetName() const override { return name; } - Status GetDescriptor(ColumnFamilyDescriptor* ) override { + Status GetDescriptor(ColumnFamilyDescriptor*) override { assert(0); return Status::NotSupported(); } @@ -132,7 +128,6 @@ TEST_F(TransactionLockMgrTest, LockNonExistingColumnFamily) { delete txn; } - TEST_P(AnyLockManagerTest, LockStatus) { locker_->AddColumnFamily(&cf_1024); locker_->AddColumnFamily(&cf_2048); @@ -299,11 +294,10 @@ port::Thread BlockUntilWaitingTxn(bool use_range_locking, std::function<void()> f) { std::atomic<bool> reached(false); const char* sync_point_name = - use_range_locking? "RangeLockMgr::TryRangeLock:WaitingTxn": - "TransactionLockMgr::AcquireWithTimeout:WaitingTxn"; + use_range_locking ? "RangeLockMgr::TryRangeLock:WaitingTxn" + : "TransactionLockMgr::AcquireWithTimeout:WaitingTxn"; ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack( - sync_point_name, - [&](void* /*arg*/) { reached.store(true); }); + sync_point_name, [&](void* /*arg*/) { reached.store(true); }); ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing(); port::Thread t(f); @@ -330,7 +324,6 @@ TEST_P(AnyLockManagerTest, SharedLocks) { // Cleanup locker_->UnLock(txn1, 1, "k", env_); locker_->UnLock(txn2, 1, "k", env_); - } TEST_P(AnyLockManagerTest, Deadlock) { @@ -385,7 +378,6 @@ TEST_P(AnyLockManagerTest, Deadlock) { delete txn1; } - // This test doesn't work with Range Lock Manager, because Range Lock Manager // doesn't support deadlock_detect_depth. diff --git a/utilities/transactions/write_unprepared_txn.cc b/utilities/transactions/write_unprepared_txn.cc index 41fab015b..d2dfda31c 100644 --- a/utilities/transactions/write_unprepared_txn.cc +++ b/utilities/transactions/write_unprepared_txn.cc @@ -800,7 +800,7 @@ Status WriteUnpreparedTxn::RollbackInternal() { void WriteUnpreparedTxn::Clear() { if (!recovered_txn_) { - txn_db_impl_->UnLock(this, *tracked_locks_, /*all_keys_hint=*/ true); + txn_db_impl_->UnLock(this, *tracked_locks_, /*all_keys_hint=*/true); } unprep_seqs_.clear(); flushed_save_points_.reset(nullptr);

1 0

[Commits] 24ca9e111: Range Locking: Cleanup and more test coverage
by Sergei Petrunia 10 Aug '20

10 Aug '20

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

1 0