Hi Luís,
Good work, the source patch does the work and I think the test tests the
right things. I have some suggestions for the test case, and a comment
on the source code comment, could you fix that?
/Sven
Luis Soares wrote:
> #At file:///stuff/workspace/mysql-server/bugfix/28796/5.0-bugteam/
>
> 2746 Luis Soares 2008-12-20
> BUG#28796: CHANGE MASTER TO MASTER_HOST="" leads to invalid master.info
>
> This patch addresses the bug reported by checking wether
> host argument is an empty string or not. If empty, an error is
> reported to the client, otherwise continue normally.
>
> This commit is based on the originally proposed patch and adds
> a test case as requested during review.
> added:
> mysql-test/r/rpl_empty_master_host.result
> mysql-test/t/rpl_empty_master_host.test
> modified:
> sql/sql_repl.cc
>
> === added file 'mysql-test/r/rpl_empty_master_host.result'
> --- a/mysql-test/r/rpl_empty_master_host.result 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/r/rpl_empty_master_host.result 2008-12-20 03:16:54 +0000
> @@ -0,0 +1,24 @@
> +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;
> +SHOW SLAVE STATUS;
>
> +Slave_IO_State Master_Host Master_User Master_Port Connect_Retry Master_Log_File Read_Master_Log_Pos Relay_Log_File Relay_Log_Pos Relay_Master_Log_File Slave_IO_Running Slave_SQL_Running Replicate_Do_DB Replicate_Ignore_DB Replicate_Do_Table Replicate_Ignore_Table Replicate_Wild_Do_Table Replicate_Wild_Ignore_Table Last_Errno Last_Error Skip_Counter Exec_Master_Log_Pos Relay_Log_Space Until_Condition Until_Log_File Until_Log_Pos Master_SSL_Allowed Master_SSL_CA_File Master_SSL_CA_Path Master_SSL_Cert Master_SSL_Cipher Master_SSL_Key Seconds_Behind_Master
> +X 127.0.0.1 X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X
> +CHANGE MASTER TO MASTER_HOST="";
> +ERROR HY000: Incorrect arguments to MASTER_HOST
> +SHOW SLAVE STATUS;
>
> +Slave_IO_State Master_Host Master_User Master_Port Connect_Retry Master_Log_File Read_Master_Log_Pos Relay_Log_File Relay_Log_Pos Relay_Master_Log_File Slave_IO_Running Slave_SQL_Running Replicate_Do_DB Replicate_Ignore_DB Replicate_Do_Table Replicate_Ignore_Table Replicate_Wild_Do_Table Replicate_Wild_Ignore_Table Last_Errno Last_Error Skip_Counter Exec_Master_Log_Pos Relay_Log_Space Until_Condition Until_Log_File Until_Log_Pos Master_SSL_Allowed Master_SSL_CA_File Master_SSL_CA_Path Master_SSL_Cert Master_SSL_Cipher Master_SSL_Key Seconds_Behind_Master
> +X 127.0.0.1 X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X
> +CHANGE MASTER TO MASTER_HOST="foo";
> +SHOW SLAVE STATUS;
>
> +Slave_IO_State Master_Host Master_User Master_Port Connect_Retry Master_Log_File Read_Master_Log_Pos Relay_Log_File Relay_Log_Pos Relay_Master_Log_File Slave_IO_Running Slave_SQL_Running Replicate_Do_DB Replicate_Ignore_DB Replicate_Do_Table Replicate_Ignore_Table Replicate_Wild_Do_Table Replicate_Wild_Ignore_Table Last_Errno Last_Error Skip_Counter Exec_Master_Log_Pos Relay_Log_Space Until_Condition Until_Log_File Until_Log_Pos Master_SSL_Allowed Master_SSL_CA_File Master_SSL_CA_Path Master_SSL_Cert Master_SSL_Cipher Master_SSL_Key Seconds_Behind_Master
> +X foo X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X
> +CHANGE MASTER TO MASTER_HOST="127.0.0.1";
> +SHOW SLAVE STATUS;
>
> +Slave_IO_State Master_Host Master_User Master_Port Connect_Retry Master_Log_File Read_Master_Log_Pos Relay_Log_File Relay_Log_Pos Relay_Master_Log_File Slave_IO_Running Slave_SQL_Running Replicate_Do_DB Replicate_Ignore_DB Replicate_Do_Table Replicate_Ignore_Table Replicate_Wild_Do_Table Replicate_Wild_Ignore_Table Last_Errno Last_Error Skip_Counter Exec_Master_Log_Pos Relay_Log_Space Until_Condition Until_Log_File Until_Log_Pos Master_SSL_Allowed Master_SSL_CA_File Master_SSL_CA_Path Master_SSL_Cert Master_SSL_Cipher Master_SSL_Key Seconds_Behind_Master
> +X 127.0.0.1 X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X
> +START SLAVE;
>
> === added file 'mysql-test/t/rpl_empty_master_host.test'
> --- a/mysql-test/t/rpl_empty_master_host.test 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/t/rpl_empty_master_host.test 2008-12-20 03:16:54 +0000
> @@ -0,0 +1,38 @@
> +# tests error when setting empty master host
> +# Details in BUG#28796
Please state the purpose of the test and give some hint on how it is
implemented.
- I think the purpose is to verify that (1) an error is generated when
setting MASTER_HOST = ''; (2) no error is generated when setting
MASTER_HOST to a nonempty string; (3) replication works after setting a
correct MASTER_HOST.
- It is implemented by issuing CHANGE MASTER TO MASTER_HOST = x and
verifying that error/no error is generated. You use SHOW SLAVE STATUS to
verify that the master host was set correctly.
See e.g., rpl_trunc_temp.test for examples.
> +
> +--source include/master-slave.inc
> +
> +connection slave;
> +STOP SLAVE;
> +--source include/wait_for_slave_to_stop.inc
> +
> +replace_column 1 X 3 X 4 X 5 X 6 X 7 X 8 X 9 X 10 X 11 X 12 X 13 X 14 X 15 X 16 X 17
> X 18 X 19 X 20 X 21 X 22 X 23 X 24 X 25 X 26 X 27 X 28 X 29 X 30 X 31 X 32 X 33 X;
> +SHOW SLAVE STATUS;
There is a much nicer way to do this:
let $master_host= query_get_value(SHOW SLAVE STATUS, Master_Host, 1);
--echo Master_Host = '$master_host' (expected '')
Could you do that here, and in three places below?
> +
> +# attempt to change to an empty master host should
> +# result in error 1210: "Incorrect arguments to ..."
> +
> +--error 1210
Please use named constants instead of numbers:
error ER_WRONG_ARGUMENTS;
> +CHANGE MASTER TO MASTER_HOST="";
> +
> +# show slave status still holds previous information
> +replace_column 1 X 3 X 4 X 5 X 6 X 7 X 8 X 9 X 10 X 11 X 12 X 13 X 14 X 15 X 16 X 17
> X 18 X 19 X 20 X 21 X 22 X 23 X 24 X 25 X 26 X 27 X 28 X 29 X 30 X 31 X 32 X 33 X;
> +SHOW SLAVE STATUS;
> +
> +# changing master to other than empty master host succeeds
> +CHANGE MASTER TO MASTER_HOST="foo";
> +
> +# show slave status should hold "foo" as master host
> +replace_column 1 X 3 X 4 X 5 X 6 X 7 X 8 X 9 X 10 X 11 X 12 X 13 X 14 X 15 X 16 X 17
> X 18 X 19 X 20 X 21 X 22 X 23 X 24 X 25 X 26 X 27 X 28 X 29 X 30 X 31 X 32 X 33 X;
> +SHOW SLAVE STATUS;
> +
> +# changing back to localhost
> +CHANGE MASTER TO MASTER_HOST="127.0.0.1";
> +
> +replace_column 1 X 3 X 4 X 5 X 6 X 7 X 8 X 9 X 10 X 11 X 12 X 13 X 14 X 15 X 16 X 17
> X 18 X 19 X 20 X 21 X 22 X 23 X 24 X 25 X 26 X 27 X 28 X 29 X 30 X 31 X 32 X 33 X;
> +SHOW SLAVE STATUS;
> +
> +# start slave must succeed.
> +START SLAVE;
> +--source include/wait_for_slave_to_start.inc
>
> === modified file 'sql/sql_repl.cc'
> --- a/sql/sql_repl.cc 2008-01-31 06:19:29 +0000
> +++ b/sql/sql_repl.cc 2008-12-20 03:16:54 +0000
> @@ -1080,6 +1080,15 @@ bool change_master(THD* thd, MASTER_INFO
>
> thd->proc_info = "Changing master";
> LEX_MASTER_INFO* lex_mi= &thd->lex->mi;
> +
> + /* checks whether host argument is an empty string or not. */
I don't think you need to document that we check if host is empty, it's
easy to see if you read the next line. But could you add a short
sentence explaining why we need to generate an error if the host
argument is empty?
> + if(lex_mi->host && strcmp(lex_mi->host, "") == 0)
> + {
> + my_error(ER_WRONG_ARGUMENTS, MYF(0), "MASTER_HOST");
> + unlock_slave_threads(mi);
> + DBUG_RETURN(TRUE);
> + }
> +
> // TODO: see if needs re-write
> if (init_master_info(mi, master_info_file, relay_log_info_file, 0,
> thread_mask))
>
>
--
Sven Sandberg, Software Engineer
MySQL AB, www.mysql.com
