maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

Thanks for your feedback. Answers inline.

On Thu, Dec 15, 2016 at 02:23:03PM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
> 
> On Dec 13, Sergey Vojtovich wrote:
> > revision-id: 5f02c661f3d30baf08071acad29cef40fad0739c (mariadb-10.2.2-225-g5f02c66)
> > parent(s): 65b4d7457e40ee25302d2df28b0d200ff59d9e6d
> > committer: Sergey Vojtovich
> > timestamp: 2016-12-13 16:17:49 +0400
> > message:
> > 
> > MDEV-11227 - mysqlimport -l doesn't issue UNLOCK TABLES
> > 
> > Implementation of MDEV-7660 introduced unwanted incompatible change:
> > modifications under LOCK TABLES with autocommit enabled are rolled back on
> > disconnect. Previously everything was committed, because LOCK TABLES didn't
> > adjust autocommit setting.
> 
> Thanks, very helpful comment (that I didn't quote in my reply for
> brevity)!
> 
> Few questions below:
> 
> > diff --git a/mysql-test/t/delayed.test b/mysql-test/t/delayed.test
> > index dea16c8..30fb694 100644
> > --- a/mysql-test/t/delayed.test
> > +++ b/mysql-test/t/delayed.test
> > @@ -319,7 +319,7 @@ drop table if exists t1;
> >  --enable_warnings
> >  create table t1 (a int, b int);
> >  insert into t1 values (1,1);
> > -lock table t1 read;
> > +lock table t1 read local;
> 
> why?
This is remnant of intermediate patch, when MDL_SHARED_READ_ONLY was acquired for
MyISAM too. It'd block subsequent "insert delayed" much earlier, making this test
useless.

I reverted this change. Thanks for spotting!

> 
> >  connect (update,localhost,root,,);
> >  connection update;
> >  --send insert delayed into t1 values (2,2);
> > diff --git a/sql/mdl.h b/sql/mdl.h
> > index 7d659af..d77c07c 100644
> > --- a/sql/mdl.h
> > +++ b/sql/mdl.h
> > @@ -196,6 +196,12 @@ enum enum_mdl_type {
> >    */
> >    MDL_SHARED_UPGRADABLE,
> >    /*
> > +    A shared metadata lock for cases when we need to read data from table
> > +    and block all concurrent modifications to it (for both data and metadata).
> > +    Used by LOCK TABLES READ statement.
> > +  */
> > +  MDL_SHARED_READ_ONLY,
> 
> Hmm, stronger than MDL_SHARED_WRITE? All read locks were
> before write locks until now. Why is that changed?
I don't think your statement is completely valid: there's MDL_SHARED_NO_WRITE
(which is data read lock) after MDL_SHARED_READ_ONLY.

Also I don't completely understand how to evalutate strength of MDL_SHARED_*
locks by their position. They all do different things and have rather complex
compatibility and weight matrices.

This position was chosen by MySQL. I can only guess that they want to keep locks
up to SW to be used by DML and locks starting with SU used by DDL or LOCK TABLES.

> 
> > +  /*
> >      An upgradable shared metadata lock which blocks all attempts to update
> >      table data, allowing reads.
> >      A connection holding this kind of lock can read table metadata and read
> > diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> > index 1ca7e9c..d3f4517 100644
> > --- a/sql/sql_base.cc
> > +++ b/sql/sql_base.cc
> > @@ -1496,6 +1496,7 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx)
> >      to general/slow log would be disabled in read only transactions.
> >    */
> >    if (table_list->mdl_request.type >= MDL_SHARED_WRITE &&
> > +      table_list->mdl_request.type != MDL_SHARED_READ_ONLY &&
> 
> if you're going to keep read and write locks mixed,
> then I'd suggest to move this condition into a small inline helper,
> like table_list->mdl_request.is_write_lock().  it's used few times
> already in this very file and there will be only one place to change
> if we'll need to add, say, MDL_SHARED_WRITE_LOW_PRIO
I can merge this from MySQL, it is called is_write_lock_request() there. In fact
MySQL didn't have to change this condition for MDL_SHARED_WRITE_LOW_PRIO.

> 
> >        thd->tx_read_only &&
> >        !(flags & (MYSQL_LOCK_LOG_TABLE | MYSQL_OPEN_HAS_MDL_LOCK)))
> >    {
> > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> > index 6dc9192..0d740bb 100644
> > --- a/sql/sql_parse.cc
> > +++ b/sql/sql_parse.cc
> > @@ -2728,27 +2728,76 @@ bool sp_process_definer(THD *thd)
> >  static bool lock_tables_open_and_lock_tables(THD *thd, TABLE_LIST *tables)
> >  {
> >    Lock_tables_prelocking_strategy lock_tables_prelocking_strategy;
> > +  MDL_deadlock_and_lock_abort_error_handler deadlock_handler;
> > +  MDL_savepoint mdl_savepoint= thd->mdl_context.mdl_savepoint();
> >    uint counter;
> >    TABLE_LIST *table;
> >  
> >    thd->in_lock_tables= 1;
> >  
> > +retry:
> > +
> >    if (open_tables(thd, &tables, &counter, 0, &lock_tables_prelocking_strategy))
> >      goto err;
> >  
> > -  /*
> > -    We allow to change temporary tables even if they were locked for read
> > -    by LOCK TABLES. To avoid a discrepancy between lock acquired at LOCK
> > -    TABLES time and by the statement which is later executed under LOCK TABLES
> > -    we ensure that for temporary tables we always request a write lock (such
> > -    discrepancy can cause problems for the storage engine).
> > -    We don't set TABLE_LIST::lock_type in this case as this might result in
> > -    extra warnings from THD::decide_logging_format() even though binary logging
> > -    is totally irrelevant for LOCK TABLES.
> > -  */
> >    for (table= tables; table; table= table->next_global)
> > -    if (!table->placeholder() && table->table->s->tmp_table)
> > -      table->table->reginfo.lock_type= TL_WRITE;
> > +  {
> > +    if (!table->placeholder())
> > +    {
> > +      if (table->table->s->tmp_table)
> > +      {
> > +        /*
> > +          We allow to change temporary tables even if they were locked for read
> > +          by LOCK TABLES. To avoid a discrepancy between lock acquired at LOCK
> > +          TABLES time and by the statement which is later executed under LOCK
> > +          TABLES we ensure that for temporary tables we always request a write
> > +          lock (such discrepancy can cause problems for the storage engine).
> > +          We don't set TABLE_LIST::lock_type in this case as this might result
> > +          in extra warnings from THD::decide_logging_format() even though
> > +          binary logging is totally irrelevant for LOCK TABLES.
> > +        */
> > +        table->table->reginfo.lock_type= TL_WRITE;
> > +      }
> > +      else if (table->mdl_request.type == MDL_SHARED_READ &&
> > +               ! table->prelocking_placeholder &&
> > +               table->table->file->lock_count() == 0)
> > +      {
> > +        /*
> > +          In case when LOCK TABLE ... READ LOCAL was issued for table with
> > +          storage engine which doesn't support READ LOCAL option and doesn't
> > +          use THR_LOCK locks we need to upgrade weak SR metadata lock acquired
> > +          in open_tables() to stronger SRO metadata lock.
> > +          This is not needed for tables used through stored routines or
> > +          triggers as we always acquire SRO (or even stronger SNRW) metadata
> > +          lock for them.
> > +        */
> > +        deadlock_handler.init();
> > +        thd->push_internal_handler(&deadlock_handler);
> > +
> > +        bool result= thd->mdl_context.upgrade_shared_lock(
> > +                                        table->table->mdl_ticket,
> > +                                        MDL_SHARED_READ_ONLY,
> > +                                        thd->variables.lock_wait_timeout);
> > +
> > +        thd->pop_internal_handler();
> > +
> > +        if (deadlock_handler.need_reopen())
> > +        {
> > +          /*
> > +            Deadlock occurred during upgrade of metadata lock.
> > +            Let us restart acquring and opening tables for LOCK TABLES.
> 
> will you use MDL_SHARED_READ_ONLY right away when retrying?
> or it'll do the upgrade thing again?
Upgrade thing again, which is farily stupid. That's how it was in original code
and I didn't change it in a hurry.

Thanks,
Sergey