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

On Tue, 2010-01-05 at 10:08 +0000, Alfranio Correia wrote:
> 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.

OK. I get it. Thanks for the nice explanation on IRC. I get the 
full picture now. I am not skeptical anymore.

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

Ok. Given that the function is strict about it's return it would hurt
to enforce returning either 0 or 1, maybe: 

(!binlog_flush_stmt_cache(thd, cache_mngr) || !
binlog_flush_trx_cache(thd, cache_mngr, &xle) ? 1 : 0)

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