List:Commits« Previous MessageNext Message »
From:Alfranio Correia Date:January 5 2010 10:08am
Subject:Re: bzr commit into mysql-5.1-rep+3 branch (alfranio.correia:3130)
Bug#50038
View as plain text  
Hi Luis,

Thank you for the review.

Luís Soares wrote:
> Hi Alfranio,
>   Nice Work. Please find my review comments below.
> 
> STATUS
> ------
>   Approved after consideration.
> 
> REQUIRED CHANGES
> ----------------
>  n/a
> 
> REQUESTS
> --------
> 
>   R1. This is a request for clarification. 
> 
>       So this patch will avoid flushing the stmt cache in
>       *binlog_commit*, always, whenever one has XIDs
>       involved. This makes indeed flushing the statement cache
>       before the transactional one. My only concern is: what if
>       there are already prepared XIDs by the time we flush stmt
>       cache? Can this happen at all? If it does, wont we hit the
>       same deadlock, ie, FLUSH LOGS gets log_lock, and sleeps on
>       prep_xids_cond and immediately after, on another thread, we
>       try to flush stmt cache and block on log_lock?
> 
>       In other words, this patch assumes that prepared_xids is
>       always zero by the time binlog_flush_stmt_cache is called,
>       otherwise, there can be a deadlock between FLUSH LOGS and
>       the flushing... Is it not? If so, should we assert?

You have the wrong assumption.

The patch never assumes that the prepared_xids is zero when the flush_stmt is
called. It just ensures that the prepared_xids is always increased after calling
flush_stmt.

Thus if you call flush_stmt and a concurrent flush is issued, the flush_stmt may block,
but a deadlock will never happen because the prepared_xids will eventually get to zero.
Bottom line, there will not be any new transaction able to increase the prepared_xids
because they will block in (&LOCK_log) and those that increased the prepared_xids will
eventually commit.

Answering your question, we cannot have the assertion.


> 
>   R2. Why the bitwise operator '|' ? Wouldn't a logic or ('||')
>       suit better? See comment inline.

I will use it.

> 
> SUGGESTIONS
> -----------
>  n/a
> 
> DETAILS 
> -------
>   See some more comments inline.
> 
> On Tue, 2010-01-05 at 00:15 +0000, Alfranio Correia wrote:
>> #At
> file:///home/acorreia/workspace.sun/repository.mysql/bzrwork/bug-50038/mysql-5.1-rep%2B3/
> based on revid:dao-gang.qu@stripped
>>
>>  3130 Alfranio Correia	2010-01-05
>>       BUG#50038 Deadlock on flush logs with concurrent DML and RBR
>>       
>>       In auto-commit mode, updating both trx and non-trx tables (i.e. issuing a
> mixed statement)
>>       causes the following sequence of events:
>>       
>>       1 - Flush trx changes (MYSQL_BIN_LOG::write):
>>         1.1 - mutex_lock (&LOCK_log)
>>         1.2 - mutex_lock (&LOCK_prep_xids)
>>         1.3 - increase prepared_xids
>>         1.4 - mutex_unlock (&LOCK_prep_xids)
>>         1.5 - mutex_unlock (&LOCK_log)
>>       
>>       2 - Flush non-trx changes (MYSQL_BIN_LOG::write):
>>         2.1 - mutex_lock (&LOCK_log)
>>         2.2 - mutex_unlock (&LOCK_log)
> 
>         3. unlog 
>           3.1 - mutex_lock (&LOCK_prep_xids)
>           3.2 - decrease prepared xids
>           3.3 - pthread_cond_signal(&COND_prep_xids);
>           3.4 - mutex_unlock (&LOCK_prep_xids)


There is no need to clutter the explanation with that.
We just need to explain that eventually the prepared_xids will be decreased when the
transaction
commits.

> 
>>       
>>       If a FLUSH logs happens between the two flushes, a deadlock may arise due
> to the following
>>       sequence of events while processing the command:
>>       
>>       3 - FLUSH logs command (MYSQL_BIN_LOG::new_file_impl):
>>         3.1 - mutex_lock (&LOCK_log)
>>         3.2 - mutex_lock (&LOCK_prep_xids)
>>         3.3 - while (prepared_xids)  pthread_cond_wait(...,
> &LOCK_prep_xids);
>>         3.4 - mutex_unlock (&LOCK_prep_xids)
>>         3.5 - mutex_unlock (&LOCK_log)
>>       
>>       The execution will pass the while (item 3.3), when the prepared_xids is
> decreased what
>>       will happen after the non-trx changes are flushed. However, the transaction
> cannot
>>       progress because the mutex LOCK_log is grabbed on behalf of the FLUSH
> command.
> 
> This is not clear. Can you please rewrite (maybe with references 
> to ha_commit and unlog) ?

I will rewrite that but I don't think we need references to the ha_commit and unlog.

> 
>>       To fix the problem, we ensure that the non-trx changes are always flushed
> before the
>>       trx changes.
>>
>>     modified:
>>       sql/log.cc
>> === modified file 'sql/log.cc'
>> --- a/sql/log.cc	2009-12-14 10:40:42 +0000
>> +++ b/sql/log.cc	2010-01-05 00:15:27 +0000
>> @@ -5958,7 +5958,8 @@ int TC_LOG_BINLOG::log_xid(THD *thd, my_
>>      We always commit the entire transaction when writing an XID. Also
>>      note that the return value is inverted.
>>     */
>> -  DBUG_RETURN(!binlog_flush_trx_cache(thd, cache_mngr, &xle));
>> +  DBUG_RETURN(!binlog_flush_stmt_cache(thd, cache_mngr) |
>> +              !binlog_flush_trx_cache(thd, cache_mngr, &xle));
>>  }
> 
> R2. Why:
> 
>     !binlog_flush_stmt_cache(thd, cache_mngr) | 
>     !binlog_flush_trx_cache(thd, cache_mngr, &xle)
> 
>     and not:
> 
>     !binlog_flush_stmt_cache(thd, cache_mngr) ||
>     !binlog_flush_trx_cache(thd, cache_mngr, &xle)
> 
>     ?
> 
>>  void TC_LOG_BINLOG::unlog(ulong cookie, my_xid xid)
>>
> 
> 
Thread
bzr commit into mysql-5.1-rep+3 branch (alfranio.correia:3130)Bug#50038Alfranio Correia5 Jan
  • Re: bzr commit into mysql-5.1-rep+3 branch (alfranio.correia:3130)Bug#50038Luís Soares5 Jan
    • Re: bzr commit into mysql-5.1-rep+3 branch (alfranio.correia:3130)Bug#50038Alfranio Correia5 Jan
      • Re: bzr commit into mysql-5.1-rep+3 branch (alfranio.correia:3130)Bug#50038Luís Soares5 Jan