Re: bk commit into 5.0 tree (aelkin:1.2598) BUG#18199

[Mats Kindahl]

Andrei Elkin wrote:
> Mats, hello.
>
>   
>> Hi Andrei!
>>
>> Comments on patch inline below.
>>
>> Just my few cents,
>> Mats Kindahl
>>
>> Andrei Elkin wrote:
>>     
>>> Below is the list of changes that have just been committed into a local
>>> 5.0 repository of aelkin.  When aelkin does a push these changes
>>> will be propagated to the main repository and, within 24 hours after the
>>> push, to the public repository.
>>> For information on how to access the public repository
>>> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>>>
>>> ChangeSet@stripped, 2008-03-13 19:40:41+02:00, aelkin@mysql1000.(none) +4 -0
>>>   Bug #18199  PURGE BINARY LOGS fails silently with missing logs;
>>>   Bug #18453  Warning/error message if there is a mismatch between ...
>>>     There were three problems:
>>>     1. the reported lack of warnings for the BEFORE syntax of PURGE;
>>>   2. the similar lack of warnings for the TO syntax;
>>>   3. incompatible behaviour between the two in that the latter blanked out
>>>      regardlessly of presence or lack the actual file corresponding to
>>>      an index record; the former version gave up at the first mismatch.
>>>     fixed with polishing and synronizing logics of purge_logs() and
>>>   purge_logs_before_date().
>>>   
>>>       
>
>   
>> Polishing in what sense?
>>     
>
> The old definition of purge_logs{,_before_data}() invoked 
> my_delete() even if lstat is NULL.
> I call it polishing to mean polishing of logics.
>   

Yes, but could you be more specific in the cset comment, please.

[snip]

>>> +
>>> +#
>>> +# testing purge binary logs TO
>>> +#
>>> +
>>> +flush logs;
>>> +flush logs;
>>> +flush logs;
>>>   
>>>       
>> Why three?
>>     
>
> just some number. 2 or 4 would be same.
>   

Why not 1?


[snip]

>>> diff -Nrup a/sql/log.cc b/sql/log.cc
>>> --- a/sql/log.cc	2008-01-31 17:15:35 +02:00
>>> +++ b/sql/log.cc	2008-03-13 19:40:40 +02:00
>>> @@ -1209,16 +1209,15 @@ int MYSQL_LOG::purge_logs(const char *to
>>>           !log_in_use(log_info.log_file_name))
>>>    {
>>>      ulong file_size= 0;
>>> +    MY_STAT s, *lstat= my_stat(log_info.log_file_name, &s, MYF(0));
>>>      if (decrease_log_space) //stat the file we want to delete
>>>      {
>>> -      MY_STAT s;
>>> -
>>>        /*           If we could not stat, we can't know the amount
>>>           of space that deletion will free. In most cases,
>>>           deletion won't work either, so it's not a problem.
>>>        */
>>> -      if (my_stat(log_info.log_file_name,&s,MYF(0)))
>>> +      if (lstat)
>>>          file_size= s.st_size;
>>>        else
>>>  	sql_print_information("Failed to execute my_stat on file '%s'",
>>>   
>>>       
>> Why did you move the call to my_stat()? The cset comment does not hint
>> at that, and I cannot see why that is necessary.
>>     
>
> Obviously, my_stat() is the most reliable idicator of lack of the file. 
> You must have noticed that purge_logs_before_date() calls my_stat().
> The current hunk's purge_logs() also did it in the case
> decrease_log_space == true.
> A possbile error from my_delete() of a file is not necessary after the
> file does not exist. 
> It might be even that my_delete(of_unexisted_file) can return an error of
> some unexpected kind.
>   

The important issue is not if I can figure it out, but that a comment 
should probably be added to tell why you had to move the my_stat(). 
Also, it is reasonable that my_delete() should return an error of some 
kind if the file does not exist, so it is not obvious why the code is 
changed in this manner. It can be for efficiency reasons, but it could 
just as well be for some flaw in the logic.

>> Why did you have to introduce an extra pointer variable. Can't you
>> examine the contents of 's'?
>>     
>
> we need to check my_stat() return results that can be NULL.
>   

OK

>>> @@ -1286,11 +1290,16 @@ int MYSQL_LOG::purge_logs_before_date(ti
>>>    while (strcmp(log_file_name, log_info.log_file_name) &&
>>>  	 !log_in_use(log_info.log_file_name))
>>>    {
>>> -    /* It's not fatal even if we can't delete a log file */
>>> -    if (!my_stat(log_info.log_file_name, &stat_area, MYF(0)) ||
>>> -	stat_area.st_mtime >= purge_time)
>>> +    MY_STAT *lstat= my_stat(log_info.log_file_name, &stat_area,
> MYF(0));
>>> +    if (!lstat)
>>> +      push_warning_printf(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN,
>>> +                          ER_LOG_PURGE_NO_FILE, ER(ER_LOG_PURGE_NO_FILE),
>>> +                          log_info.log_file_name);
>>> +    if (stat_area.st_mtime >= purge_time)
>>>        break;
>>> -    my_delete(log_info.log_file_name, MYF(0));
>>> +    /* It's not fatal even if we can't delete a log file */
>>> +    if (lstat)
>>> +      my_delete(log_info.log_file_name, MYF(0));
>>>   
>>>       
>> I see the need for the addition of the warning, but I don't see why
>> you have to move the call to my_stat().
>>
>>     
>
> Actually it's not moved, but rather provided a new local `lstat' with
> the value. The local is tested twice - to generate the warn and to
> avoid my_delete().
> In the new patch I polished this hunk to combine two tests of the local
> and by this to remove the new local.
> I would not do the same for the earlier hunk purge_logs(); it is
> better to keep the local as there is an extra interleaving logics
> (file_size).
>   

Please clarify the rationale behind the change in the cset comment.

Just my few cents,
Mats Kindahl

-- 
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com