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.
>
> 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.
>
> 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?
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.
Btw. how many plugins do we have?
>
> 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
>>
