List:Commits« Previous MessageNext Message »
From:Ingo Strüwing Date:December 9 2009 2:19pm
Subject:Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2903)
Bug#43596
View as plain text  
Hi Chuck,

a comment from one reviewer to the other.

Charles Bell, 07.12.2009 17:53:

...
>> +--lower-case-table-names=1
> 
> [3] With so many tests now using lctn values, wouldn't it make more
> sense to have a backup_lctn suite that iterates over the values rather
> than a series of ...lcntN tests?


Nice idea. Worth to think about it. But I fear, we may need different
result files per lctn value. If this can be done in a special suite,
then go ahead.

...
>> -  DBUG_EXECUTE_IF("ER_BACKUP_LIST_DBS", dbit= 0;);
>> +  DBUG_EXECUTE_IF("ER_BACKUP_LIST_DBS", { delete dbit; dbit= NULL; };);
> 
> [5] I think the convention should be to do this:
> 
>    DBUG_EXECUTE_IF("something",
>    {
>      <stmt>
>      <stmt>
>    };);
> 
> Indeed, we see in stream.cc this is how we've done it in the past.
> 
>   {
>     DBUG_EXECUTE_IF("backup_read_simulate_pipe",{
>       /*
>         Simulate reading from a pipe, when less bytes is read...
>         requested.
>       */
>       if (howmuch > 1024) howmuch= 1024;
>     });
> 
> Please use this format for the debug execute if statements that have
> more than one statement. This repeats several times in this patch.


I vote against such convention. It is debugging code. It should be
compact to leave the least possible optical impact on the source, while
still somewhat readable.

But if I would get outvoted, then I would insist in correctly formatted
code within the macro. 'howmuch= 1024' must not be on the same line as
the 'if'.

...
>> === modified file 'sql/backup/kernel.cc'
...
>> +    switch (item->type)
>> +    {
...
>> +      default: DBUG_ASSERT(FALSE);    
> 
> [9] Why is the default done like this. I don't understand why it would
> be written this way. Shouldn't it simply return or thrown an error? It
> seems this would be a far more severe issue than a debugging solution.


I missed that one. If item->type has been checked immediately after
reading it from the image, then an assert is fine here. If it has not
been checked, we need to consider corrupt images and catch it smoothly.

...

Regards
Ingo
-- 
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder,   Wolfgang Engels,   Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring   HRB München 161028
Thread
bzr commit into mysql-6.0-backup branch (Rafal.Somla:2903) Bug#43596Rafal Somla4 Dec
  • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2903)Bug#43596Charles Bell7 Dec
    • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2903)Bug#43596Charles Bell8 Dec
    • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2903)Bug#43596Ingo Strüwing9 Dec
    • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2903)Bug#43596Rafal Somla10 Dec
  • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2903)Bug#43596Ingo Strüwing9 Dec
    • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2903)Bug#43596Rafal Somla10 Dec