List:Commits« Previous MessageNext Message »
From:Sven Sandberg Date:December 20 2008 6:32am
Subject:Re: bzr commit into mysql-5.0-bugteam branch (luis.soares:2746) Bug#28796
View as plain text  
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
Thread
bzr commit into mysql-5.0-bugteam branch (luis.soares:2746) Bug#28796Luis Soares20 Dec
  • Re: bzr commit into mysql-5.0-bugteam branch (luis.soares:2746) Bug#28796Sven Sandberg20 Dec
    • Re: bzr commit into mysql-5.0-bugteam branch (luis.soares:2746)Bug#28796Luís Soares20 Dec