Re: [Maria-developers] MDEV-7660 MySQL WL#6671 "Improve scalability by not using thr_lock.c locks for InnoDB tables"

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