List:Commits« Previous MessageNext Message »
From:Luís Soares Date:June 15 2011 2:23pm
Subject:Re: bzr commit into mysql-trunk branch (magnus.blaudd:3580) Bug#59844
View as plain text  
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