maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

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 <svoj@xxxxxxxxxxx>
> > 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@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.
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