maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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 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?

> ---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?

>  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?

>  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?

> +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?

>  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?

>  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?

>  
> -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?

>  }
>  
>  
> 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)

>  	/* Always fetch all columns in the index record */
>  
>  	prebuilt->hint_need_to_fetch_extra_cols = ROW_RETRIEVE_ALL_COLS;

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx