List:Commits« Previous MessageNext Message »
From:Luís Soares Date:January 5 2010 9:41am
Subject:Re: bzr commit into mysql-5.1-rep+3 branch (alfranio.correia:3130)
Bug#50038
View as plain text  
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?

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

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)

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

>       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