Do you agree with the changes in attachment?
Cheers.
Alfranio Correia wrote:
> Mats Kindahl wrote:
>
>> Hi Alfranio,
>>
>> I have some comments below.
>>
>> Just my few cents,
>> Mats Kindahl
>>
>> Alfranio Correia wrote:
>>
>>
>>> #At
> file:///home/acorreia/workspace.sun/repository.mysql/bzrwork/bug-40337/mysql-6.0-rpl/
> based on revid:luis.soares@stripped
>>>
>>> 2810 Alfranio Correia 2009-02-04
>>> BUG#40337 Fsyncing master and relay log to disk after every event is
> too slow
>>>
>>>
>> [snip]
>>
>>
>>
>>> === modified file 'sql/rpl_rli.h'
>>> --- a/sql/rpl_rli.h 2008-08-18 05:43:50 +0000
>>> +++ b/sql/rpl_rli.h 2009-02-04 16:11:20 +0000
>>>
>>>
>> [snip]
>>
>>
>>
>>> + /*
>>> + * Identifies when the recovery process is going on.
>>> + * See sql/slave.cc:init_recovery for further details.
>>> + */
>>> + bool is_relay_log_recovery;
>>>
>>>
>> I don't see an initializer for this field in the constructor above. (See below.)
>>
>> [snip]
>>
>>
>>
>>> === modified file 'sql/slave.cc'
>>> --- a/sql/slave.cc 2009-01-26 16:03:39 +0000
>>> +++ b/sql/slave.cc 2009-02-04 16:11:20 +0000
>>>
>>>
>> [snip]
>>
>>
>>
>>> @@ -254,6 +255,10 @@ int init_slave()
>>> goto err;
>>> }
>>>
>>> + active_mi->rli.is_relay_log_recovery= relay_log_recovery;
>>> + if (active_mi->rli.is_relay_log_recovery &&
> init_recovery(active_mi))
>>> + goto err;
>>> +
>>>
>>>
>> Don't you need to move the first line to just after you have created the
>> Master_info, and then add a guard inside init_master_info so that
>> init_relay_log_info() does not try to open (at least) the relay log file since
>> that is corrupt?
>>
> You are right.
> This was a mistake while moving the code between the different versions.
> In some point, I've made this mistake.
>
> (Fixed)
>
>
>> Also, isn't it the case that the contents read from
>> relay-log.info will be thrown away if you first run init_relay_log_info() and
>> then init_recovery()? In that case, it seems redundant to execute
>> init_relay_log_info() at all.
>>
>>
> You are right, but the original code calls init_relay_log_info() from the
> init_master and I did not want to change too much the code.
> I really would like to decouple this but I think we should do it later.
>
>> Actually, I would prefer if you could initialize the is_relay_log_recovery field
>> in the constructor to avoid it having a random value at the wrong time, so
>> something like:
>>
>> active_mi = new Master_info(relay_log_recovery);
>>
>> ... and then change the constructor for Master_info and Relay_log_info
> accordingly.
>>
>>
> done. Do you have any objections against that?
>
> Relay_log_info::Relay_log_info()
> :Slave_reporting_capability("SQL"),
> no_storage(FALSE), replicate_same_server_id(::replicate_same_server_id),
> info_fd(-1), cur_log_fd(-1),
> relay_log(&sync_relaylog_period), sync_counter(0),
> is_relay_log_recovery(relay_log_recovery),
>
>
>>
>>
>>> @@ -269,15 +274,86 @@ int init_slave()
>>> goto err;
>>> }
>>> }
>>> + active_mi->rli.is_relay_log_recovery= 0;
>>> pthread_mutex_unlock(&LOCK_active_mi);
>>> DBUG_RETURN(0);
>>>
>>> err:
>>> + active_mi->rli.is_relay_log_recovery= 0;
>>> pthread_mutex_unlock(&LOCK_active_mi);
>>> DBUG_RETURN(1);
>>>
>>>
>>
>>
> done.
>
>> This is two pieces of code that is almost identical. I think it could be a good
>> idea to try to merge these cases into one piece of code.
>>
>>
>> ------------------------------------------------------------------------
>>
>>
>>
>>
>
> Cheers.
>
>
Attachment: [text/x-patch] diff.patch
