Andrei Elkin wrote:
> Luis, Alfranio, hello!
>
> I have a few comments on this work.
> The patch is basically okay with me.
>
>
>
>> #At file:///stuff/workspace/mysql-server/bugfix/5.1-rpl-38826-39325/
>>
>> 2690 Luis Soares 2008-11-04
>> This is a combined patch for: BUG#38826 and BUG#39325.
>> It merges the previous two proposed patches into a single one.
>>
>>
>
> Please add the problem description and the solution sections
> into the top level change set comments. This is a requirement for all
> patches.
> Considering other requirements, such as the coding style there are few
> assignments "x = " (which I personally prefer) instead of the required
> "x= ", could you please fix that?
>
Sorry for that.
> There is no specific test although lines like
>
>> + DBUG_EXECUTE_IF("crash_before_flush_relay_log_info", abort(););
>>
> makes me think we can have some.
>
We have something, we will publish today or in the worst case tomorrow.
>
>>
>> The difference from previous patch is that it calls
>> "ha_binlog_index_purge_file" before purging the relay log
>> file.
>>
>
> As I asked on #rep on Fri about if rpl_ndb had passed, could you
> please confirm they have done or if there are some issues still (then
> what kind of?).
>
I haven't had the rpl_ndb enabled but I can re-run the tests.
I've asked the ndb team to take a look at the patch because their engine
receives
a notification when a log file is deleted but there is no description on
the expected semantics, etc, etc.
So I was wondering if the patch does not break anything.
Even if I re-run the tests with ndb enabled, I think they should take a
look at the patch.
Luis, do you want me to re-run the tests?
>
>
>> modified:
>> mysql-test/suite/binlog/r/binlog_index.result
>> sql/log.cc
>> sql/log.h
>>
>> === modified file 'mysql-test/suite/binlog/r/binlog_index.result'
>> --- a/mysql-test/suite/binlog/r/binlog_index.result 2008-04-05 11:09:53 +0000
>> +++ b/mysql-test/suite/binlog/r/binlog_index.result 2008-11-04 12:20:59 +0000
>> @@ -21,6 +21,7 @@ flush logs;
>> *** must be a warning master-bin.000001 was not found ***
>> Warnings:
>> Warning 1612 Being purged log master-bin.000001 was not found
>> +Warning 1612 Being purged log master-bin.000001 was not found
>> *** must show one record, of the active binlog, left in the index file after
> PURGE ***
>> show binary logs;
>> Log_name File_size
>>
>> === modified file 'sql/log.cc'
>> --- a/sql/log.cc 2008-10-23 19:27:09 +0000
>> +++ b/sql/log.cc 2008-11-04 12:20:59 +0000
>> @@ -2358,6 +2358,7 @@ MYSQL_BIN_LOG::MYSQL_BIN_LOG()
>> */
>> index_file_name[0] = 0;
>> bzero((char*) &index_file, sizeof(index_file));
>> + bzero((char*) &purge_temp, sizeof(purge_temp));
>> }
>>
>> /* this is called only once */
>> @@ -2918,10 +2919,11 @@ err:
>>
>> IMPLEMENTATION
>> - Protects index file with LOCK_index
>> + - Read the next file name from the index file and store in rli->linfo
>> - Delete relevant relay log files
>> - Copy all file names after these ones to the front of the index file
>> - If the OS has truncate, truncate the file, else fill it with \n'
>> - - Read the next file name from the index file and store in rli->linfo
>> +
>>
>> @param rli Relay log information
>> @param included If false, all relay logs that are strictly before
>> @@ -2952,6 +2954,7 @@ err:
>> int MYSQL_BIN_LOG::purge_first_log(Relay_log_info* rli, bool included)
>> {
>> int error;
>> + char *to_purge_if_included = NULL;
>> DBUG_ENTER("purge_first_log");
>>
>> DBUG_ASSERT(is_open());
>> @@ -2959,36 +2962,20 @@ int MYSQL_BIN_LOG::purge_first_log(Relay
>>
> DBUG_ASSERT(!strcmp(rli->linfo.log_file_name,rli->event_relay_log_name));
>>
>> pthread_mutex_lock(&LOCK_index);
>> - pthread_mutex_lock(&rli->log_space_lock);
>> - rli->relay_log.purge_logs(rli->group_relay_log_name, included,
>> - 0, 0, &rli->log_space_total);
>> - // Tell the I/O thread to take the relay_log_space_limit into account
>> - rli->ignore_log_space_limit= 0;
>> - pthread_mutex_unlock(&rli->log_space_lock);
>> + to_purge_if_included = my_strdup(rli->group_relay_log_name, MYF(0));
>>
>> /*
>> - Ok to broadcast after the critical region as there is no risk of
>> - the mutex being destroyed by this thread later - this helps save
>> - context switches
>> - */
>> - pthread_cond_broadcast(&rli->log_space_cond);
>> -
>> - /*
>> Read the next log file name from the index file and pass it back to
>> - the caller
>> - If included is true, we want the first relay log;
>> - otherwise we want the one after event_relay_log_name.
>> - */
>> - if ((included && (error=find_log_pos(&rli->linfo, NullS, 0)))
> ||
>> - (!included &&
>> - ((error=find_log_pos(&rli->linfo, rli->event_relay_log_name,
> 0)) ||
>> - (error=find_next_log(&rli->linfo, 0)))))
>> + the caller.
>> + */
>> + if((error=find_log_pos(&rli->linfo, rli->event_relay_log_name, 0))
> ||
>> + (error=find_next_log(&rli->linfo, 0)))
>> {
>> char buff[22];
>> sql_print_error("next log error: %d offset: %s log: %s included: %d",
>> error,
>> llstr(rli->linfo.index_file_offset,buff),
>> - rli->group_relay_log_name,
>> + rli->event_relay_log_name,
>> included);
>> goto err;
>> }
>> @@ -3013,10 +3000,48 @@ int MYSQL_BIN_LOG::purge_first_log(Relay
>> rli->notify_group_relay_log_name_update();
>> }
>>
>> + DBUG_EXECUTE_IF("crash_before_flush_relay_log_info", abort(););
>> +
>> /* Store where we are in the new file for the execution thread */
>> flush_relay_log_info(rli);
>>
>> + DBUG_EXECUTE_IF("crash_before_purge_logs", abort(););
>>
>
> Could you please make a test that confirms the correct execution
> happens despite of the simulated failures?
>
Of course. Answer above ;) ^^
>
>
>> +
>> + pthread_mutex_lock(&rli->log_space_lock);
>> + rli->relay_log.purge_logs(to_purge_if_included, included,
>> + 0, 0, &rli->log_space_total);
>> + // Tell the I/O thread to take the relay_log_space_limit into account
>> + rli->ignore_log_space_limit= 0;
>> + pthread_mutex_unlock(&rli->log_space_lock);
>> +
>> + /*
>> + Ok to broadcast after the critical region as there is no risk of
>> + the mutex being destroyed by this thread later - this helps save
>> + context switches
>> + */
>> + pthread_cond_broadcast(&rli->log_space_cond);
>> +
>>
>
>
>> + /*
>> + * Need to update the log pos because purge logs has been called
>> + * after fetching initially the log pos at the begining of the method.
>> + */
>> + if(error=find_log_pos(&rli->linfo, rli->event_relay_log_name, 0))
>> + {
>> + char buff[22];
>> + sql_print_error("next log error: %d offset: %s log: %s included: %d",
>> + error,
>> + llstr(rli->linfo.index_file_offset,buff),
>> + rli->group_relay_log_name,
>> + included);
>> + goto err;
>> + }
>>
>
> I wonder if we really need to call `find_next_log()' at this point.
> How about to force `MYSQL_BIN_LOG::purge_logs()' to validate
> correctness of playing around with the logs and the index
> with `find_log_pos()' at the end of the method?
> I.e if `include' is false then `purge_logs(to_purge_if_included,
> included...)' must find `to_purge_if_included', otherwise
> `find_log_pos(&rli->linfo, 0, 0)' should not return an error.
>
>
>
>
Not following you here... I will take a look at the code
and drop a few comments later on.
>
>> +
>> + /* If included was passed, rli->linfo should be the first entry. */
>> + DBUG_ASSERT(!included || rli->linfo.index_file_start_offset == 0);
>> +
>> +
>> err:
>> + my_free(to_purge_if_included, MYF(0));
>> pthread_mutex_unlock(&LOCK_index);
>> DBUG_RETURN(error);
>> }
>> @@ -3076,8 +3101,37 @@ int MYSQL_BIN_LOG::purge_logs(const char
>>
>> if (need_mutex)
>> pthread_mutex_lock(&LOCK_index);
>> - if ((error=find_log_pos(&log_info, to_log, 0 /*no mutex*/)))
>> +
>> + if ((error=find_log_pos(&log_info, to_log, 0 /*no mutex*/)))
>> + {
>> + sql_print_error("MYSQL_LOG::purge_logs was called with file %s not "
>> + "listed in the index.", to_log);
>> goto err;
>> + }
>> +
>> + /*
>> + For crash recovery reasons the index needs to be updated before
>> + any files are deleted. Move files to be deleted into a temp file
>> + to be processed after the index is updated.
>> + */
>> + if (!my_b_inited(&purge_temp))
>> + {
>> + if (error=open_cached_file(&purge_temp, mysql_tmpdir, TEMP_PREFIX,
>> + DISK_BUFFER_SIZE, MYF(MY_WME)))
>> + {
>> + sql_print_error("MYSQL_LOG::purge_logs failed to open purge_temp");
>> + goto err;
>> + }
>> + }
>> + else
>> + {
>> + if (error=reinit_io_cache(&purge_temp, WRITE_CACHE, 0, 0, 1))
>> + {
>> + sql_print_error("MYSQL_LOG::purge_logs failed to reinit purge_temp "
>> + "for write");
>> + goto err;
>> + }
>> + }
>>
>> /*
>> File name exists in index file; delete until we find this file
>> @@ -3088,6 +3142,61 @@ int MYSQL_BIN_LOG::purge_logs(const char
>> while ((strcmp(to_log,log_info.log_file_name) || (exit_loop=included))
> &&
>> !log_in_use(log_info.log_file_name))
>> {
>> + if ((error=my_b_write(&purge_temp, (const
> uchar*)log_info.log_file_name,
>> + strlen(log_info.log_file_name))) ||
>> + (error=my_b_write(&purge_temp, (const uchar*)"\n", 1)))
>> + {
>> + sql_print_error("MYSQL_LOG::purge_logs failed to copy %s to purge_temp",
>> + log_info.log_file_name);
>> + goto err;
>> + }
>> +
>> + if (find_next_log(&log_info, 0) || exit_loop)
>> + break;
>> + }
>> +
>> + /* We know how many files to delete. Update index file. */
>> + if (error=update_log_index(&log_info, need_update_threads))
>> + {
>> + sql_print_error("MSYQL_LOG::purge_logs failed to update the index file");
>> + goto err;
>> + }
>> +
>> + DBUG_EXECUTE_IF("crash_after_update_index", abort(););
>> +
>> + /* Switch purge_temp for read. */
>> + if (error=reinit_io_cache(&purge_temp, READ_CACHE, 0, 0, 0))
>> + {
>> + sql_print_error("MSYQL_LOG::purge_logs failed to reinit purge_temp "
>> + "for read");
>> + goto err;
>> + }
>> +
>> + /* Read each entry from purge_temp and delete the file. */
>> + for (;;)
>> + {
>> + uint length;
>> +
>> + if ((length=my_b_gets(&purge_temp, log_info.log_file_name,
>> + FN_REFLEN)) <= 1)
>> + {
>> + if (purge_temp.error)
>> + {
>> + error= purge_temp.error;
>> + sql_print_error("MSYQL_LOG::purge_logs error %d reading from "
>> + "purge_temp", error);
>> + goto err;
>> + }
>> +
>> + /* Reached EOF */
>> + break;
>> + }
>> +
>> + /* Get rid of the trailing '\n' */
>> + log_info.log_file_name[length-1]= 0;
>> +
>> + ha_binlog_index_purge_file(current_thd, log_info.log_file_name);
>> +
>> MY_STAT s;
>> if (!my_stat(log_info.log_file_name, &s, MYF(0)))
>> {
>> @@ -3188,22 +3297,8 @@ int MYSQL_BIN_LOG::purge_logs(const char
>> }
>> }
>> }
>> -
>> - ha_binlog_index_purge_file(current_thd, log_info.log_file_name);
>> -
>> - if (find_next_log(&log_info, 0) || exit_loop)
>> - break;
>> }
>>
>> - /*
>> - If we get killed -9 here, the sysadmin would have to edit
>> - the log index file after restart - otherwise, this should be safe
>> - */
>> - error= update_log_index(&log_info, need_update_threads);
>> - if (error == 0) {
>> - error = ret;
>> - }
>> -
>> err:
>> if (need_mutex)
>> pthread_mutex_unlock(&LOCK_index);
>> @@ -3214,8 +3309,7 @@ err:
>> Remove all logs before the given file date from disk and from the
>> index file.
>>
>> - @param thd Thread pointer
>> - @param before_date Delete all log files before given date.
>> + @param purge_time Delete all log files before given date.
>>
>> @note
>> If any of the logs before the deleted one is in use,
>> @@ -3232,6 +3326,7 @@ err:
>> int MYSQL_BIN_LOG::purge_logs_before_date(time_t purge_time)
>> {
>> int error;
>> + char to_log[FN_REFLEN];
>> LOG_INFO log_info;
>> MY_STAT stat_area;
>> THD *thd= current_thd;
>> @@ -3240,11 +3335,8 @@ int MYSQL_BIN_LOG::purge_logs_before_dat
>>
>> pthread_mutex_lock(&LOCK_index);
>>
>> - /*
>> - Delete until we find curren file
>> - or a file that is used or a file
>> - that is older than purge_time.
>> - */
>> + to_log[0] = 0;
>> +
>> if ((error=find_log_pos(&log_info, NullS, 0 /*no mutex*/)))
>> goto err;
>>
>> @@ -3294,55 +3386,19 @@ int MYSQL_BIN_LOG::purge_logs_before_dat
>> }
>> 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 */
>> - if (thd)
>> - {
>> - push_warning_printf(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 delete file '%s'",
>> - log_info.log_file_name);
>> - my_errno= 0;
>> - }
>> - else
>> - {
>> - if (thd)
>> - {
>> - push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_ERROR,
>> - ER_BINLOG_PURGE_FATAL_ERR,
>> - "a problem with deleting %s; "
>> - "consider examining correspondence "
>> - "of your binlog index file "
>> - "to the actual binlog files",
>> - log_info.log_file_name);
>> - }
>> - else
>> - {
>> - sql_print_information("Failed to delete log file '%s'",
>> - log_info.log_file_name);
>> - }
>> - error= LOG_INFO_FATAL;
>> - goto err;
>> - }
>> + if(stat_area.st_mtime < purge_time) {
>> + strmake(to_log,
>> + log_info.log_file_name,
>> + sizeof(log_info.log_file_name));
>> }
>> - ha_binlog_index_purge_file(current_thd, log_info.log_file_name);
>> + else
>> + break;
>> }
>> if (find_next_log(&log_info, 0))
>> break;
>> }
>>
>> - /*
>> - If we get killed -9 here, the sysadmin would have to edit
>> - the log index file after restart - otherwise, this should be safe
>> - */
>> - error= update_log_index(&log_info, 1);
>> + error = (to_log[0] ? purge_logs(to_log, 1, 0, 1, (ulonglong *) 0) : 0);
>>
>
> This looks to me as a very ellegant approach to redefine
> the before through the most generic method. Thanks for making it!
>
Just doing the job ;).
>
>>
>> err:
>> pthread_mutex_unlock(&LOCK_index);
>>
>> === modified file 'sql/log.h'
>> --- a/sql/log.h 2008-10-06 08:27:36 +0000
>> +++ b/sql/log.h 2008-11-04 12:20:59 +0000
>> @@ -233,6 +233,15 @@ class MYSQL_BIN_LOG: public TC_LOG, priv
>> pthread_cond_t update_cond;
>> ulonglong bytes_written;
>> IO_CACHE index_file;
>> +
>> + /*
>> + purge_temp is a temp file used in purge_logs so that the index file
>> + can be updated before deleting files from disk, yielding better crash
>> + recovery. It is created on demand the first time purge_logs is called
>> + and then reused for subsequent calls. It is cleaned up in cleanup().
>> + */
>> + IO_CACHE purge_temp;
>> +
>> char index_file_name[FN_REFLEN];
>> /*
>> The max size before rotation (usable only if log_type == LOG_BIN: binary
>>
>>
>> --
>> MySQL Code Commits Mailing List
>> For list archives: http://lists.mysql.com/commits
>> To unsubscribe: http://lists.mysql.com/commits?unsub=1
>>
>>
>
> cheers,
>
>
