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

[Andrei Elkin]

Sergey, hello.



> Hi Andrei,
>
> this patch looks much closer to something that I would approve, but still a
> few comments inline.
>
> On Sat, Mar 15, 2008 at 05:59:45PM +0200, Andrei Elkin wrote:
>> 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-15 17:58:41 +02:00
>> @@ -1180,6 +1180,8 @@ int MYSQL_LOG::update_log_index(LOG_INFO
>>    RETURN VALUES
>>      0				ok
>>      LOG_INFO_EOF		to_log not found
>> +    LOG_INFO_FATAL              if any other than ENOENT error from
>> +                                my_stat() or my_delete()
>>  */
>>  
>>  int MYSQL_LOG::purge_logs(const char *to_log, 
>> @@ -1189,6 +1191,7 @@ int MYSQL_LOG::purge_logs(const char *to
>>                            ulonglong *decrease_log_space)
>>  {
>>    int error;
>> +  int save_errno= my_errno;
>>    bool exit_loop= 0;
>>    LOG_INFO log_info;
>>    DBUG_ENTER("purge_logs");
>> @@ -1209,32 +1212,85 @@ int MYSQL_LOG::purge_logs(const char *to
>>           !log_in_use(log_info.log_file_name))
>>    {
>>      ulong file_size= 0;
>> -    if (decrease_log_space) //stat the file we want to delete
>> +    my_errno= 0;

> In general I do not understand why do you need to handle my_errno this way.
> At least resetting my_errno here looks needless. Could you please explain
> why do we need to handle it this way?

As we discussed on #engines, i have removed save/restore idea.
I left resetting my_errno to zero after my_stat() or my_delete() made
it ENOENT.

>
>> +    MY_STAT s, *lstat= my_stat(log_info.log_file_name, &s, MYF(0));
>> +    if (!lstat)
>>      {
>> -      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)))
>> -        file_size= s.st_size;
>> -      else
>> +      if (my_errno == ENOENT) 
>> +      {
>> +        /*
>> +          It's not fatal if we can't stat a log file that does not exist;
>> +          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.
>> +        */     
>> +        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);
>>  	sql_print_information("Failed to execute my_stat on file '%s'",
>>  			      log_info.log_file_name);

> According to what sql_print_information() states here, I think it should be
> outside of if (my_errno == ENOENT).

It's just a warning not an error if my_errno == ENOENT.

The error is when if my_stat() returned NULL my_errno != ENOENT which
is in the following `else' branch.

>
>> +      }
>> +      else
>> +      {
>> +        /*
>> +          Other than ENOENT are fatal
>> +        */
>> +        push_warning_printf(current_thd, MYSQL_ERROR::WARN_LEVEL_ERROR,
>> +                            ER_BINLOG_PURGE_FATAL_ERR,
>> +                            "a problem with getting info on being purged %s; "
>> +                            "consider examining corrspondence "
> s/corrspondence/correspondence/

ops (4 times) fixed.

>
>> +                            "of your binlog index file "
>> +                            "to the actual binlog files",
>> +                            log_info.log_file_name);
>> +        error= LOG_INFO_FATAL;
>> +        goto err;
>> +      }
>>      }
>> -    /*
>> -      It's not fatal if we can't delete a log file ;
>> -      if we could delete it, take its size into account
>> -    */
>> +    else if (decrease_log_space)
>> +    {
>> +      file_size= s.st_size;
>> +    }
>> +
>>      DBUG_PRINT("info",("purging %s",log_info.log_file_name));
>> -    if (!my_delete(log_info.log_file_name, MYF(0)) &&
> decrease_log_space)
>> -      *decrease_log_space-= file_size;
>> +    if (lstat)
>> +      if (!my_delete(log_info.log_file_name, MYF(0)))
>> +      {
>> +        if (decrease_log_space)
>> +          *decrease_log_space-= file_size;
>> +      }
>> +      else
>> +      {
>> +        if (my_errno == ENOENT) 
>> +        {
>> +          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);
>> +          sql_print_information("Failed to execute my_stat on file '%s'",
>> +                                log_info.log_file_name);

the same comment on delete on stat that is following applies.


>> +        }
>> +        else
>> +        {
>> +          push_warning_printf(current_thd, MYSQL_ERROR::WARN_LEVEL_ERROR,
>> +                              ER_BINLOG_PURGE_FATAL_ERR,
>> +                              "a problem with deleting %s; "
>> +                              "consider examining corrspondence "
> s/corrspondence/correspondence/
>
>> +                              "of your binlog index file "
>> +                              "to the actual binlog files",
>> +                              log_info.log_file_name);
>> +          error= LOG_INFO_FATAL;
>> +          goto err;
>> +        }
>> +      }
> Please add braces around if (lstat) block.
>

ok

>>      if (find_next_log(&log_info, 0) || exit_loop)
>>        break;
>>    }
>> -
>> +  
>> +  /*
>> +    restore the last errno except other than file not found failure
>> +  */
>> +  if (my_errno == 0 || my_errno == ENOENT)
>> +    my_errno= save_errno;
>> +  
>>    /*
>>      If we get killed -9 here, the sysadmin would have to edit
>>      the log index file after restart - otherwise, this should be safe
>> @@ -1263,11 +1319,14 @@ err:
>>    RETURN VALUES
>>      0				ok
>>      LOG_INFO_PURGE_NO_ROTATE	Binary file that can't be rotated
>> +    LOG_INFO_FATAL              if any other than ENOENT error from
>> +                                my_stat() or my_delete()
>>  */
>>  
>>  int MYSQL_LOG::purge_logs_before_date(time_t purge_time)
>>  {
>>    int error;
>> +  int save_errno= my_errno;
>>    LOG_INFO log_info;
>>    MY_STAT stat_area;
>>  
>> @@ -1286,15 +1345,73 @@ 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)
>> -      break;
>> -    my_delete(log_info.log_file_name, MYF(0));
>> +    my_errno= 0;
>> +    if (!my_stat(log_info.log_file_name, &stat_area, MYF(0)))
>> +    {
>> +      if (my_errno == ENOENT) 
>> +      {
>> +        /*
>> +          It's not fatal if we can't stat a log file that does not exist.
>> +        */     
>> +        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);
>> +	sql_print_information("Failed to execute my_stat on file '%s'",
> Please remove tabs above.

done

>
>> +			      log_info.log_file_name);
>> +      }
>> +      else
>> +      {
>> +        /*
>> +          Other than ENOENT are fatal
>> +        */
>> +        push_warning_printf(current_thd, MYSQL_ERROR::WARN_LEVEL_ERROR,
>> +                            ER_BINLOG_PURGE_FATAL_ERR,
>> +                            "a problem with getting info on being purged %s; "
>> +                            "consider examining corrspondence "
> s/corrspondence/correspondence/
>
>> +                            "of your binlog index file "
>> +                            "to the actual binlog files",
>> +                            log_info.log_file_name);
>> +        error= LOG_INFO_FATAL;
>> +        goto err;
>> +      }
>> +    }
>> +    else
>> +    {
>> +      if (stat_area.st_mtime >= purge_time)
>> +        break;
>> +      if (my_delete(log_info.log_file_name, MYF(0)))
>> +      {
>> +        if (my_errno == ENOENT) 
>> +        {
>> +          /* It's not fatal even if we can't delete a log file */
>> +          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);
>> +          sql_print_information("Failed to execute my_stat on file '%s'",
>> +                                log_info.log_file_name);

> Hmm...that doesn't sound correct, it failed to my_delete, but you report
> that it failed to my_stat.
>

copy/paste typo, corrected here and above

>> +        }
>> +        else
>> +        {
>> +          push_warning_printf(current_thd, MYSQL_ERROR::WARN_LEVEL_ERROR,
>> +                              ER_BINLOG_PURGE_FATAL_ERR,
>> +                              "a problem with deleting %s; "
>> +                              "consider examining corrspondence "
> s/corrspondence/correspondence/
>
>> +                              "of your binlog index file "
>> +                              "to the actual binlog files",
>> +                              log_info.log_file_name);
>> +          error= LOG_INFO_FATAL;
>> +          goto err;
>> +        }
>> +      }
>> +    }
>>      if (find_next_log(&log_info, 0))
>>        break;
>>    }
>> -
>> +  /*
>> +    restore the last errno except other than file not found failure
>> +  */
>> +  if (my_errno == 0 || my_errno == ENOENT)
>> +    my_errno= save_errno;
>>    /*
>>      If we get killed -9 here, the sysadmin would have to edit
>>      the log index file after restart - otherwise, this should be safe
>
> Regards,
> Sergey
> -- 
> Sergey Vojtovich <svoj@stripped>
> MySQL AB, Software Engineer
> Izhevsk, Russia, www.mysql.com


regards,

Andrei