List:Commits« Previous MessageNext Message »
From:Luís Soares Date:December 20 2008 3:15pm
Subject:Re: bzr commit into mysql-5.0-bugteam branch (luis.soares:2746)
Bug#28796
View as plain text  
Hi Sven,

   Thanks for the comments. These are indeed added value for this patch. 
I have committed the new patch with your requests on it.

   Additional comments inline.

Regards,
Luís

Sven Sandberg wrote:
> 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.

Done.

>  - 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?

Done. Good suggestion, thanks.

>> +
>> +# 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;

Done.

>> +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?

Agree and done.

> 
>> +  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))
>>
>>
> 
> 

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