Re: [Maria-developers] [Commits] 321a43d9ac5: MDEV-8874 Replication filters configured in my.cnf are ignored if slave reset and reconfigured
Sachin, salute! The patch looks good, but I think it misses some verbosity in few places, please read on.
revision-id: 321a43d9ac586034790407dc519c72e90b3bff81 (mariadb-10.1.39-54-g321a43d9ac5) parent(s): b003b0c934cf6b59358e31144c4f69cf34622fb8 author: Sachin committer: Sachin timestamp: 2019-06-06 12:03:45 +0530 message:
MDEV-8874 Replication filters configured in my.cnf are ignored if slave reset and reconfigured
Don't delete the rpl_filter on RESET SLAVE.
--- mysql-test/lib/My/Config.pm | 8 +- mysql-test/suite/multi_source/mdev-8874.cnf | 25 +++++ mysql-test/suite/multi_source/mdev-8874.result | 93 +++++++++++++++++++ mysql-test/suite/multi_source/mdev-8874.test | 123 +++++++++++++++++++++++++ sql/rpl_mi.cc | 2 - 5 files changed, 248 insertions(+), 3 deletions(-)
diff --git a/mysql-test/lib/My/Config.pm b/mysql-test/lib/My/Config.pm index 12647edf0a4..349a397ed6d 100644 --- a/mysql-test/lib/My/Config.pm +++ b/mysql-test/lib/My/Config.pm @@ -327,7 +327,13 @@ sub new { # Skip comment next; } - + + # Replication Filter + elsif ( $line =~ /^([\w]+.[\w]+)\s*=\s*(.*)\s*/){ + my $option= $1; + my $value= $2; + $self->insert($group_name, $option, $value); + }
What was a problem at the above point? [Anything that you think may not be straightforward for a patch reader is always worth to highlight in the description.]
else { croak "Unexpected line '$line' found in '$path'"; } diff --git a/mysql-test/suite/multi_source/mdev-8874.cnf b/mysql-test/suite/multi_source/mdev-8874.cnf new file mode 100644 index 00000000000..01da70cbaa0 --- /dev/null +++ b/mysql-test/suite/multi_source/mdev-8874.cnf @@ -0,0 +1,25 @@ +!include my.cnf + +[mysqld.1] +log-bin +log-slave-updates + +[mysqld.2] +log-bin +log-slave-updates + +[mysqld.3] +log-bin +log-slave-updates + +[mysqld.4] +server-id=4 +log-bin=server4-bin +log-slave-updates +m1.replicate_ignore_table='a.t1' +m2.replicate_ignore_table='b.t1' +replicate_ignore_table='c.t1' + +[ENV] +SERVER_MYPORT_4= @mysqld.4.port +SERVER_MYSOCK_4= @mysqld.4.socket diff --git a/mysql-test/suite/multi_source/mdev-8874.result b/mysql-test/suite/multi_source/mdev-8874.result new file mode 100644 index 00000000000..6fb364a3d2d --- /dev/null +++ b/mysql-test/suite/multi_source/mdev-8874.result @@ -0,0 +1,93 @@ +create database a; +use a; +create table t1(a int); +insert into t1 values(1); +create table t2(a int); +insert into t2 values(1); +create database b; +use b; +create table t1(a int); +insert into t1 values(1); +create table t2(a int); +insert into t2 values(1); +create database c; +use c; +create table t1(a int); +insert into t1 values(1); +create table t2(a int); +insert into t2 values(1); +change master 'm1' to master_port=MYPORT_1 , master_host='127.0.0.1', master_user='root'; +change master 'm2' to master_port=MYPORT_2 , master_host='127.0.0.1', master_user='root'; +change master to master_port=MYPORT_3 , master_host='127.0.0.1', master_user='root'; +start all slaves; +set default_master_connection = 'm1'; +include/wait_for_slave_to_start.inc +set default_master_connection = 'm2'; +include/wait_for_slave_to_start.inc +set default_master_connection = ''; +include/wait_for_slave_to_start.inc +use a; +show tables; +Tables_in_a +t2 +use b; +show tables; +Tables_in_b +t2 +use c; +show tables; +Tables_in_c +t2 +#TEST +STOP ALL SLAVES; +Warnings: +Note 1938 SLAVE 'm2' stopped +Note 1938 SLAVE '' stopped +Note 1938 SLAVE 'm1' stopped +RESET SLAVE 'm1' ALL ; +RESET SLAVE 'm2' ALL ; +RESET SLAVE ALL ; +drop database a; +drop database b; +drop database c; +change master 'm1' to master_port=MYPORT_1 , master_host='127.0.0.1', master_user='root'; +change master 'm2' to master_port=MYPORT_2 , master_host='127.0.0.1', master_user='root'; +change master to master_port=MYPORT_3 , master_host='127.0.0.1', master_user='root'; +start all slaves; +Warnings: +Note 1937 SLAVE 'm2' started +Note 1937 SLAVE '' started +Note 1937 SLAVE 'm1' started +set default_master_connection = 'm1'; +include/wait_for_slave_to_start.inc +set default_master_connection = 'm2'; +include/wait_for_slave_to_start.inc +set default_master_connection = ''; +include/wait_for_slave_to_start.inc +use a; +show tables; +Tables_in_a +t2 +use b; +show tables; +Tables_in_b +t2 +use c; +show tables; +Tables_in_c +t2 +#CleanUp +drop database a; +drop database b; +drop database c; +stop all slaves; +Warnings: +Note 1938 SLAVE 'm2' stopped +Note 1938 SLAVE '' stopped +Note 1938 SLAVE 'm1' stopped +SET default_master_connection = "m1"; +include/wait_for_slave_to_stop.inc +SET default_master_connection = "m2"; +include/wait_for_slave_to_stop.inc +SET default_master_connection = ""; +include/wait_for_slave_to_stop.inc
To the test part, I suggest we always follow a well-know description pattern (which is a great time saver when it comes to future test maintenance, apart from the current reviewing), to include the task id (present in the name though) and title, what failed and *how* the test proves the fixes. I can bet the failure was also exposed by reset @@global.'connection_name'.'rule_type' variables, which the test is really missing. Could you please add up that?
diff --git a/mysql-test/suite/multi_source/mdev-8874.test b/mysql-test/suite/multi_source/mdev-8874.test new file mode 100644 index 00000000000..b6d1aafe139 --- /dev/null +++ b/mysql-test/suite/multi_source/mdev-8874.test @@ -0,0 +1,123 @@ +--source include/not_embedded.inc +--source include/have_innodb.inc +--source include/have_debug.inc + +--connect (server_1,127.0.0.1,root,,,$SERVER_MYPORT_1) +--connect (server_2,127.0.0.1,root,,,$SERVER_MYPORT_2) +--connect (server_3,127.0.0.1,root,,,$SERVER_MYPORT_3) +--connect (server_4,127.0.0.1,root,,,$SERVER_MYPORT_4) + +--connection server_1 +create database a; +use a; +create table t1(a int); +insert into t1 values(1); +create table t2(a int); +insert into t2 values(1); +--save_master_pos + +--connection server_2 +create database b; +use b; +create table t1(a int); +insert into t1 values(1); +create table t2(a int); +insert into t2 values(1); +--save_master_pos + +--connection server_3 +create database c; +use c; +create table t1(a int); +insert into t1 values(1); +create table t2(a int); +insert into t2 values(1); +--save_master_pos + +--connection server_4 +--disable_warnings +--replace_result $SERVER_MYPORT_1 MYPORT_1 +eval change master 'm1' to master_port=$SERVER_MYPORT_1 , master_host='127.0.0.1', master_user='root'; +--replace_result $SERVER_MYPORT_2 MYPORT_2 +eval change master 'm2' to master_port=$SERVER_MYPORT_2 , master_host='127.0.0.1', master_user='root'; +--replace_result $SERVER_MYPORT_3 MYPORT_3 +eval change master to master_port=$SERVER_MYPORT_3 , master_host='127.0.0.1', master_user='root'; +start all slaves; +set default_master_connection = 'm1'; +--source include/wait_for_slave_to_start.inc +set default_master_connection = 'm2'; +--source include/wait_for_slave_to_start.inc +set default_master_connection = ''; +--source include/wait_for_slave_to_start.inc + +--enable_warnings +--sync_with_master 0,'m1' +--sync_with_master 0,'m2' +--sync_with_master 0,'' +use a;
When the test proves something via printing out, it's always good to prepare the output with an --echo what-is-expected so a thankful reader will bless you .. and the others may mercy :-)
+show tables; +use b; +show tables; +use c; +show tables; +--echo #TEST +STOP ALL SLAVES; +RESET SLAVE 'm1' ALL ; +RESET SLAVE 'm2' ALL ; +RESET SLAVE ALL ; +drop database a; +drop database b; +drop database c; +--replace_result $SERVER_MYPORT_1 MYPORT_1 +eval change master 'm1' to master_port=$SERVER_MYPORT_1 , master_host='127.0.0.1', master_user='root'; +--replace_result $SERVER_MYPORT_2 MYPORT_2 +eval change master 'm2' to master_port=$SERVER_MYPORT_2 , master_host='127.0.0.1', master_user='root'; +--replace_result $SERVER_MYPORT_3 MYPORT_3 +eval change master to master_port=$SERVER_MYPORT_3 , master_host='127.0.0.1', master_user='root'; +start all slaves; +set default_master_connection = 'm1'; +--source include/wait_for_slave_to_start.inc +set default_master_connection = 'm2'; +--source include/wait_for_slave_to_start.inc +set default_master_connection = ''; +--source include/wait_for_slave_to_start.inc +--sync_with_master 0,'m1' +--sync_with_master 0,'m2' +--sync_with_master 0,'' +
Naturally, the same applies to the following block.
+use a; +show tables; +use b; +show tables; +use c; +show tables;
+ + +#--echo #restart the server +#--source include/restart_mysqld.inc + + +--echo #CleanUp +--connection server_1 +drop database a; +--save_master_pos + +--connection server_2 +drop database b; +--save_master_pos + +--connection server_3 +drop database c; +--save_master_pos + +--connection server_4 +--sync_with_master 0,'m1' +--sync_with_master 0,'m2' +--sync_with_master 0,'' +stop all slaves; +SET default_master_connection = "m1"; +--source include/wait_for_slave_to_stop.inc +SET default_master_connection = "m2"; +--source include/wait_for_slave_to_stop.inc +SET default_master_connection = ""; +--source include/wait_for_slave_to_stop.inc diff --git a/sql/rpl_mi.cc b/sql/rpl_mi.cc index 70e60b1d4ad..7aea89337a7 100644 --- a/sql/rpl_mi.cc +++ b/sql/rpl_mi.cc @@ -122,8 +122,6 @@ Master_info::~Master_info() */ if (strncmp(connection_name.str, STRING_WITH_LEN("wsrep"))) #endif - rpl_filters.delete_element(connection_name.str, connection_name.length, - (void (*)(const char*, uchar*)) free_rpl_filter); my_free(connection_name.str); delete_dynamic(&ignore_server_ids); mysql_mutex_destroy(&run_lock); _______________________________________________ commits mailing list commits@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
Hi Andrei!
On Mon, Jun 10, 2019 at 8:31 PM
Sachin, salute!
The patch looks good, but I think it misses some verbosity in few places, please read on.
revision-id: 321a43d9ac586034790407dc519c72e90b3bff81 (mariadb-10.1.39-54-g321a43d9ac5) parent(s): b003b0c934cf6b59358e31144c4f69cf34622fb8 author: Sachin committer: Sachin timestamp: 2019-06-06 12:03:45 +0530 message:
MDEV-8874 Replication filters configured in my.cnf are ignored if slave reset and reconfigured
Don't delete the rpl_filter on RESET SLAVE.
--- mysql-test/lib/My/Config.pm | 8 +- mysql-test/suite/multi_source/mdev-8874.cnf | 25 +++++ mysql-test/suite/multi_source/mdev-8874.result | 93 +++++++++++++++++++ mysql-test/suite/multi_source/mdev-8874.test | 123 +++++++++++++++++++++++++ sql/rpl_mi.cc | 2 - 5 files changed, 248 insertions(+), 3 deletions(-)
diff --git a/mysql-test/lib/My/Config.pm b/mysql-test/lib/My/Config.pm index 12647edf0a4..349a397ed6d 100644 --- a/mysql-test/lib/My/Config.pm +++ b/mysql-test/lib/My/Config.pm @@ -327,7 +327,13 @@ sub new { # Skip comment next; } - + + # Replication Filter + elsif ( $line =~ /^([\w]+.[\w]+)\s*=\s*(.*)\s*/){ + my $option= $1; + my $value= $2; + $self->insert($group_name, $option, $value); + }
What was a problem at the above point? [Anything that you think may not be straightforward for a patch reader is always worth to highlight in the description.]
Added a comment in code. Earlier it was unable to parse replication filter with connection name
else { croak "Unexpected line '$line' found in '$path'"; } diff --git a/mysql-test/suite/multi_source/mdev-8874.cnf b/mysql-test/suite/multi_source/mdev-8874.cnf new file mode 100644 index 00000000000..01da70cbaa0 --- /dev/null +++ b/mysql-test/suite/multi_source/mdev-8874.cnf @@ -0,0 +1,25 @@ +!include my.cnf + +[mysqld.1] +log-bin +log-slave-updates + +[mysqld.2] +log-bin +log-slave-updates + +[mysqld.3] +log-bin +log-slave-updates + +[mysqld.4] +server-id=4 +log-bin=server4-bin +log-slave-updates +m1.replicate_ignore_table='a.t1' +m2.replicate_ignore_table='b.t1' +replicate_ignore_table='c.t1' + +[ENV] +SERVER_MYPORT_4= @mysqld.4.port +SERVER_MYSOCK_4= @mysqld.4.socket diff --git a/mysql-test/suite/multi_source/mdev-8874.result b/mysql-test/suite/multi_source/mdev-8874.result new file mode 100644 index 00000000000..6fb364a3d2d --- /dev/null +++ b/mysql-test/suite/multi_source/mdev-8874.result @@ -0,0 +1,93 @@ +create database a; +use a; +create table t1(a int); +insert into t1 values(1); +create table t2(a int); +insert into t2 values(1); +create database b; +use b; +create table t1(a int); +insert into t1 values(1); +create table t2(a int); +insert into t2 values(1); +create database c; +use c; +create table t1(a int); +insert into t1 values(1); +create table t2(a int); +insert into t2 values(1); +change master 'm1' to master_port=MYPORT_1 , master_host='127.0.0.1', master_user='root'; +change master 'm2' to master_port=MYPORT_2 , master_host='127.0.0.1', master_user='root'; +change master to master_port=MYPORT_3 , master_host='127.0.0.1', master_user='root'; +start all slaves; +set default_master_connection = 'm1'; +include/wait_for_slave_to_start.inc +set default_master_connection = 'm2'; +include/wait_for_slave_to_start.inc +set default_master_connection = ''; +include/wait_for_slave_to_start.inc +use a; +show tables; +Tables_in_a +t2 +use b; +show tables; +Tables_in_b +t2 +use c; +show tables; +Tables_in_c +t2 +#TEST +STOP ALL SLAVES; +Warnings: +Note 1938 SLAVE 'm2' stopped +Note 1938 SLAVE '' stopped +Note 1938 SLAVE 'm1' stopped +RESET SLAVE 'm1' ALL ; +RESET SLAVE 'm2' ALL ; +RESET SLAVE ALL ; +drop database a; +drop database b; +drop database c; +change master 'm1' to master_port=MYPORT_1 , master_host='127.0.0.1', master_user='root'; +change master 'm2' to master_port=MYPORT_2 , master_host='127.0.0.1', master_user='root'; +change master to master_port=MYPORT_3 , master_host='127.0.0.1', master_user='root'; +start all slaves; +Warnings: +Note 1937 SLAVE 'm2' started +Note 1937 SLAVE '' started +Note 1937 SLAVE 'm1' started +set default_master_connection = 'm1'; +include/wait_for_slave_to_start.inc +set default_master_connection = 'm2'; +include/wait_for_slave_to_start.inc +set default_master_connection = ''; +include/wait_for_slave_to_start.inc +use a; +show tables; +Tables_in_a +t2 +use b; +show tables; +Tables_in_b +t2 +use c; +show tables; +Tables_in_c +t2 +#CleanUp +drop database a; +drop database b; +drop database c; +stop all slaves; +Warnings: +Note 1938 SLAVE 'm2' stopped +Note 1938 SLAVE '' stopped +Note 1938 SLAVE 'm1' stopped +SET default_master_connection = "m1"; +include/wait_for_slave_to_stop.inc +SET default_master_connection = "m2"; +include/wait_for_slave_to_stop.inc +SET default_master_connection = ""; +include/wait_for_slave_to_stop.inc
To the test part, I suggest we always follow a well-know description pattern (which is a great time saver when it comes to future test maintenance, apart from the current reviewing), to include the task id (present in the name though) and title, what failed and *how* the test proves the fixes.
Sorry , Corrected.
I can bet the failure was also exposed by reset @@global.'connection_name'.'rule_type' variables, which the test is really missing. Could you please add up that?
Right, Done
diff --git a/mysql-test/suite/multi_source/mdev-8874.test b/mysql-test/suite/multi_source/mdev-8874.test new file mode 100644 index 00000000000..b6d1aafe139 --- /dev/null +++ b/mysql-test/suite/multi_source/mdev-8874.test @@ -0,0 +1,123 @@ +--source include/not_embedded.inc +--source include/have_innodb.inc +--source include/have_debug.inc + +--connect (server_1,127.0.0.1,root,,,$SERVER_MYPORT_1) +--connect (server_2,127.0.0.1,root,,,$SERVER_MYPORT_2) +--connect (server_3,127.0.0.1,root,,,$SERVER_MYPORT_3) +--connect (server_4,127.0.0.1,root,,,$SERVER_MYPORT_4) + +--connection server_1 +create database a; +use a; +create table t1(a int); +insert into t1 values(1); +create table t2(a int); +insert into t2 values(1); +--save_master_pos + +--connection server_2 +create database b; +use b; +create table t1(a int); +insert into t1 values(1); +create table t2(a int); +insert into t2 values(1); +--save_master_pos + +--connection server_3 +create database c; +use c; +create table t1(a int); +insert into t1 values(1); +create table t2(a int); +insert into t2 values(1); +--save_master_pos + +--connection server_4 +--disable_warnings +--replace_result $SERVER_MYPORT_1 MYPORT_1 +eval change master 'm1' to master_port=$SERVER_MYPORT_1 , master_host='127.0.0.1', master_user='root'; +--replace_result $SERVER_MYPORT_2 MYPORT_2 +eval change master 'm2' to master_port=$SERVER_MYPORT_2 , master_host='127.0.0.1', master_user='root'; +--replace_result $SERVER_MYPORT_3 MYPORT_3 +eval change master to master_port=$SERVER_MYPORT_3 , master_host='127.0.0.1', master_user='root'; +start all slaves; +set default_master_connection = 'm1'; +--source include/wait_for_slave_to_start.inc +set default_master_connection = 'm2'; +--source include/wait_for_slave_to_start.inc +set default_master_connection = ''; +--source include/wait_for_slave_to_start.inc + +--enable_warnings +--sync_with_master 0,'m1' +--sync_with_master 0,'m2' +--sync_with_master 0,'' +use a;
When the test proves something via printing out, it's always good to prepare the output with an
--echo what-is-expected
Done, Sorry
so a thankful reader will bless you .. and the others may mercy :-)
+show tables; +use b; +show tables; +use c; +show tables; +--echo #TEST +STOP ALL SLAVES; +RESET SLAVE 'm1' ALL ; +RESET SLAVE 'm2' ALL ; +RESET SLAVE ALL ; +drop database a; +drop database b; +drop database c; +--replace_result $SERVER_MYPORT_1 MYPORT_1 +eval change master 'm1' to master_port=$SERVER_MYPORT_1 , master_host='127.0.0.1', master_user='root'; +--replace_result $SERVER_MYPORT_2 MYPORT_2 +eval change master 'm2' to master_port=$SERVER_MYPORT_2 , master_host='127.0.0.1', master_user='root'; +--replace_result $SERVER_MYPORT_3 MYPORT_3 +eval change master to master_port=$SERVER_MYPORT_3 , master_host='127.0.0.1', master_user='root'; +start all slaves; +set default_master_connection = 'm1'; +--source include/wait_for_slave_to_start.inc +set default_master_connection = 'm2'; +--source include/wait_for_slave_to_start.inc +set default_master_connection = ''; +--source include/wait_for_slave_to_start.inc +--sync_with_master 0,'m1' +--sync_with_master 0,'m2' +--sync_with_master 0,'' +
Naturally, the same applies to the following block.
Done.
+use a; +show tables; +use b; +show tables; +use c; +show tables;
+ + +#--echo #restart the server +#--source include/restart_mysqld.inc + + +--echo #CleanUp +--connection server_1 +drop database a; +--save_master_pos + +--connection server_2 +drop database b; +--save_master_pos + +--connection server_3 +drop database c; +--save_master_pos + +--connection server_4 +--sync_with_master 0,'m1' +--sync_with_master 0,'m2' +--sync_with_master 0,'' +stop all slaves; +SET default_master_connection = "m1"; +--source include/wait_for_slave_to_stop.inc +SET default_master_connection = "m2"; +--source include/wait_for_slave_to_stop.inc +SET default_master_connection = ""; +--source include/wait_for_slave_to_stop.inc diff --git a/sql/rpl_mi.cc b/sql/rpl_mi.cc index 70e60b1d4ad..7aea89337a7 100644 --- a/sql/rpl_mi.cc +++ b/sql/rpl_mi.cc @@ -122,8 +122,6 @@ Master_info::~Master_info() */ if (strncmp(connection_name.str, STRING_WITH_LEN("wsrep"))) #endif - rpl_filters.delete_element(connection_name.str, connection_name.length, - (void (*)(const char*, uchar*)) free_rpl_filter); my_free(connection_name.str); delete_dynamic(&ignore_server_ids); mysql_mutex_destroy(&run_lock); _______________________________________________ commits mailing list commits@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
Patch Link http://lists.askmonty.org/pipermail/commits/2019-June/013851.html -- Regards Sachin Setiya Software Engineer at MariaDB
participants (2)
-
andrei.elkin＠pp.inet.fi -
Sachin Setiya