List:Commits« Previous MessageNext Message »
From:He Zhenxing Date:September 3 2010 2:40am
Subject:Re: bzr commit into mysql-5.5-bugfixing branch (alfranio.correia:3196)
Bug#56343
View as plain text  
Hi Alfranio,

Nice work, please look for some inline comments!

Alfranio Correia wrote:
> #At
> file:///home/acorreia/workspace.sun/repository.mysql.new/bzrwork/bug-56343/mysql-5.5-bugfixing/
> based on revid:dao-gang.qu@stripped
> 
>  3196 Alfranio Correia	2010-09-02
>       BUG#56343 binlog_cache_use status is bigger than expected
>       
>       The binlog_cache_use is incremented twice when changes to a transactional
> table
>       is committed and TC_LOG_BINLOG::log_xid calls is called. The problem happens
>       because log_xid calls both binlog_flush_stmt_cache and binlog_flush_trx_cache
>       without checking if such caches are empty thus unintentionally increasing the
>       binlog_cache_use value twice.
>       
>       To fix the problem we did what follows:
>       
>       === modified file 'sql/log.cc'
>       --- sql/log.cc  2010-08-20 02:59:58 +0000
>       +++ sql/log.cc  2010-08-30 13:21:23 +0000
>       @@ -6278,8 +6278,11 @@
>            We always commit the entire transaction when writing an XID. Also
>            note that the return value is inverted.
>           */
>       -  DBUG_RETURN(!binlog_flush_stmt_cache(thd, cache_mngr) &&
>       -              !binlog_flush_trx_cache(thd, cache_mngr, &xle));
>       +  DBUG_RETURN(
>       +   (!cache_mngr->stmt_cache.empty() ?
>       +    !binlog_flush_stmt_cache(thd, cache_mngr) : TRUE) &&
>       +   (!cache_mngr->trx_cache.empty() ?
>       +    !binlog_flush_trx_cache(thd, cache_mngr, &xle) : TRUE));
>        }

Please describe what you did instead of using patch code in the commit
messages.

> 
>     modified:
>       mysql-test/suite/binlog/r/binlog_innodb.result
>       mysql-test/suite/binlog/r/binlog_mix_innodb_stat.result
>       mysql-test/suite/binlog/r/binlog_row_innodb_stat.result
>       mysql-test/suite/binlog/r/binlog_stm_innodb_stat.result
>       sql/log.cc
> === modified file 'mysql-test/suite/binlog/r/binlog_innodb.result'
> --- a/mysql-test/suite/binlog/r/binlog_innodb.result	2010-06-17 13:31:51 +0000
> +++ b/mysql-test/suite/binlog/r/binlog_innodb.result	2010-09-02 17:49:31 +0000
> @@ -123,7 +123,7 @@ Binlog_cache_disk_use	0
>  create table t1 (a int) engine=innodb;
>  show status like "binlog_cache_use";
>  Variable_name	Value
> -Binlog_cache_use	2
> +Binlog_cache_use	1
>  show status like "binlog_cache_disk_use";
>  Variable_name	Value
>  Binlog_cache_disk_use	1
> @@ -132,7 +132,7 @@ delete from t1;
>  commit;
>  show status like "binlog_cache_use";
>  Variable_name	Value
> -Binlog_cache_use	4
> +Binlog_cache_use	2
>  show status like "binlog_cache_disk_use";
>  Variable_name	Value
>  Binlog_cache_disk_use	1
> 
> === modified file 'mysql-test/suite/binlog/r/binlog_mix_innodb_stat.result'
> --- a/mysql-test/suite/binlog/r/binlog_mix_innodb_stat.result	2010-01-05 16:55:23
> +0000
> +++ b/mysql-test/suite/binlog/r/binlog_mix_innodb_stat.result	2010-09-02 17:49:31
> +0000
> @@ -9,7 +9,7 @@ drop table if exists t1;
>  create table t1 (a int) engine=innodb;
>  show status like "binlog_cache_use";
>  Variable_name	Value
> -Binlog_cache_use	2
> +Binlog_cache_use	1
>  show status like "binlog_cache_disk_use";
>  Variable_name	Value
>  Binlog_cache_disk_use	1
> @@ -18,7 +18,7 @@ delete from t1;
>  commit;
>  show status like "binlog_cache_use";
>  Variable_name	Value
> -Binlog_cache_use	4
> +Binlog_cache_use	2
>  show status like "binlog_cache_disk_use";
>  Variable_name	Value
>  Binlog_cache_disk_use	1
> 
> === modified file 'mysql-test/suite/binlog/r/binlog_row_innodb_stat.result'
> --- a/mysql-test/suite/binlog/r/binlog_row_innodb_stat.result	2010-01-05 16:55:23
> +0000
> +++ b/mysql-test/suite/binlog/r/binlog_row_innodb_stat.result	2010-09-02 17:49:31
> +0000
> @@ -9,7 +9,7 @@ drop table if exists t1;
>  create table t1 (a int) engine=innodb;
>  show status like "binlog_cache_use";
>  Variable_name	Value
> -Binlog_cache_use	2
> +Binlog_cache_use	1
>  show status like "binlog_cache_disk_use";
>  Variable_name	Value
>  Binlog_cache_disk_use	1
> @@ -18,7 +18,7 @@ delete from t1;
>  commit;
>  show status like "binlog_cache_use";
>  Variable_name	Value
> -Binlog_cache_use	4
> +Binlog_cache_use	2
>  show status like "binlog_cache_disk_use";
>  Variable_name	Value
>  Binlog_cache_disk_use	1
> 
> === modified file 'mysql-test/suite/binlog/r/binlog_stm_innodb_stat.result'
> --- a/mysql-test/suite/binlog/r/binlog_stm_innodb_stat.result	2010-01-05 16:55:23
> +0000
> +++ b/mysql-test/suite/binlog/r/binlog_stm_innodb_stat.result	2010-09-02 17:49:31
> +0000
> @@ -9,7 +9,7 @@ drop table if exists t1;
>  create table t1 (a int) engine=innodb;
>  show status like "binlog_cache_use";
>  Variable_name	Value
> -Binlog_cache_use	2
> +Binlog_cache_use	1
>  show status like "binlog_cache_disk_use";
>  Variable_name	Value
>  Binlog_cache_disk_use	1
> @@ -18,7 +18,7 @@ delete from t1;
>  commit;
>  show status like "binlog_cache_use";
>  Variable_name	Value
> -Binlog_cache_use	4
> +Binlog_cache_use	2
>  show status like "binlog_cache_disk_use";
>  Variable_name	Value
>  Binlog_cache_disk_use	1
> 
> === modified file 'sql/log.cc'
> --- a/sql/log.cc	2010-08-20 02:59:58 +0000
> +++ b/sql/log.cc	2010-09-02 17:49:31 +0000
> @@ -1543,9 +1543,11 @@ binlog_flush_trx_cache(THD *thd, binlog_
>    */
>    error= mysql_bin_log.write(thd, &cache_mngr->trx_cache.cache_log, end_ev,
>                               cache_mngr->trx_cache.has_incident());
> +
> +  if (!cache_mngr->trx_cache.empty())
> +    statistic_increment(binlog_cache_use, &LOCK_status);

I think the function can return here if the cache is empty.

if (cache_mngr->trx_cache.empty())
  DBUG_RETURN(error);

And I also think we can avoid calling the mysql_bin_log.write() above if
the cache is empty because it will write nothing.

>    cache_mngr->reset_cache(&cache_mngr->trx_cache);
>  
> -  statistic_increment(binlog_cache_use, &LOCK_status);
>    if (cache_log->disk_writes != 0)
>    {
>      statistic_increment(binlog_cache_disk_use, &LOCK_status);
> @@ -1653,9 +1655,11 @@ binlog_flush_stmt_cache(THD *thd, binlog
>    if ((error= mysql_bin_log.write(thd, cache_log, &qev,
>                                    cache_mngr->stmt_cache.has_incident())))
>      DBUG_RETURN(error);
> +
> +  if (!cache_mngr->stmt_cache.empty())
> +    statistic_increment(binlog_cache_use, &LOCK_status);
>    cache_mngr->reset_cache(&cache_mngr->stmt_cache);
>  
> -  statistic_increment(binlog_cache_use, &LOCK_status);
>    if (cache_log->disk_writes != 0)
>    {
>      statistic_increment(binlog_cache_disk_use, &LOCK_status);
> @@ -6278,8 +6282,11 @@ 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_stmt_cache(thd, cache_mngr) &&
> -              !binlog_flush_trx_cache(thd, cache_mngr, &xle));
> +  DBUG_RETURN(
> +   (!cache_mngr->stmt_cache.empty() ?
> +    !binlog_flush_stmt_cache(thd, cache_mngr) : TRUE) &&
> +   (!cache_mngr->trx_cache.empty() ?
> +    !binlog_flush_trx_cache(thd, cache_mngr, &xle) : TRUE));

I think this is not necessary since binlog_flush_trx_cache() and
binlog_flush_stmt_cache() already do the check also. So I'd suggest to
avoid duplicate the check here.

>  }
>  
>  void TC_LOG_BINLOG::unlog(ulong cookie, my_xid xid)


Thread
bzr commit into mysql-5.5-bugfixing branch (alfranio.correia:3196) Bug#56343Alfranio Correia2 Sep
  • Re: bzr commit into mysql-5.5-bugfixing branch (alfranio.correia:3196)Bug#56343He Zhenxing3 Sep
    • Re: bzr commit into mysql-5.5-bugfixing branch (alfranio.correia:3196)Bug#56343Alfranio Correia3 Sep