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

[Mats Kindahl]

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?

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

> +
> +#
> +# testing purge binary logs TO
> +#
> +
> +flush logs;
> +flush logs;
> +flush logs;
>   

Why three?

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

> +--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?

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

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

Why did you have to introduce an extra pointer variable. Can't you 
examine the contents of 's'?

> @@ -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().

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

Just my few cents,
Mats Kindahl

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