maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

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