List:Commits« Previous MessageNext Message »
From:Alfranio Correia Date:October 18 2010 1:40am
Subject:Re: bzr commit into mysql-5.5-bugfixing branch (alfranio.correia:3203)
Bug#56343
View as plain text  
Hi Luis,

Thank you for the review.

On 10/14/2010 03:43 PM, Luís Soares wrote:
> Hi Alfranio,
>
>    Nice Work. We're not there yet... but we're on the right
>    track. Thanks.  Please find my review comments below.
>
> STATUS
> ------
>
>    Not approved.
>
> REQUIRED CHANGES
> ----------------
>
>    RC1. Please make it clear what happens for binlog_cache_disk_use
>
>         See commit message:
>
>         "
>         (...)
>         Note that binlog_cache_use is incremented for both types of
>         cache, transactional and non-transactional and that the
>         behavior presented in this patch also applies to the
>         binlog_cache_disk_use.
>         (...)
>         "

ok.

>
>    RC2. Please rewrite/make it more clear.
>
>       @ sql/log_event.cc
>          Introduced debug information to check if Query_log_event("BEGIN"),
>          Query_log_event("COMMIT") and Query_log_event("ROLLBACK").
>
>         What exactly does this mean?

This was something introduced and used in some assertions created in the patch
and related to some suggestions made by Andrei.

He had suggested to create a static reference to "BEGIN", "COMMIT", "ROLLBACK"
and XID, thus avoiding to allocate such objects every time they are needed.

However, as I will not do the refactory suggested by Andrei I will remove this.

>
>    RC3. log.cc hunk: @@ -262,6 +265,14 @@
>
>         The comment is still not clear to me.
>
>         "Binlog_cache_disk_use
>
>          The number of transactions that used the temporary binary
>          log cache but that exceeded the value of
>          binlog_cache_size and used a temporary file to store
>          statements from the transaction."
>
>         Why can't we set it to one and increment it on
>         binlog_compute_statistics ? I guess that's because it can
>         happen when calling reset on the binlog_cache_data (which
>         truncates the cache), and that is after
>         binlog_compute_statistics. Please improve the comment.
>
>         Why is truncate setting the io_cache disk_writes ?

ok.

>
>
>    RC4. Why all these flush functions? Can't we just use
>         binlog_flush_cache directly?
>
>         I think these can be removed and replaced by call to
>         binlog_flush_cache:
>
>         binlog_commit_flush_stmt_cache
>         binlog_commit_flush_trx_cache
>         binlog_rollback_flush_trx_cache
>         binlog_commit_flush_trx_cache

This does not introduce any performance penalty and makes the
code clear. Besides this was a requirement to Andrei's suggestions.

>
>    RC5. There are some places in the source code that the return
>         value for binlog_..._flush_..._cache is disregarded and
>         some where they are not. Eg, in binlog_rollback. Why ?
>         Shouldn't we check for errors everywhere ?

Only when the statement cache is flushed, the return value is
disregarded. Note that the patch does not change this behavior.

I can investigate this as part of another bug report if you
want to.

>
>    RC6. In sql/log_event.cc it would be better to add to the
>         comment, how the test case benefits from this. Quite
>         frankly, it's not obvious to me either.

I don't know what you had in mind with this comment but the
debug information introduced is not related to the test cases.
Note that there is no sync or debug point.

Anyway, I will remove such information as they are not necessary
for now. See comments above on this.

>
>    RC7. Given that there are some dependencies for code that is
>         compiled in only in debug mode, should the test case(s)
>         have:
>
>         -- source include/have_debug.inc ?

There is no need for that. See comments above.

>
>    RC8. The assertions are quite loose, and vulnerable to result
>         rewrites... Can you please rewrite them in some way
>         similar to:
>
>         let $expected= 1
>         let $got= query_get_value(show status like "binlog_cache_disk_use", Value,
> 1);
>
>         if (`SELECT $expected<>  $got`)
>         {
>            -- echo "Fill your assertion failed message here"
>            -- die
>         }

I agree with you. I started writing an improved version of the test case with several
combinations, bla, bla and with assertions. I don't know if I will have time to finish
it so I will keep the current test cases and just introduce the assertions.

>
> REQUESTS
> --------
>   n/a
>
> SUGGESTIONS
> -----------
>
>    S1. Please rewrite:
>
>       @ sql/log_event.h
>          Guaranteed Xid_log_event is always classified as a transactional event.
>
>         I would suggest: Makes Xid_log_event to be always classified ...

ok.

Cheers.
Thread
bzr commit into mysql-5.5-bugfixing branch (alfranio.correia:3203) Bug#56343Alfranio Correia6 Oct
  • Re: bzr commit into mysql-5.5-bugfixing branch (alfranio.correia:3203)Bug#56343Luís Soares14 Oct
    • Re: bzr commit into mysql-5.5-bugfixing branch (alfranio.correia:3203)Bug#56343Alfranio Correia18 Oct
      • Re: bzr commit into mysql-5.5-bugfixing branch (alfranio.correia:3203)Bug#56343Luís Soares5 Nov
  • Re: bzr commit into mysql-5.5-bugfixing branch (alfranio.correia:3203)Bug#56343Alfranio Correia18 Oct