Hi Alfranio,
Patch approved. Just one observation inline.
On 10/27/2010 03:30 PM, Alfranio Correia wrote:
> #At
> file:///home/acorreia/workspace.sun/repository.mysql.new/bzrwork/bug-56343/mysql-5.5-bugteam/
> based on revid:li-bing.song@stripped
>
> 3247 Alfranio Correia 2010-10-27 [merge]
> BUG#56343 binlog_cache_use status is bigger than expected
>
> The binlog_cache_use is incremented twice when changes to a transactional
> table
> are committed, i.e. 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.
>
> Although not explicitly mentioned in the bug, the binlog_cache_disk_use
> presents
> the same problem.
>
> The binlog_cache_use and binlog_cache_disk_use are status variables that are
> incremented when transactional (i.e. trx-cache) or non-transactional (i.e.
> stmt-cache) changes are committed. They are used to compute the ratio between
> the use of disk and memory while gathering updates for a transaction.
>
> The problem reported is fixed by avoiding incrementing the binlog_cache_use
> and binlog_cache_disk_use if a cache is empty. We also have decided to
> increment
> both variables when a cache is truncated as the cache is used although its
> content is discarded and is not written to the binary log.
>
> In this patch, we take the opportunity to re-organize the code around the
> function binlog_flush_trx_cache and binlog_flush_stmt_cache.
> @ mysql-test/extra/binlog_tests/binlog_cache_stat.test
> Updated the test case with comments and additional tests to check both
> transactional and non-transactional changes and what happens when a
> is transaction either committed or aborted.
> @ mysql-test/suite/binlog/r/binlog_innodb.result
> Updated the result file.
> @ mysql-test/suite/binlog/r/binlog_mixed_cache_stat.result
> Updated the result file.
> @ mysql-test/suite/binlog/r/binlog_row_cache_stat.result
> Updated the result file.
> @ mysql-test/suite/binlog/r/binlog_stm_cache_stat.result
> Updated the result file.
> @ mysql-test/suite/binlog/t/binlog_mixed_cache_stat.test
> Updated the test case with a new source file.
> @ mysql-test/suite/binlog/t/binlog_row_cache_stat.test
> Updated the test case with a new source file.
> @ mysql-test/suite/binlog/t/binlog_stm_cache_stat.test
> Updated the test case with a new source file.
> @ sql/log_event.h
> Guaranteed Xid_log_event is always classified as a transactional event.
>
[snip]
>
> It has the responsibility to flush the caches to the binary log on commits.
> @@ -1690,19 +1739,7 @@ static int binlog_commit(handlerton *hto
> YESNO(thd->transaction.all.modified_non_trans_table),
> YESNO(thd->transaction.stmt.modified_non_trans_table)));
>
> - if (!cache_mngr->stmt_cache.empty())
> - {
> - binlog_flush_stmt_cache(thd, cache_mngr);
> - }
> -
> - if (cache_mngr->trx_cache.empty())
> - {
> - /*
> - we're here because cache_log was flushed in MYSQL_BIN_LOG::log_xid()
> - */
> - cache_mngr->reset_cache(&cache_mngr->trx_cache);
> - DBUG_RETURN(0);
> - }
I was just worried about the removal of this if ^.
But, since you'd rather remove it and since I have no
strong opinion, then I guess you can keep it.
> + error= binlog_commit_flush_stmt_cache(thd, cache_mngr);
>
> /*
> We commit the transaction if:
> @@ -1710,17 +1747,15 @@ static int binlog_commit(handlerton *hto
> - We are in a transaction and a full transaction is committed.
> Otherwise, we accumulate the changes.
> */
> - if (ending_trans(thd, all))
> - {
> - Query_log_event qev(thd, STRING_WITH_LEN("COMMIT"), TRUE, FALSE, TRUE, 0);
> - error= binlog_flush_trx_cache(thd, cache_mngr,&qev);
> - }
> + if (!error&& ending_trans(thd, all))
> + error= binlog_commit_flush_trx_cache(thd, cache_mngr);
>
> /*
> This is part of the stmt rollback.
> */
> if (!all)
> cache_mngr->trx_cache.set_prev_position(MY_OFF_T_UNDEF);
> +
> DBUG_RETURN(error);
> }
>
> @@ -1755,19 +1790,8 @@ static int binlog_rollback(handlerton *h
> error= mysql_bin_log.write_incident(thd, TRUE);
> cache_mngr->reset_cache(&cache_mngr->stmt_cache);
> }
> - else if (!cache_mngr->stmt_cache.empty())
> - {
> - binlog_flush_stmt_cache(thd, cache_mngr);
> - }
> -
> - if (cache_mngr->trx_cache.empty())
> - {
> - /*
> - we're here because cache_log was flushed in MYSQL_BIN_LOG::log_xid()
> - */
> - cache_mngr->reset_cache(&cache_mngr->trx_cache);
> - DBUG_RETURN(0);
> - }
Same here ^.
> + else
> + error= binlog_commit_flush_stmt_cache(thd, cache_mngr);
>
> if (mysql_bin_log.check_write_error(thd))
> {
> @@ -1784,7 +1808,7 @@ static int binlog_rollback(handlerton *h
> */
> error= binlog_truncate_trx_cache(thd, cache_mngr, all);
> }
> - else
> + else if (!error)
> {
> /*
> We flush the cache wrapped in a beging/rollback if:
> @@ -1796,7 +1820,6 @@ static int binlog_rollback(handlerton *h
> . the format is MIXED, non-trans table was updated and
> aborting a single statement transaction;
> */
> -
> if (ending_trans(thd, all)&&
> ((thd->variables.option_bits& OPTION_KEEP_LOG) ||
> (trans_has_updated_non_trans_table(thd)&&
> @@ -1806,10 +1829,7 @@ static int binlog_rollback(handlerton *h
> (trans_has_updated_non_trans_table(thd)&&
> ending_single_stmt_trans(thd,all)&&
> thd->variables.binlog_format == BINLOG_FORMAT_MIXED)))
> - {
> - Query_log_event qev(thd, STRING_WITH_LEN("ROLLBACK"), TRUE, FALSE, TRUE, 0);
> - error= binlog_flush_trx_cache(thd, cache_mngr,&qev);
> - }
> + error= binlog_rollback_flush_trx_cache(thd, cache_mngr);
> /*
> Truncate the cache if:
> . aborting a single or multi-statement transaction or;
> @@ -1832,7 +1852,8 @@ static int binlog_rollback(handlerton *h
> This is part of the stmt rollback.
> */
> if (!all)
> - cache_mngr->trx_cache.set_prev_position(MY_OFF_T_UNDEF);
> + cache_mngr->trx_cache.set_prev_position(MY_OFF_T_UNDEF);
> +
> DBUG_RETURN(error);
> }
>
> @@ -6309,15 +6330,14 @@ void TC_LOG_BINLOG::close()
> int TC_LOG_BINLOG::log_xid(THD *thd, my_xid xid)
> {
> DBUG_ENTER("TC_LOG_BINLOG::log");
> - Xid_log_event xle(thd, xid);
> binlog_cache_mngr *cache_mngr=
> (binlog_cache_mngr*) thd_get_ha_data(thd, binlog_hton);
> /*
> 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(!binlog_commit_flush_stmt_cache(thd, cache_mngr)&&
> + !binlog_commit_flush_trx_cache(thd, cache_mngr, xid));
> }
>
> void TC_LOG_BINLOG::unlog(ulong cookie, my_xid xid)
>
> === modified file 'sql/log_event.h'
> --- a/sql/log_event.h 2010-10-13 10:08:39 +0000
> +++ b/sql/log_event.h 2010-10-27 14:29:53 +0000
> @@ -2506,7 +2506,7 @@ class Xid_log_event: public Log_event
> my_xid xid;
>
> #ifdef MYSQL_SERVER
> - Xid_log_event(THD* thd_arg, my_xid x): Log_event(thd_arg,0,0), xid(x) {}
> + Xid_log_event(THD* thd_arg, my_xid x): Log_event(thd_arg, 0, TRUE), xid(x) {}
> #ifdef HAVE_REPLICATION
> void pack_info(Protocol* protocol);
> #endif /* HAVE_REPLICATION */
>
>
>
>
>
