From 040069f4baead789bcb9dd55bb4932f6d1388d7c Mon Sep 17 00:00:00 2001 From: mariadb-DebarunBanerjee Date: Wed, 17 Apr 2024 15:16:50 +0530 Subject: [PATCH] MDEV-33431 Latching order violation reported fil_system.sys_space.latch and ibuf_pessimistic_insert_mutex Issue: ------ The actual order of acquisition of the IBUF pessimistic insert mutex (SYNC_IBUF_PESS_INSERT_MUTEX) and IBUF header page latch (SYNC_IBUF_HEADER) w.r.t space latch (SYNC_FSP) differs from the order defined in sync0types.h. It was not discovered earlier as the path to ibuf_remove_free_page was not covered by the mtr test. Ideal order and one defined in sync0types.h is as follows. SYNC_IBUF_HEADER -> SYNC_IBUF_PESS_INSERT_MUTEX -> SYNC_FSP In ibuf_remove_free_page, we acquire space latch earlier and we have the order as follows resulting in the assert with innodb_sync_debug=on. SYNC_FSP -> SYNC_IBUF_HEADER -> SYNC_IBUF_PESS_INSERT_MUTEX Fix: --- We do maintain this order in other places and there doesn't seem to be any real issue here. To reduce impact in GA versions, we avoid doing extensive changes in mutex ordering to match the current SYNC_IBUF_PESS_INSERT_MUTEX order. Instead we relax the ordering check for IBUF pessimistic insert mutex using SYNC_NO_ORDER_CHECK. --- .../suite/innodb/r/change_buffer_free.result | 32 +++++++++++ .../suite/innodb/t/change_buffer_free.opt | 2 + .../suite/innodb/t/change_buffer_free.test | 54 +++++++++++++++++++ storage/innobase/ibuf/ibuf0ibuf.cc | 5 ++ storage/innobase/sync/sync0debug.cc | 19 ++++++- 5 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 mysql-test/suite/innodb/r/change_buffer_free.result create mode 100644 mysql-test/suite/innodb/t/change_buffer_free.opt create mode 100644 mysql-test/suite/innodb/t/change_buffer_free.test diff --git a/mysql-test/suite/innodb/r/change_buffer_free.result b/mysql-test/suite/innodb/r/change_buffer_free.result new file mode 100644 index 00000000000..a22ed16917e --- /dev/null +++ b/mysql-test/suite/innodb/r/change_buffer_free.result @@ -0,0 +1,32 @@ +SET @saved_change_buffering = @@GLOBAL.innodb_change_buffering; +SET @saved_file_per_table = @@GLOBAL.innodb_file_per_table; +SET @saved_change_buffering_debug = @@GLOBAL.innodb_change_buffering_debug; +SET GLOBAL innodb_change_buffering = NONE; +SET GLOBAL innodb_file_per_table = OFF; +CREATE TABLE t2(c1 INT AUTO_INCREMENT PRIMARY KEY,c2 CHAR(100))ENGINE=InnoDB; +CREATE INDEX i1 ON t2 (c2); +INSERT INTO t2(c2) VALUES('mariadb'); +INSERT INTO t2(c2) SELECT c2 FROM t2; +INSERT INTO t2(c2) SELECT c2 FROM t2; +INSERT INTO t2(c2) SELECT c2 FROM t2; +INSERT INTO t2(c2) SELECT c2 FROM t2; +INSERT INTO t2(c2) SELECT c2 FROM t2; +CREATE TABLE t1(c1 INT AUTO_INCREMENT PRIMARY KEY,c2 CHAR(100))ENGINE=InnoDB; +CREATE INDEX i1 ON t1 (c2); +INSERT INTO t1(c2) VALUES('mariadb'); +INSERT INTO t1(c2) SELECT c2 FROM t1; +INSERT INTO t1(c2) SELECT c2 FROM t1; +INSERT INTO t1(c2) SELECT c2 FROM t1; +INSERT INTO t1(c2) SELECT c2 FROM t1; +INSERT INTO t1(c2) SELECT c2 FROM t1; +SET GLOBAL innodb_change_buffering = all; +SET GLOBAL innodb_change_buffering_debug = 1; +SET DEBUG_DBUG='+d,ibuf_force_remove_free_page'; +INSERT INTO t2(c2) SELECT c2 FROM t2 IGNORE INDEX (i1) LIMIT 4; +INSERT INTO t1(c2) SELECT c2 FROM t1 IGNORE INDEX (i1) LIMIT 4; +SET DEBUG_DBUG='-d,ibuf_force_remove_free_page'; +SET GLOBAL innodb_change_buffering_debug = @saved_change_buffering_debug; +SET GLOBAL innodb_change_buffering = @saved_change_buffering; +DROP TABLE t2; +DROP TABLE t1; +SET GLOBAL innodb_file_per_table = @saved_file_per_table; diff --git a/mysql-test/suite/innodb/t/change_buffer_free.opt b/mysql-test/suite/innodb/t/change_buffer_free.opt new file mode 100644 index 00000000000..296def4f05a --- /dev/null +++ b/mysql-test/suite/innodb/t/change_buffer_free.opt @@ -0,0 +1,2 @@ +--innodb-page-size=4k +--innodb_sync_debug=on diff --git a/mysql-test/suite/innodb/t/change_buffer_free.test b/mysql-test/suite/innodb/t/change_buffer_free.test new file mode 100644 index 00000000000..0db9e3186ae --- /dev/null +++ b/mysql-test/suite/innodb/t/change_buffer_free.test @@ -0,0 +1,54 @@ +# +# MDEV 33431: Latching order violation reported fil_system.sys_space.latch and ibuf_pessimistic_insert_mutex +# +--source include/have_innodb.inc +--source include/have_debug.inc +--source include/have_debug_sync.inc + +SET @saved_change_buffering = @@GLOBAL.innodb_change_buffering; +SET @saved_file_per_table = @@GLOBAL.innodb_file_per_table; +SET @saved_change_buffering_debug = @@GLOBAL.innodb_change_buffering_debug; + +SET GLOBAL innodb_change_buffering = NONE; +SET GLOBAL innodb_file_per_table = OFF; + +let $loop=2; +while ($loop) +{ + eval CREATE TABLE t$loop(c1 INT AUTO_INCREMENT PRIMARY KEY,c2 CHAR(100))ENGINE=InnoDB; + eval CREATE INDEX i1 ON t$loop (c2); + + eval INSERT INTO t$loop(c2) VALUES('mariadb'); + eval INSERT INTO t$loop(c2) SELECT c2 FROM t$loop; + eval INSERT INTO t$loop(c2) SELECT c2 FROM t$loop; + eval INSERT INTO t$loop(c2) SELECT c2 FROM t$loop; + eval INSERT INTO t$loop(c2) SELECT c2 FROM t$loop; + eval INSERT INTO t$loop(c2) SELECT c2 FROM t$loop; + + dec $loop; +} + +SET GLOBAL innodb_change_buffering = all; +SET GLOBAL innodb_change_buffering_debug = 1; + +SET DEBUG_DBUG='+d,ibuf_force_remove_free_page'; +let $loop=2; + +while ($loop) +{ + eval INSERT INTO t$loop(c2) SELECT c2 FROM t$loop IGNORE INDEX (i1) LIMIT 4; + dec $loop; +} +SET DEBUG_DBUG='-d,ibuf_force_remove_free_page'; + +SET GLOBAL innodb_change_buffering_debug = @saved_change_buffering_debug; +SET GLOBAL innodb_change_buffering = @saved_change_buffering; + +let $loop=2; +while ($loop) +{ + eval DROP TABLE t$loop; + dec $loop; +} + +SET GLOBAL innodb_file_per_table = @saved_file_per_table; diff --git a/storage/innobase/ibuf/ibuf0ibuf.cc b/storage/innobase/ibuf/ibuf0ibuf.cc index cdb4967b663..8888e538e05 100644 --- a/storage/innobase/ibuf/ibuf0ibuf.cc +++ b/storage/innobase/ibuf/ibuf0ibuf.cc @@ -2011,6 +2011,11 @@ ibuf_free_excess_pages(void) /* Free at most a few pages at a time, so that we do not delay the requested service too much */ + DBUG_EXECUTE_IF("ibuf_force_remove_free_page", + ibuf_remove_free_page(); + return; + ); + for (ulint i = 0; i < 4; i++) { ibool too_much_free; diff --git a/storage/innobase/sync/sync0debug.cc b/storage/innobase/sync/sync0debug.cc index dd40e95dd50..78f6028b60d 100644 --- a/storage/innobase/sync/sync0debug.cc +++ b/storage/innobase/sync/sync0debug.cc @@ -1208,7 +1208,24 @@ sync_latch_meta_init() LATCH_ADD_MUTEX(IBUF, SYNC_IBUF_MUTEX, ibuf_mutex_key); - LATCH_ADD_MUTEX(IBUF_PESSIMISTIC_INSERT, SYNC_IBUF_PESS_INSERT_MUTEX, + /* The actual order of acquisition of the IBUF pessimistic insert mutex + (SYNC_IBUF_PESS_INSERT_MUTEX) and IBUF header page latch + (SYNC_IBUF_HEADER) w.r.t space latch (SYNC_FSP) differs from the order + defined in sync0types.h. It was not discovered earlier as the path to + ibuf_remove_free_page was not covered by the mtr test. Ideal order and + one defined in sync0types.h is as follows. + SYNC_IBUF_HEADER -> SYNC_IBUF_PESS_INSERT_MUTEX -> SYNC_FSP + + In ibuf_remove_free_page, we acquire space latch earlier and we have + the order as follows resulting in the assert with innodb_sync_debug=on. + SYNC_FSP -> SYNC_IBUF_HEADER -> SYNC_IBUF_PESS_INSERT_MUTEX + + We do maintain this order in other places and there doesn't seem to be + any real issue here. To reduce impact in GA versions, we avoid doing + extensive changes in mutex ordering to match the current + SYNC_IBUF_PESS_INSERT_MUTEX order. Instead we relax the ordering check + for IBUF pessimistic insert mutex using SYNC_NO_ORDER_CHECK. */ + LATCH_ADD_MUTEX(IBUF_PESSIMISTIC_INSERT, SYNC_NO_ORDER_CHECK, ibuf_pessimistic_insert_mutex_key); LATCH_ADD_MUTEX(PURGE_SYS_PQ, SYNC_PURGE_QUEUE,