maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #09680
Re: MDEV-7660 MySQL WL#6671 "Improve scalability by not using thr_lock.c locks for InnoDB tables"
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@xxxxxxxxxxx>
> > > 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@xxxxxxxxxxx
Follow ups
References