maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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?

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

> +  /*
>      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

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

> +          */
> +          close_tables_for_reopen(thd, &tables, mdl_savepoint);
> +          if (thd->open_temporary_tables(tables))
> +            goto err;
> +          goto retry;
> +        }
> +
> +        if (result)
> +          goto err;
> +      }
> +    }
> +  }
>  
>    if (lock_tables(thd, tables, counter, 0) ||
>        thd->locked_tables_list.init_locked_tables(thd))


Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx