From b82abc71639661c642b9d753393be0f85e09c101 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Wed, 15 Jan 2025 16:55:01 +0200 Subject: [PATCH 1/2] MDEV-35701 trx_t::autoinc_locks causes unnecessary dynamic memory allocation trx_t::autoinc_locks: Use small_vector in order to avoid any dynamic memory allocation in the most common case (a statement is holding AUTO_INCREMENT locks on at most 4 tables or partitions). lock_cancel_waiting_and_release(): Instead of removing elements from the middle, simply assign nullptr, like lock_table_remove_autoinc_lock(). The added test innodb.auto_increment_lock_mode covers the dynamic memory allocation as well as nondeterministically (occasionally) covers the out-of-order lock release in lock_table_remove_autoinc_lock(). Reviewed by: Debarun Banerjee --- .../innodb/r/auto_increment_lock_mode.result | 43 +++++ .../innodb/r/innodb-autoinc-56228.result | 35 ++-- .../t/auto_increment_lock_mode.combinations | 6 + .../innodb/t/auto_increment_lock_mode.test | 61 +++++++ .../suite/innodb/t/innodb-autoinc-56228.test | 36 ++-- storage/innobase/dict/drop.cc | 2 +- storage/innobase/handler/ha_innodb.cc | 2 +- storage/innobase/include/small_vector.h | 65 +++++-- storage/innobase/include/trx0trx.h | 14 +- storage/innobase/lock/lock0lock.cc | 160 +++++++----------- storage/innobase/mtr/mtr0mtr.cc | 4 +- storage/innobase/trx/trx0trx.cc | 28 +-- 12 files changed, 273 insertions(+), 183 deletions(-) create mode 100644 mysql-test/suite/innodb/r/auto_increment_lock_mode.result create mode 100644 mysql-test/suite/innodb/t/auto_increment_lock_mode.combinations create mode 100644 mysql-test/suite/innodb/t/auto_increment_lock_mode.test diff --git a/mysql-test/suite/innodb/r/auto_increment_lock_mode.result b/mysql-test/suite/innodb/r/auto_increment_lock_mode.result new file mode 100644 index 0000000000000..b19d5cf40fccb --- /dev/null +++ b/mysql-test/suite/innodb/r/auto_increment_lock_mode.result @@ -0,0 +1,43 @@ +CREATE TABLE t1(a TINYINT UNSIGNED AUTO_INCREMENT PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t2(a TINYINT UNSIGNED AUTO_INCREMENT PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t3(a TINYINT UNSIGNED AUTO_INCREMENT PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t4(a TINYINT UNSIGNED AUTO_INCREMENT PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t5(a TINYINT UNSIGNED AUTO_INCREMENT PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t6(a SERIAL, b INT) ENGINE=InnoDB; +CREATE FUNCTION p1() RETURNS INT +BEGIN +INSERT INTO t1() VALUES(); +INSERT INTO t2() VALUES(); +INSERT INTO t3() VALUES(); +INSERT INTO t4() VALUES(); +INSERT INTO t5() VALUES(); +RETURN 1; +END$$ +INSERT INTO t6(b) SELECT p1(); +UPDATE t1,t2,t3,t4,t5 SET t1.a=2,t2.a=2,t3.a=2,t4.a=2,t5.a=2; +SELECT * FROM t1; +a +2 +connect con1,localhost,root,,; +connect con2,localhost,root,,; +connect con3,localhost,root,,; +connection con1; +INSERT INTO t6(b) SELECT SLEEP(p1()); +connection con2; +INSERT INTO t6(b) SELECT SLEEP(p1()); +connection con3; +UPDATE t1,t2,t3,t4,t5 SET t1.a=0,t2.a=0,t3.a=0,t4.a=0,t5.a=0 +WHERE t1.a=2 AND t2.a=2 AND t3.a=2 AND t4.a=2 AND t5.a=2; +connection default; +KILL QUERY $ID1; +KILL QUERY $ID2; +KILL QUERY $ID3; +connection con1; +disconnect con1; +connection con2; +disconnect con2; +connection con3; +disconnect con3; +connection default; +DROP FUNCTION p1; +DROP TABLE t1,t2,t3,t4,t5,t6; diff --git a/mysql-test/suite/innodb/r/innodb-autoinc-56228.result b/mysql-test/suite/innodb/r/innodb-autoinc-56228.result index 6a3fd85ebd33a..f86167721511c 100644 --- a/mysql-test/suite/innodb/r/innodb-autoinc-56228.result +++ b/mysql-test/suite/innodb/r/innodb-autoinc-56228.result @@ -1,15 +1,6 @@ -DROP TABLE IF EXISTS t1_56228; -Warnings: -Note 1051 Unknown table 'test.t1_56228' -DROP TABLE IF EXISTS t2_56228; -Warnings: -Note 1051 Unknown table 'test.t2_56228' -DROP FUNCTION IF EXISTS bug56228; -Warnings: -Note 1305 FUNCTION test.bug56228 does not exist -CREATE TEMPORARY TABLE t1_56228( +CREATE TABLE t1_56228( c1 iNT AUTO_INCREMENT PRIMARY KEY) ENGINE=InnoDB; -CREATE TEMPORARY TABLE t2_56228( +CREATE TABLE t2_56228( c1 iNT AUTO_INCREMENT PRIMARY KEY) ENGINE=InnoDB; CREATE FUNCTION bug56228() RETURNS INT DETERMINISTIC BEGIN @@ -17,14 +8,18 @@ INSERT INTO t1_56228 VALUES(NULL); INSERT INTO t2_56228 VALUES(NULL); INSERT INTO t1_56228 VALUES(NULL); INSERT INTO t2_56228 VALUES(NULL); -DROP TEMPORARY TABLE t1_56228; +DROP TABLE t1_56228; RETURN 42; END // -SELECT bug56228(); -bug56228() -42 -DROP FUNCTION bug56228; -DROP TEMPORARY TABLE t2_56228; -DROP TEMPORARY TABLE IF EXISTS t1_56228; -Warnings: -Note 1051 Unknown table 'test.t1_56228' +ERROR HY000: Explicit or implicit commit is not allowed in stored function or trigger +CREATE PROCEDURE bug56228() +BEGIN +INSERT INTO t1_56228 VALUES(NULL); +INSERT INTO t2_56228 VALUES(NULL); +INSERT INTO t1_56228 VALUES(NULL); +INSERT INTO t2_56228 VALUES(NULL); +DROP TABLE t1_56228; +END // +CALL bug56228(); +DROP PROCEDURE bug56228; +DROP TABLE t2_56228; diff --git a/mysql-test/suite/innodb/t/auto_increment_lock_mode.combinations b/mysql-test/suite/innodb/t/auto_increment_lock_mode.combinations new file mode 100644 index 0000000000000..efaee252a4ecd --- /dev/null +++ b/mysql-test/suite/innodb/t/auto_increment_lock_mode.combinations @@ -0,0 +1,6 @@ +[old] +--innodb-autoinc-lock-mode=0 +[new] +--innodb-autoinc-lock-mode=1 +[none] +--innodb-autoinc-lock-mode=2 diff --git a/mysql-test/suite/innodb/t/auto_increment_lock_mode.test b/mysql-test/suite/innodb/t/auto_increment_lock_mode.test new file mode 100644 index 0000000000000..27497960e7264 --- /dev/null +++ b/mysql-test/suite/innodb/t/auto_increment_lock_mode.test @@ -0,0 +1,61 @@ +--source include/have_innodb.inc + +CREATE TABLE t1(a TINYINT UNSIGNED AUTO_INCREMENT PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t2(a TINYINT UNSIGNED AUTO_INCREMENT PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t3(a TINYINT UNSIGNED AUTO_INCREMENT PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t4(a TINYINT UNSIGNED AUTO_INCREMENT PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t5(a TINYINT UNSIGNED AUTO_INCREMENT PRIMARY KEY) ENGINE=InnoDB; +CREATE TABLE t6(a SERIAL, b INT) ENGINE=InnoDB; + +DELIMITER $$; +CREATE FUNCTION p1() RETURNS INT +BEGIN + INSERT INTO t1() VALUES(); + INSERT INTO t2() VALUES(); + INSERT INTO t3() VALUES(); + INSERT INTO t4() VALUES(); + INSERT INTO t5() VALUES(); + RETURN 1; +END$$ +DELIMITER ;$$ + +INSERT INTO t6(b) SELECT p1(); + +UPDATE t1,t2,t3,t4,t5 SET t1.a=2,t2.a=2,t3.a=2,t4.a=2,t5.a=2; +SELECT * FROM t1; +--source include/count_sessions.inc + +--connect(con1,localhost,root,,) +let $ID1= `SELECT @id := CONNECTION_ID()`; +--connect(con2,localhost,root,,) +let $ID2= `SELECT @id := CONNECTION_ID()`; +--connect(con3,localhost,root,,) +let $ID3= `SELECT @id := CONNECTION_ID()`; +--connection con1 +send INSERT INTO t6(b) SELECT SLEEP(p1()); +--connection con2 +send INSERT INTO t6(b) SELECT SLEEP(p1()); +--connection con3 +send UPDATE t1,t2,t3,t4,t5 SET t1.a=0,t2.a=0,t3.a=0,t4.a=0,t5.a=0 +WHERE t1.a=2 AND t2.a=2 AND t3.a=2 AND t4.a=2 AND t5.a=2; +--connection default +evalp KILL QUERY $ID1; +evalp KILL QUERY $ID2; +evalp KILL QUERY $ID3; +--connection con1 +--error 0,ER_QUERY_INTERRUPTED,ER_AUTOINC_READ_FAILED +--reap +--disconnect con1 +--connection con2 +--error 0,ER_QUERY_INTERRUPTED,ER_AUTOINC_READ_FAILED +--reap +--disconnect con2 +--connection con3 +--error 0,ER_QUERY_INTERRUPTED,ER_AUTOINC_READ_FAILED +--reap +--disconnect con3 +--connection default + +DROP FUNCTION p1; +DROP TABLE t1,t2,t3,t4,t5,t6; +--source include/wait_until_count_sessions.inc diff --git a/mysql-test/suite/innodb/t/innodb-autoinc-56228.test b/mysql-test/suite/innodb/t/innodb-autoinc-56228.test index a02f7b4383a8d..e1d8741c730a3 100644 --- a/mysql-test/suite/innodb/t/innodb-autoinc-56228.test +++ b/mysql-test/suite/innodb/t/innodb-autoinc-56228.test @@ -2,33 +2,45 @@ ## # Bug #56228: dropping tables from within an active statement crashes server # -DROP TABLE IF EXISTS t1_56228; -DROP TABLE IF EXISTS t2_56228; -DROP FUNCTION IF EXISTS bug56228; -CREATE TEMPORARY TABLE t1_56228( +# This test used to use TEMPORARY TABLE, which before MySQL 5.7 or +# MariaDB Server 10.2 were covered by InnoDB locks. +# In MariaDB Server 10.6, the locking and logging was corrected for Atomic DDL. +# Hence, even if we tweaked create_table_info_t::innobase_table_flags() +# so that TEMPORARY TABLE are created as persistent tables, +# the DROP TEMPORARY TABLE statement inside the function would +# fail due to HA_ERR_LOCK_WAIT_TIMEOUT, instead of breaking locks +# like it used to do before MDEV-26603 and possibly other changes. +CREATE TABLE t1_56228( c1 iNT AUTO_INCREMENT PRIMARY KEY) ENGINE=InnoDB; -CREATE TEMPORARY TABLE t2_56228( +CREATE TABLE t2_56228( c1 iNT AUTO_INCREMENT PRIMARY KEY) ENGINE=InnoDB; DELIMITER //; +--error ER_COMMIT_NOT_ALLOWED_IN_SF_OR_TRG CREATE FUNCTION bug56228() RETURNS INT DETERMINISTIC BEGIN INSERT INTO t1_56228 VALUES(NULL); INSERT INTO t2_56228 VALUES(NULL); INSERT INTO t1_56228 VALUES(NULL); INSERT INTO t2_56228 VALUES(NULL); - DROP TEMPORARY TABLE t1_56228; + DROP TABLE t1_56228; RETURN 42; END // +CREATE PROCEDURE bug56228() +BEGIN + INSERT INTO t1_56228 VALUES(NULL); + INSERT INTO t2_56228 VALUES(NULL); + INSERT INTO t1_56228 VALUES(NULL); + INSERT INTO t2_56228 VALUES(NULL); + DROP TABLE t1_56228; +END // + DELIMITER ;// ---disable_ps_protocol -SELECT bug56228(); ---enable_ps2_protocol +CALL bug56228(); -DROP FUNCTION bug56228; -DROP TEMPORARY TABLE t2_56228; -DROP TEMPORARY TABLE IF EXISTS t1_56228; +DROP PROCEDURE bug56228; +DROP TABLE t2_56228; diff --git a/storage/innobase/dict/drop.cc b/storage/innobase/dict/drop.cc index f46b4b04752d3..80bf8c97eadaa 100644 --- a/storage/innobase/dict/drop.cc +++ b/storage/innobase/dict/drop.cc @@ -247,7 +247,7 @@ void trx_t::commit(std::vector &deleted) mutex_lock(); lock_release_on_drop(this); ut_ad(UT_LIST_GET_LEN(lock.trx_locks) == 0); - ut_ad(ib_vector_is_empty(autoinc_locks)); + ut_ad(autoinc_locks.empty()); mem_heap_empty(lock.lock_heap); lock.table_locks.clear(); /* commit_persist() already reset this. */ diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 4fedd79790b11..31c4f525adfb2 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -5097,7 +5097,7 @@ static void innobase_kill_query(handlerton*, THD *thd, enum thd_kill_levels) may be executed as part of a multi-transaction DDL operation, such as rollback_inplace_alter_table() or ha_innobase::delete_table(). */; trx->error_state= DB_INTERRUPTED; - lock_sys_t::cancel(trx, lock); + lock_sys.cancel(trx, lock); } lock_sys.deadlock_check(); } diff --git a/storage/innobase/include/small_vector.h b/storage/innobase/include/small_vector.h index d28a36184b8e2..2acdc49f6682f 100644 --- a/storage/innobase/include/small_vector.h +++ b/storage/innobase/include/small_vector.h @@ -31,14 +31,14 @@ class small_vector_base small_vector_base()= delete; small_vector_base(void *small, size_t small_size) : BeginX(small), Capacity(Size_T(small_size)) {} - ATTRIBUTE_COLD void grow_by_1(void *small, size_t element_size); + ATTRIBUTE_COLD void grow_by_1(void *small, size_t element_size) noexcept; public: - size_t size() const { return Size; } - size_t capacity() const { return Capacity; } - bool empty() const { return !Size; } - void clear() { Size= 0; } + size_t size() const noexcept { return Size; } + size_t capacity() const noexcept { return Capacity; } + bool empty() const noexcept { return !Size; } + void clear() noexcept { Size= 0; } protected: - void set_size(size_t N) { Size= Size_T(N); } + void set_size(size_t N) noexcept { Size= Size_T(N); } }; template @@ -49,7 +49,7 @@ class small_vector : public small_vector_base using small_vector_base::set_size; - void grow_if_needed() + void grow_if_needed() noexcept { if (unlikely(size() >= capacity())) grow_by_1(small, sizeof *small); @@ -60,38 +60,67 @@ class small_vector : public small_vector_base { TRASH_ALLOC(small, sizeof small); } - ~small_vector() + ~small_vector() noexcept { if (small != begin()) my_free(begin()); MEM_MAKE_ADDRESSABLE(small, sizeof small); } + void fake_defined() const noexcept + { + ut_ad(empty()); + MEM_MAKE_DEFINED(small, sizeof small); + } + void make_undefined() const noexcept { MEM_UNDEFINED(small, sizeof small); } + + bool is_small() const noexcept { return small == BeginX; } + + void deep_clear() noexcept + { + if (!is_small()) + { + my_free(BeginX); + BeginX= small; + Capacity= N; + } + ut_ad(capacity() == N); + set_size(0); + } + using iterator= T *; using const_iterator= const T *; using reverse_iterator= std::reverse_iterator; using reference= T &; using const_reference= const T&; - iterator begin() { return static_cast(BeginX); } - const_iterator begin() const { return static_cast(BeginX); } - iterator end() { return begin() + size(); } - const_iterator end() const { return begin() + size(); } + iterator begin() noexcept { return static_cast(BeginX); } + const_iterator begin() const noexcept + { return static_cast(BeginX); } + iterator end() noexcept { return begin() + size(); } + const_iterator end() const noexcept { return begin() + size(); } - reverse_iterator rbegin() { return reverse_iterator(end()); } - reverse_iterator rend() { return reverse_iterator(begin()); } + reverse_iterator rbegin() noexcept { return reverse_iterator(end()); } + reverse_iterator rend() noexcept { return reverse_iterator(begin()); } - reference operator[](size_t i) { assert(i < size()); return begin()[i]; } - const_reference operator[](size_t i) const + reference operator[](size_t i) noexcept + { assert(i < size()); return begin()[i]; } + const_reference operator[](size_t i) const noexcept { return const_cast(*this)[i]; } - void erase(const_iterator S, const_iterator E) + void erase(const_iterator S, const_iterator E) noexcept { set_size(std::move(const_cast(E), end(), const_cast(S)) - begin()); } - void emplace_back(T &&arg) + void emplace_back(T &&arg) noexcept + { + grow_if_needed(); + ::new (end()) T(arg); + set_size(size() + 1); + } + void emplace_back(T &arg) noexcept { grow_if_needed(); ::new (end()) T(arg); diff --git a/storage/innobase/include/trx0trx.h b/storage/innobase/include/trx0trx.h index 9a1c5edd43f47..d79aafb1cedb1 100644 --- a/storage/innobase/include/trx0trx.h +++ b/storage/innobase/include/trx0trx.h @@ -32,10 +32,10 @@ Created 3/26/1996 Heikki Tuuri #include "que0types.h" #include "mem0mem.h" #include "trx0xa.h" -#include "ut0vec.h" #include "fts0fts.h" #include "read0types.h" #include "ilist.h" +#include "small_vector.h" #include @@ -857,12 +857,10 @@ struct trx_t : ilist_node<> ulint n_autoinc_rows; /*!< no. of AUTO-INC rows required for an SQL statement. This is useful for multi-row INSERTs */ - ib_vector_t* autoinc_locks; /* AUTOINC locks held by this - transaction. Note that these are - also in the lock list trx_locks. This - vector needs to be freed explicitly - when the trx instance is destroyed. - Protected by lock_sys.latch. */ + typedef small_vector autoinc_lock_vector; + /** AUTO_INCREMENT locks held by this transaction; a subset of trx_locks, + protected by lock_sys.latch. */ + autoinc_lock_vector autoinc_locks; /*------------------------------*/ bool read_only; /*!< true if transaction is flagged as a READ-ONLY transaction. @@ -1059,7 +1057,7 @@ struct trx_t : ilist_node<> ut_ad(!lock.wait_lock); ut_ad(UT_LIST_GET_LEN(lock.trx_locks) == 0); ut_ad(lock.table_locks.empty()); - ut_ad(!autoinc_locks || ib_vector_is_empty(autoinc_locks)); + ut_ad(autoinc_locks.empty()); ut_ad(UT_LIST_GET_LEN(lock.evicted_tables) == 0); ut_ad(!dict_operation); ut_ad(!apply_online_log); diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc index 5a6857678fc79..5e6ed1502af8b 100644 --- a/storage/innobase/lock/lock0lock.cc +++ b/storage/innobase/lock/lock0lock.cc @@ -2302,7 +2302,7 @@ static void lock_grant(lock_t *lock) dict_table_t *table= lock->un_member.tab_lock.table; ut_ad(!table->autoinc_trx); table->autoinc_trx= trx; - ib_vector_push(trx->autoinc_locks, &lock); + trx->autoinc_locks.emplace_back(lock); } DBUG_PRINT("ib_lock", ("wait for trx " TRX_ID_FMT " ends", trx->id)); @@ -3646,7 +3646,7 @@ lock_t *lock_table_create(dict_table_t *table, unsigned type_mode, trx_t *trx, ut_ad(!table->autoinc_trx); table->autoinc_trx = trx; - ib_vector_push(trx->autoinc_locks, &lock); + trx->autoinc_locks.emplace_back(lock); goto allocated; } @@ -3695,79 +3695,45 @@ lock_t *lock_table_create(dict_table_t *table, unsigned type_mode, trx_t *trx, return(lock); } -/*************************************************************//** -Pops autoinc lock requests from the transaction's autoinc_locks. We -handle the case where there are gaps in the array and they need to -be popped off the stack. */ -UNIV_INLINE -void -lock_table_pop_autoinc_locks( -/*=========================*/ - trx_t* trx) /*!< in/out: transaction that owns the AUTOINC locks */ +/** Release a granted AUTO_INCREMENT lock. +@param lock AUTO_INCREMENT lock +@param trx transaction that owns the lock */ +static void lock_table_remove_autoinc_lock(lock_t *lock, trx_t *trx) noexcept { - ut_ad(!ib_vector_is_empty(trx->autoinc_locks)); - - /* Skip any gaps, gaps are NULL lock entries in the - trx->autoinc_locks vector. */ - - do { - ib_vector_pop(trx->autoinc_locks); - - if (ib_vector_is_empty(trx->autoinc_locks)) { - return; - } - - } while (*(lock_t**) ib_vector_get_last(trx->autoinc_locks) == NULL); -} - -/*************************************************************//** -Removes an autoinc lock request from the transaction's autoinc_locks. */ -UNIV_INLINE -void -lock_table_remove_autoinc_lock( -/*===========================*/ - lock_t* lock, /*!< in: table lock */ - trx_t* trx) /*!< in/out: transaction that owns the lock */ -{ - ut_ad(lock->type_mode == (LOCK_AUTO_INC | LOCK_TABLE)); - lock_sys.assert_locked(*lock->un_member.tab_lock.table); - ut_ad(trx->mutex_is_owner()); - - auto s = ib_vector_size(trx->autoinc_locks); - ut_ad(s); - - /* With stored functions and procedures the user may drop - a table within the same "statement". This special case has - to be handled by deleting only those AUTOINC locks that were - held by the table being dropped. */ - - lock_t* autoinc_lock = *static_cast( - ib_vector_get(trx->autoinc_locks, --s)); - - /* This is the default fast case. */ - - if (autoinc_lock == lock) { - lock_table_pop_autoinc_locks(trx); - } else { - /* The last element should never be NULL */ - ut_a(autoinc_lock != NULL); - - /* Handle freeing the locks from within the stack. */ - - while (s) { - autoinc_lock = *static_cast( - ib_vector_get(trx->autoinc_locks, --s)); + ut_ad(lock->type_mode == (LOCK_AUTO_INC | LOCK_TABLE)); + lock_sys.assert_locked(*lock->un_member.tab_lock.table); + ut_ad(trx->mutex_is_owner()); - if (autoinc_lock == lock) { - void* null_var = NULL; - ib_vector_set(trx->autoinc_locks, s, &null_var); - return; - } - } + auto begin= trx->autoinc_locks.begin(), end= trx->autoinc_locks.end(), i=end; + ut_ad(begin != end); + if (*--i == lock) + { + /* Normally, the last acquired lock is released first, in order to + avoid unnecessary traversal of trx->autoinc_locks, which + only stores granted locks. */ + + /* We remove the last lock, as well as any nullptr entries + immediately preceding it, which might have been created by the + "else" branch below, or by lock_cancel_waiting_and_release(). */ + while (begin != i && !i[-1]) i--; + trx->autoinc_locks.erase(i, end); + } + else + { + ut_a(*i); + /* Clear the lock when it is not the last one. */ + while (begin != i) + { + if (*--i == lock) + { + *i= nullptr; + return; + } + } - /* Must find the autoinc lock. */ - ut_error; - } + /* The lock must exist. */ + ut_error; + } } /*************************************************************//** @@ -3799,14 +3765,7 @@ lock_table_remove_low( ut_ad((table->autoinc_trx == trx) == !lock->is_waiting()); if (table->autoinc_trx == trx) { - table->autoinc_trx = NULL; - /* The locks must be freed in the reverse order from - the one in which they were acquired. This is to avoid - traversing the AUTOINC lock vector unnecessarily. - - We only store locks that were granted in the - trx->autoinc_locks vector (see lock_table_create() - and lock_grant()). */ + table->autoinc_trx = nullptr; lock_table_remove_autoinc_lock(lock, trx); } @@ -6443,44 +6402,31 @@ lock_clust_rec_read_check_and_lock_alt( return(err); } -/*******************************************************************//** -Check if a transaction holds any autoinc locks. -@return TRUE if the transaction holds any AUTOINC locks. */ -static -ibool -lock_trx_holds_autoinc_locks( -/*=========================*/ - const trx_t* trx) /*!< in: transaction */ -{ - ut_a(trx->autoinc_locks != NULL); - - return(!ib_vector_is_empty(trx->autoinc_locks)); -} - /** Release all AUTO_INCREMENT locks of the transaction. */ static void lock_release_autoinc_locks(trx_t *trx) { { + auto begin= trx->autoinc_locks.begin(), end= trx->autoinc_locks.end(); + ut_ad(begin != end); LockMutexGuard g{SRW_LOCK_CALL}; mysql_mutex_lock(&lock_sys.wait_mutex); trx->mutex_lock(); - auto autoinc_locks= trx->autoinc_locks; - ut_a(autoinc_locks); /* We release the locks in the reverse order. This is to avoid searching the vector for the element to delete at the lower level. See (lock_table_remove_low()) for details. */ - while (ulint size= ib_vector_size(autoinc_locks)) + do { - lock_t *lock= *static_cast - (ib_vector_get(autoinc_locks, size - 1)); + lock_t *lock= *--end; ut_ad(lock->type_mode == (LOCK_AUTO_INC | LOCK_TABLE)); lock_table_dequeue(lock, true); lock_trx_table_locks_remove(lock); } + while (begin != end); } mysql_mutex_unlock(&lock_sys.wait_mutex); trx->mutex_unlock(); + trx->autoinc_locks.clear(); } /** Cancel a waiting lock request and release possibly waiting transactions */ @@ -6502,8 +6448,18 @@ void lock_cancel_waiting_and_release(lock_t *lock) { if (lock->type_mode == (LOCK_AUTO_INC | LOCK_TABLE)) { - ut_ad(trx->autoinc_locks); - ib_vector_remove(trx->autoinc_locks, lock); + /* This is similar to lock_table_remove_autoinc_lock() */ + auto begin= trx->autoinc_locks.begin(), end= trx->autoinc_locks.end(); + ut_ad(begin != end); + if (*--end == lock) + trx->autoinc_locks.erase(end, end + 1); + else + while (begin != end) + if (*--end == lock) + { + *end= nullptr; + break; + } } lock_table_dequeue(lock, true); /* Remove the lock from table lock vector too. */ @@ -6703,7 +6659,7 @@ lock_unlock_table_autoinc( ut_ad(trx_state == TRX_STATE_ACTIVE || trx_state == TRX_STATE_PREPARED || trx_state == TRX_STATE_NOT_STARTED); - if (lock_trx_holds_autoinc_locks(trx)) + if (!trx->autoinc_locks.empty()) lock_release_autoinc_locks(trx); } diff --git a/storage/innobase/mtr/mtr0mtr.cc b/storage/innobase/mtr/mtr0mtr.cc index bd0ef0eee1bbf..d8f694d80f052 100644 --- a/storage/innobase/mtr/mtr0mtr.cc +++ b/storage/innobase/mtr/mtr0mtr.cc @@ -1268,14 +1268,14 @@ void mtr_t::free(const fil_space_t &space, uint32_t offset) m_log.close(log_write(id, nullptr)); } -void small_vector_base::grow_by_1(void *small, size_t element_size) +void small_vector_base::grow_by_1(void *small, size_t element_size) noexcept { const size_t cap= Capacity*= 2, s= cap * element_size; void *new_begin; if (BeginX == small) { new_begin= my_malloc(PSI_NOT_INSTRUMENTED, s, MYF(0)); - memcpy(new_begin, BeginX, size() * element_size); + memcpy(new_begin, BeginX, s / 2); TRASH_FREE(small, size() * element_size); } else diff --git a/storage/innobase/trx/trx0trx.cc b/storage/innobase/trx/trx0trx.cc index 7935cdb6bc664..ade8dea929aef 100644 --- a/storage/innobase/trx/trx0trx.cc +++ b/storage/innobase/trx/trx0trx.cc @@ -170,6 +170,8 @@ struct TrxFactory { allocated object. trx_t objects are allocated by ut_zalloc_nokey() in Pool::Pool() which would not call the constructors of the trx_t members. */ + new(&trx->autoinc_locks) trx_t::autoinc_lock_vector(); + new(&trx->mod_tables) trx_mod_tables_t(); new(&trx->lock.table_locks) lock_list(); @@ -232,6 +234,8 @@ struct TrxFactory { trx->mutex_destroy(); + trx->autoinc_locks.~small_vector(); + trx->mod_tables.~trx_mod_tables_t(); ut_ad(!trx->read_view.is_open()); @@ -334,21 +338,12 @@ trx_t *trx_create() trx->assert_freed(); - mem_heap_t* heap; - ib_alloc_t* alloc; - /* We just got trx from pool, it should be non locking */ ut_ad(!trx->will_lock); ut_ad(!trx->rw_trx_hash_pins); DBUG_LOG("trx", "Create: " << trx); - heap = mem_heap_create(sizeof(ib_vector_t) + sizeof(void*) * 8); - - alloc = ib_heap_allocator_create(heap); - - trx->autoinc_locks = ib_vector_create(alloc, sizeof(void**), 4); - ut_ad(trx->mod_tables.empty()); ut_ad(trx->lock.n_rec_locks == 0); ut_ad(trx->lock.set_nth_bit_calls == 0); @@ -364,6 +359,7 @@ trx_t *trx_create() /** Free the memory to trx_pools */ void trx_t::free() { + autoinc_locks.fake_defined(); #ifdef HAVE_MEM_CHECK if (xid.is_null()) MEM_MAKE_DEFINED(&xid, sizeof xid); @@ -372,6 +368,7 @@ void trx_t::free() sizeof xid.data - (xid.gtrid_length + xid.bqual_length)); #endif MEM_CHECK_DEFINED(this, sizeof *this); + autoinc_locks.make_undefined(); ut_ad(!n_mysql_tables_in_use); ut_ad(!mysql_log_file_name); @@ -390,14 +387,7 @@ void trx_t::free() trx_sys.rw_trx_hash.put_pins(this); mysql_thd= nullptr; - // FIXME: We need to avoid this heap free/alloc for each commit. - if (autoinc_locks) - { - ut_ad(ib_vector_is_empty(autoinc_locks)); - /* We allocated a dedicated heap for the vector. */ - ib_vector_free(autoinc_locks); - autoinc_locks= NULL; - } + autoinc_locks.deep_clear(); MEM_NOACCESS(&skip_lock_inheritance_and_n_ref, sizeof skip_lock_inheritance_and_n_ref); @@ -495,7 +485,7 @@ inline void trx_t::release_locks() lock_release(this); ut_ad(!lock.n_rec_locks); ut_ad(UT_LIST_GET_LEN(lock.trx_locks) == 0); - ut_ad(ib_vector_is_empty(autoinc_locks)); + ut_ad(autoinc_locks.empty()); mem_heap_empty(lock.lock_heap); } @@ -923,7 +913,7 @@ trx_start_low( trx->wsrep = wsrep_on(trx->mysql_thd); #endif /* WITH_WSREP */ - ut_a(ib_vector_is_empty(trx->autoinc_locks)); + ut_a(trx->autoinc_locks.empty()); ut_a(trx->lock.table_locks.empty()); /* No other thread can access this trx object through rw_trx_hash, From 0abef37ccdba25a4a1e8e3209abd2be64ff1f19a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Wed, 15 Jan 2025 16:55:29 +0200 Subject: [PATCH 2/2] Minor lock_sys cleanup Let us make some member functions of lock_sys_t non-static to avoid some shuffling of function parameter registers. lock_cancel_waiting_and_release(): Declare static, because there are no external callers. Reviewed by: Debarun Banerjee --- storage/innobase/include/lock0lock.h | 4 +- storage/innobase/lock/lock0lock.cc | 80 ++++++++++++++-------------- 2 files changed, 42 insertions(+), 42 deletions(-) diff --git a/storage/innobase/include/lock0lock.h b/storage/innobase/include/lock0lock.h index c996e5f8227c4..a604b5ef7b505 100644 --- a/storage/innobase/include/lock0lock.h +++ b/storage/innobase/include/lock0lock.h @@ -869,7 +869,7 @@ class lock_sys_t @retval DB_DEADLOCK if trx->lock.was_chosen_as_deadlock_victim was set @retval DB_LOCK_WAIT if the lock was canceled */ template - static dberr_t cancel(trx_t *trx, lock_t *lock); + dberr_t cancel(trx_t *trx, lock_t *lock) noexcept; /** Note that a record lock wait started */ inline void wait_start(); @@ -924,7 +924,7 @@ class lock_sys_t void prdt_page_free_from_discard(const page_id_t id, bool all= false); /** Cancel possible lock waiting for a transaction */ - static void cancel_lock_wait_for_trx(trx_t *trx); + inline void cancel_lock_wait_for_trx(trx_t *trx) noexcept; #ifdef WITH_WSREP /** Cancel lock waiting for a wsrep BF abort. */ static void cancel_lock_wait_for_wsrep_bf_abort(trx_t *trx); diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc index 5e6ed1502af8b..94eeef57bd73f 100644 --- a/storage/innobase/lock/lock0lock.cc +++ b/storage/innobase/lock/lock0lock.cc @@ -2232,7 +2232,7 @@ dberr_t lock_wait(que_thr_t *thr) if (wait_lock) { abort_wait: - lock_sys_t::cancel(trx, wait_lock); + lock_sys.cancel(trx, wait_lock); lock_sys.deadlock_check(); } @@ -6431,7 +6431,7 @@ static void lock_release_autoinc_locks(trx_t *trx) /** Cancel a waiting lock request and release possibly waiting transactions */ template -void lock_cancel_waiting_and_release(lock_t *lock) +static void lock_cancel_waiting_and_release(lock_t *lock) noexcept { lock_sys.assert_locked(*lock); mysql_mutex_assert_owner(&lock_sys.wait_mutex); @@ -6475,18 +6475,18 @@ void lock_cancel_waiting_and_release(lock_t *lock) trx->mutex_unlock(); } -void lock_sys_t::cancel_lock_wait_for_trx(trx_t *trx) +inline void lock_sys_t::cancel_lock_wait_for_trx(trx_t *trx) noexcept { - lock_sys.wr_lock(SRW_LOCK_CALL); - mysql_mutex_lock(&lock_sys.wait_mutex); + wr_lock(SRW_LOCK_CALL); + mysql_mutex_lock(&wait_mutex); if (lock_t *lock= trx->lock.wait_lock) { /* check if victim is still waiting */ if (lock->is_waiting()) lock_cancel_waiting_and_release(lock); } - lock_sys.wr_unlock(); - mysql_mutex_unlock(&lock_sys.wait_mutex); + wr_unlock(); + mysql_mutex_unlock(&wait_mutex); } #ifdef WITH_WSREP @@ -6510,10 +6510,10 @@ void lock_sys_t::cancel_lock_wait_for_wsrep_bf_abort(trx_t *trx) @retval DB_DEADLOCK if trx->lock.was_chosen_as_deadlock_victim was set @retval DB_LOCK_WAIT if the lock was canceled */ template -dberr_t lock_sys_t::cancel(trx_t *trx, lock_t *lock) +dberr_t lock_sys_t::cancel(trx_t *trx, lock_t *lock) noexcept { DEBUG_SYNC_C("lock_sys_t_cancel_enter"); - mysql_mutex_assert_owner(&lock_sys.wait_mutex); + mysql_mutex_assert_owner(&wait_mutex); ut_ad(trx->state == TRX_STATE_ACTIVE); /* trx->lock.wait_lock may be changed by other threads as long as we are not holding lock_sys.latch. @@ -6521,27 +6521,27 @@ dberr_t lock_sys_t::cancel(trx_t *trx, lock_t *lock) So, trx->lock.wait_lock==lock does not necessarily hold, but both pointers should be valid, because other threads cannot assign trx->lock.wait_lock=nullptr (or invalidate *lock) while we are - holding lock_sys.wait_mutex. Also, the type of trx->lock.wait_lock + holding wait_mutex. Also, the type of trx->lock.wait_lock (record or table lock) cannot be changed by other threads. So, it is - safe to call lock->is_table() while not holding lock_sys.latch. If - we have to release and reacquire lock_sys.wait_mutex, we must reread + safe to call lock->is_table() while not holding latch. If + we have to release and reacquire wait_mutex, we must reread trx->lock.wait_lock. We must also reread trx->lock.wait_lock after - lock_sys.latch acquiring, as it can be changed to not-null in lock moving - functions even if we hold lock_sys.wait_mutex. */ + latch acquiring, as it can be changed to not-null in lock moving + functions even if we hold wait_mutex. */ dberr_t err= DB_SUCCESS; /* This would be too large for a memory transaction, except in the DB_DEADLOCK case, which was already tested in lock_trx_handle_wait(). */ if (lock->is_table()) { - if (!lock_sys.rd_lock_try()) + if (!rd_lock_try()) { - mysql_mutex_unlock(&lock_sys.wait_mutex); - lock_sys.rd_lock(SRW_LOCK_CALL); - mysql_mutex_lock(&lock_sys.wait_mutex); + mysql_mutex_unlock(&wait_mutex); + rd_lock(SRW_LOCK_CALL); + mysql_mutex_lock(&wait_mutex); lock= trx->lock.wait_lock; - /* Even if waiting lock was cancelled while lock_sys.wait_mutex was - unlocked, we need to return deadlock error if transaction was chosen - as deadlock victim to rollback it */ + /* Even if the waiting lock was cancelled while we did not hold + wait_mutex, we need to return deadlock error if the transaction + was chosen as deadlock victim to be rolled back. */ if (check_victim && trx->lock.was_chosen_as_deadlock_victim) err= DB_DEADLOCK; else if (lock) @@ -6552,10 +6552,10 @@ dberr_t lock_sys_t::cancel(trx_t *trx, lock_t *lock) /* This function is invoked from the thread which executes the transaction. Table locks are requested before record locks. Some other transaction can't change trx->lock.wait_lock from table to record for the - current transaction at this point, because the current transaction has not - requested record locks yet. There is no need to move any table locks by - other threads. And trx->lock.wait_lock can't be set to null while we are - holding lock_sys.wait_mutex. That's why there is no need to reload + current transaction at this point, because the current transaction has + not requested record locks yet. There is no need to move any table locks + by other threads. And trx->lock.wait_lock can't be set to null while we + are holding wait_mutex. That's why there is no need to reload trx->lock.wait_lock here. */ ut_ad(lock == trx->lock.wait_lock); resolve_table_lock: @@ -6563,11 +6563,11 @@ dberr_t lock_sys_t::cancel(trx_t *trx, lock_t *lock) if (!table->lock_mutex_trylock()) { /* The correct latching order is: - lock_sys.latch, table->lock_latch, lock_sys.wait_mutex. - Thus, we must release lock_sys.wait_mutex for a blocking wait. */ - mysql_mutex_unlock(&lock_sys.wait_mutex); + latch, table->lock_latch, wait_mutex. + Thus, we must release wait_mutex for a blocking wait. */ + mysql_mutex_unlock(&wait_mutex); table->lock_mutex_lock(); - mysql_mutex_lock(&lock_sys.wait_mutex); + mysql_mutex_lock(&wait_mutex); /* Cache trx->lock.wait_lock under the corresponding latches. */ lock= trx->lock.wait_lock; if (!lock) @@ -6594,20 +6594,20 @@ dberr_t lock_sys_t::cancel(trx_t *trx, lock_t *lock) retreat: table->lock_mutex_unlock(); } - lock_sys.rd_unlock(); + rd_unlock(); } else { /* To prevent the record lock from being moved between pages - during a page split or merge, we must hold exclusive lock_sys.latch. */ - if (!lock_sys.wr_lock_try()) + during a page split or merge, we must hold exclusive latch. */ + if (!wr_lock_try()) { - mysql_mutex_unlock(&lock_sys.wait_mutex); - lock_sys.wr_lock(SRW_LOCK_CALL); - mysql_mutex_lock(&lock_sys.wait_mutex); + mysql_mutex_unlock(&wait_mutex); + wr_lock(SRW_LOCK_CALL); + mysql_mutex_lock(&wait_mutex); /* Cache trx->lock.wait_lock under the corresponding latches. */ lock= trx->lock.wait_lock; - /* Even if waiting lock was cancelled while lock_sys.wait_mutex was + /* Even if waiting lock was cancelled while wait_mutex was unlocked, we need to return deadlock error if transaction was chosen as deadlock victim to rollback it */ if (check_victim && trx->lock.was_chosen_as_deadlock_victim) @@ -6631,13 +6631,13 @@ dberr_t lock_sys_t::cancel(trx_t *trx, lock_t *lock) rpl.rpl_parallel_optimistic_xa_lsu_off */ err= DB_LOCK_WAIT; } - lock_sys.wr_unlock(); + wr_unlock(); } return err; } -template dberr_t lock_sys_t::cancel(trx_t *, lock_t *); +template dberr_t lock_sys_t::cancel(trx_t *, lock_t *) noexcept; /*********************************************************************//** Unlocks AUTO_INC type locks that were possibly reserved by a trx. This @@ -6689,7 +6689,7 @@ dberr_t lock_trx_handle_wait(trx_t *trx) err= DB_DEADLOCK; /* Cache trx->lock.wait_lock to avoid unnecessary atomic variable load */ else if (lock_t *wait_lock= trx->lock.wait_lock) - err= lock_sys_t::cancel(trx, wait_lock); + err= lock_sys.cancel(trx, wait_lock); lock_sys.deadlock_check(); mysql_mutex_unlock(&lock_sys.wait_mutex); return err; @@ -7153,7 +7153,7 @@ static lock_t *Deadlock::check_and_resolve(trx_t *trx, lock_t *wait_lock) return wait_lock; if (wait_lock) - lock_sys_t::cancel(trx, wait_lock); + lock_sys.cancel(trx, wait_lock); lock_sys.deadlock_check(); return reinterpret_cast(-1);