Re: bzr commit into mysql-6.0 branch (davi:2683) Bug#989, WL#4284

[Konstantin Osipov]

* Davi Arnaut <Davi.Arnaut@stripped> [08/07/23 20:17]:

> === modified file 'sql/backup/data_backup.cc'
> --- a/sql/backup/data_backup.cc	2008-03-20 14:53:16 +0000
> +++ b/sql/backup/data_backup.cc	2008-07-23 16:12:09 +0000
> @@ -1585,7 +1585,7 @@ int restore_table_data(THD* thd, Restore
>      Close all tables if default or snapshot driver used.
>    */
>    if (table_list)
> -    close_thread_tables(::current_thd);
> +    close_thread_tables(::current_thd, CTT_RELEASE_TRANS_LOCKS);

Is the transaction active? If not, why do you need to specify
explicitly that the locks got to be released?

As far as I can tell, your patch has gone quite a bit off the low
level design. Could you please describe what assumptions
in the low level design were incorrect, and what solutions you 
suggest to deploy for the problems you've encountered?

> +  /*
> +    Allocate a new metadata lock request for the table object that
> +    doesn't have one already. This is done at this point because a
> +    implicit commit (with locks release and freeing) might have been
> +    issued after this object was assembled.
> +  */
> +  if (! table_list->mdl_lock_data)
> +   table_list->mdl_lock_data= mdl_alloc_lock(0, table_list->db,
> +                                             table_list->table_name,
> +                                             thd->lock_data_root());
> +

In that case we're better off never allocating a lock outside
open_tables(). I will discuss with Dmitry.
> @@ -3518,6 +3538,7 @@ int open_tables(THD *thd, TABLE_LIST **s
>    /* Also used for indicating that prelocking is need */
>    TABLE_LIST **query_tables_last_own;
>    bool safe_to_ignore_table;
> +  bool has_locks= mdl_has_locks(&thd->mdl_context);
>  
>    DBUG_ENTER("open_tables");
>    /*
> @@ -3660,6 +3681,18 @@ int open_tables(THD *thd, TABLE_LIST **s
>        if (action)
>        {
>          /*
> +          We have met a exclusive metadata lock or a old version of table and
> +          we are inside a transaction that already hold locks. We can't follow
> +          the locking protocol in this scenario as it might lead to deadlocks.
> +        */
> +        if ((thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))
> && has_locks)
> +        {
> +          my_error(ER_LOCK_DEADLOCK, MYF(0));
> +          result= -1;
> +          goto err;
> +        }

I think you should merge close_thread_tables() into
recover_from_* function, and move this code there. 


> +  /**
> +    Returns a pointer to the memory root from which to allocate
> +    metadata locks. It returns the transactional mdl root if
> +    inside a transaction context, otherwise return the suitable
> +    memory root for the scope of this statement.
> +  */
> +  MEM_ROOT *lock_data_root()
> +  {
> +    MEM_ROOT *root= mem_root;
> +
> +    if (locked_tables_root)
> +      root= locked_tables_root;
> +    else if (options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))
> +      root= &trans_mdl_root;
> +
> +    return root;

What's the point of ever returning thd->mem_root?
In autocommit mode, there is a transaction too, so we can always
use trans_mdl_root, no?

> -    if (trans_commit_implicit(thd))
> +    if (trans_commit_implicit(thd, TRANS_RELEASE_MDL))
>        goto error;

Now the loop has closed. close-thread-tables commits the
transaction, sometimes, and commit of transaction sometimes closes
the thread tables. 
Why do you need this?

--