Hi, Sergey,
I think it's fine. I had a few questions though, see below:
> commit 47aa5f9
> Author: Sergey Vojtovich <s...@mariadb.org>
> 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
> /* 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 <s...@mariadb.org>
> 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?
> - 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?
2. What if someone changes @@autocommit under LOCK TABLES?
Do you have a test for that?
3. Do you need to set SERVER_STATUS_AUTOCOMMIT here?
> }
>
>
> 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 :)
>
> 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?
> 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.
what does then "drop table t1" do?
> +if ($engine_type != 'InnoDB')
> +{
> --echo #
> --echo # Bug #46224 HANDLER statements within a transaction might
> --echo # lead to deadlocks
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
