Hi, Sergey, I think it's fine. I had a few questions though, see below:
commit 47aa5f9 Author: Sergey Vojtovich <svoj@mariadb.org> Date: Fri May 6 13:44:07 2016 +0400
MDEV-7660 - MySQL WL#6671 "Improve scalability by not using thr_lock.c locks for InnoDB tables"
Don't use thr_lock.c locks for InnoDB tables.
Let HANDLER READ call external_lock() even if SE is not going to be locked by THR_LOCK. This fixes at least main.implicit_commit failure.
Removed tests for BUG#45143 and BUG#55930 which cover InnoDB + THR_LOCK. To operate properly these tests require code flow to go through THR_LOCK debug sync points, which is not the case after this patch. These tests are removed by WL#6671 as well. An alternative is to port them to different storage engine.
For the very same reason partition_debug_sync test was adjusted to use MyISAM.
diff --git a/sql/sql_handler.cc b/sql/sql_handler.cc index e8ade81..76107ae 100644 --- a/sql/sql_handler.cc +++ b/sql/sql_handler.cc @@ -752,11 +752,12 @@ bool mysql_ha_read(THD *thd, TABLE_LIST *tables, tables->table= table; // This is used by fix_fields table->pos_in_table_list= tables;
- if (handler->lock->lock_count > 0) + if (handler->lock->table_count > 0) { int lock_error;
- handler->lock->locks[0]->type= handler->lock->locks[0]->org_type; + if (handler->lock->lock_count > 0) + handler->lock->locks[0]->type= handler->lock->locks[0]->org_type;
I don't understand this code in mysql_ha_read() at all :( even before your changes
/* save open_tables state */ TABLE* backup_open_tables= thd->open_tables; diff --git a/storage/xtradb/handler/ha_innodb.h b/storage/xtradb/handler/ha_innodb.h index 2027a59..efb8120 100644 --- a/storage/xtradb/handler/ha_innodb.h +++ b/storage/xtradb/handler/ha_innodb.h @@ -218,6 +218,7 @@ class ha_innobase: public handler bool can_switch_engines(); uint referenced_by_foreign_key(); void free_foreign_key_create_info(char* str); + uint lock_count(void) const; THR_LOCK_DATA **store_lock(THD *thd, THR_LOCK_DATA **to, enum thr_lock_type lock_type); void init_table_handle_for_HANDLER();
commit 5645626 Author: Sergey Vojtovich <svoj@mariadb.org> Date: Tue May 24 12:25:56 2016 +0400
MDEV-7660 - MySQL WL#6671 "Improve scalability by not using thr_lock.c locks for InnoDB tables"
- InnoDB now acquires shared lock for HANDLER ... READ
Why?
- LOCK TABLES now disables autocommit implicitely - UNLOCK TABLES now re-enables autocommit implicitely if it was disabled by LOCK TABLES - adjusted test cases to this new behavior
diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 3091bd6..7d39484 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -2824,6 +2824,8 @@ Locked_tables_list::unlock_locked_tables(THD *thd) request for metadata locks and TABLE_LIST elements. */ reset(); + if (thd->variables.option_bits & OPTION_AUTOCOMMIT) + thd->variables.option_bits&= ~(OPTION_NOT_AUTOCOMMIT);
1. Was it possible - before your change - for OPTION_AUTOCOMMIT and OPTION_NOT_AUTOCOMMIT to be out of sync? 2. What if someone changes @@autocommit under LOCK TABLES? Do you have a test for that? 3. Do you need to set SERVER_STATUS_AUTOCOMMIT here?
}
diff --git a/mysql-test/t/innodb_mysql_lock.test b/mysql-test/t/innodb_mysql_lock.test index cb57c09..85ba418 100644 --- a/mysql-test/t/innodb_mysql_lock.test +++ b/mysql-test/t/innodb_mysql_lock.test @@ -150,14 +150,16 @@ let $wait_condition= --source include/wait_condition.inc LOCK TABLES t1 READ; SELECT release_lock('bug42147_lock'); +let $wait_condition= + SELECT COUNT(*) > 0 FROM information_schema.processlist + WHERE state = 'executing' + AND info = 'INSERT INTO t1 SELECT get_lock(\'bug42147_lock\', 60)'; +--source include/wait_condition.inc +UNLOCK TABLES;
I don't understand the original test case. But after your changes it actually makes sense :)
connection default; --reap
-connection con2; -UNLOCK TABLES; - -connection default; disconnect con2; DROP TABLE t1;
diff --git a/mysql-test/r/partition_explicit_prune.result b/mysql-test/r/partition_explicit_prune.result index 765803d..7b9c53d 100644 --- a/mysql-test/r/partition_explicit_prune.result +++ b/mysql-test/r/partition_explicit_prune.result @@ -281,7 +281,7 @@ UNLOCK TABLES; SELECT * FROM INFORMATION_SCHEMA.SESSION_STATUS WHERE VARIABLE_NAME LIKE 'HANDLER_%' AND VARIABLE_VALUE > 0; VARIABLE_NAME VARIABLE_VALUE -HANDLER_COMMIT 2 +HANDLER_COMMIT 3
why is that?
HANDLER_READ_RND_NEXT 52 HANDLER_TMP_WRITE 72 HANDLER_WRITE 2 diff --git a/mysql-test/suite/handler/handler.inc b/mysql-test/suite/handler/handler.inc index b1e881f..8cad6a5 100644 --- a/mysql-test/suite/handler/handler.inc +++ b/mysql-test/suite/handler/handler.inc @@ -1091,6 +1091,12 @@ connection default; --reap drop table t2;
+# This test expects "handler t1 read a next" to get blocked on table level +# lock so that further "drop table t1" can break the lock and close handler. +# This notification mechanism doesn't work with InnoDB since it bypasses +# table level locks.
what happens for InnoDB then? I'd expect "handler t1 read a next" to get blocked inside the InnoDB. what does then "drop table t1" do?
+if ($engine_type != 'InnoDB') +{ --echo # --echo # Bug #46224 HANDLER statements within a transaction might --echo # lead to deadlocks
Regards, Sergei Chief Architect MariaDB and security@mariadb.org