From: Date: July 24 2008 2:53pm
Subject: Re: bzr commit into mysql-6.0 branch (davi:2683) Bug#989, WL#4284
List-Archive: http://lists.mysql.com/commits/50409
Message-Id: <48887B52.3030309@Sun.COM>
MIME-Version: 1.0
Content-Type: text/plain; format=flowed; charset=ISO-8859-1
Content-Transfer-Encoding: 7BIT

Konstantin Osipov wrote:
> * 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?

Because that how the code behaved before and since I'm ignorant of the 
backup internals, I prefer to leave it as is.

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

Please be more specific, I tried to follow the assumptions in the low 
level design. I only made new assumptions for problems that the low 
level design wasn't specific, for example, the large amounts of code 
that don't respect a transaction protocol.

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

OK.

>> @@ -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.

I would have been much easier if you guys did move the the code before 
it to recover_from_*. Since that piece is not there, it makes impossible 
to move this piece.

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

There are places (DDL) that commits and release the locks but expect to 
be able to use the lock request again. If we changed this assumption, we 
would need a good amount of code rework to cleanup dangling references.

> In autocommit mode, there is a transaction too, so we can always
> use trans_mdl_root, no?

Not quite, see above.

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

You removed the patch context...