List:Commits« Previous MessageNext Message »
From:Magnus Blåudd Date:June 15 2011 2:26pm
Subject:Re: bzr commit into mysql-trunk branch (magnus.blaudd:3580) Bug#59844
View as plain text  
Hi Luis,

this task has stalled. Been so busy with trying to get 5.5-cluster 
working lately. Haven't really seen any testcase which depends on this 
patch...

Thanks for keep track of it.

/ Magnus

On 06/15/2011 04:23 PM, Luís Soares wrote:
> Hi Magnus,
>
> On 02/15/2011 07:41 AM, Magnus Blåudd wrote:
>> On 02/08/2011 06:26 PM, Luís Soares wrote:
>>> Hi Magnus,
>>>
>>> Nice Work.
>>> Please find my review comments below.
>>>
>>> STATUS
>>> ------
>>>
>>> Not approved.
>>>
>>> REQUIRED CHANGES
>>> ----------------
>>>
>>> RC1. Regression testing needed.
>>>
>>> Perhaps using DBUG_EXECUTE_IF(...);
>>>
>>> See rpl_binlog_errors.test for some examples.
>>
>>
>> Very good idea. Will do.
>
> How is this going ?
>
>>>
>>> RC2. FLUSH LOGS calls In sql_reload.cc:reload_acl_and_cache,
>>> which in turn may call reset_slave.
>>>
>>> Can you please add a test for this as well ?
>>
>> Of course.
>>
>
> And this ?
>
>>>
>>> REQUESTS
>>> --------
>>>
>>> R1. This is a request for feedback.
>>>
>>> In binlog_func_foreach, why not stop when the first plugin
>>> reports an error ? Won't this cause problems as is?
>>
>>
>> Maybe it depends on which function we use it from?
>
> Right!
>
>> For example reset_slave and reset_logs could _maybe_ bail out after an
>> error but binlog_wait and binlog_end should
>> continue also if one plugin reports error.
>
> OK. Since none of us is 100% sure whether we should make
> it or not, lets keep it as you are proposing. After all,
> you're definitely improving the overal picture here. Thus
> if this is/was ever a problem, we will notice it more
> clearly now.
>
>> Btw. how many plugins do we have?
>
> Dunno.
>
> Regards,
> Luís
>
>>>
>>> SUGGESTIONS
>>> -----------
>>> n/a
>>>
>>> DETAILS
>>> -------
>>> n/a
>>>
>>>> #At file:///home/msvensson/mysql/trunk-bug59844/ based on
>>>> revid:jon.hauglid@strippedfc9h
>>>>
>>>> 3580 Magnus Blåudd 2011-02-01
>>>> Bug#59844 Return value of ndb binlog functions are not checked
>>>> - return error code from ha_reset_slave, ha_reset_logs,
>>>> ha_binlog_end and
>>>> ha_binlog_index_purge_file functions.
>>>> - check and handler return code from ha_reset_logs and ha_reset_slave
>>>> - Since ha_binlog_end is called during shutdown there is no error
>>>> handling
>>>> to perform, the function should have been void.
>>>> - The ha_binlog_index_purge_file function might be necessary to handle
>>>> the return code from.
>>>>
>>>> modified:
>>>> sql/binlog.cc
>>>> sql/handler.cc
>>>> sql/handler.h
>>>> sql/rpl_slave.cc
>>>> === modified file 'sql/binlog.cc'
>>>> --- a/sql/binlog.cc 2010-12-17 02:01:32 +0000
>>>> +++ b/sql/binlog.cc 2011-02-01 09:48:52 +0000
>>>> @@ -2130,7 +2130,18 @@ bool MYSQL_BIN_LOG::reset_logs(THD* thd)
>>>> const char* save_name;
>>>> DBUG_ENTER("reset_logs");
>>>>
>>>> - ha_reset_logs(thd);
>>>> + if (ha_reset_logs(thd))
>>>> + {
>>>> + /*
>>>> + Failed to reset logs in handler(which should have pushed warnings
>>>> + describing the problem in more detail) -> raise fatal binlog
>>>> + purge error
>>>> + */
>>>> + thd->raise_error(ER_BINLOG_PURGE_FATAL_ERR);
>>>> +
>>>> + DBUG_RETURN(1);
>>>> + }
>>>> +
>>>> /*
>>>> We need to get both locks to be sure that no one is trying to
>>>> write to the index log file.
>>>>
>>>> === modified file 'sql/handler.cc'
>>>> --- a/sql/handler.cc 2011-01-21 10:50:31 +0000
>>>> +++ b/sql/handler.cc 2011-02-01 09:48:52 +0000
>>>> @@ -4231,27 +4231,28 @@ static my_bool binlog_func_foreach(THD *
>>>> {
>>>> hton_list_st hton_list;
>>>> uint i, sz;
>>>> + int res= 0;
>>>>
>>>> hton_list.sz= 0;
>>>> plugin_foreach(thd, binlog_func_list,
>>>> MYSQL_STORAGE_ENGINE_PLUGIN, &hton_list);
>>>>
>>>> for (i= 0, sz= hton_list.sz; i < sz ; i++)
>>>> - hton_list.hton[i]->binlog_func(hton_list.hton[i], thd, bfn->fn,
>>>> bfn->arg);
>>>> - return FALSE;
>>>> + res |= hton_list.hton[i]->binlog_func(hton_list.hton[i], thd,
>>>> + bfn->fn, bfn->arg);
>>>> + return res != 0;
>>>> }
>>>>
>>>> int ha_reset_logs(THD *thd)
>>>> {
>>>> binlog_func_st bfn= {BFN_RESET_LOGS, 0};
>>>> - binlog_func_foreach(thd, &bfn);
>>>> - return 0;
>>>> + return binlog_func_foreach(thd, &bfn);
>>>> }
>>>>
>>>> -void ha_reset_slave(THD* thd)
>>>> +int ha_reset_slave(THD* thd)
>>>> {
>>>> binlog_func_st bfn= {BFN_RESET_SLAVE, 0};
>>>> - binlog_func_foreach(thd, &bfn);
>>>> + return binlog_func_foreach(thd, &bfn);
>>>> }
>>>>
>>>> void ha_binlog_wait(THD* thd)
>>>> @@ -4263,15 +4264,13 @@ void ha_binlog_wait(THD* thd)
>>>> int ha_binlog_end(THD* thd)
>>>> {
>>>> binlog_func_st bfn= {BFN_BINLOG_END, 0};
>>>> - binlog_func_foreach(thd, &bfn);
>>>> - return 0;
>>>> + return binlog_func_foreach(thd, &bfn);
>>>> }
>>>>
>>>> int ha_binlog_index_purge_file(THD *thd, const char *file)
>>>> {
>>>> binlog_func_st bfn= {BFN_BINLOG_PURGE_FILE, (void *)file};
>>>> - binlog_func_foreach(thd, &bfn);
>>>> - return 0;
>>>> + return binlog_func_foreach(thd, &bfn);
>>>> }
>>>>
>>>> struct binlog_log_query_st
>>>>
>>>> === modified file 'sql/handler.h'
>>>> --- a/sql/handler.h 2011-01-26 21:12:56 +0000
>>>> +++ b/sql/handler.h 2011-02-01 09:48:52 +0000
>>>> @@ -2607,7 +2607,7 @@ void trans_register_ha(THD *thd, bool al
>>>> #ifdef HAVE_NDB_BINLOG
>>>> int ha_reset_logs(THD *thd);
>>>> int ha_binlog_index_purge_file(THD *thd, const char *file);
>>>> -void ha_reset_slave(THD *thd);
>>>> +int ha_reset_slave(THD *thd);
>>>> void ha_binlog_log_query(THD *thd, handlerton *db_type,
>>>> enum_binlog_command binlog_command,
>>>> const char *query, uint query_length,
>>>> @@ -2615,12 +2615,13 @@ void ha_binlog_log_query(THD *thd, handl
>>>> void ha_binlog_wait(THD *thd);
>>>> int ha_binlog_end(THD *thd);
>>>> #else
>>>> -#define ha_reset_logs(a) do {} while (0)
>>>> -#define ha_binlog_index_purge_file(a,b) do {} while (0)
>>>> -#define ha_reset_slave(a) do {} while (0)
>>>> +static inline int ha_int_dummy(void) { return 0; }
>>>> +#define ha_reset_logs(a) ha_int_dummy()
>>>> +#define ha_binlog_index_purge_file(a,b) ha_int_dummy()
>>>> +#define ha_reset_slave(a) ha_int_dummy()
>>>> #define ha_binlog_log_query(a,b,c,d,e,f,g) do {} while (0)
>>>> #define ha_binlog_wait(a) do {} while (0)
>>>> -#define ha_binlog_end(a) do {} while (0)
>>>> +#define ha_binlog_end(a) ha_int_dummy()
>>>> #endif
>>>>
>>>> const char *get_canonical_filename(handler *file, const char *path,
>>>>
>>>> === modified file 'sql/rpl_slave.cc'
>>>> --- a/sql/rpl_slave.cc 2011-01-24 03:58:22 +0000
>>>> +++ b/sql/rpl_slave.cc 2011-02-01 09:48:52 +0000
>>>> @@ -5789,7 +5789,16 @@ int reset_slave(THD *thd, Master_info* m
>>>> goto err;
>>>> }
>>>>
>>>> - ha_reset_slave(thd);
>>>> + if ((error= ha_reset_slave(thd)))
>>>> + {
>>>> + /*
>>>> + Failed to reset slave in handler(which should have pushed warnings
>>>> + describing the problem in more detail) -> use the unknown error
>>>> + code already set
>>>> + */
>>>> + errmsg= "ha_reset_slave failed";
>>>> + goto err;
>>>> + }
>>>>
>>>> // delete relay logs, clear relay log coordinates
>>>> if ((error= mi->rli->purge_relay_logs(thd,
>>>>
>>>> Attachment: [text/bzr-bundle]
>>>> bzr/magnus.blaudd@stripped
>>>>
>>
>>
>

Thread
bzr commit into mysql-trunk branch (magnus.blaudd:3580) Bug#59844Magnus Blåudd1 Feb
  • Re: bzr commit into mysql-trunk branch (magnus.blaudd:3580) Bug#59844Sven Sandberg22 Mar
    • Re: bzr commit into mysql-trunk branch (magnus.blaudd:3580) Bug#59844Magnus Blåudd23 Mar
Re: bzr commit into mysql-trunk branch (magnus.blaudd:3580) Bug#59844Luís Soares8 Feb
  • Re: bzr commit into mysql-trunk branch (magnus.blaudd:3580) Bug#59844Magnus Blåudd15 Feb
    • Re: bzr commit into mysql-trunk branch (magnus.blaudd:3580) Bug#59844Luís Soares16 Jun
      • Re: bzr commit into mysql-trunk branch (magnus.blaudd:3580) Bug#59844Magnus Blåudd16 Jun