Re: [Maria-developers] MDEV-7660 MySQL WL#6671 "Improve scalability by not using thr_lock.c locks for InnoDB tables"

Hi, Sergey! On Oct 15, Sergey Vojtovich wrote:

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.

Oh, okay. I know that feeling, just yesterday I did two patches of various cleanups in mysql.cc and later discarded them too... Below are few replies and a review of the new patch

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.

Do you need to remove the "temporary suppression" from mysql-test-run.pl?

...

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.

So, is it waiting on InnoDB for a lock? Because if it's really executing, you cannot reliably catch it "in the fly". I assume it's waiting on the lock, you test couldn't have reliably worked otherwise.

...

...

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?

Will you do it as a part of MDEV-7660?

...

...

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.

Okay, good. Still, I'm wondering how one can keep a table opened and locked over transaction boundaries...

...

...

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.

It uses only TokuDB tables, how did your changes affect it? ============

commit c90bd38 Author: Sergey Vojtovich Date: Fri May 6 13:44:07 2016 +0400

diff --git a/mysql-test/t/truncate_coverage.test b/mysql-test/t/truncate_coverage.test index 0834f7a..3351ce8 100644 --- a/mysql-test/t/truncate_coverage.test +++ b/mysql-test/t/truncate_coverage.test @@ -17,86 +17,6 @@ DROP TABLE IF EXISTS t1; --echo # Bug#20667 - Truncate table fails for a write locked table --echo # ######## -# Attack wait_while_table_is_used(). Kill query while trying to -# upgrade MDL. -# -CREATE TABLE t1 (c1 INT); -INSERT INTO t1 VALUES (1); -# -# Acquire a shared metadata lock on table by opening HANDLER for it and wait. -# TRUNCATE shall block on this metadata lock. -# We can't use normal DML as such statements would also block LOCK TABLES. -#

I suppose you've removed this one, because HANDLER now takes different locks. but the original test was about killing TRUNCATE duing wait_while_table_is_used. Wouldn't it be better to fix the test to continue doing that?

---connect (con1, localhost, root,,) -HANDLER t1 OPEN; -# -# Get connection id of default connection. -# Lock the table and start TRUNCATE, which will block on MDL upgrade. -# ---connection default -let $ID= `SELECT @id := CONNECTION_ID()`; -LOCK TABLE t1 WRITE; -SET DEBUG_SYNC='mdl_upgrade_lock SIGNAL waiting'; -send TRUNCATE TABLE t1; -# -# Get the default connection ID into a variable in an invisible statement. -# Kill the TRUNCATE query. This shall result in an error return -# from wait_while_table_is_used(). -# ---connect (con2, localhost, root,,) -SET DEBUG_SYNC='now WAIT_FOR waiting'; -let $invisible_assignment_in_select = `SELECT @id := $ID`; -KILL QUERY @id; ---disconnect con2 ---connection default ---error ER_QUERY_INTERRUPTED -reap; -UNLOCK TABLES; ---connection con1 ---echo # Release shared metadata lock by closing HANDLER. -HANDLER t1 CLOSE; ---disconnect con1 ---connection default -DROP TABLE t1; -SET DEBUG_SYNC='RESET'; -######## -# Attack reopen_tables(). Remove form file. -# -CREATE TABLE t1 (c1 INT); -INSERT INTO t1 VALUES (1); -# -# Acquire a shared metadata lock on table by opening HANDLER for it and wait. -# TRUNCATE shall block on this metadata lock. -# We can't use normal DML as such statements would also block LOCK TABLES. -# ---connect (con1, localhost, root,,) -HANDLER t1 OPEN; -# -# Lock the table and start TRUNCATE, which will block on MDL upgrade. -# ---connection default -LOCK TABLE t1 WRITE; -SET DEBUG_SYNC='mdl_upgrade_lock SIGNAL waiting'; -send TRUNCATE TABLE t1; -# -# Remove datafile. -# Commit to let TRUNCATE continue. -# ---connect (con2, localhost, root,,) -SET DEBUG_SYNC='now WAIT_FOR waiting'; ---remove_file $MYSQLD_DATADIR/test/t1.frm ---disconnect con2 ---connection con1 -HANDLER t1 CLOSE; ---disconnect con1 ---connection default ---error ER_NO_SUCH_TABLE -reap; -UNLOCK TABLES; ---error ER_BAD_TABLE_ERROR -DROP TABLE t1; -SET DEBUG_SYNC='RESET'; -######## # Attack acquire_exclusive_locks(). Hold a global read lock. # Non-LOCK TABLE case. # diff --git a/mysql-test/suite/handler/handler.inc b/mysql-test/suite/handler/handler.inc index a4ab5f1..2432ff1 100644 --- a/mysql-test/suite/handler/handler.inc +++ b/mysql-test/suite/handler/handler.inc @@ -1118,6 +1118,7 @@ connection default; handler t1 read a next;

--echo # Unblock 'lock tables t1 write'. +select * from t1; # Release MDL_SHARED_READ held by HANDLER

why would it release MDL_SHARED_READ?

commit;

connection con1; @@ -1259,10 +1260,6 @@ handler t1 read a last; insert into t1 (a, b) values (7, 7); handler t1 read a last; commit; -connection con1; ---echo # Demonstrate that the HANDLER doesn't hold MDL_SHARED_WRITE.

because it now does? is there a test for that?

-lock table t1 write; -unlock tables; connection default; handler t1 read a prev; handler t1 close; diff --git a/mysql-test/suite/handler/myisam.result b/mysql-test/suite/handler/myisam.result index fca75f3..90e1767 100644 --- a/mysql-test/suite/handler/myisam.result +++ b/mysql-test/suite/handler/myisam.result @@ -545,7 +545,6 @@ optimize table t1; connection default; handler t1 read next; c1 -1

Do you get identical results for all engines here?

handler t1 close; connection con2; Table Op Msg_type Msg_text diff --git a/storage/xtradb/handler/ha_innodb.cc b/storage/xtradb/handler/ha_innodb.cc index aa54a5a..0667db0 100644 --- a/storage/xtradb/handler/ha_innodb.cc +++ b/storage/xtradb/handler/ha_innodb.cc @@ -16125,25 +16125,45 @@ free_share( mysql_mutex_unlock(&innobase_share_mutex); }

+/*********************************************************************//** +Returns number of THR_LOCK locks used for one instance of InnoDB table. +InnoDB no longer relies on THR_LOCK locks so 0 value is returned. +Instead of THR_LOCK locks InnoDB relies on combination of metadata locks +(e.g. for LOCK TABLES and DDL) and its own locking subsystem. +Note that even though this method returns 0, SQL-layer still calls +::store_lock(), ::start_stmt() and ::external_lock() methods for InnoDB +tables. */ + +uint +ha_innobase::lock_count(void) const +/*===============================*/ +{ + return 0; +}

don't bother. XtraDB is not used in 10.2 until it's upgraded to 5.7. your changes will only complicate the merge.

+ /*****************************************************************//** -Converts a MySQL table lock stored in the 'lock' field of the handle to -a proper type before storing pointer to the lock into an array of pointers. +Supposed to convert a MySQL table lock stored in the 'lock' field of the +handle to a proper type before storing pointer to the lock into an array +of pointers. +In practice, since InnoDB no longer relies on THR_LOCK locks and its +lock_count() method returns 0 it just informs storage engine about type +of THR_LOCK which SQL-layer would have acquired for this specific statement +on this specific table. MySQL also calls this if it wants to reset some table locks to a not-locked state during the processing of an SQL query. An example is that during a SELECT the read lock is released early on the 'const' tables where we only fetch one row. MySQL does not call this when it releases all locks at the end of an SQL statement. -@return pointer to the next element in the 'to' array */ +@return pointer to the current element in the 'to' array. */ UNIV_INTERN THR_LOCK_DATA** ha_innobase::store_lock( /*====================*/ THD* thd, /*!< in: user thread handle */ - THR_LOCK_DATA** to, /*!< in: pointer to an array - of pointers to lock structs; - pointer to the 'lock' field - of current handle is stored - next to this array */ + THR_LOCK_DATA** to, /*!< in: pointer to the current + element in an array of pointers + to lock structs; + only used as return value */ enum thr_lock_type lock_type) /*!< in: lock type to store in 'lock'; this may also be TL_IGNORE */

Regards, Sergei Chief Architect MariaDB and security@mariadb.org