From 1748a31ae8d69e4939336f644f884e9de3039e7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Tue, 3 Jul 2018 15:10:06 +0300 Subject: [PATCH] MDEV-16675 Unnecessary explicit lock acquisition during UPDATE or DELETE In InnoDB, an INSERT will not create an explicit lock object. Instead, the inserted record is initially implicitly locked by the transaction that wrote its trx_t::id to the hidden system column DB_TRX_ID. (Other transactions would check if DB_TRX_ID is referring to a transaction that has not been committed.) If a record was inserted in the current transaction, it would be implicitly locked by that transaction. Only if some other transaction is requesting access to the record, the implicit lock should be converted to an explicit one, so that the waits-for graph can be constructed for detecting deadlocks and lock wait timeouts. Before this fix, InnoDB would convert implicit locks to explicit ones, even if no conflict exists. lock_rec_convert_impl_to_expl(): Return whether caller_trx already holds an explicit lock that covers the record. row_vers_impl_x_locked_low(): Avoid a lookup if the record matches caller_trx->id. lock_trx_has_expl_x_lock(): Renamed from lock_trx_has_rec_x_lock(). row_upd_clust_step(): In a debug assertion, check for implicit lock before invoking lock_trx_has_expl_x_lock(). rw_trx_hash_t::find(): Make do_ref_count a mandatory parameter. Assert that trx_id is not 0 (the caller should check it). trx_sys_t::is_registered(): Only invoke find() if id != 0. trx_sys_t::find(): Add the optional parameter do_ref_count. lock_rec_queue_validate(): Avoid lookup for trx_id == 0. --- mysql-test/main/xa.result | 13 +-- mysql-test/main/xa.test | 22 ++--- .../innodb/r/innodb_information_schema.result | 2 +- mysql-test/suite/innodb/r/monitor.result | 16 ++- mysql-test/suite/innodb/t/monitor.test | 21 +++- storage/innobase/include/lock0lock.h | 16 +-- storage/innobase/include/trx0sys.h | 13 +-- storage/innobase/lock/lock0lock.cc | 99 +++++++++++++------ storage/innobase/row/row0upd.cc | 13 ++- storage/innobase/row/row0vers.cc | 22 +++-- storage/innobase/trx/trx0trx.cc | 2 +- 11 files changed, 157 insertions(+), 82 deletions(-) diff --git a/mysql-test/main/xa.result b/mysql-test/main/xa.result index 46bfa6e962a..f77c0afdec5 100644 --- a/mysql-test/main/xa.result +++ b/mysql-test/main/xa.result @@ -296,20 +296,21 @@ DROP TABLE t1; # ASSERTION THD->TRANSACTION.XID_STATE.XID.IS_NULL() # FAILED # -DROP TABLE IF EXISTS t1, t2; CREATE TABLE t1 (a INT) ENGINE=InnoDB; CREATE TABLE t2 (a INT) ENGINE=InnoDB; -START TRANSACTION; -INSERT INTO t1 VALUES (1); +INSERT INTO t2 VALUES (1); +COMMIT; +BEGIN; +INSERT INTO t2 VALUES (2); +UPDATE t2 SET a=a+1; connect con2,localhost,root; XA START 'xid1'; +INSERT INTO t1 VALUES (1); # Sending: -INSERT INTO t2 SELECT a FROM t1; +DELETE FROM t2; connection default; -# Waiting until INSERT ... is blocked DELETE FROM t1; connection con2; -# Reaping: INSERT INTO t2 SELECT a FROM t1 ERROR 40001: Deadlock found when trying to get lock; try restarting transaction XA COMMIT 'xid1'; ERROR XA102: XA_RBDEADLOCK: Transaction branch was rolled back: deadlock was detected diff --git a/mysql-test/main/xa.test b/mysql-test/main/xa.test index b5ef5a118b1..176ef6aa760 100644 --- a/mysql-test/main/xa.test +++ b/mysql-test/main/xa.test @@ -393,33 +393,30 @@ DROP TABLE t1; --echo # FAILED --echo # ---disable_warnings -DROP TABLE IF EXISTS t1, t2; ---enable_warnings - CREATE TABLE t1 (a INT) ENGINE=InnoDB; CREATE TABLE t2 (a INT) ENGINE=InnoDB; -START TRANSACTION; -INSERT INTO t1 VALUES (1); +INSERT INTO t2 VALUES (1); COMMIT; +BEGIN; +INSERT INTO t2 VALUES (2); +UPDATE t2 SET a=a+1; --connect (con2,localhost,root) XA START 'xid1'; +INSERT INTO t1 VALUES (1); --echo # Sending: ---send INSERT INTO t2 SELECT a FROM t1 +--send DELETE FROM t2 --connection default let $wait_condition= SELECT COUNT(*) = 1 FROM information_schema.processlist - WHERE state = "Sending data" - AND info = "INSERT INTO t2 SELECT a FROM t1"; ---echo # Waiting until INSERT ... is blocked + WHERE state = "Updating" + AND info = "DELETE FROM t2"; --source include/wait_condition.inc --sleep 0.1 -DELETE FROM t1; +--send DELETE FROM t1 --connection con2 ---echo # Reaping: INSERT INTO t2 SELECT a FROM t1 --error ER_LOCK_DEADLOCK --reap --error ER_XA_RBDEADLOCK @@ -427,6 +424,7 @@ XA COMMIT 'xid1'; connection default; +reap; COMMIT; connection con2; diff --git a/mysql-test/suite/innodb/r/innodb_information_schema.result b/mysql-test/suite/innodb/r/innodb_information_schema.result index c1625f2bc3c..766d5f47c2d 100644 --- a/mysql-test/suite/innodb/r/innodb_information_schema.result +++ b/mysql-test/suite/innodb/r/innodb_information_schema.result @@ -45,7 +45,7 @@ trx_last_foreign_key_error varchar(256) YES NULL trx_is_read_only int(1) NO 0 trx_autocommit_non_locking int(1) NO 0 trx_state trx_weight trx_tables_in_use trx_tables_locked trx_rows_locked trx_rows_modified trx_concurrency_tickets trx_isolation_level trx_unique_checks trx_foreign_key_checks -RUNNING 4 0 1 7 1 0 REPEATABLE READ 1 1 +RUNNING 3 0 1 5 1 0 REPEATABLE READ 1 1 trx_isolation_level trx_unique_checks trx_foreign_key_checks SERIALIZABLE 0 0 trx_state trx_isolation_level trx_last_foreign_key_error diff --git a/mysql-test/suite/innodb/r/monitor.result b/mysql-test/suite/innodb/r/monitor.result index 2700479e7f7..a40bfdac0d0 100644 --- a/mysql-test/suite/innodb/r/monitor.result +++ b/mysql-test/suite/innodb/r/monitor.result @@ -662,7 +662,21 @@ SELECT NAME, COUNT > 0 FROM INFORMATION_SCHEMA.INNODB_METRICS WHERE NAME LIKE 'buffer_page_written_index_leaf'; NAME COUNT > 0 buffer_page_written_index_leaf 1 +DROP TABLE t1; +CREATE TABLE t1(id INT PRIMARY KEY, a INT, b CHAR(1), UNIQUE KEY u(a,b)) +ENGINE=InnoDB; +SET @start = (SELECT COUNT FROM INFORMATION_SCHEMA.INNODB_METRICS WHERE NAME += 'lock_rec_lock_created'); +BEGIN; +INSERT INTO t1 VALUES(1,1,'a'),(2,9999,'b'),(3,10000,'c'),(4,4,'d'); +DELETE FROM t1 WHERE a = 9999 AND b='b'; +COMMIT; +SET @end = (SELECT COUNT FROM INFORMATION_SCHEMA.INNODB_METRICS WHERE NAME += 'lock_rec_lock_created'); +SELECT @end - @start; +@end - @start +0 +DROP TABLE t1; SET GLOBAL innodb_monitor_enable=default; SET GLOBAL innodb_monitor_disable=default; SET GLOBAL innodb_monitor_reset_all=default; -DROP TABLE t1; diff --git a/mysql-test/suite/innodb/t/monitor.test b/mysql-test/suite/innodb/t/monitor.test index dfae93694bf..3535c9c85ad 100644 --- a/mysql-test/suite/innodb/t/monitor.test +++ b/mysql-test/suite/innodb/t/monitor.test @@ -422,10 +422,27 @@ UNLOCK TABLES; SELECT NAME, COUNT > 0 FROM INFORMATION_SCHEMA.INNODB_METRICS WHERE NAME LIKE 'buffer_page_written_index_leaf'; +DROP TABLE t1; + +CREATE TABLE t1(id INT PRIMARY KEY, a INT, b CHAR(1), UNIQUE KEY u(a,b)) +ENGINE=InnoDB; + +SET @start = (SELECT COUNT FROM INFORMATION_SCHEMA.INNODB_METRICS WHERE NAME += 'lock_rec_lock_created'); + +BEGIN; +INSERT INTO t1 VALUES(1,1,'a'),(2,9999,'b'),(3,10000,'c'),(4,4,'d'); +DELETE FROM t1 WHERE a = 9999 AND b='b'; +COMMIT; + +SET @end = (SELECT COUNT FROM INFORMATION_SCHEMA.INNODB_METRICS WHERE NAME += 'lock_rec_lock_created'); +SELECT @end - @start; + +DROP TABLE t1; + --disable_warnings SET GLOBAL innodb_monitor_enable=default; SET GLOBAL innodb_monitor_disable=default; SET GLOBAL innodb_monitor_reset_all=default; --enable_warnings - -DROP TABLE t1; diff --git a/storage/innobase/include/lock0lock.h b/storage/innobase/include/lock0lock.h index 2159f56d018..e4b8947ac3a 100644 --- a/storage/innobase/include/lock0lock.h +++ b/storage/innobase/include/lock0lock.h @@ -789,19 +789,21 @@ const lock_t* lock_trx_has_sys_table_locks( /*=========================*/ const trx_t* trx) /*!< in: transaction to check */ - MY_ATTRIBUTE((warn_unused_result)); + MY_ATTRIBUTE((nonnull, warn_unused_result)); -/*******************************************************************//** -Check if the transaction holds an exclusive lock on a record. -@return whether the locks are held */ +/** Check if the transaction holds an explicit exclusive lock on a record. +@param[in] trx transaction +@param[in] table table +@param[in] block leaf page +@param[in] heap_no heap number identifying the record +@return whether an explicit X-lock is held */ bool -lock_trx_has_rec_x_lock( -/*====================*/ +lock_trx_has_expl_x_lock( const trx_t* trx, /*!< in: transaction to check */ const dict_table_t* table, /*!< in: table to check */ const buf_block_t* block, /*!< in: buffer block of the record */ ulint heap_no)/*!< in: record heap number */ - MY_ATTRIBUTE((warn_unused_result)); + MY_ATTRIBUTE((nonnull, warn_unused_result)); #endif /* UNIV_DEBUG */ /** diff --git a/storage/innobase/include/trx0sys.h b/storage/innobase/include/trx0sys.h index ea01d698b3b..69374ab3fba 100644 --- a/storage/innobase/include/trx0sys.h +++ b/storage/innobase/include/trx0sys.h @@ -616,7 +616,7 @@ public: @retval pointer to trx */ - trx_t *find(trx_t *caller_trx, trx_id_t trx_id, bool do_ref_count= false) + trx_t *find(trx_t *caller_trx, trx_id_t trx_id, bool do_ref_count) { /* In MariaDB 10.3, purge will reset DB_TRX_ID to 0 @@ -624,9 +624,10 @@ public: always have a nonzero trx_t::id; there the value 0 is reserved for transactions that did not write or lock anything yet. + + The caller should already have handled trx_id==0 specially. */ - if (!trx_id) - return NULL; + ut_ad(trx_id); if (caller_trx && caller_trx->id == trx_id) { if (do_ref_count) @@ -1044,13 +1045,13 @@ public: bool is_registered(trx_t *caller_trx, trx_id_t id) { - return rw_trx_hash.find(caller_trx, id); + return id && find(caller_trx, id, false); } - trx_t *find(trx_t *caller_trx, trx_id_t id) + trx_t *find(trx_t *caller_trx, trx_id_t id, bool do_ref_count= true) { - return rw_trx_hash.find(caller_trx, id, true); + return rw_trx_hash.find(caller_trx, id, do_ref_count); } diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc index 4987d60dd5a..cd65a9ac8e0 100644 --- a/storage/innobase/lock/lock0lock.cc +++ b/storage/innobase/lock/lock0lock.cc @@ -4937,8 +4937,12 @@ lock_rec_queue_validate( /* Unlike the non-debug code, this invariant can only succeed if the check and assertion are covered by the lock mutex. */ - const trx_t *impl_trx = trx_sys.rw_trx_hash.find(current_trx(), - lock_clust_rec_some_has_impl(rec, index, offsets)); + const trx_id_t impl_trx_id = lock_clust_rec_some_has_impl( + rec, index, offsets); + + const trx_t *impl_trx = impl_trx_id + ? trx_sys.find(current_trx(), impl_trx_id, false) + : 0; ut_ad(lock_mutex_own()); /* impl_trx cannot be committed until lock_mutex_exit() @@ -5547,18 +5551,31 @@ static void lock_rec_other_trx_holds_expl(trx_t *caller_trx, trx_t *trx, #endif /* UNIV_DEBUG */ -/*********************************************************************//** -If a transaction has an implicit x-lock on a record, but no explicit x-lock -set on the record, sets one for it. */ +/** If an implicit x-lock exists on a record, convert it to an explicit one. + +Often, this is called by a transaction that is about to enter a lock wait +due to the lock conflict. Two explicit locks would be created: first the +exclusive lock on behalf of the lock-holder transaction in this function, +and then a wait request on behalf of caller_trx, in the calling function. + +This may also be called by the same transaction that is already holding +an implicit exclusive lock on the record. In this case, no explicit lock +should be created. + +@param[in,out] caller_trx current transaction +@param[in] block index tree leaf page +@param[in] rec record on the leaf page +@param[in] index the index of the record +@param[in] offsets rec_get_offsets(rec,index) +@return whether caller_trx already holds an exclusive lock on rec */ static -void +bool lock_rec_convert_impl_to_expl( -/*==========================*/ - trx_t* caller_trx,/*!id)) { + return true; + } + trx = trx_sys.find(caller_trx, trx_id); } else { ut_ad(!dict_index_is_online_ddl(index)); trx = lock_sec_rec_some_has_impl(caller_trx, rec, index, offsets); + if (trx == caller_trx) { + trx->release_reference(); + return true; + } ut_d(lock_rec_other_trx_holds_expl(caller_trx, trx, rec, block)); @@ -5597,6 +5625,8 @@ lock_rec_convert_impl_to_expl( lock_rec_convert_impl_to_expl_for_trx( block, rec, index, trx, heap_no); } + + return false; } /*********************************************************************//** @@ -5641,8 +5671,11 @@ lock_clust_rec_modify_check_and_lock( /* If a transaction has no explicit x-lock set on the record, set one for it */ - lock_rec_convert_impl_to_expl(thr_get_trx(thr), block, rec, index, - offsets); + if (lock_rec_convert_impl_to_expl(thr_get_trx(thr), block, rec, index, + offsets)) { + /* We already hold an implicit exclusive lock. */ + return DB_SUCCESS; + } err = lock_rec_lock(TRUE, LOCK_X | LOCK_REC_NOT_GAP, block, heap_no, index, thr); @@ -5786,10 +5819,11 @@ lock_sec_rec_read_check_and_lock( database recovery is running. */ if (!page_rec_is_supremum(rec) - && page_get_max_trx_id(block->frame) >= trx_sys.get_min_trx_id()) { - - lock_rec_convert_impl_to_expl(thr_get_trx(thr), block, rec, - index, offsets); + && page_get_max_trx_id(block->frame) >= trx_sys.get_min_trx_id() + && lock_rec_convert_impl_to_expl(thr_get_trx(thr), block, rec, + index, offsets)) { + /* We already hold an implicit exclusive lock. */ + return DB_SUCCESS; } err = lock_rec_lock(FALSE, ulint(mode) | gap_mode, @@ -5850,10 +5884,11 @@ lock_clust_rec_read_check_and_lock( heap_no = page_rec_get_heap_no(rec); - if (heap_no != PAGE_HEAP_NO_SUPREMUM) { - - lock_rec_convert_impl_to_expl(thr_get_trx(thr), block, rec, - index, offsets); + if (heap_no != PAGE_HEAP_NO_SUPREMUM + && lock_rec_convert_impl_to_expl(thr_get_trx(thr), block, rec, + index, offsets)) { + /* We already hold an implicit exclusive lock. */ + return DB_SUCCESS; } err = lock_rec_lock(FALSE, ulint(mode) | gap_mode, @@ -6560,12 +6595,14 @@ lock_trx_has_sys_table_locks( return(strongest_lock); } -/*******************************************************************//** -Check if the transaction holds an exclusive lock on a record. -@return whether the locks are held */ +/** Check if the transaction holds an explicit exclusive lock on a record. +@param[in] trx transaction +@param[in] table table +@param[in] block leaf page +@param[in] heap_no heap number identifying the record +@return whether an explicit X-lock is held */ bool -lock_trx_has_rec_x_lock( -/*====================*/ +lock_trx_has_expl_x_lock( const trx_t* trx, /*!< in: transaction to check */ const dict_table_t* table, /*!< in: table to check */ const buf_block_t* block, /*!< in: buffer block of the record */ @@ -6574,11 +6611,9 @@ lock_trx_has_rec_x_lock( ut_ad(heap_no > PAGE_HEAP_NO_SUPREMUM); lock_mutex_enter(); - ut_a(lock_table_has(trx, table, LOCK_IX) - || table->is_temporary()); - ut_a(lock_rec_has_expl(LOCK_X | LOCK_REC_NOT_GAP, - block, heap_no, trx) - || table->is_temporary()); + ut_ad(lock_table_has(trx, table, LOCK_IX)); + ut_ad(lock_rec_has_expl(LOCK_X | LOCK_REC_NOT_GAP, block, heap_no, + trx)); lock_mutex_exit(); return(true); } diff --git a/storage/innobase/row/row0upd.cc b/storage/innobase/row/row0upd.cc index 39206e66d75..447132dcf86 100644 --- a/storage/innobase/row/row0upd.cc +++ b/storage/innobase/row/row0upd.cc @@ -3093,9 +3093,7 @@ row_upd_clust_step( ulint mode; - DEBUG_SYNC_C_IF_THD( - thr_get_trx(thr)->mysql_thd, - "innodb_row_upd_clust_step_enter"); + DEBUG_SYNC_C_IF_THD(trx->mysql_thd, "innodb_row_upd_clust_step_enter"); if (dict_index_is_online_ddl(index)) { ut_ad(node->table->id != DICT_INDEXES_ID); @@ -3157,10 +3155,11 @@ row_upd_clust_step( } } - ut_ad(index->table->no_rollback() - || lock_trx_has_rec_x_lock(thr_get_trx(thr), index->table, - btr_pcur_get_block(pcur), - page_rec_get_heap_no(rec))); + ut_ad(index->table->no_rollback() || index->table->is_temporary() + || row_get_rec_trx_id(rec, index, offsets) == trx->id + || lock_trx_has_expl_x_lock(trx, index->table, + btr_pcur_get_block(pcur), + page_rec_get_heap_no(rec))); /* NOTE: the following function calls will also commit mtr */ diff --git a/storage/innobase/row/row0vers.cc b/storage/innobase/row/row0vers.cc index bfaa2721746..81f9c795a7c 100644 --- a/storage/innobase/row/row0vers.cc +++ b/storage/innobase/row/row0vers.cc @@ -126,14 +126,22 @@ row_vers_impl_x_locked_low( DBUG_RETURN(0); } - trx_t* trx = trx_sys.find(caller_trx, trx_id); + trx_t* trx; - if (trx == 0) { - /* The transaction that modified or inserted clust_rec is no - longer active, or it is corrupt: no implicit lock on rec */ - lock_check_trx_id_sanity(trx_id, clust_rec, clust_index, clust_offsets); - mem_heap_free(heap); - DBUG_RETURN(0); + if (trx_id == caller_trx->id) { + trx = caller_trx; + trx->reference(); + } else { + trx = trx_sys.find(caller_trx, trx_id); + if (trx == 0) { + /* The transaction that modified or inserted + clust_rec is no longer active, or it is + corrupt: no implicit lock on rec */ + lock_check_trx_id_sanity(trx_id, clust_rec, + clust_index, clust_offsets); + mem_heap_free(heap); + DBUG_RETURN(0); + } } comp = page_rec_is_comp(rec); diff --git a/storage/innobase/trx/trx0trx.cc b/storage/innobase/trx/trx0trx.cc index 11d83c96a92..c13ccdf53ed 100644 --- a/storage/innobase/trx/trx0trx.cc +++ b/storage/innobase/trx/trx0trx.cc @@ -761,7 +761,7 @@ trx_lists_init_at_db_start() for (undo = UT_LIST_GET_FIRST(rseg->undo_list); undo != NULL; undo = UT_LIST_GET_NEXT(undo_list, undo)) { - trx_t *trx = trx_sys.rw_trx_hash.find(0, undo->trx_id); + trx_t *trx = trx_sys.find(0, undo->trx_id, false); if (!trx) { trx_resurrect(undo, rseg, start_time, &rows_to_undo, false);