Hi Zhenxing,
Patch is basically Ok. Nevertheless, I have some comments. Please,
check them in-line.
Furthermore, I have run mtr (rpl,binlog suites) during review and one
test failed in the binlog suite:
binlog.binlog_auto_increment_bug33029:
"failed: 1380: Failed initializing relay log position: Could not find
target log during relay log initialization"
Regards,
Luís
On Thu, 2009-03-26 at 17:40 +0800, He Zhenxing wrote:
> #At file:///media/sdb2/hezx/work/mysql/bzrwork/b12190/6.0-rpl/
>
> 2821 He Zhenxing 2009-03-26
> BUG#12190 CHANGE MASTER has differ path requiremts on MASTER_LOG_FILE and
> RELAY_LOG_FILE
>
> CHANGE MASTER TO command required the value for RELAY_LOG_FILE to
> be an absolute path, which was different from the requirement of
> MASTER_LOG_FILE.
OK.
>
> This patch fixed the problem by changed the value for RELAY_LOG_FILE
SUGGESTION: This patch fixed the problem by *changing*
> to be the basename of the log file as that for MASTER_LOG_FILE.
> modified:
> mysql-test/suite/rpl/r/rpl_change_master.result
> mysql-test/suite/rpl/t/rpl_change_master.test
> sql/sql_repl.cc
>
> === modified file 'mysql-test/suite/rpl/r/rpl_change_master.result'
> --- a/mysql-test/suite/rpl/r/rpl_change_master.result 2008-07-17 19:11:37 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_change_master.result 2009-03-26 09:40:28 +0000
> @@ -100,3 +100,25 @@ n
> 1
> 2
> drop table t1;
> +reset master;
> +include/stop_slave.inc
> +reset slave;
> +include/start_slave.inc
> +create table t1 (a int);
> +insert into t1 values (1);
> +flush logs;
> +insert into t1 values (2);
> +select * from t1;
> +a
> +1
> +2
> +include/stop_slave.inc
> +delete from t1;
> +CHANGE MASTER TO relay_log_file='slave-relay-bin.000005', relay_log_pos=4;
> +start slave sql_thread;
> +select * from t1;
> +a
> +2
> +start slave io_thread;
> +set global relay_log_purge=1;
> +drop table t1;
>
> === modified file 'mysql-test/suite/rpl/t/rpl_change_master.test'
> --- a/mysql-test/suite/rpl/t/rpl_change_master.test 2008-06-22 20:05:19 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_change_master.test 2009-03-26 09:40:28 +0000
> @@ -31,3 +31,47 @@ drop table t1;
> sync_slave_with_master;
>
> # End of 4.1 tests
> +
> +#
> +# BUG#12190 CHANGE MASTER has differ path requiremts on MASTER_LOG_FILE and
> RELAY_LOG_FILE
> +#
> +
> +connection master;
> +reset master;
> +connection slave;
> +source include/stop_slave.inc;
> +reset slave;
> +source include/start_slave.inc;
SUGGESTION:
Why not just simply "source include/master-slave-reset.inc" ?
> +
> +connection master;
> +create table t1 (a int);
> +insert into t1 values (1);
> +flush logs;
> +insert into t1 values (2);
> +
> +sync_slave_with_master;
> +select * from t1;
> +source include/stop_slave.inc;
> +delete from t1;
SUGGESTION:
To me is more intuitive to:
1. delete from t1 where a=2
2. then do a diff_tables below (check blow)
This is just a suggestion, it is correct as it is.
> +
> +# start replication from the second insert, after fix of BUG#12190,
> +# relay_log_file does not use absolute path, only the filename is
> +# required
> +#
> +# Note: the follow change master will automatically reset
> +# relay_log_purge to false, save the old value to restore
> +let $relay_log_purge= `select @@global.relay_log_purge`;
> +CHANGE MASTER TO relay_log_file='slave-relay-bin.000005', relay_log_pos=4;
> +start slave sql_thread;
> +source include/wait_for_slave_sql_to_start.inc;
> +sync_with_master;
QUESTION:
Why use sync_with_master? Wouldn't you need a save_master_pos earlier
in the test (or is it implicit somewhere I have not detected)?
> +select * from t1;
> +
SUGGESTION: (continuation)
If deleting just the a=2 row, one could do the diff_tables here and
avoid adding the selected rows to the result file.
> +# clean up
> +connection slave;
> +start slave io_thread;
> +source include/wait_for_slave_io_to_start.inc;
> +eval set global relay_log_purge=$relay_log_purge;
> +connection master;
> +drop table t1;
> +sync_slave_with_master;
>
> === modified file 'sql/sql_repl.cc'
> --- a/sql/sql_repl.cc 2009-02-27 13:20:11 +0000
> +++ b/sql/sql_repl.cc 2009-03-26 09:40:28 +0000
> @@ -1536,9 +1536,11 @@ bool change_master(THD* thd, Master_info
> if (lex_mi->relay_log_name)
> {
> need_relay_log_purge= 0;
> - strmake(mi->rli.group_relay_log_name,lex_mi->relay_log_name,
> + char relay_log_name[FN_REFLEN];
> + mi->rli.relay_log.make_log_name(relay_log_name, lex_mi->relay_log_name);
> + strmake(mi->rli.group_relay_log_name, relay_log_name,
> sizeof(mi->rli.group_relay_log_name)-1);
> - strmake(mi->rli.event_relay_log_name,lex_mi->relay_log_name,
> + strmake(mi->rli.event_relay_log_name, relay_log_name,
> sizeof(mi->rli.event_relay_log_name)-1);
> }
>
The above seems ok.
Setting to in-progress due to test failure and you may want to address
some of the suggestions above.
--
Luís
