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.
>
>> mysql-test/r/binlog_index.result@stripped, 2008-03-13 19:40:40+02:00,
> aelkin@mysql1000.(none) +29 -0
>> new results
>>
>> mysql-test/r/binlog_index.result@stripped, 2008-03-13 19:40:40+02:00,
> aelkin@mysql1000.(none) +0 -0
>>
>> mysql-test/t/binlog_index.test@stripped, 2008-03-13 19:40:40+02:00,
> aelkin@mysql1000.(none) +53 -0
>> a regression test for the bugs
>>
>> mysql-test/t/binlog_index.test@stripped, 2008-03-13 19:40:40+02:00,
> aelkin@mysql1000.(none) +0 -0
>>
>> sql/log.cc@stripped, 2008-03-13 19:40:40+02:00, aelkin@mysql1000.(none) +17 -8
>> Bug #18199 PURGE BINARY LOGS fails silently with missing logs;
>> Bug #18453 Warning/error message if there is a mismatch between ...
>>
>> sql/share/errmsg.txt@stripped, 2008-03-13 19:40:40+02:00, aelkin@mysql1000.(none)
> +2 -0
>> new error message as there had been no suitable
>>
>> diff -Nrup a/mysql-test/r/binlog_index.result b/mysql-test/r/binlog_index.result
>> --- /dev/null Wed Dec 31 16:00:00 196900
>> +++ b/mysql-test/r/binlog_index.result 2008-03-13 19:40:40 +02:00
>> @@ -0,0 +1,29 @@
>> +flush logs;
>> +flush logs;
>> +flush logs;
>> +show binary logs;
>> +Log_name File_size
>> +master-bin.000001 #
>> +master-bin.000002 #
>> +master-bin.000003 #
>> +master-bin.000004 #
>> +purge binary logs TO 'master-bin.000003';
>> +Warnings:
>> +Warning 1476 Being purged log MYSQLTEST_VARDIR/log/master-bin.000001 was not
> found
>> +must be of one record
>> +show binary logs;
>> +Log_name File_size
>> +master-bin.000003 #
>> +master-bin.000004 #
>> +reset master;
>> +flush logs;
>> +flush logs;
>> +flush logs;
>> +must be a warning master-bin.000001 was not found
>> +Warnings:
>> +Warning 1476 Being purged log MYSQLTEST_VARDIR/log/master-bin.000001 was not
> found
>> +must be of one record
>> +show binary logs;
>> +Log_name File_size
>> +master-bin.000004 #
>> +End of tests
>> diff -Nrup a/mysql-test/t/binlog_index.test b/mysql-test/t/binlog_index.test
>> --- /dev/null Wed Dec 31 16:00:00 196900
>> +++ b/mysql-test/t/binlog_index.test 2008-03-13 19:40:40 +02:00
>> @@ -0,0 +1,53 @@
>> +#
>> +# testing of purging of binary log files bug#18199/Bug#18453
>> +#
>> +-- source include/have_log_bin.inc
>> +-- source include/not_embedded.inc
>>
>
> Please use the form without -- to allow syntax checks. For the "replace_X" commands
> and "echo", I feel it is OK to use the form with --
> since it is hard to see if the ; last is part of the arguments or not.
>
ok
>> +
>> +#
>> +# testing purge binary logs TO
>> +#
>> +
>> +flush logs;
>> +flush logs;
>> +flush logs;
>>
>
> Why three?
just some number. 2 or 4 would be same.
>
>> +
>> +--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
>> +--replace_column 2 #
>> +show binary logs;
>> +
>> +remove_file $MYSQLTEST_VARDIR/log/master-bin.000001;
>> +
>> +# there must be a warning with file names
>> +--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
>> +purge binary logs TO 'master-bin.000003';
>> +
>> +--echo must be of one record
>>
>
> Please use some form of market to see that this is a comment and not a
> statement when reading the result file. Also, I am not sure what you
> mean with this comment, so please elaborate.
>
ok.
ok.
>> +--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
>> +--replace_column 2 #
>> +show binary logs;
>> +
>> +#
>> +# testing purge binary logs BEFORE
>> +#
>> +
>> +reset master;
>> +
>> +flush logs;
>> +flush logs;
>> +flush logs;
>>
>
> Why three?
as above.
>
>> +remove_file $MYSQLTEST_VARDIR/log/master-bin.000001;
>> +
>> +--echo must be a warning master-bin.000001 was not found
>> +let $date=`select NOW() + INTERVAL 1 MINUTE`;
>> +--disable_query_log
>> +--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
>> +eval purge binary logs BEFORE '$date';
>> +--enable_query_log
>> +
>> +--echo must be of one record
>> +--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
>> +--replace_column 2 #
>> +show binary logs;
>>
>
> This could probably be made into an include file.
>
good point, converted, a new file added.
>> +
>> +--echo End of tests
>> 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.
>
> 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.
>
>> @@ -1228,8 +1227,13 @@ int MYSQL_LOG::purge_logs(const char *to
>> It's not fatal if we can't delete a log file ;
>> if we could delete it, take its size into account
>> */
>> + 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);
>> DBUG_PRINT("info",("purging %s",log_info.log_file_name));
>> - if (!my_delete(log_info.log_file_name, MYF(0)) &&
> decrease_log_space)
>> + if ((lstat && !my_delete(log_info.log_file_name, MYF(0)))
>> + && decrease_log_space)
>> *decrease_log_space-= file_size;
>> if (find_next_log(&log_info, 0) || exit_loop)
>> break;
>> @@ -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).
>> if (find_next_log(&log_info, 0))
>> break;
>> }
>> diff -Nrup a/sql/share/errmsg.txt b/sql/share/errmsg.txt
>> --- a/sql/share/errmsg.txt 2007-10-23 14:15:27 +03:00
>> +++ b/sql/share/errmsg.txt 2008-03-13 19:40:40 +02:00
>> @@ -5641,3 +5641,5 @@ ER_NAME_BECOMES_EMPTY
>> eng "Name '%-.64s' has become ''"
>> ER_AMBIGUOUS_FIELD_TERM
>> eng "First character of the FIELDS TERMINATED string is ambiguous; please use
> non-optional and non-empty FIELDS ENCLOSED BY"
>> +ER_LOG_PURGE_NO_FILE + eng "Being purged log %s was not
>> found"
>>
>>
>
> "Log file %s was not found while trying to purge it"
>
regards,
Andrei
