[Maria-developers] MDEV-7660 MySQL WL#6671 "Improve scalability by not using thr_lock.c locks for InnoDB tables"
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
Hi, Sergey,
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
Hi Sergei, On Tue, May 31, 2016 at 06:48:31PM +0200, Sergei Golubchik wrote: this patch due to many behavior changes. I can try to make a list if you like.
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
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().
/* 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?
It is about change in ha_innobase::init_table_handle_for_HANDLER(). To isolate HANDLER READ from LOCK TABLES WRITE. It is InnoDB-side implementation of MDEV-7895.
- 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?
No.
2. What if someone changes @@autocommit under LOCK TABLES? Do you have a test for that?
Commit transaction, release locks. I couldn't immediately find test for that, but there're tests for LOCK TABLES + COMMIT. More interesting to see what shall happen if we lock InnoDB + MyISAM tables and then do commit/rollback/set @@autocommit. I'll check that.
3. Do you need to set SERVER_STATUS_AUTOCOMMIT here?
Yes, I likely missed it. Good catch!
}
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". Now it's blocked and I had to move "UNLOCK TABLES" before reap.
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?
Because a few lines above we do UNLOCK TABLES, which does commit now.
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.
Yes, that's exactly what happens.
what does then "drop table t1" do? It gets blocked too, because it can't abort InnoDB locks.
+if ($engine_type != 'InnoDB') +{ --echo # --echo # Bug #46224 HANDLER statements within a transaction might --echo # lead to deadlocks
Thanks, Sergey
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
Hi Sergei,
On Wed, Jun 01, 2016 at 08:39:36AM +0200, Sergei Golubchik wrote:
> 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!
- InnoDB lock wait timeouts are now honored which are much shorter by default
than server lock wait timeouts (1 year vs 50 seconds)
- with @@autocommit= 1 LOCK TABLES disables autocommit implicitely, though
user still sees @@autocommt= 1
- the above starts implicit transaction
- transactions started by LOCK TABLES are now rolled back on disconnect
(previously everything was committed due to autocommit)
- transactions started by LOCK TABLES are now rolled back by ROLLBACK
(previously everything was committed due to autocommit)
- it is now impossible to change BINLOG_FORMAT under LOCK TABLES (at least
to statement) due to running transaction
- LOCK TABLES WRITE is additionally handled by MDL, except for HANDLER READ
(which causes deadlock)
- the above makes impossible to break HANDLER READ lock
- ...in contrast LOCK TABLES READ protection against DML is pure InnoDB
- combining transactional and non-transactional tables under LOCK TABLES
may cause rolled back changes in transactional table and "committed"
changes in non-transactional table
- user may disable innodb_table_locks, which will cause LOCK TABLES to be
noop basically
There're likely more minor behavior changes that I missed.
> > > > 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.
LOCK TABLES WRITE acquires MDL_SHARED_NO_READ_WRITE
SELECT acquires MDL_SHARED read (incompatible with MDL_SHARED_NO_READ_WRITE)
HANDLER READ acquires no MDL lock
In other words SELECT is isolated from LOCK TABLES WRITE on MDL level, while
HANDLER READ was isolated on THR_LOCK level. Now HANDLER READ is isolated on
InnoDB level.
> > > > 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.
I supposed there's a bug in THR_LOCK still.
>
> > > > 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?
Yes. :(
DROP TABLE is waiting for open handler to get closed, while HANDLER READ
is waiting for LOCK TABLES.
Below is simplified test that gets stuck on "drop table":
--source include/have_xtradb.inc
create table t1 (a int, key using btree (a)) engine=InnoDB;
lock tables t1 write;
connect(con1, localhost, root);
handler t1 open;
--send handler t1 read a next
connection default;
--sleep 1
drop table t1;
Regards,
Sergey
Hi, Sergey! Here's a review of the last two commits in bb-10.2-mdev7660 (I've quickly looked through other, cleanup, commits, didn't have any comments, so I've excluded them from the diff to make it smaller)
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;
1. What did this test do? 2. What does this 'executing' mean? Really executing or waiting inside InnoDB on a lock?
connection default; --reap
-connection con2; -UNLOCK TABLES; - -connection default; disconnect con2; DROP TABLE t1;
diff --git a/mysql-test/t/lock_sync.test b/mysql-test/t/lock_sync.test index c090e3a..07c16ac 100644 --- a/mysql-test/t/lock_sync.test +++ b/mysql-test/t/lock_sync.test @@ -873,116 +879,6 @@ set @@global.concurrent_insert= @old_concurrent_insert;
--echo # ---echo # Test for bug #45143 "All connections hang on concurrent ALTER TABLE". ---echo # ---echo # Concurrent execution of statements which required weak write lock ---echo # (TL_WRITE_ALLOW_WRITE) on several instances of the same table and ---echo # statements which tried to acquire stronger write lock (TL_WRITE, ---echo # TL_WRITE_ALLOW_READ) on this table might have led to deadlock.
Is this test no longer applicable? Why?
-# -# Suppress warnings for INSERTs that use get_lock(). -# -disable_query_log; -call mtr.add_suppression("Unsafe statement written to the binary log using statement format since BINLOG_FORMAT = STATEMENT"); -enable_query_log; - ---disable_warnings -drop table if exists t1; -drop view if exists v1; ---enable_warnings ---echo # Create auxiliary connections used through the test. -connect (con_bug45143_1,localhost,root,,test,,); -connect (con_bug45143_3,localhost,root,,test,,); -connect (con_bug45143_2,localhost,root,,test,,); -connection default; ---echo # Reset DEBUG_SYNC facility before using it. -set debug_sync= 'RESET'; ---echo # Turn off logging so calls to locking subsystem performed ---echo # for general_log table won't interfere with our test. -set @old_general_log = @@global.general_log; -set @@global.general_log= OFF; - -create table t1 (i int) engine=InnoDB; ---echo # We have to use view in order to make LOCK TABLES avoid ---echo # acquiring SNRW metadata lock on table. -create view v1 as select * from t1; -insert into t1 values (1); ---echo # Prepare user lock which will be used for resuming execution of ---echo # the first statement after it acquires TL_WRITE_ALLOW_WRITE lock. -select get_lock("lock_bug45143_wait", 0); - -connection con_bug45143_1; ---echo # Sending: ---send insert into t1 values (get_lock("lock_bug45143_wait", 100)); - -connection con_bug45143_2; ---echo # Wait until the above INSERT takes TL_WRITE_ALLOW_WRITE lock on 't1' ---echo # and then gets blocked on user lock 'lock_bug45143_wait'. -let $wait_condition= select count(*)= 1 from information_schema.processlist - where state= 'User lock' and - info='insert into t1 values (get_lock("lock_bug45143_wait", 100))'; ---source include/wait_condition.inc ---echo # Ensure that upcoming SELECT waits after acquiring TL_WRITE_ALLOW_WRITE ---echo # lock for the first instance of 't1'. -set debug_sync='thr_multi_lock_after_thr_lock SIGNAL parked WAIT_FOR go'; ---echo # Sending: ---send select count(*) > 0 from t1 as a, t1 as b for update; - -connection con_bug45143_3; ---echo # Wait until the above SELECT ... FOR UPDATE is blocked after ---echo # acquiring lock for the the first instance of 't1'. -set debug_sync= 'now WAIT_FOR parked'; ---echo # Send LOCK TABLE statement which will try to get TL_WRITE lock on 't1': ---send lock table v1 write; - -connection default; ---echo # Wait until this LOCK TABLES statement starts waiting for table lock. -let $wait_condition= select count(*)= 1 from information_schema.processlist - where state= 'Waiting for table level lock' and - info='lock table v1 write'; ---source include/wait_condition.inc ---echo # Allow SELECT ... FOR UPDATE to resume. ---echo # Since it already has TL_WRITE_ALLOW_WRITE lock on the first instance ---echo # of 't1' it should be able to get lock on the second instance without ---echo # waiting, even although there is another thread which has such lock ---echo # on this table and also there is a thread waiting for a TL_WRITE on it. -set debug_sync= 'now SIGNAL go'; - -connection con_bug45143_2; ---echo # Reap SELECT ... FOR UPDATE ---reap - -connection default; ---echo # Resume execution of the INSERT statement. -select release_lock("lock_bug45143_wait"); - -connection con_bug45143_1; ---echo # Reap INSERT statement. ---echo # In Statement and Mixed replication mode we get here "Unsafe ---echo # for binlog" warnings. In row mode there are no warnings. ---echo # Hide the discrepancy. ---disable_warnings ---reap ---enable_warnings - - -connection con_bug45143_3; ---echo # Reap LOCK TABLES statement. ---reap -unlock tables; - -connection default; ---echo # Do clean-up. -disconnect con_bug45143_1; -disconnect con_bug45143_2; -disconnect con_bug45143_3; -set debug_sync= 'RESET'; -set @@global.general_log= @old_general_log; -drop view v1; -drop table t1; - - ---echo # --echo # Bug#50821 Deadlock between LOCK TABLES and ALTER TABLE --echo #
@@ -1051,55 +947,6 @@ SET DEBUG_SYNC="RESET";
--echo # ---echo # Bug#55930 Assertion `thd->transaction.stmt.is_empty() || ---echo # thd->in_sub_stmt || (thd->state.. ---echo #
what about this one?
---disable_warnings -DROP TABLE IF EXISTS t1; ---enable_warnings - -CREATE TABLE t1(a INT) engine=InnoDB; -INSERT INTO t1 VALUES (1), (2); - -connect (con1, localhost, root); -connect (con2, localhost, root); - -connection con1; -SET SESSION lock_wait_timeout= 1; -SET DEBUG_SYNC= 'ha_admin_open_ltable SIGNAL opti_recreate WAIT_FOR opti_analyze'; ---echo # Sending: ---send OPTIMIZE TABLE t1 - -connection con2; -SET DEBUG_SYNC= 'now WAIT_FOR opti_recreate'; -SET DEBUG_SYNC= 'after_lock_tables_takes_lock SIGNAL thrlock WAIT_FOR release_thrlock'; ---echo # Sending: ---send INSERT INTO t1 VALUES (3) - -connection default; -SET DEBUG_SYNC= 'now WAIT_FOR thrlock'; -SET DEBUG_SYNC= 'now SIGNAL opti_analyze'; - -connection con1; ---echo # Reaping: OPTIMIZE TABLE t1 ---reap -SET DEBUG_SYNC= 'now SIGNAL release_thrlock'; -disconnect con1; ---source include/wait_until_disconnected.inc - -connection con2; ---echo # Reaping: INSERT INTO t1 VALUES (3) ---reap -disconnect con2; ---source include/wait_until_disconnected.inc - -connection default; -DROP TABLE t1; -SET DEBUG_SYNC= 'RESET'; - - ---echo # --echo # Bug#57130 crash in Item_field::print during SHOW CREATE TABLE or VIEW --echo #
diff --git a/mysql-test/r/lock_tables_lost_commit.result b/mysql-test/r/lock_tables_lost_commit.result index 769e973..394ef0a 100644 --- a/mysql-test/r/lock_tables_lost_commit.result +++ b/mysql-test/r/lock_tables_lost_commit.result @@ -9,7 +9,6 @@ disconnect con1; connection con2; SELECT * FROM t1; a -10
This test doesn't make sense after your fix. and the original bug#578 reappears. The problem is that mysqlimport issues LOCK TABLES, but does not issue UNLOCK TABLES at the end. I suspect this has to be fixed now. Alternatively, you can make LOCK TABLE to commit (not rollback) a transaction at disconnect. But that would be weird, wouldn't it?
DROP TABLE t1; connection default; disconnect con2; 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
is this expected?
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.
This is interesting. The test does begin; handler open; commit; handler read next; if logically the handler is just manually doing select-like access, it should not keep the table open over the transaction boundaries. how does it work now?
+if ($engine_type != 'InnoDB') +{ --echo # --echo # Bug #46224 HANDLER statements within a transaction might --echo # lead to deadlocks diff --git a/mysql-test/suite/handler/innodb.result b/mysql-test/suite/handler/innodb.result index fc1089e..9f2a0dd 100644 --- a/mysql-test/suite/handler/innodb.result +++ b/mysql-test/suite/handler/innodb.result @@ -545,7 +545,6 @@ optimize table t1; connection default; handler t1 read next; c1 -1
why is that? is optimize blocked now?
handler t1 close; connection con2; Table Op Msg_type Msg_text diff --git a/mysql-test/suite/rpl/t/rpl_switch_stm_row_mixed.test b/mysql-test/suite/rpl/t/rpl_switch_stm_row_mixed.test index 575fdb2..e5e2f7a 100644 --- a/mysql-test/suite/rpl/t/rpl_switch_stm_row_mixed.test +++ b/mysql-test/suite/rpl/t/rpl_switch_stm_row_mixed.test @@ -524,6 +524,7 @@ CREATE TABLE t11 (song VARCHAR(255)); LOCK TABLES t11 WRITE; SET SESSION BINLOG_FORMAT=ROW; INSERT INTO t11 VALUES('Several Species of Small Furry Animals Gathered Together in a Cave and Grooving With a Pict'); +--error ER_INSIDE_TRANSACTION_PREVENTS_SWITCH_BINLOG_FORMAT
even with myisam tables?
SET SESSION BINLOG_FORMAT=STATEMENT; INSERT INTO t11 VALUES('Careful With That Axe, Eugene'); UNLOCK TABLES; diff --git a/storage/tokudb/mysql-test/tokudb_bugs/t/db806.test b/storage/tokudb/mysql-test/tokudb_bugs/t/db806.test index 3815e59..8dcebe1 100644 --- a/storage/tokudb/mysql-test/tokudb_bugs/t/db806.test +++ b/storage/tokudb/mysql-test/tokudb_bugs/t/db806.test @@ -7,7 +7,7 @@ enable_warnings; CREATE TABLE t3(a int,c int,d int)engine=TOKUDB; lock table t3 read; create temporary table t1 engine=tokudb as SELECT 1; -select * from t1; unlock tables; +select * from t1;
why?
-drop table t1,t3; +drop table t1,t3; diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 3091bd6..b7c2e4a 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -2824,6 +2824,11 @@ 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); + thd->server_status|= SERVER_STATUS_AUTOCOMMIT; + }
Okay, you restore autocommit. Where a transaction is actually committed?
}
diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 150f21a..b1dd81b 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -3267,17 +3267,6 @@ ha_innobase::init_table_handle_for_HANDLER(void)
innobase_register_trx(ht, user_thd, prebuilt->trx);
- /* We did the necessary inits in this function, no need to repeat them - in row_search_for_mysql */ - - prebuilt->sql_stat_start = FALSE; - - /* We let HANDLER always to do the reads as consistent reads, even - if the trx isolation level would have been specified as SERIALIZABLE */ - - prebuilt->select_lock_type = LOCK_NONE; - prebuilt->stored_select_lock_type = LOCK_NONE; -
why this change? (removing LOCK_NONE, moving sql_stat_start=FALSE to start_stmt)
/* Always fetch all columns in the index record */
prebuilt->hint_need_to_fetch_extra_cols = ROW_RETRIEVE_ALL_COLS;
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Sergei, Sorry to say this, but you reviewed outdated commits. There's just one in bb-10.2-mdev7660 now. Since value of those cleanups was rather questionable and I got conflicts on rebase I discarded them. Still some answers inline. On Fri, Oct 14, 2016 at 08:12:52PM +0200, Sergei Golubchik wrote:
Hi, Sergey!
Here's a review of the last two commits in bb-10.2-mdev7660 (I've quickly looked through other, cleanup, commits, didn't have any comments, so I've excluded them from the diff to make it smaller)
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; This change is in new patch too.
1. What did this test do?
Attempts to trigger a warning in thr_lock.c code, see: http://lists.mysql.com/commits/82819 Since InnoDB doesn't use thr_lock, this test doesn't cover problematic code anymore.
2. What does this 'executing' mean? Really executing or waiting inside InnoDB on a lock? IIRC my intent was to make sure INSERT goes through THR_LOCK stage before UNLOCK, which is supposed to trigger this warning.
connection default; --reap
-connection con2; -UNLOCK TABLES; - -connection default; disconnect con2; DROP TABLE t1;
diff --git a/mysql-test/t/lock_sync.test b/mysql-test/t/lock_sync.test index c090e3a..07c16ac 100644 --- a/mysql-test/t/lock_sync.test +++ b/mysql-test/t/lock_sync.test @@ -873,116 +879,6 @@ set @@global.concurrent_insert= @old_concurrent_insert;
--echo # ---echo # Test for bug #45143 "All connections hang on concurrent ALTER TABLE". ---echo # ---echo # Concurrent execution of statements which required weak write lock ---echo # (TL_WRITE_ALLOW_WRITE) on several instances of the same table and ---echo # statements which tried to acquire stronger write lock (TL_WRITE, ---echo # TL_WRITE_ALLOW_READ) on this table might have led to deadlock.
Is this test no longer applicable? Why?
This change is in new patch too.
From comment: 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.
-# -# Suppress warnings for INSERTs that use get_lock(). -# -disable_query_log; -call mtr.add_suppression("Unsafe statement written to the binary log using statement format since BINLOG_FORMAT = STATEMENT"); -enable_query_log; - ---disable_warnings -drop table if exists t1; -drop view if exists v1; ---enable_warnings ---echo # Create auxiliary connections used through the test. -connect (con_bug45143_1,localhost,root,,test,,); -connect (con_bug45143_3,localhost,root,,test,,); -connect (con_bug45143_2,localhost,root,,test,,); -connection default; ---echo # Reset DEBUG_SYNC facility before using it. -set debug_sync= 'RESET'; ---echo # Turn off logging so calls to locking subsystem performed ---echo # for general_log table won't interfere with our test. -set @old_general_log = @@global.general_log; -set @@global.general_log= OFF; - -create table t1 (i int) engine=InnoDB; ---echo # We have to use view in order to make LOCK TABLES avoid ---echo # acquiring SNRW metadata lock on table. -create view v1 as select * from t1; -insert into t1 values (1); ---echo # Prepare user lock which will be used for resuming execution of ---echo # the first statement after it acquires TL_WRITE_ALLOW_WRITE lock. -select get_lock("lock_bug45143_wait", 0); - -connection con_bug45143_1; ---echo # Sending: ---send insert into t1 values (get_lock("lock_bug45143_wait", 100)); - -connection con_bug45143_2; ---echo # Wait until the above INSERT takes TL_WRITE_ALLOW_WRITE lock on 't1' ---echo # and then gets blocked on user lock 'lock_bug45143_wait'. -let $wait_condition= select count(*)= 1 from information_schema.processlist - where state= 'User lock' and - info='insert into t1 values (get_lock("lock_bug45143_wait", 100))'; ---source include/wait_condition.inc ---echo # Ensure that upcoming SELECT waits after acquiring TL_WRITE_ALLOW_WRITE ---echo # lock for the first instance of 't1'. -set debug_sync='thr_multi_lock_after_thr_lock SIGNAL parked WAIT_FOR go'; ---echo # Sending: ---send select count(*) > 0 from t1 as a, t1 as b for update; - -connection con_bug45143_3; ---echo # Wait until the above SELECT ... FOR UPDATE is blocked after ---echo # acquiring lock for the the first instance of 't1'. -set debug_sync= 'now WAIT_FOR parked'; ---echo # Send LOCK TABLE statement which will try to get TL_WRITE lock on 't1': ---send lock table v1 write; - -connection default; ---echo # Wait until this LOCK TABLES statement starts waiting for table lock. -let $wait_condition= select count(*)= 1 from information_schema.processlist - where state= 'Waiting for table level lock' and - info='lock table v1 write'; ---source include/wait_condition.inc ---echo # Allow SELECT ... FOR UPDATE to resume. ---echo # Since it already has TL_WRITE_ALLOW_WRITE lock on the first instance ---echo # of 't1' it should be able to get lock on the second instance without ---echo # waiting, even although there is another thread which has such lock ---echo # on this table and also there is a thread waiting for a TL_WRITE on it. -set debug_sync= 'now SIGNAL go'; - -connection con_bug45143_2; ---echo # Reap SELECT ... FOR UPDATE ---reap - -connection default; ---echo # Resume execution of the INSERT statement. -select release_lock("lock_bug45143_wait"); - -connection con_bug45143_1; ---echo # Reap INSERT statement. ---echo # In Statement and Mixed replication mode we get here "Unsafe ---echo # for binlog" warnings. In row mode there are no warnings. ---echo # Hide the discrepancy. ---disable_warnings ---reap ---enable_warnings - - -connection con_bug45143_3; ---echo # Reap LOCK TABLES statement. ---reap -unlock tables; - -connection default; ---echo # Do clean-up. -disconnect con_bug45143_1; -disconnect con_bug45143_2; -disconnect con_bug45143_3; -set debug_sync= 'RESET'; -set @@global.general_log= @old_general_log; -drop view v1; -drop table t1; - - ---echo # --echo # Bug#50821 Deadlock between LOCK TABLES and ALTER TABLE --echo #
@@ -1051,55 +947,6 @@ SET DEBUG_SYNC="RESET";
--echo # ---echo # Bug#55930 Assertion `thd->transaction.stmt.is_empty() || ---echo # thd->in_sub_stmt || (thd->state.. ---echo #
what about this one?
This change is in new patch too. Same as above.
---disable_warnings -DROP TABLE IF EXISTS t1; ---enable_warnings - -CREATE TABLE t1(a INT) engine=InnoDB; -INSERT INTO t1 VALUES (1), (2); - -connect (con1, localhost, root); -connect (con2, localhost, root); - -connection con1; -SET SESSION lock_wait_timeout= 1; -SET DEBUG_SYNC= 'ha_admin_open_ltable SIGNAL opti_recreate WAIT_FOR opti_analyze'; ---echo # Sending: ---send OPTIMIZE TABLE t1 - -connection con2; -SET DEBUG_SYNC= 'now WAIT_FOR opti_recreate'; -SET DEBUG_SYNC= 'after_lock_tables_takes_lock SIGNAL thrlock WAIT_FOR release_thrlock'; ---echo # Sending: ---send INSERT INTO t1 VALUES (3) - -connection default; -SET DEBUG_SYNC= 'now WAIT_FOR thrlock'; -SET DEBUG_SYNC= 'now SIGNAL opti_analyze'; - -connection con1; ---echo # Reaping: OPTIMIZE TABLE t1 ---reap -SET DEBUG_SYNC= 'now SIGNAL release_thrlock'; -disconnect con1; ---source include/wait_until_disconnected.inc - -connection con2; ---echo # Reaping: INSERT INTO t1 VALUES (3) ---reap -disconnect con2; ---source include/wait_until_disconnected.inc - -connection default; -DROP TABLE t1; -SET DEBUG_SYNC= 'RESET'; - - ---echo # --echo # Bug#57130 crash in Item_field::print during SHOW CREATE TABLE or VIEW --echo #
diff --git a/mysql-test/r/lock_tables_lost_commit.result b/mysql-test/r/lock_tables_lost_commit.result index 769e973..394ef0a 100644 --- a/mysql-test/r/lock_tables_lost_commit.result +++ b/mysql-test/r/lock_tables_lost_commit.result @@ -9,7 +9,6 @@ disconnect con1; connection con2; SELECT * FROM t1; a -10
This test doesn't make sense after your fix. and the original bug#578 reappears. The problem is that mysqlimport issues LOCK TABLES, but does not issue UNLOCK TABLES at the end. I suspect this has to be fixed now.
Alternatively, you can make LOCK TABLE to commit (not rollback) a transaction at disconnect. But that would be weird, wouldn't it?
This change is in new patch too. You're right, we should fix mysqlimport. I just wonder why this trivial fix wasn't introduced before?
DROP TABLE t1; connection default; disconnect con2; 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
is this expected?
This change is in new patch too. Sure, a few lines above it does UNLOCK TABLES, which is commits now.
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.
This is interesting. The test does
begin; handler open; commit; handler read next;
if logically the handler is just manually doing select-like access, it should not keep the table open over the transaction boundaries. how does it work now?
This is fixed differently in new patch. It should work the very same way as it used to work before this patch. It seem to continue scan as if nothing has happened.
+if ($engine_type != 'InnoDB') +{ --echo # --echo # Bug #46224 HANDLER statements within a transaction might --echo # lead to deadlocks diff --git a/mysql-test/suite/handler/innodb.result b/mysql-test/suite/handler/innodb.result index fc1089e..9f2a0dd 100644 --- a/mysql-test/suite/handler/innodb.result +++ b/mysql-test/suite/handler/innodb.result @@ -545,7 +545,6 @@ optimize table t1; connection default; handler t1 read next; c1 -1
why is that? is optimize blocked now?
It wasn't blocked in this patch, it is blocked on MDL level in new patch. In fact if it's blocked it should return no rows. There is no such change in new patch, because you removed "1" in 932646b1ff6a8f5815a961340a9e1ee4702f5b44
handler t1 close; connection con2; Table Op Msg_type Msg_text diff --git a/mysql-test/suite/rpl/t/rpl_switch_stm_row_mixed.test b/mysql-test/suite/rpl/t/rpl_switch_stm_row_mixed.test index 575fdb2..e5e2f7a 100644 --- a/mysql-test/suite/rpl/t/rpl_switch_stm_row_mixed.test +++ b/mysql-test/suite/rpl/t/rpl_switch_stm_row_mixed.test @@ -524,6 +524,7 @@ CREATE TABLE t11 (song VARCHAR(255)); LOCK TABLES t11 WRITE; SET SESSION BINLOG_FORMAT=ROW; INSERT INTO t11 VALUES('Several Species of Small Furry Animals Gathered Together in a Cave and Grooving With a Pict'); +--error ER_INSIDE_TRANSACTION_PREVENTS_SWITCH_BINLOG_FORMAT
even with myisam tables?
This change is in new patch. Yes, it relies on server_status & SERVER_STATUS_IN_TRANS, which is set by LOCK TABLES now.
SET SESSION BINLOG_FORMAT=STATEMENT; INSERT INTO t11 VALUES('Careful With That Axe, Eugene'); UNLOCK TABLES; diff --git a/storage/tokudb/mysql-test/tokudb_bugs/t/db806.test b/storage/tokudb/mysql-test/tokudb_bugs/t/db806.test index 3815e59..8dcebe1 100644 --- a/storage/tokudb/mysql-test/tokudb_bugs/t/db806.test +++ b/storage/tokudb/mysql-test/tokudb_bugs/t/db806.test @@ -7,7 +7,7 @@ enable_warnings; CREATE TABLE t3(a int,c int,d int)engine=TOKUDB; lock table t3 read; create temporary table t1 engine=tokudb as SELECT 1; -select * from t1; unlock tables; +select * from t1;
why?
This change is in new patch too. Looks like some TokuDB specific thing, it didn't like query against t1 under LOCK TABLES: mysqltest: At line 10: query 'select * from t1' failed: 1412: Table definition has changed, please retry transaction Has something to do with TOKUDB_MVCC_DICTIONARY_TOO_NEW. Though it didn't happen under BEGIN...COMMIT.
-drop table t1,t3; +drop table t1,t3; diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 3091bd6..b7c2e4a 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -2824,6 +2824,11 @@ 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); + thd->server_status|= SERVER_STATUS_AUTOCOMMIT; + }
Okay, you restore autocommit. Where a transaction is actually committed?
This change is in new patch too. In sql_parse.cc, it was there.
}
diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 150f21a..b1dd81b 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -3267,17 +3267,6 @@ ha_innobase::init_table_handle_for_HANDLER(void)
innobase_register_trx(ht, user_thd, prebuilt->trx);
- /* We did the necessary inits in this function, no need to repeat them - in row_search_for_mysql */ - - prebuilt->sql_stat_start = FALSE; - - /* We let HANDLER always to do the reads as consistent reads, even - if the trx isolation level would have been specified as SERIALIZABLE */ - - prebuilt->select_lock_type = LOCK_NONE; - prebuilt->stored_select_lock_type = LOCK_NONE; -
why this change? (removing LOCK_NONE, moving sql_stat_start=FALSE to start_stmt)
This change is not in new patch. It was supposed to let InnoDB acquire shared lock on HANDLER READ (to isolate from concurrent LOCK TABLES WRITE). Now it is handled by MDL. Thanks, Sergey
Hi, Sergey! On Oct 15, Sergey Vojtovich wrote:
Hi Sergei,
Sorry to say this, but you reviewed outdated commits. There's just one in bb-10.2-mdev7660 now. Since value of those cleanups was rather questionable and I got conflicts on rebase I discarded them.
Oh, okay. I know that feeling, just yesterday I did two patches of various cleanups in mysql.cc and later discarded them too... Below are few replies and a review of the new patch
Still some answers inline.
On Fri, Oct 14, 2016 at 08:12:52PM +0200, Sergei Golubchik wrote:
Hi, Sergey!
Here's a review of the last two commits in bb-10.2-mdev7660 (I've quickly looked through other, cleanup, commits, didn't have any comments, so I've excluded them from the diff to make it smaller)
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; This change is in new patch too.
1. What did this test do?
Attempts to trigger a warning in thr_lock.c code, see: http://lists.mysql.com/commits/82819
Since InnoDB doesn't use thr_lock, this test doesn't cover problematic code anymore.
Do you need to remove the "temporary suppression" from mysql-test-run.pl?
2. What does this 'executing' mean? Really executing or waiting inside InnoDB on a lock? IIRC my intent was to make sure INSERT goes through THR_LOCK stage before UNLOCK, which is supposed to trigger this warning.
So, is it waiting on InnoDB for a lock? Because if it's really executing, you cannot reliably catch it "in the fly". I assume it's waiting on the lock, you test couldn't have reliably worked otherwise.
diff --git a/mysql-test/r/lock_tables_lost_commit.result b/mysql-test/r/lock_tables_lost_commit.result index 769e973..394ef0a 100644 --- a/mysql-test/r/lock_tables_lost_commit.result +++ b/mysql-test/r/lock_tables_lost_commit.result @@ -9,7 +9,6 @@ disconnect con1; connection con2; SELECT * FROM t1; a -10
This test doesn't make sense after your fix. and the original bug#578 reappears. The problem is that mysqlimport issues LOCK TABLES, but does not issue UNLOCK TABLES at the end. I suspect this has to be fixed now.
Alternatively, you can make LOCK TABLE to commit (not rollback) a transaction at disconnect. But that would be weird, wouldn't it? This change is in new patch too.
You're right, we should fix mysqlimport. I just wonder why this trivial fix wasn't introduced before?
Will you do it as a part of MDEV-7660?
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.
This is interesting. The test does
begin; handler open; commit; handler read next;
if logically the handler is just manually doing select-like access, it should not keep the table open over the transaction boundaries. how does it work now? This is fixed differently in new patch.
It should work the very same way as it used to work before this patch. It seem to continue scan as if nothing has happened.
Okay, good. Still, I'm wondering how one can keep a table opened and locked over transaction boundaries...
diff --git a/storage/tokudb/mysql-test/tokudb_bugs/t/db806.test b/storage/tokudb/mysql-test/tokudb_bugs/t/db806.test index 3815e59..8dcebe1 100644 --- a/storage/tokudb/mysql-test/tokudb_bugs/t/db806.test +++ b/storage/tokudb/mysql-test/tokudb_bugs/t/db806.test @@ -7,7 +7,7 @@ enable_warnings; CREATE TABLE t3(a int,c int,d int)engine=TOKUDB; lock table t3 read; create temporary table t1 engine=tokudb as SELECT 1; -select * from t1; unlock tables; +select * from t1;
why? This change is in new patch too. Looks like some TokuDB specific thing, it didn't like query against t1 under LOCK TABLES: mysqltest: At line 10: query 'select * from t1' failed: 1412: Table definition has changed, please retry transaction
Has something to do with TOKUDB_MVCC_DICTIONARY_TOO_NEW. Though it didn't happen under BEGIN...COMMIT.
It uses only TokuDB tables, how did your changes affect it? ============
commit c90bd38 Author: Sergey Vojtovich <svoj@mariadb.org> Date: Fri May 6 13:44:07 2016 +0400
diff --git a/mysql-test/t/truncate_coverage.test b/mysql-test/t/truncate_coverage.test index 0834f7a..3351ce8 100644 --- a/mysql-test/t/truncate_coverage.test +++ b/mysql-test/t/truncate_coverage.test @@ -17,86 +17,6 @@ DROP TABLE IF EXISTS t1; --echo # Bug#20667 - Truncate table fails for a write locked table --echo # ######## -# Attack wait_while_table_is_used(). Kill query while trying to -# upgrade MDL. -# -CREATE TABLE t1 (c1 INT); -INSERT INTO t1 VALUES (1); -# -# Acquire a shared metadata lock on table by opening HANDLER for it and wait. -# TRUNCATE shall block on this metadata lock. -# We can't use normal DML as such statements would also block LOCK TABLES. -#
I suppose you've removed this one, because HANDLER now takes different locks. but the original test was about killing TRUNCATE duing wait_while_table_is_used. Wouldn't it be better to fix the test to continue doing that?
---connect (con1, localhost, root,,) -HANDLER t1 OPEN; -# -# Get connection id of default connection. -# Lock the table and start TRUNCATE, which will block on MDL upgrade. -# ---connection default -let $ID= `SELECT @id := CONNECTION_ID()`; -LOCK TABLE t1 WRITE; -SET DEBUG_SYNC='mdl_upgrade_lock SIGNAL waiting'; -send TRUNCATE TABLE t1; -# -# Get the default connection ID into a variable in an invisible statement. -# Kill the TRUNCATE query. This shall result in an error return -# from wait_while_table_is_used(). -# ---connect (con2, localhost, root,,) -SET DEBUG_SYNC='now WAIT_FOR waiting'; -let $invisible_assignment_in_select = `SELECT @id := $ID`; -KILL QUERY @id; ---disconnect con2 ---connection default ---error ER_QUERY_INTERRUPTED -reap; -UNLOCK TABLES; ---connection con1 ---echo # Release shared metadata lock by closing HANDLER. -HANDLER t1 CLOSE; ---disconnect con1 ---connection default -DROP TABLE t1; -SET DEBUG_SYNC='RESET'; -######## -# Attack reopen_tables(). Remove form file. -# -CREATE TABLE t1 (c1 INT); -INSERT INTO t1 VALUES (1); -# -# Acquire a shared metadata lock on table by opening HANDLER for it and wait. -# TRUNCATE shall block on this metadata lock. -# We can't use normal DML as such statements would also block LOCK TABLES. -# ---connect (con1, localhost, root,,) -HANDLER t1 OPEN; -# -# Lock the table and start TRUNCATE, which will block on MDL upgrade. -# ---connection default -LOCK TABLE t1 WRITE; -SET DEBUG_SYNC='mdl_upgrade_lock SIGNAL waiting'; -send TRUNCATE TABLE t1; -# -# Remove datafile. -# Commit to let TRUNCATE continue. -# ---connect (con2, localhost, root,,) -SET DEBUG_SYNC='now WAIT_FOR waiting'; ---remove_file $MYSQLD_DATADIR/test/t1.frm ---disconnect con2 ---connection con1 -HANDLER t1 CLOSE; ---disconnect con1 ---connection default ---error ER_NO_SUCH_TABLE -reap; -UNLOCK TABLES; ---error ER_BAD_TABLE_ERROR -DROP TABLE t1; -SET DEBUG_SYNC='RESET'; -######## # Attack acquire_exclusive_locks(). Hold a global read lock. # Non-LOCK TABLE case. # diff --git a/mysql-test/suite/handler/handler.inc b/mysql-test/suite/handler/handler.inc index a4ab5f1..2432ff1 100644 --- a/mysql-test/suite/handler/handler.inc +++ b/mysql-test/suite/handler/handler.inc @@ -1118,6 +1118,7 @@ connection default; handler t1 read a next;
--echo # Unblock 'lock tables t1 write'. +select * from t1; # Release MDL_SHARED_READ held by HANDLER
why would it release MDL_SHARED_READ?
commit;
connection con1; @@ -1259,10 +1260,6 @@ handler t1 read a last; insert into t1 (a, b) values (7, 7); handler t1 read a last; commit; -connection con1; ---echo # Demonstrate that the HANDLER doesn't hold MDL_SHARED_WRITE.
because it now does? is there a test for that?
-lock table t1 write; -unlock tables; connection default; handler t1 read a prev; handler t1 close; diff --git a/mysql-test/suite/handler/myisam.result b/mysql-test/suite/handler/myisam.result index fca75f3..90e1767 100644 --- a/mysql-test/suite/handler/myisam.result +++ b/mysql-test/suite/handler/myisam.result @@ -545,7 +545,6 @@ optimize table t1; connection default; handler t1 read next; c1 -1
Do you get identical results for all engines here?
handler t1 close; connection con2; Table Op Msg_type Msg_text diff --git a/storage/xtradb/handler/ha_innodb.cc b/storage/xtradb/handler/ha_innodb.cc index aa54a5a..0667db0 100644 --- a/storage/xtradb/handler/ha_innodb.cc +++ b/storage/xtradb/handler/ha_innodb.cc @@ -16125,25 +16125,45 @@ free_share( mysql_mutex_unlock(&innobase_share_mutex); }
+/*********************************************************************//** +Returns number of THR_LOCK locks used for one instance of InnoDB table. +InnoDB no longer relies on THR_LOCK locks so 0 value is returned. +Instead of THR_LOCK locks InnoDB relies on combination of metadata locks +(e.g. for LOCK TABLES and DDL) and its own locking subsystem. +Note that even though this method returns 0, SQL-layer still calls +::store_lock(), ::start_stmt() and ::external_lock() methods for InnoDB +tables. */ + +uint +ha_innobase::lock_count(void) const +/*===============================*/ +{ + return 0; +}
don't bother. XtraDB is not used in 10.2 until it's upgraded to 5.7. your changes will only complicate the merge.
+ /*****************************************************************//** -Converts a MySQL table lock stored in the 'lock' field of the handle to -a proper type before storing pointer to the lock into an array of pointers. +Supposed to convert a MySQL table lock stored in the 'lock' field of the +handle to a proper type before storing pointer to the lock into an array +of pointers. +In practice, since InnoDB no longer relies on THR_LOCK locks and its +lock_count() method returns 0 it just informs storage engine about type +of THR_LOCK which SQL-layer would have acquired for this specific statement +on this specific table. MySQL also calls this if it wants to reset some table locks to a not-locked state during the processing of an SQL query. An example is that during a SELECT the read lock is released early on the 'const' tables where we only fetch one row. MySQL does not call this when it releases all locks at the end of an SQL statement. -@return pointer to the next element in the 'to' array */ +@return pointer to the current element in the 'to' array. */ UNIV_INTERN THR_LOCK_DATA** ha_innobase::store_lock( /*====================*/ THD* thd, /*!< in: user thread handle */ - THR_LOCK_DATA** to, /*!< in: pointer to an array - of pointers to lock structs; - pointer to the 'lock' field - of current handle is stored - next to this array */ + THR_LOCK_DATA** to, /*!< in: pointer to the current + element in an array of pointers + to lock structs; + only used as return value */ enum thr_lock_type lock_type) /*!< in: lock type to store in 'lock'; this may also be TL_IGNORE */
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Sergei! On Sat, Oct 15, 2016 at 11:13:24AM +0200, Sergei Golubchik wrote:
Hi, Sergey!
On Oct 15, Sergey Vojtovich wrote:
Hi Sergei,
Sorry to say this, but you reviewed outdated commits. There's just one in bb-10.2-mdev7660 now. Since value of those cleanups was rather questionable and I got conflicts on rebase I discarded them.
Oh, okay. I know that feeling, just yesterday I did two patches of various cleanups in mysql.cc and later discarded them too...
Below are few replies and a review of the new patch
Still some answers inline.
On Fri, Oct 14, 2016 at 08:12:52PM +0200, Sergei Golubchik wrote:
Hi, Sergey!
Here's a review of the last two commits in bb-10.2-mdev7660 (I've quickly looked through other, cleanup, commits, didn't have any comments, so I've excluded them from the diff to make it smaller)
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; This change is in new patch too.
1. What did this test do?
Attempts to trigger a warning in thr_lock.c code, see: http://lists.mysql.com/commits/82819
Since InnoDB doesn't use thr_lock, this test doesn't cover problematic code anymore.
Do you need to remove the "temporary suppression" from mysql-test-run.pl? Removed.
2. What does this 'executing' mean? Really executing or waiting inside InnoDB on a lock? IIRC my intent was to make sure INSERT goes through THR_LOCK stage before UNLOCK, which is supposed to trigger this warning.
So, is it waiting on InnoDB for a lock?
Yes.
Because if it's really executing, you cannot reliably catch it "in the fly". I assume it's waiting on the lock, you test couldn't have reliably worked otherwise. I just attempted to trigger this warning on a vanilla 10.2 tree with no luck. Also I don't completely understand value of this test. Should we remove it?
diff --git a/mysql-test/r/lock_tables_lost_commit.result b/mysql-test/r/lock_tables_lost_commit.result index 769e973..394ef0a 100644 --- a/mysql-test/r/lock_tables_lost_commit.result +++ b/mysql-test/r/lock_tables_lost_commit.result @@ -9,7 +9,6 @@ disconnect con1; connection con2; SELECT * FROM t1; a -10
This test doesn't make sense after your fix. and the original bug#578 reappears. The problem is that mysqlimport issues LOCK TABLES, but does not issue UNLOCK TABLES at the end. I suspect this has to be fixed now.
Alternatively, you can make LOCK TABLE to commit (not rollback) a transaction at disconnect. But that would be weird, wouldn't it? This change is in new patch too.
You're right, we should fix mysqlimport. I just wonder why this trivial fix wasn't introduced before?
Will you do it as a part of MDEV-7660?
Better not.
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.
This is interesting. The test does
begin; handler open; commit; handler read next;
if logically the handler is just manually doing select-like access, it should not keep the table open over the transaction boundaries. how does it work now? This is fixed differently in new patch.
It should work the very same way as it used to work before this patch. It seem to continue scan as if nothing has happened.
Okay, good. Still, I'm wondering how one can keep a table opened and locked over transaction boundaries...
Well... it behaves similarly under LOCK TABLES. One can run multiple transactions while lock is held.
diff --git a/storage/tokudb/mysql-test/tokudb_bugs/t/db806.test b/storage/tokudb/mysql-test/tokudb_bugs/t/db806.test index 3815e59..8dcebe1 100644 --- a/storage/tokudb/mysql-test/tokudb_bugs/t/db806.test +++ b/storage/tokudb/mysql-test/tokudb_bugs/t/db806.test @@ -7,7 +7,7 @@ enable_warnings; CREATE TABLE t3(a int,c int,d int)engine=TOKUDB; lock table t3 read; create temporary table t1 engine=tokudb as SELECT 1; -select * from t1; unlock tables; +select * from t1;
why? This change is in new patch too. Looks like some TokuDB specific thing, it didn't like query against t1 under LOCK TABLES: mysqltest: At line 10: query 'select * from t1' failed: 1412: Table definition has changed, please retry transaction
Has something to do with TOKUDB_MVCC_DICTIONARY_TOO_NEW. Though it didn't happen under BEGIN...COMMIT.
It uses only TokuDB tables, how did your changes affect it?
This is because of this change: - thd->variables.option_bits|= OPTION_TABLE_LOCK; + thd->variables.option_bits|= OPTION_TABLE_LOCK | OPTION_NOT_AUTOCOMMIT; It somehow affects ha_tokudb::create_txn(). I managed to reproduce the very same failure on a vanilla 10.2 if I just add "SET autocommit=0" before "lock table t3 read".
============
commit c90bd38 Author: Sergey Vojtovich <svoj@mariadb.org> Date: Fri May 6 13:44:07 2016 +0400
diff --git a/mysql-test/t/truncate_coverage.test b/mysql-test/t/truncate_coverage.test index 0834f7a..3351ce8 100644 --- a/mysql-test/t/truncate_coverage.test +++ b/mysql-test/t/truncate_coverage.test @@ -17,86 +17,6 @@ DROP TABLE IF EXISTS t1; --echo # Bug#20667 - Truncate table fails for a write locked table --echo # ######## -# Attack wait_while_table_is_used(). Kill query while trying to -# upgrade MDL. -# -CREATE TABLE t1 (c1 INT); -INSERT INTO t1 VALUES (1); -# -# Acquire a shared metadata lock on table by opening HANDLER for it and wait. -# TRUNCATE shall block on this metadata lock. -# We can't use normal DML as such statements would also block LOCK TABLES. -#
I suppose you've removed this one, because HANDLER now takes different locks. but the original test was about killing TRUNCATE duing wait_while_table_is_used. Wouldn't it be better to fix the test to continue doing that?
It definitely would be better to keep all tests. There're complexities though... Strictily speaking original test was about killing TRUNCATE duing wait_while_table_is_used while it is upgrading MDL lock. This wait is only possible if another thread holds MDL_SHARED, but currently it's really hard to find anything that acquires MDL_SHARED. I only found that prepared statements may do that for a short time. I can try to use them for this purpose.
---connect (con1, localhost, root,,) -HANDLER t1 OPEN; -# -# Get connection id of default connection. -# Lock the table and start TRUNCATE, which will block on MDL upgrade. -# ---connection default -let $ID= `SELECT @id := CONNECTION_ID()`; -LOCK TABLE t1 WRITE; -SET DEBUG_SYNC='mdl_upgrade_lock SIGNAL waiting'; -send TRUNCATE TABLE t1; -# -# Get the default connection ID into a variable in an invisible statement. -# Kill the TRUNCATE query. This shall result in an error return -# from wait_while_table_is_used(). -# ---connect (con2, localhost, root,,) -SET DEBUG_SYNC='now WAIT_FOR waiting'; -let $invisible_assignment_in_select = `SELECT @id := $ID`; -KILL QUERY @id; ---disconnect con2 ---connection default ---error ER_QUERY_INTERRUPTED -reap; -UNLOCK TABLES; ---connection con1 ---echo # Release shared metadata lock by closing HANDLER. -HANDLER t1 CLOSE; ---disconnect con1 ---connection default -DROP TABLE t1; -SET DEBUG_SYNC='RESET'; -######## -# Attack reopen_tables(). Remove form file. -# -CREATE TABLE t1 (c1 INT); -INSERT INTO t1 VALUES (1); -# -# Acquire a shared metadata lock on table by opening HANDLER for it and wait. -# TRUNCATE shall block on this metadata lock. -# We can't use normal DML as such statements would also block LOCK TABLES. -# ---connect (con1, localhost, root,,) -HANDLER t1 OPEN; -# -# Lock the table and start TRUNCATE, which will block on MDL upgrade. -# ---connection default -LOCK TABLE t1 WRITE; -SET DEBUG_SYNC='mdl_upgrade_lock SIGNAL waiting'; -send TRUNCATE TABLE t1; -# -# Remove datafile. -# Commit to let TRUNCATE continue. -# ---connect (con2, localhost, root,,) -SET DEBUG_SYNC='now WAIT_FOR waiting'; ---remove_file $MYSQLD_DATADIR/test/t1.frm ---disconnect con2 ---connection con1 -HANDLER t1 CLOSE; ---disconnect con1 ---connection default ---error ER_NO_SUCH_TABLE -reap; -UNLOCK TABLES; ---error ER_BAD_TABLE_ERROR -DROP TABLE t1; -SET DEBUG_SYNC='RESET'; -######## # Attack acquire_exclusive_locks(). Hold a global read lock. # Non-LOCK TABLE case. # diff --git a/mysql-test/suite/handler/handler.inc b/mysql-test/suite/handler/handler.inc index a4ab5f1..2432ff1 100644 --- a/mysql-test/suite/handler/handler.inc +++ b/mysql-test/suite/handler/handler.inc @@ -1118,6 +1118,7 @@ connection default; handler t1 read a next;
--echo # Unblock 'lock tables t1 write'. +select * from t1; # Release MDL_SHARED_READ held by HANDLER
why would it release MDL_SHARED_READ?
See open_tables(): /* Close HANDLER tables which are marked for flush or against which there are pending exclusive metadata locks. This is needed both in order to avoid deadlocks and to have a point during statement execution at which such HANDLERs are closed even if they don't create problems for the current session (i.e. to avoid having a DDL blocked by HANDLERs opened for a long time). */ if (thd->handler_tables_hash.records) mysql_ha_flush(thd);
commit;
connection con1; @@ -1259,10 +1260,6 @@ handler t1 read a last; insert into t1 (a, b) values (7, 7); handler t1 read a last; commit; -connection con1; ---echo # Demonstrate that the HANDLER doesn't hold MDL_SHARED_WRITE.
because it now does? is there a test for that?
I guess it was supposed to say either "doesn't hold MDL_SHARED_READ" or "is compatible with MDL_SHARE_NO_READ_WRITE".
-lock table t1 write; -unlock tables; connection default; handler t1 read a prev; handler t1 close; diff --git a/mysql-test/suite/handler/myisam.result b/mysql-test/suite/handler/myisam.result index fca75f3..90e1767 100644 --- a/mysql-test/suite/handler/myisam.result +++ b/mysql-test/suite/handler/myisam.result @@ -545,7 +545,6 @@ optimize table t1; connection default; handler t1 read next; c1 -1
Do you get identical results for all engines here?
Yes.
handler t1 close; connection con2; Table Op Msg_type Msg_text diff --git a/storage/xtradb/handler/ha_innodb.cc b/storage/xtradb/handler/ha_innodb.cc index aa54a5a..0667db0 100644 --- a/storage/xtradb/handler/ha_innodb.cc +++ b/storage/xtradb/handler/ha_innodb.cc @@ -16125,25 +16125,45 @@ free_share( mysql_mutex_unlock(&innobase_share_mutex); }
+/*********************************************************************//** +Returns number of THR_LOCK locks used for one instance of InnoDB table. +InnoDB no longer relies on THR_LOCK locks so 0 value is returned. +Instead of THR_LOCK locks InnoDB relies on combination of metadata locks +(e.g. for LOCK TABLES and DDL) and its own locking subsystem. +Note that even though this method returns 0, SQL-layer still calls +::store_lock(), ::start_stmt() and ::external_lock() methods for InnoDB +tables. */ + +uint +ha_innobase::lock_count(void) const +/*===============================*/ +{ + return 0; +}
don't bother. XtraDB is not used in 10.2 until it's upgraded to 5.7. your changes will only complicate the merge.
Ok, reverted this change. Thanks, Sergey
Hi, Sergey! Just one comment: On Oct 18, Sergey Vojtovich wrote:
diff --git a/mysql-test/r/lock_tables_lost_commit.result b/mysql-test/r/lock_tables_lost_commit.result index 769e973..394ef0a 100644 --- a/mysql-test/r/lock_tables_lost_commit.result +++ b/mysql-test/r/lock_tables_lost_commit.result @@ -9,7 +9,6 @@ disconnect con1; connection con2; SELECT * FROM t1; a -10
This test doesn't make sense after your fix. and the original bug#578 reappears. The problem is that mysqlimport issues LOCK TABLES, but does not issue UNLOCK TABLES at the end. I suspect this has to be fixed now.
Alternatively, you can make LOCK TABLE to commit (not rollback) a transaction at disconnect. But that would be weird, wouldn't it? This change is in new patch too.
You're right, we should fix mysqlimport. I just wonder why this trivial fix wasn't introduced before?
Will you do it as a part of MDEV-7660? Better not.
Could you make an MDEV for that? Type=Bug, FixVersion=10.2. Otherwise all ok, I think. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
participants (2)
-
Sergei Golubchik -
Sergey Vojtovich