Hi, Sergey! On Jun 01, Sergey Vojtovich wrote:
On Tue, May 31, 2016 at 06:48:31PM +0200, Sergei Golubchik wrote:
I think it's fine. I had a few questions though, see below:
Thanks for positive feedback. As I mentioned before, I'm less optimistic about this patch due to many behavior changes. I can try to make a list if you like.
Yes, please!
--- 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;
I don't understand this code in mysql_ha_read() at all :( even before your changes I guess you're referring to org_type. I'm not sure I understand it either: looks like it is needed to let HANDLER READ use the same lock type as it got during HANDLER OPEN. I have no idea why is it needed.
This particular change was needed so that mysql_lock_tables()/external_lock() is called even if we got 0 from lock_count().
Yes, your change was clear enough.
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? It is about change in ha_innobase::init_table_handle_for_HANDLER(). To isolate HANDLER READ from LOCK TABLES WRITE.
But should it be done? SELECT works without locks, why should HANDLER be different? On the other hand, I don't have strong preference for either case. Do whatever you think is better.
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 :) Previously "INSERT INTO t1 SELECT get_lock('bug42147_lock', 60)" was able to complete while concurrent thread was holding "LOCK TABLES t1 READ".
Exactly! This didn't make much sense, it actually looked like a bug.
Now it's blocked and I had to move "UNLOCK TABLES" before reap.
Which is good.
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. Yes, that's exactly what happens.
what does then "drop table t1" do? It gets blocked too, because it can't abort InnoDB locks.
Is drop blocked by MDL? Is it a deadlock then? Regards, Sergei Chief Architect MariaDB and security@mariadb.org