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 
> > > > 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 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 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 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