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)
