maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei!

On Sat, Oct 15, 2016 at 11:13:24AM +0200, Sergei Golubchik wrote:
> 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?
Removed.

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

> 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.
I just attempted to trigger this warning on a vanilla 10.2 tree with no luck.
Also I don't completely understand value of this test. Should we remove it?

> 
> > > > 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?
Better not.

> 
> > > > 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...
Well... it behaves similarly under LOCK TABLES. One can run multiple
transactions while lock is held.

> 
> > > > 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?
This is because of this change:
-    thd->variables.option_bits|= OPTION_TABLE_LOCK;
+    thd->variables.option_bits|= OPTION_TABLE_LOCK | OPTION_NOT_AUTOCOMMIT;

It somehow affects ha_tokudb::create_txn().

I managed to reproduce the very same failure on a vanilla 10.2 if I just add
"SET autocommit=0" before "lock table t3 read".

> 
> ============
> 
> > 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?
It definitely would be better to keep all tests. There're complexities though...

Strictily speaking original test was about killing TRUNCATE duing
wait_while_table_is_used while it is upgrading MDL lock.

This wait is only possible if another thread holds MDL_SHARED, but currently
it's really hard to find anything that acquires MDL_SHARED. I only found that
prepared statements may do that for a short time. I can try to use them for
this purpose.

> 
> > ---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?
See open_tables():

  /*
    Close HANDLER tables which are marked for flush or against which there
    are pending exclusive metadata locks. This is needed both in order to
    avoid deadlocks and to have a point during statement execution at
    which such HANDLERs are closed even if they don't create problems for
    the current session (i.e. to avoid having a DDL blocked by HANDLERs
    opened for a long time).
  */
  if (thd->handler_tables_hash.records)
    mysql_ha_flush(thd);

> 
> >  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?
I guess it was supposed to say either "doesn't hold MDL_SHARED_READ" or
"is compatible with MDL_SHARE_NO_READ_WRITE".

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

> 
> >  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.
Ok, reverted this change.

Thanks,
Sergey