Hi Libing,
Some minor comments, patch approved!
Li-Bing.Song@stripped wrote:
> #At file:///home/anders/work/bzrwork/September/bug13963/mysql-next-bugfixing/ based
> on revid:alik@stripped
>
> 2818 Li-Bing.Song@stripped 2009-09-28
> Bug #13963 SHOW SLAVE HOSTS is unreliable
>
> Before the patch, slaves only appear in the output of SHOW SLAVE HOSTS
> when report-host option is set. If an expected slave does not appear in
> the list, nobody knows whether the slave does not connect or has started
> without the "report-host" option. The output also contains a strange
> field "Rpl_recovery_rank" which has never been implemented and the manual
> of MySQL6.0 declares that the field has been removed from MySQL6.0.
>
6.0 => 5.4
> This patch is done with these,
> According to the manual of MySQL5.4, "Rpl_recovery_rank" is removed.
> Slaves will register themself to master no matter if report_host option is set
themselves
> or not. When slaves are registering themselves, their Server_ids, report_host
> and other information are together sent to master. Sever_ids are never null
> and is unique in one replication group. Slaves always can be identified with
> different Server_ids no matter if report_host exists.
> @ mysql-test/suite/rpl/r/rpl_mixed_ddl_dml.result
> 'Rpl_recovery_rank' field has been removed.
> @ sql/repl_failsafe.cc
> rpl_recovery_rank has been removed.
> @ sql/slave.cc
>
>
> added:
> mysql-test/suite/rpl/r/rpl_show_slave_hosts.result
> mysql-test/suite/rpl/t/rpl_show_slave_hosts.cnf
> mysql-test/suite/rpl/t/rpl_show_slave_hosts.test
> modified:
> mysql-test/include/wait_show_condition.inc
> mysql-test/suite/rpl/r/rpl_mixed_ddl_dml.result
> sql/repl_failsafe.cc
> sql/slave.cc
> === modified file 'mysql-test/include/wait_show_condition.inc'
> --- a/mysql-test/include/wait_show_condition.inc 2009-04-30 10:42:50 +0000
> +++ b/mysql-test/include/wait_show_condition.inc 2009-09-28 14:24:36 +0000
> @@ -2,20 +2,23 @@
> #
> # SUMMARY
> #
> -# Waits until the show statement ($show_statement) has at least within one of
> -# the rows of the result set for the field ($field) a value which fulfils
> +# Waits until the show statement ($show_statement) has one or all of the
> +# rows of the result set for the field ($field) a value which fulfils
> # a condition ($condition), or the operation times out.
> #
> #
> # USAGE
> #
> +# All rows of the result must fulfil the condition if $all_rows_fulfil is 1
> +# else at least one of the result must fulfil the condition.
> +# let $all_rows_fulfil= 1;
I'd suggest: $wait_for_all
> # let $show_statement= SHOW PROCESSLIST;
> # let $field= State;
> # let $condition= = 'Updating';
> # --source include/wait_show_condition.inc
> #
> # OR
> -#
> +#
> # let $wait_timeout= 60; # Override default of 30 seconds with 60.
> # let $show_statement= SHOW PROCESSLIST;
> # let $field= State;
> @@ -33,8 +36,9 @@ if ($wait_timeout)
> {
> let $max_run_time= $wait_timeout;
> }
> -# Reset $wait_timeout so that its value won't be used on subsequent
> -# calls, and default will be used instead.
> +
> +# Reset $wait_timeout so that its value won't be used on subsequent calls, and
> +# default will be used instead.
> let $wait_timeout= 0;
>
> # The smallest timespan till UNIX_TIMESTAMP() gets incremented is ~0 seconds.
> @@ -46,27 +50,56 @@ inc $max_run_time;
>
> let $found= 0;
> let $max_end_time= `SELECT UNIX_TIMESTAMP() + $max_run_time`;
> -while (`SELECT UNIX_TIMESTAMP() <= $max_end_time AND $found = 0`)
> +
> +if (`SELECT '$all_rows_fulfil' != '1'`)
> +{
> + while (`SELECT UNIX_TIMESTAMP() <= $max_end_time AND $found = 0`)
> + {
> + # Sleep a bit to avoid too heavy load.
> + real_sleep 0.2;
> + let $rowno= 1;
> + let $process_result= 1;
> + while (`SELECT $process_result = 1 AND $found = 0`)
> + {
> + let $field_value= query_get_value($show_statement, $field, $rowno);
> + if (`SELECT '$field_value' $condition`)
> + {
> + let $found= 1;
> + }
> + if (`SELECT '$field_value' = 'No such row'`)
> + {
> + # We are behind the last row of the result set.
> + let $process_result= 0;
> + }
> + inc $rowno;
> + }
> + }
> +}
> +
> +if (`SELECT '$all_rows_fulfil' = '1'`)
> {
> - # Sleep a bit to avoid too heavy load.
> - real_sleep 0.2;
> - let $rowno= 1;
> - let $process_result= 1;
> - while (`SELECT $process_result = 1 AND $found = 0`)
> - {
> + while (`SELECT UNIX_TIMESTAMP() <= $max_end_time AND $found = 0`)
> + {
> + # Sleep a bit to avoid too heavy load.
> + real_sleep 0.2;
> + let $rowno= 1;
> + let $process_result= 1;
> + while (`SELECT $process_result = 1 AND $found = 0`)
> + {
> let $field_value= query_get_value($show_statement, $field, $rowno);
> - if (`SELECT '$field_value' $condition`)
> + if (`SELECT '$field_value' = 'No such row'`)
> {
> - let $found= 1;
> + let $found= 1;
> }
> - if (`SELECT '$field_value' = 'No such row'`)
> + if (`SELECT $found = 0 AND NOT '$field_value' $condition`)
> {
> - # We are behind the last row of the result set.
> - let $process_result= 0;
> + let process_result= 0;
> }
> inc $rowno;
> - }
Please fix the indentation of above block.
> + }
> + }
> }
> +
> if (!$found)
> {
> echo # Timeout in include/wait_show_condition.inc for $wait_condition;
>
> === modified file 'mysql-test/suite/rpl/r/rpl_mixed_ddl_dml.result'
> --- a/mysql-test/suite/rpl/r/rpl_mixed_ddl_dml.result 2008-11-04 17:07:14 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_mixed_ddl_dml.result 2009-09-28 14:24:36 +0000
> @@ -13,8 +13,8 @@ n
> 2001
> 2002
> show slave hosts;
> -Server_id Host Port Rpl_recovery_rank Master_id
> -2 127.0.0.1 9999 0 1
> +Server_id Host Port Master_id
> +2 127.0.0.1 9999 1
> drop table t1;
> stop slave;
> create table t2(id int auto_increment primary key, created datetime);
>
> === added file 'mysql-test/suite/rpl/r/rpl_show_slave_hosts.result'
> --- a/mysql-test/suite/rpl/r/rpl_show_slave_hosts.result 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_show_slave_hosts.result 2009-09-28 14:24:36 +0000
> @@ -0,0 +1,17 @@
> +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;
> +RESET SLAVE;
> +CHANGE MASTER TO master_host='127.0.0.1',master_port=13000,master_user='root';
> +START SLAVE IO_THREAD;
> +SHOW SLAVE HOSTS;
> +Server_id Host Port Master_id
> +3 slave2 3306 1
> +2 13001 1
> +STOP SLAVE IO_THREAD;
> +SHOW SLAVE HOSTS;
> +Server_id Host Port Master_id
> +2 13001 1
>
> === added file 'mysql-test/suite/rpl/t/rpl_show_slave_hosts.cnf'
> --- a/mysql-test/suite/rpl/t/rpl_show_slave_hosts.cnf 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_show_slave_hosts.cnf 2009-09-28 14:24:36 +0000
> @@ -0,0 +1,23 @@
> +!include ../my.cnf
> +
> +[mysqld.1]
> +server_id=1
> +mysql-backup=1
> +
> +[mysqld.2]
> +server_id=2
> +mysql-backup=1
> +report-host=
> +report-user=
> +
> +[mysqld.3]
> +server_id=3
> +mysql-backup=1
> +report-host=slave2
> +slave-net-timeout=5
> +
> +[ENV]
> +SLAVE_MYPORT2= @mysqld.3.port
> +SLAVE_MYSOCK2= @mysqld.3.socket
> +
> +
>
> === added file 'mysql-test/suite/rpl/t/rpl_show_slave_hosts.test'
> --- a/mysql-test/suite/rpl/t/rpl_show_slave_hosts.test 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_show_slave_hosts.test 2009-09-28 14:24:36 +0000
> @@ -0,0 +1,44 @@
> +###############################################################################
> +# Bug#13963 SHOW SLAVE HOSTS is unreliable
> +#
> +# Slaves only appear in the output of SHOW SLAVE HOSTS when report-host option
> +# is set. If an expected slave does not appear in the list, nobody knows
> +# whether the slave does not connect or has started without the "report-host"
> +# option.
> +#
> +# Remove the "Rpl_recovery_rank" column from SHOW SLAVE HOSTS, It is not
> +# implemented.
> +#######################################################################
> +source include/master-slave.inc;
> +connect (slave2,127.0.0.1,root,,test,$SLAVE_MYPORT2,);
> +
> +connection slave2;
> +RESET SLAVE;
> +--eval CHANGE MASTER TO
> master_host='127.0.0.1',master_port=$MASTER_MYPORT,master_user='root'
> +START SLAVE IO_THREAD;
> +source include/wait_for_slave_io_to_start.inc;
> +
> +connection master;
> +let $show_statement= SHOW SLAVE HOSTS;
> +let $field= Server_id;
> +# 3 is server_id of slave2.
> +let $connection= ='3';
> +source include/wait_show_condition.inc;
> +SHOW SLAVE HOSTS;
> +
> +connection slave2;
> +STOP SLAVE IO_THREAD;
> +source include/wait_for_slave_io_to_stop.inc;
> +
> +connection master;
> +let $show_statement= SHOW SLAVE HOSTS;
> +let $field= Server_id;
> +# 3 is server_id of slave2.
> +let $condition= <> '3';
> +# All rows of 'SHOW SLAVE HOSTS' are not equal to 3. It mean that master has
> +# knew the leave of slave2 and has unregistered it.
> +let $all_rows_fulfil= 1;
> +source include/wait_show_condition.inc;
> +SHOW SLAVE HOSTS;
> +
> +source include/master-slave-end.inc;
>
> === modified file 'sql/repl_failsafe.cc'
> --- a/sql/repl_failsafe.cc 2009-05-15 13:45:06 +0000
> +++ b/sql/repl_failsafe.cc 2009-09-28 14:24:36 +0000
> @@ -183,12 +183,11 @@ int register_slave(THD* thd, uchar* pack
> get_object(p,si->host, "Failed to register slave: too long 'report-host'");
> get_object(p,si->user, "Failed to register slave: too long 'report-user'");
> get_object(p,si->password, "Failed to register slave; too long
> 'report-password'");
> - if (p+10 > p_end)
> + /*6 is the total length of port and master_id*/
> + if (p+6 != p_end)
> goto err;
> si->port= uint2korr(p);
> p += 2;
> - si->rpl_recovery_rank= uint4korr(p);
> - p += 4;
> if (!(si->master_id= uint4korr(p)))
> si->master_id= server_id;
> si->thd= thd;
> @@ -670,8 +669,6 @@ bool show_slave_hosts(THD* thd)
> field_list.push_back(new Item_empty_string("Password",20));
> }
> field_list.push_back(new Item_return_int("Port", 7, MYSQL_TYPE_LONG));
> - field_list.push_back(new Item_return_int("Rpl_recovery_rank", 7,
> - MYSQL_TYPE_LONG));
> field_list.push_back(new Item_return_int("Master_id", 10,
> MYSQL_TYPE_LONG));
>
> @@ -693,7 +690,6 @@ bool show_slave_hosts(THD* thd)
> protocol->store(si->password, &my_charset_bin);
> }
> protocol->store((uint32) si->port);
> - protocol->store((uint32) si->rpl_recovery_rank);
> protocol->store((uint32) si->master_id);
> if (protocol->write())
> {
>
> === modified file 'sql/slave.cc'
> --- a/sql/slave.cc 2009-09-14 16:03:38 +0000
> +++ b/sql/slave.cc 2009-09-28 14:24:36 +0000
> @@ -1672,28 +1672,48 @@ int register_slave_on_master(MYSQL* mysq
> bool *suppress_warnings)
> {
> uchar buf[1024], *pos= buf;
> - uint report_host_len, report_user_len=0, report_password_len=0;
> + uint report_host_len=0, report_user_len=0, report_password_len=0;
> DBUG_ENTER("register_slave_on_master");
>
> *suppress_warnings= FALSE;
> - if (!report_host)
> + if (report_host)
> + report_host_len= strlen(report_host);
> + if (report_host_len > HOSTNAME_LENGTH)
> + {
> + sql_print_warning("The length of report_host is %d. "
> + "It is larger than the max length(%d), so this "
> + "slave cannot be registered to the master.",
> + report_host_len, HOSTNAME_LENGTH);
> DBUG_RETURN(0);
> - report_host_len= strlen(report_host);
> + }
> +
> if (report_user)
> report_user_len= strlen(report_user);
> + if (report_user_len > USERNAME_LENGTH)
> + {
> + sql_print_warning("The length of report_user is %d. "
> + "It is larger than the max length(%d), so this "
> + "slave cannot be registered to the master.",
> + report_user_len, USERNAME_LENGTH);
> + DBUG_RETURN(0);
> + }
> +
> if (report_password)
> report_password_len= strlen(report_password);
> - /* 30 is a good safety margin */
> - if (report_host_len + report_user_len + report_password_len + 30 >
> - sizeof(buf))
> - DBUG_RETURN(0); // safety
> + if (report_password_len > MAX_PASSWORD_LENGTH)
> + {
> + sql_print_warning("The length of report_password is %d. "
> + "It is larger than the max length(%d), so this "
> + "slave cannot be registered to the master.",
> + report_password_len, MAX_PASSWORD_LENGTH);
> + DBUG_RETURN(0);
> + }
>
> int4store(pos, server_id); pos+= 4;
> pos= net_store_data(pos, (uchar*) report_host, report_host_len);
> pos= net_store_data(pos, (uchar*) report_user, report_user_len);
> pos= net_store_data(pos, (uchar*) report_password, report_password_len);
> int2store(pos, (uint16) report_port); pos+= 2;
> - int4store(pos, rpl_recovery_rank); pos+= 4;
> /* The master will fill in master_id */
> int4store(pos, 0); pos+= 4;
>
>
