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

[Sergei Golubchik]

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 secur...@mariadb.org

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

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

[Sergey Vojtovich]

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 

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

[Sergei Golubchik]

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 

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

[Sergey Vojtovich]

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 

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

[Sergei Golubchik]

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 

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

[Sergey Vojtovich]

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 

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

[Sergei Golubchik]

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 
> > > 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 secur...@mariadb.org

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

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

[Sergey Vojtovich]

Hi Sergei,

On Tue, May 31, 2016 at 06:48:31PM +0200, Sergei Golubchik wrote:
> 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
this patch due to many behavior changes. I can try to make a list if you like.

> 
> > commit 47aa5f9
> > Author: Sergey Vojtovich 
> > 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 
> > 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
> > ---