List:Commits« Previous MessageNext Message »
From:Luís Soares Date:November 5 2010 11:45am
Subject:Re: bzr commit into mysql-5.5-bugteam branch (alfranio.correia:3247)
Bug#56343
View as plain text  
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 */
>
>
>
>
>

Thread
bzr commit into mysql-5.5-bugteam branch (alfranio.correia:3247) Bug#56343Alfranio Correia27 Oct
  • Re: bzr commit into mysql-5.5-bugteam branch (alfranio.correia:3247)Bug#56343Luís Soares5 Nov