Hi Andrei,
Nice work.
I am approving the patch.
I just have a minor suggestion. Find it in-line.
On Thu, 2009-04-09 at 12:52 +0300, Andrei Elkin wrote:
> #At
> file:///home/andrei/MySQL/BZR/FIXES/6.0-rpl-bug41902-reset_logs_no_my_error_2_assert/
> based on revid:alfranio.correia@stripped
>
> 2840 Andrei Elkin 2009-04-09
> Bug#41902 MYSQL_BIN_LOG::reset_logs() doesn't call my_error() in face of an
> error
>
> The assert happened because thd->stmt_da->status() reacted on the
> fact of a gainer error which was not prepared and sent.
>
> Fixed with deploying a part the former purge_error_message() in the reported
> error branch of
> reset_logs(). A user level error gets prepared for reporting via my_message().
>
> Side effects: Bug#44179 appears be fixed with the current patch, at least in
> 6.0.
> added:
> mysql-test/suite/rpl/r/rpl_bug41902.result
> mysql-test/suite/rpl/t/rpl_bug41902-slave.opt
> mysql-test/suite/rpl/t/rpl_bug41902.test
> modified:
> sql/log.cc
> sql/log.h
> sql/share/errmsg.txt
> sql/sql_repl.cc
>
> per-file messages:
> mysql-test/suite/rpl/r/rpl_bug41902.result
> a new results file for Bug#41902.
> mysql-test/suite/rpl/t/rpl_bug41902-slave.opt
> The opt is added to avoid using the server after performing error simulation
> as a precaution from Bug #44181 and possible others.
> mysql-test/suite/rpl/t/rpl_bug41902.test
> Regression tests for bug#41902.
> Bug #44181 forces to add the first of +change master to master_host='dummy'
> lines.
> sql/log.cc
> a new purge_log_get_error() is factored out of purge_error_message()
> to be called without necessary my_error or my_ok to follow;
> MYSQL_BIN_LOG::find_log_pos() simulates LOG_INFO_EOF purge error;
> MYSQL_BIN_LOG::reset_logs() calls my_message(err_code,...) - the central matter
> of the bug fix;
> MYSQL_BIN_LOG::reset_logs() restores name= save_name in the error branches. That
> happens
> to be a separate issue not letting to re-issue RESET SLAVE once again after the
> failing invocation of the sql command.
> sql/log.h
> Exporting the new purge_log_get_error() interface
> for purge_error_message() the "parent" code.
> sql/share/errmsg.txt
> New error message as part of Bug#44179 fixing.
> sql/sql_repl.cc
> separating purge_log_get_error() out of purge_error_message();
> enfining reset_slave() to set sql_errno= ER_SLAVE_PURGE_RELAY_LOGS
> when purge_relay_logs() fails - that's Bug#44179 fixing.
> === added file 'mysql-test/suite/rpl/r/rpl_bug41902.result'
> --- a/mysql-test/suite/rpl/r/rpl_bug41902.result 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_bug41902.result 2009-04-09 09:52:32 +0000
> @@ -0,0 +1,34 @@
> +stop slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +reset master;
> +reset slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +start slave;
> +stop slave;
> +SET @@debug="d,simulate_find_log_pos_error";
> +reset slave;
> +ERROR HY000: Target log not found in binlog index
> +show warnings;
> +Level Code Message
> +Error 1373 Target log not found in binlog index
> +Error 1779 Relay logs purge failure: Failed during log reset
> +SET @@debug="";
> +reset slave;
> +change master to master_host='dummy';
> +SET @@debug="d,simulate_find_log_pos_error";
> +change master to master_host='dummy';
> +ERROR HY000: Target log not found in binlog index
> +SET @@debug="";
> +reset slave;
> +change master to master_host='dummy';
> +SET @@debug="d,simulate_find_log_pos_error";
> +reset master;
> +ERROR HY000: Target log not found in binlog index
> +SET @@debug="";
> +reset master;
> +SET @@debug="d,simulate_find_log_pos_error";
> +purge binary logs to 'master-bin.000001';
> +ERROR HY000: Target log not found in binlog index
> +SET @@debug="";
> +purge binary logs to 'master-bin.000001';
> +End of the tests
>
> === added file 'mysql-test/suite/rpl/t/rpl_bug41902-slave.opt'
> --- a/mysql-test/suite/rpl/t/rpl_bug41902-slave.opt 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_bug41902-slave.opt 2009-04-09 09:52:32 +0000
> @@ -0,0 +1 @@
> +--loose-debug=-d,simulate_find_log_pos_error
>
> === added file 'mysql-test/suite/rpl/t/rpl_bug41902.test'
> --- a/mysql-test/suite/rpl/t/rpl_bug41902.test 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_bug41902.test 2009-04-09 09:52:32 +0000
> @@ -0,0 +1,53 @@
> +# Test for Bug #41902 MYSQL_BIN_LOG::reset_logs() doesn't call my_error()
> +# in face of an error
> +#
> +
> +source include/have_debug.inc;
> +source include/master-slave.inc;
> +
> +#
> +# test checks that
> +# a. there is no crash when find_log_pos() returns with an error
> +# that tests expect to receive;
> +# b. in the case of multiple error messages the first error message is
> +# reported to the user and others are available as warnings.
> +#
> +
> +connection slave;
> +stop slave;
> +
> +SET @@debug="d,simulate_find_log_pos_error";
> +
> +--error ER_UNKNOWN_TARGET_BINLOG
> +reset slave;
> +show warnings;
> +
> +SET @@debug="";
> +reset slave;
> +change master to master_host='dummy';
> +
> +SET @@debug="d,simulate_find_log_pos_error";
> +
> +--error ER_UNKNOWN_TARGET_BINLOG
> +change master to master_host='dummy';
> +
> +SET @@debug="";
> +reset slave;
> +change master to master_host='dummy';
> +
> +connection master;
> +SET @@debug="d,simulate_find_log_pos_error";
> +--error ER_UNKNOWN_TARGET_BINLOG
> +reset master;
> +
> +SET @@debug="";
> +reset master;
> +
> +SET @@debug="d,simulate_find_log_pos_error";
> +--error ER_UNKNOWN_TARGET_BINLOG
> +purge binary logs to 'master-bin.000001';
> +
> +SET @@debug="";
> +purge binary logs to 'master-bin.000001';
> +
> +--echo End of the tests
>
> === modified file 'sql/log.cc'
> --- a/sql/log.cc 2009-04-02 16:14:14 +0000
> +++ b/sql/log.cc 2009-04-09 09:52:32 +0000
> @@ -66,6 +66,35 @@ static int binlog_rollback(handlerton *h
> static int binlog_prepare(handlerton *hton, THD *thd, bool all);
>
> /**
> + purge logs, master and slave sides both, related error code
> + convertor.
> + Called from @c purge_error_message(), @c MYSQL_BIN_LOG::reset_logs()
> +
> + @param res an internal to purging routines error code
> +
> + @return the user level error code ER_*
> +*/
> +uint purge_log_get_error(int res)
> +{
> + uint errmsg= 0;
> +
> + switch (res) {
> + case 0: break;
> + case LOG_INFO_EOF: errmsg= ER_UNKNOWN_TARGET_BINLOG; break;
> + case LOG_INFO_IO: errmsg= ER_IO_ERR_LOG_INDEX_READ; break;
> + case LOG_INFO_INVALID:errmsg= ER_BINLOG_PURGE_PROHIBITED; break;
> + case LOG_INFO_SEEK: errmsg= ER_FSEEK_FAIL; break;
> + case LOG_INFO_MEM: errmsg= ER_OUT_OF_RESOURCES; break;
> + case LOG_INFO_FATAL: errmsg= ER_BINLOG_PURGE_FATAL_ERR; break;
> + case LOG_INFO_IN_USE: errmsg= ER_LOG_IN_USE; break;
> + case LOG_INFO_EMFILE: errmsg= ER_BINLOG_PURGE_EMFILE; break;
> + default: errmsg= ER_LOG_PURGE_UNKNOWN_ERR; break;
> + }
> +
> + return errmsg;
> +}
> +
> +/**
> Silence all errors and warnings reported when performing a write
> to a log table.
> Errors and warnings are not reported to the client or SQL exception
> @@ -4501,8 +4530,10 @@ int MYSQL_BIN_LOG::find_log_pos(LOG_INFO
> {
> uint length;
> my_off_t offset= my_b_tell(&index_file);
> - /* If we get 0 or 1 characters, this is the end of the file */
>
> + DBUG_EXECUTE_IF("simulate_find_log_pos_error",
> + error= LOG_INFO_EOF; break;);
> + /* If we get 0 or 1 characters, this is the end of the file */
> if ((length= my_b_gets(&index_file, fname, FN_REFLEN)) <= 1)
> {
> /* Did not find the given entry; Return not found or error */
> @@ -4604,6 +4635,7 @@ bool MYSQL_BIN_LOG::reset_logs(THD* thd)
> {
> LOG_INFO linfo;
> bool error=0;
> + int err;
> const char* save_name;
> DBUG_ENTER("reset_logs");
>
> @@ -4634,9 +4666,11 @@ bool MYSQL_BIN_LOG::reset_logs(THD* thd)
>
> /* First delete all old log files */
>
> - if (find_log_pos(&linfo, NullS, 0))
> + if ((err= find_log_pos(&linfo, NullS, 0)) != 0)
> {
> - error=1;
> + uint err_code= purge_log_get_error(err);
> + my_message(err_code, ER(err_code), MYF(0));
> + error= 1;
> goto err;
> }
>
> @@ -4705,6 +4739,8 @@ bool MYSQL_BIN_LOG::reset_logs(THD* thd)
> my_free((uchar*) save_name, MYF(0));
>
> err:
> + if (error == 1)
> + name= const_cast<char*>(save_name);
> pthread_mutex_unlock(&LOCK_thread_count);
> pthread_mutex_unlock(&LOCK_index);
> pthread_mutex_unlock(&LOCK_log);
>
> === modified file 'sql/log.h'
> --- a/sql/log.h 2009-01-26 16:32:29 +0000
> +++ b/sql/log.h 2009-04-09 09:52:32 +0000
> @@ -813,4 +813,6 @@ enum enum_binlog_format {
> };
> extern TYPELIB binlog_format_typelib;
>
> +uint purge_log_get_error(int res);
> +
> #endif /* LOG_H */
>
> === modified file 'sql/share/errmsg.txt'
> --- a/sql/share/errmsg.txt 2009-03-18 21:09:40 +0000
> +++ b/sql/share/errmsg.txt 2009-04-09 09:52:32 +0000
> @@ -6508,4 +6508,5 @@ ER_COND_ITEM_TOO_LONG
> eng "Data too long for condition item '%s'"
> ER_PATH_LENGTH
> eng "The path specified for %.64s is too long."
> -
> +ER_SLAVE_PURGE_RELAY_LOGS
> + eng "Relay logs purge failure: %s"
>
> === modified file 'sql/sql_repl.cc'
> --- a/sql/sql_repl.cc 2009-04-02 16:14:14 +0000
> +++ b/sql/sql_repl.cc 2009-04-09 09:52:32 +0000
> @@ -235,22 +235,9 @@ bool log_in_use(const char* log_name)
>
> bool purge_error_message(THD* thd, int res)
> {
> - uint errmsg= 0;
> + uint errmsg;
>
> - switch (res) {
> - case 0: break;
> - case LOG_INFO_EOF: errmsg= ER_UNKNOWN_TARGET_BINLOG; break;
> - case LOG_INFO_IO: errmsg= ER_IO_ERR_LOG_INDEX_READ; break;
> - case LOG_INFO_INVALID:errmsg= ER_BINLOG_PURGE_PROHIBITED; break;
> - case LOG_INFO_SEEK: errmsg= ER_FSEEK_FAIL; break;
> - case LOG_INFO_MEM: errmsg= ER_OUT_OF_RESOURCES; break;
> - case LOG_INFO_FATAL: errmsg= ER_BINLOG_PURGE_FATAL_ERR; break;
> - case LOG_INFO_IN_USE: errmsg= ER_LOG_IN_USE; break;
> - case LOG_INFO_EMFILE: errmsg= ER_BINLOG_PURGE_EMFILE; break;
> - default: errmsg= ER_LOG_PURGE_UNKNOWN_ERR; break;
> - }
> -
> - if (errmsg)
> + if ((errmsg= purge_log_get_error(res)) != 0)
> {
> my_message(errmsg, ER(errmsg), MYF(0));
> return TRUE;
> @@ -1295,7 +1282,10 @@ int reset_slave(THD *thd, Master_info* m
> if ((error= purge_relay_logs(mi->rli, thd,
> 1 /* just reset */,
> &errmsg)))
> + {
> + sql_errno= ER_SLAVE_PURGE_RELAY_LOGS;
I assume you added this new ER_SLAVE_... because the you want more
detail on the error message. Otherwise, why not use the
ER_RELAY_LOG_FAIL already used in change_master when it calls
purge_relay_logs? Wouldn't it be more consistent to use the same ER_ in
both places?
This is not a show blocker, so I am approving the patch. You decide what
to do.
SUGGESTION: use the same ER_RELAY_LOG_FAIL instead of
ER_SLAVE_PURGE_RELAY_LOGS.
> goto err;
> + }
>
> /* Clear master's log coordinates */
> init_master_log_pos(mi);
>
>
--
Luís
