maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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