Hi Andrei!
Patch looks fine, but I have some minor comments to make maintenance
easier. See my comments inline below.
Just my few cents,
Mats Kindahl
Andrei Elkin wrote:
> Below is the list of changes that have just been committed into a local
> 5.1 repository of aelkin. When aelkin does a push these changes
> will be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2008-03-04 11:29:50+02:00, aelkin@mysql1000.(none) +4 -0
> Bug #28780 report_host is not available through SELECT @@report_host
>
> There was no way to see if report-{host,port,user,password} were set up.
>
> Fixed with introducing new global variables.
> The variables are made read-only because of a possible need to change them
> most probably require the slave server restart.
>
> Todo: transform the startup options to be CHANGE master parameters - i.e
> to deprecate `report-' options, and to change the new vars
> to be updatable at time of CHANGE master executes with new
> values.
>
> mysql-test/suite/rpl/r/rpl_report.result@stripped, 2008-03-04 11:29:48+02:00,
> aelkin@mysql1000.(none) +27 -0
> new results file
>
> mysql-test/suite/rpl/r/rpl_report.result@stripped, 2008-03-04 11:29:48+02:00,
> aelkin@mysql1000.(none) +0 -0
>
> mysql-test/suite/rpl/t/rpl_replicate-slave.opt@stripped, 2008-03-04 11:29:48+02:00,
> aelkin@mysql1000.(none) +2 -0
> options to be the source for values for global vars
>
> mysql-test/suite/rpl/t/rpl_replicate-slave.opt@stripped, 2008-03-04 11:29:48+02:00,
> aelkin@mysql1000.(none) +0 -0
>
> mysql-test/suite/rpl/t/rpl_report.test@stripped, 2008-03-04 11:29:48+02:00,
> aelkin@mysql1000.(none) +21 -0
> The new test to check SHOW-ability and SELECT-ablity for the report
> global vars.
>
> mysql-test/suite/rpl/t/rpl_report.test@stripped, 2008-03-04 11:29:48+02:00,
> aelkin@mysql1000.(none) +0 -0
>
> sql/set_var.cc@stripped, 2008-03-04 11:29:48+02:00, aelkin@mysql1000.(none) +13 -0
> Adding associations of the server init arguments with the new global read-only
> variables.
>
> diff -Nrup a/mysql-test/suite/rpl/r/rpl_report.result
> b/mysql-test/suite/rpl/r/rpl_report.result
> --- /dev/null Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/rpl/r/rpl_report.result 2008-03-04 11:29:48 +02:00
> @@ -0,0 +1,27 @@
> +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;
> +select * from Information_schema.GLOBAL_VARIABLES where variable_name regexp
> 'report_\(host\|port\|user\|password\)';
> +VARIABLE_NAME VARIABLE_VALUE
> +REPORT_HOST 127.0.0.1
> +REPORT_PORT 9308
>
This will be different for different runs, e.g., for different test
runs. Please just mask out values that you know might change depending
on configuration parameters that are outside your control.
In this case I would also suggest using a vertical output.
> +REPORT_PASSWORD
> +REPORT_USER root
> +show global variables like 'report_host';
> +Variable_name Value
> +report_host 127.0.0.1
> +show global variables like 'report_port';
> +Variable_name Value
> +report_port 9308
> +show global variables like 'report_user';
> +Variable_name Value
> +report_user root
> +show global variables like 'report_password';
> +Variable_name Value
> +report_password
>
I would suggest using:
query_vertical show global variables like 'report_%'
> +set @@global.report_host='my.new.address.net';
> +ERROR HY000: Variable 'report_host' is a read only variable
> +end of tests
> diff -Nrup a/mysql-test/suite/rpl/t/rpl_replicate-slave.opt
> b/mysql-test/suite/rpl/t/rpl_replicate-slave.opt
> --- /dev/null Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/rpl/t/rpl_replicate-slave.opt 2008-03-04 11:29:48 +02:00
> @@ -0,0 +1,2 @@
> +--report-host=127.0.0.1 --report-user='my_user' --report-password='my_password'
> --report-port=9308
> +
>
OK. So the comment above does not apply, these values are decided by the
configuration options.
> diff -Nrup a/mysql-test/suite/rpl/t/rpl_report.test
> b/mysql-test/suite/rpl/t/rpl_report.test
> --- /dev/null Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/rpl/t/rpl_report.test 2008-03-04 11:29:48 +02:00
> @@ -0,0 +1,21 @@
> +# Verify that mysqld init time --report-{host,port,user,password} parameters
> +# are SHOW-able and SELECT-able FROM INFORMATION_SCHEMA.global_variables
> +
> +source include/master-slave.inc;
> +
> +connection master;
> +sync_slave_with_master;
>
What's the purpose of this? Isn't that already done in the master-slave.inc?
> +
> +connection slave;
> +select * from Information_schema.GLOBAL_VARIABLES where variable_name regexp
> 'report_\(host\|port\|user\|password\)';
> +show global variables like 'report_host';
> +show global variables like 'report_port';
> +show global variables like 'report_user';
> +show global variables like 'report_password';
> +
> +# to demonstrate that report global variables are read-only
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
>
Would suggest using the form without -- so that syntax checks are made.
> +set @@global.report_host='my.new.address.net';
> +
> +
> +--echo end of tests
> diff -Nrup a/sql/set_var.cc b/sql/set_var.cc
> --- a/sql/set_var.cc 2008-02-01 13:15:10 +02:00
> +++ b/sql/set_var.cc 2008-03-04 11:29:48 +02:00
> @@ -648,6 +648,19 @@ sys_var_thd_time_zone sys_tim
>
> /* Global read-only variable containing hostname */
> static sys_var_const_str sys_hostname(&vars, "hostname", glob_hostname);
> +static sys_var_const_str_ptr sys_repl_report_host(&vars, "report_host",
> &report_host);
> +static sys_var_const_str_ptr sys_repl_report_user(&vars, "report_user",
> &report_user);
> +static sys_var_const_str_ptr sys_repl_report_password(&vars,
> "report_password", &report_password);
> +
> +static uchar *slave_get_report_port(THD *thd)
> +{
> + thd->sys_var_tmp.long_value= report_port;
> + return (uchar*) &thd->sys_var_tmp.long_value;
> +}
> +
> +static sys_var_readonly sys_repl_report_port(&vars, "report_port",
> OPT_GLOBAL, SHOW_INT, slave_get_report_port);
> +
> +
>
> sys_var_thd_bool sys_keep_files_on_create(&vars, "keep_files_on_create",
> &SV::keep_files_on_create);
>
>
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
