List:Commits« Previous MessageNext Message »
From:Luís Soares Date:November 5 2010 10:06am
Subject:Re: bzr commit into mysql-5.5-bugfixing branch (alfranio.correia:3203)
Bug#56343
View as plain text  
Hi,

On 10/18/2010 02:40 AM, Alfranio Correia wrote:
> 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.

Thanks.

>>
>> 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.

OK.

>>
>> 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.

It's good now.

>>
>>
>> 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.

You're not doing Andrei's suggestion anymore. But, I have no
strong opinion (it just seems a waste of CPU with one more level
of indirection... however, the compiler should optimize it anyway).

Meh... Do as you like (which means you get to keep them because
that's how it is in the new patch).

>>
>> 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.

Yes.

>>
>> 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.

That's how much it confused me...

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

Good.

>>
>> 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.

Sure.

>>
>> 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.

OK.

>>
>> 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.
>

Regards,
Luís Soares
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