List:Commits« Previous MessageNext Message »
From:Daogang Qu Date:August 19 2010 10:23am
Subject:Please review Bug#51639
View as plain text  
Hi Bar,
Please review the following patch:
http://lists.mysql.com/commits/116221

The patch is based on your new collation and the following review comments.

Best Regards,

Daogang



Sven Sandberg wrote:
> Hi Dao-Gang,
>
> Good work with this. There are some mistakes that I think need to be 
> fixed, so I will reject this patch.
>
> (1) Check return values from functions. See comments inline.
Updated.
>
> (2) Can you add a comment above add_table_rule_to_hash(), explaining 
> why this function is needed and documenting the parameters and return 
> value? I think it's good if we say that rules are initially added to 
> DYNAMIC_LIST, and then, when the charset to use for tables has been 
> established, inserted into a HASH for faster filter checking.
Updated.
>
> (3) Please simplify build_*_table_hash: The two new functions 
> build_do_table_hash and build_ignore_table_hash are almost identical, 
> so I think you can merge them to one, like:
>
>  int build_table_hash(DYNAMIC_ARRAY *table_array, HASH *table_hash,
>                       int array_inited, int *hash_inited);
Updated.
>
> Also note that it should return an int, because it is possible that 
> the function fails. Moreover, add_table_rule_to_hash() is only called 
> from these functions, so I think it can be merged into the function.
I don't think the functions merge is good. Every function should has one 
duty.
And The both functions do well.
>
> (4) init_table_rule_hash() only needs to be called if the dynamic 
> array is inited.
Updated.
>
> (5) There is no test case for replicate-wild-do-table or 
> replicate-ignore-table. I think you can easily add those in the 
> -slave.opt files, see comments inline.
Added for replicate-ignore-table.
But replicate-wild-do-table have another problem.
So I will report a bug for it.
>
> (6) There are memory leaks in add_table_rule() and 
> add_table_rule_hash(). It's not your fault, and it's not serious, but 
> it's very easy to fix and I think it's ok to do it as part of this 
> patch. add_table_rule() first allocates memory for 'e', then it does 
> this:
>
>   return my_hash_insert(h, (uchar*)e);
>
> If my_hash_insert fails, e will not be inserted into the hash. So e 
> should be freed if my_hash_insert fails. So I think the above line 
> should be replaced by:
>
>   if (my_hash_insert(h, (uchar*)e))
>   {
>     free(e, MYF(0));
>     return 1;
>   }
>   return 0;
>
> The same problem exists in add_table_rule() in case insert_dynamic() 
> fails. Could you fix this?
Yes. Updated.
>
> /Sven
>
>
> Dao-Gang.Qu@stripped wrote:
>> #At 
>> file:///home/daogangqu/mysql/bzrwork1/bug51639/mysql-next-mr-bugfixing/ 
>> based on revid:aelkin@stripped
>>
>>  3310 Dao-Gang.Qu@stripped    2010-08-13
>>       Bug #51639  Some replication filters are case sensitive, some 
>> are not!
>>             There is an inconsistency in name comparison of rpl filters.
>>       Some are case sensitive, while others are not. And they did
>>       not follow the setting of lower_case_table_names.
>>             To fix the problem to make all the filters follow the 
>> setting
>>       of lower_case_table_name to be case sensitive when setting
>>       lower_case_table_name=0. Otherwise they will be case insensitive.
>>      @ mysql-test/suite/rpl/r/rpl_do_filter.result
>>         Test result for BUG# 51639
>>      @ mysql-test/suite/rpl/r/rpl_ignore_db_filter.result
>>         Test result for BUG# 51639
>>      @ mysql-test/suite/rpl/r/rpl_ignore_table_filter.result
>>         Test result for BUG# 51639
>>      @ mysql-test/suite/rpl/r/rpl_rewrite_db_filter.result
>>         Test result for BUG# 51639
>>      @ mysql-test/suite/rpl/t/rpl_do_filter.test
>>         Added test file to verify that 'do db' and 'do table 'filters
>>         will follow the setting of lower_case_table_name to be case
>>         insensitive when setting lower_case_table_name > 0
>>      @ mysql-test/suite/rpl/t/rpl_ignore_db_filter.test
>>         Added test file to verify that 'ignore db' filter will follow
>>         the setting of lower_case_table_name to be case insensitive when
>>         setting lower_case_table_name > 0
>>      @ mysql-test/suite/rpl/t/rpl_ignore_table_filter.test
>>         Added test file to verify that 'ignore table' filter will follow
>>         the setting of lower_case_table_name to be case sensitive when
>>         setting lower_case_table_name=0
>>      @ mysql-test/suite/rpl/t/rpl_rewrite_db_filter.test
>>         Added test file to verify that 'rewrite db' filter will follow
>>         the setting of lower_case_table_name to be case insensitive
>>         when setting lower_case_table_name > 0
>>      @ sql/mysqld.cc
>>         Added code to build do_table and ignore_table rules to hush
>>         after resetting of table_alias_charset, so that the filters
>>         is affected by the setting of lower_case_table_name
>>         regardless of the setting order.
>>      @ sql/rpl_filter.cc
>>         Added code to make filters follow the setting of 
>> lower_case_table_name
>>         to be case sensitive when setting lower_case_table_name=0. 
>> Otherwise they
>>         will be case insensitive. And build do_table and ignore table 
>> rules to
>>         Hash from DYNAMIC_ARRAY for quick search.
>>
>>     added:
>>       mysql-test/extra/rpl_tests/rpl_filters.test
>>       mysql-test/suite/rpl/r/rpl_do_filter.result
>>       mysql-test/suite/rpl/r/rpl_ignore_db_filter.result
>>       mysql-test/suite/rpl/r/rpl_ignore_table_filter.result
>>       mysql-test/suite/rpl/r/rpl_rewrite_db_filter.result
>>       mysql-test/suite/rpl/t/rpl_do_filter-master.opt
>>       mysql-test/suite/rpl/t/rpl_do_filter-slave.opt
>>       mysql-test/suite/rpl/t/rpl_do_filter.test
>>       mysql-test/suite/rpl/t/rpl_ignore_db_filter-master.opt
>>       mysql-test/suite/rpl/t/rpl_ignore_db_filter-slave.opt
>>       mysql-test/suite/rpl/t/rpl_ignore_db_filter.test
>>       mysql-test/suite/rpl/t/rpl_ignore_table_filter-master.opt
>>       mysql-test/suite/rpl/t/rpl_ignore_table_filter-slave.opt
>>       mysql-test/suite/rpl/t/rpl_ignore_table_filter.test
>>       mysql-test/suite/rpl/t/rpl_rewrite_db_filter-master.opt
>>       mysql-test/suite/rpl/t/rpl_rewrite_db_filter-slave.opt
>>       mysql-test/suite/rpl/t/rpl_rewrite_db_filter.test
>>     modified:
>>       sql/mysqld.cc
>>       sql/rpl_filter.cc
>>       sql/rpl_filter.h
>> === added file 'mysql-test/extra/rpl_tests/rpl_filters.test'
>> --- a/mysql-test/extra/rpl_tests/rpl_filters.test    1970-01-01 
>> 00:00:00 +0000
>> +++ b/mysql-test/extra/rpl_tests/rpl_filters.test    2010-08-13 
>> 07:33:56 +0000
>> @@ -0,0 +1,22 @@
>> +# +# Bug #51639
>> +# The common part of Bug #51639 to test filters
>> +#
>> +
>> +CREATE DATABASE mydb; +USE mydb;
>> +CREATE TABLE T1 (a INT) ENGINE= MYISAM;
>> +CREATE TABLE t2 (a INT) ENGINE= MYISAM;
>> +INSERT INTO T1 VALUES (1);
>> +INSERT INTO t2 VALUES (1);
>> +-- echo # On master
>> +-- source include/show_binlog_events.inc
>> +
>> +-- sync_slave_with_master
>> +-- echo # On slave
>> +-- source include/show_binlog_events.inc
>> +
>> +-- connection master
>> +DROP DATABASE mydb;
>> +-- sync_slave_with_master
>> +
>>
>> === added file 'mysql-test/suite/rpl/r/rpl_do_filter.result'
>> --- a/mysql-test/suite/rpl/r/rpl_do_filter.result    1970-01-01 
>> 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/r/rpl_do_filter.result    2010-08-13 
>> 07:33:56 +0000
>> @@ -0,0 +1,37 @@
>> +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;
>> +CREATE DATABASE mydb;
>> +USE mydb;
>> +CREATE TABLE T1 (a INT) ENGINE= MYISAM;
>> +CREATE TABLE t2 (a INT) ENGINE= MYISAM;
>> +INSERT INTO T1 VALUES (1);
>> +INSERT INTO t2 VALUES (1);
>> +# On master
>> +show binlog events from <binlog_start>;
>> +Log_name    Pos    Event_type    Server_id    End_log_pos    Info
>> +master-bin.000001    #    Query    #    #    CREATE DATABASE mydb
>> +master-bin.000001    #    Query    #    #    use `mydb`; CREATE 
>> TABLE T1 (a INT) ENGINE= MYISAM
>> +master-bin.000001    #    Query    #    #    use `mydb`; CREATE 
>> TABLE t2 (a INT) ENGINE= MYISAM
>> +master-bin.000001    #    Query    #    #    BEGIN
>> +master-bin.000001    #    Query    #    #    use `mydb`; INSERT INTO 
>> T1 VALUES (1)
>> +master-bin.000001    #    Query    #    #    COMMIT
>> +master-bin.000001    #    Query    #    #    BEGIN
>> +master-bin.000001    #    Query    #    #    use `mydb`; INSERT INTO 
>> t2 VALUES (1)
>> +master-bin.000001    #    Query    #    #    COMMIT
>> +# On slave
>> +show binlog events from <binlog_start>;
>> +Log_name    Pos    Event_type    Server_id    End_log_pos    Info
>> +slave-bin.000001    #    Query    #    #    CREATE DATABASE mydb
>> +slave-bin.000001    #    Query    #    #    use `mydb`; CREATE TABLE 
>> T1 (a INT) ENGINE= MYISAM
>> +slave-bin.000001    #    Query    #    #    use `mydb`; CREATE TABLE 
>> t2 (a INT) ENGINE= MYISAM
>> +slave-bin.000001    #    Query    #    #    BEGIN
>> +slave-bin.000001    #    Query    #    #    use `mydb`; INSERT INTO 
>> T1 VALUES (1)
>> +slave-bin.000001    #    Query    #    #    COMMIT
>> +slave-bin.000001    #    Query    #    #    BEGIN
>> +slave-bin.000001    #    Query    #    #    use `mydb`; INSERT INTO 
>> t2 VALUES (1)
>> +slave-bin.000001    #    Query    #    #    COMMIT
>> +DROP DATABASE mydb;
>>
>> === added file 'mysql-test/suite/rpl/r/rpl_ignore_db_filter.result'
>> --- a/mysql-test/suite/rpl/r/rpl_ignore_db_filter.result    
>> 1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/r/rpl_ignore_db_filter.result    
>> 2010-08-13 07:33:56 +0000
>> @@ -0,0 +1,28 @@
>> +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;
>> +CREATE DATABASE mydb;
>> +USE mydb;
>> +CREATE TABLE T1 (a INT) ENGINE= MYISAM;
>> +CREATE TABLE t2 (a INT) ENGINE= MYISAM;
>> +INSERT INTO T1 VALUES (1);
>> +INSERT INTO t2 VALUES (1);
>> +# On master
>> +show binlog events from <binlog_start>;
>> +Log_name    Pos    Event_type    Server_id    End_log_pos    Info
>> +master-bin.000001    #    Query    #    #    CREATE DATABASE mydb
>> +master-bin.000001    #    Query    #    #    use `mydb`; CREATE 
>> TABLE T1 (a INT) ENGINE= MYISAM
>> +master-bin.000001    #    Query    #    #    use `mydb`; CREATE 
>> TABLE t2 (a INT) ENGINE= MYISAM
>> +master-bin.000001    #    Query    #    #    BEGIN
>> +master-bin.000001    #    Query    #    #    use `mydb`; INSERT INTO 
>> T1 VALUES (1)
>> +master-bin.000001    #    Query    #    #    COMMIT
>> +master-bin.000001    #    Query    #    #    BEGIN
>> +master-bin.000001    #    Query    #    #    use `mydb`; INSERT INTO 
>> t2 VALUES (1)
>> +master-bin.000001    #    Query    #    #    COMMIT
>> +# On slave
>> +show binlog events from <binlog_start>;
>> +Log_name    Pos    Event_type    Server_id    End_log_pos    Info
>> +DROP DATABASE mydb;
>>
>> === added file 'mysql-test/suite/rpl/r/rpl_ignore_table_filter.result'
>> --- a/mysql-test/suite/rpl/r/rpl_ignore_table_filter.result    
>> 1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/r/rpl_ignore_table_filter.result    
>> 2010-08-13 07:33:56 +0000
>> @@ -0,0 +1,33 @@
>> +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;
>> +CREATE DATABASE mydb;
>> +USE mydb;
>> +CREATE TABLE T1 (a INT) ENGINE= MYISAM;
>> +CREATE TABLE t2 (a INT) ENGINE= MYISAM;
>> +INSERT INTO T1 VALUES (1);
>> +INSERT INTO t2 VALUES (1);
>> +# On master
>> +show binlog events from <binlog_start>;
>> +Log_name    Pos    Event_type    Server_id    End_log_pos    Info
>> +master-bin.000001    #    Query    #    #    CREATE DATABASE mydb
>> +master-bin.000001    #    Query    #    #    use `mydb`; CREATE 
>> TABLE T1 (a INT) ENGINE= MYISAM
>> +master-bin.000001    #    Query    #    #    use `mydb`; CREATE 
>> TABLE t2 (a INT) ENGINE= MYISAM
>> +master-bin.000001    #    Query    #    #    BEGIN
>> +master-bin.000001    #    Query    #    #    use `mydb`; INSERT INTO 
>> T1 VALUES (1)
>> +master-bin.000001    #    Query    #    #    COMMIT
>> +master-bin.000001    #    Query    #    #    BEGIN
>> +master-bin.000001    #    Query    #    #    use `mydb`; INSERT INTO 
>> t2 VALUES (1)
>> +master-bin.000001    #    Query    #    #    COMMIT
>> +# On slave
>> +show binlog events from <binlog_start>;
>> +Log_name    Pos    Event_type    Server_id    End_log_pos    Info
>> +slave-bin.000001    #    Query    #    #    CREATE DATABASE mydb
>> +slave-bin.000001    #    Query    #    #    use `mydb`; CREATE TABLE 
>> t2 (a INT) ENGINE= MYISAM
>> +slave-bin.000001    #    Query    #    #    BEGIN
>> +slave-bin.000001    #    Query    #    #    use `mydb`; INSERT INTO 
>> t2 VALUES (1)
>> +slave-bin.000001    #    Query    #    #    COMMIT
>> +DROP DATABASE mydb;
>>
>> === added file 'mysql-test/suite/rpl/r/rpl_rewrite_db_filter.result'
>> --- a/mysql-test/suite/rpl/r/rpl_rewrite_db_filter.result    
>> 1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/r/rpl_rewrite_db_filter.result    
>> 2010-08-13 07:33:56 +0000
>> @@ -0,0 +1,40 @@
>> +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;
>> +CREATE DATABASE rewrite;
>> +CREATE DATABASE mydb;
>> +USE mydb;
>> +CREATE TABLE T1 (a INT) ENGINE= MYISAM;
>> +CREATE TABLE t2 (a INT) ENGINE= MYISAM;
>> +INSERT INTO T1 VALUES (1);
>> +INSERT INTO t2 VALUES (1);
>> +# On master
>> +show binlog events from <binlog_start>;
>> +Log_name    Pos    Event_type    Server_id    End_log_pos    Info
>> +master-bin.000001    #    Query    #    #    CREATE DATABASE mydb
>> +master-bin.000001    #    Query    #    #    use `mydb`; CREATE 
>> TABLE T1 (a INT) ENGINE= MYISAM
>> +master-bin.000001    #    Query    #    #    use `mydb`; CREATE 
>> TABLE t2 (a INT) ENGINE= MYISAM
>> +master-bin.000001    #    Query    #    #    BEGIN
>> +master-bin.000001    #    Query    #    #    use `mydb`; INSERT INTO 
>> T1 VALUES (1)
>> +master-bin.000001    #    Query    #    #    COMMIT
>> +master-bin.000001    #    Query    #    #    BEGIN
>> +master-bin.000001    #    Query    #    #    use `mydb`; INSERT INTO 
>> t2 VALUES (1)
>> +master-bin.000001    #    Query    #    #    COMMIT
>> +# On slave
>> +show binlog events from <binlog_start>;
>> +Log_name    Pos    Event_type    Server_id    End_log_pos    Info
>> +slave-bin.000001    #    Query    #    #    CREATE DATABASE rewrite
>> +slave-bin.000001    #    Query    #    #    CREATE DATABASE mydb
>> +slave-bin.000001    #    Query    #    #    use `rewrite`; CREATE 
>> TABLE T1 (a INT) ENGINE= MYISAM
>> +slave-bin.000001    #    Query    #    #    use `rewrite`; CREATE 
>> TABLE t2 (a INT) ENGINE= MYISAM
>> +slave-bin.000001    #    Query    #    #    BEGIN
>> +slave-bin.000001    #    Query    #    #    use `rewrite`; INSERT 
>> INTO T1 VALUES (1)
>> +slave-bin.000001    #    Query    #    #    COMMIT
>> +slave-bin.000001    #    Query    #    #    BEGIN
>> +slave-bin.000001    #    Query    #    #    use `rewrite`; INSERT 
>> INTO t2 VALUES (1)
>> +slave-bin.000001    #    Query    #    #    COMMIT
>> +DROP DATABASE mydb;
>> +DROP DATABASE rewrite;
>>
>> === added file 'mysql-test/suite/rpl/t/rpl_do_filter-master.opt'
>> --- a/mysql-test/suite/rpl/t/rpl_do_filter-master.opt    1970-01-01 
>> 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_do_filter-master.opt    2010-08-13 
>> 07:33:56 +0000
>> @@ -0,0 +1 @@
>> +--lower_case_table_names=1
>>
>> === added file 'mysql-test/suite/rpl/t/rpl_do_filter-slave.opt'
>> --- a/mysql-test/suite/rpl/t/rpl_do_filter-slave.opt    1970-01-01 
>> 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_do_filter-slave.opt    2010-08-13 
>> 07:33:56 +0000
>> @@ -0,0 +1 @@
>> +--replicate-do-db=MYDB --replicate-do-table=mydb.T1 
>> --replicate-do-table=mydb.T2 --lower_case_table_names=1
>
> (5) There is no test for replicate-wild-do-table. I suggest to replace
>  --replicate-do-table=mydb.T2
> by
>  --replicate-wild-do-table=mydb.%T2
>
>>
>> === added file 'mysql-test/suite/rpl/t/rpl_do_filter.test'
>> --- a/mysql-test/suite/rpl/t/rpl_do_filter.test    1970-01-01 
>> 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_do_filter.test    2010-08-13 
>> 07:33:56 +0000
>> @@ -0,0 +1,12 @@
>> +#
>> +# Bug #51639
>> +# This test verifies that 'do db' and 'do table'filters
>> +# will follow the setting of lower_case_table_name to be
>> +# case insensitive when setting lower_case_table_name > 0
>> +#
>> +
>> +-- source include/master-slave.inc
>> +-- source include/have_binlog_format_statement.inc
>> +
>> +-- source extra/rpl_tests/rpl_filters.test +
>>
>> === added file 'mysql-test/suite/rpl/t/rpl_ignore_db_filter-master.opt'
>> --- a/mysql-test/suite/rpl/t/rpl_ignore_db_filter-master.opt    
>> 1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_ignore_db_filter-master.opt    
>> 2010-08-13 07:33:56 +0000
>> @@ -0,0 +1 @@
>> +--lower_case_table_names=1
>>
>> === added file 'mysql-test/suite/rpl/t/rpl_ignore_db_filter-slave.opt'
>> --- a/mysql-test/suite/rpl/t/rpl_ignore_db_filter-slave.opt    
>> 1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_ignore_db_filter-slave.opt    
>> 2010-08-13 07:33:56 +0000
>> @@ -0,0 +1 @@
>> +--replicate-ignore-db=MYDB --lower_case_table_names=1
>>
>> === added file 'mysql-test/suite/rpl/t/rpl_ignore_db_filter.test'
>> --- a/mysql-test/suite/rpl/t/rpl_ignore_db_filter.test    1970-01-01 
>> 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_ignore_db_filter.test    2010-08-13 
>> 07:33:56 +0000
>> @@ -0,0 +1,11 @@
>> +#
>> +# Bug #51639
>> +# This test verifies that 'ignore db' filter will follow
>> +# the setting of lower_case_table_name to be case insensitive
>> +# when setting lower_case_table_name > 0
>> +#
>> +
>> +-- source include/master-slave.inc
>> +-- source include/have_binlog_format_statement.inc
>> +
>> +-- source extra/rpl_tests/rpl_filters.test
>>
>> === added file 
>> 'mysql-test/suite/rpl/t/rpl_ignore_table_filter-master.opt'
>> --- a/mysql-test/suite/rpl/t/rpl_ignore_table_filter-master.opt    
>> 1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_ignore_table_filter-master.opt    
>> 2010-08-13 07:33:56 +0000
>> @@ -0,0 +1 @@
>> +--lower_case_table_names=0
>>
>> === added file 
>> 'mysql-test/suite/rpl/t/rpl_ignore_table_filter-slave.opt'
>> --- a/mysql-test/suite/rpl/t/rpl_ignore_table_filter-slave.opt    
>> 1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_ignore_table_filter-slave.opt    
>> 2010-08-13 07:33:56 +0000
>> @@ -0,0 +1 @@
>> +--replicate-do-db=mydb --replicate-wild-ignore-table=my%.T1 
>> --replicate-wild-ignore-table=my%.T2 --lower_case_table_names=0
>
> (5) There is no test for replicate-ignore-table. I suggest to replace
>  --replicate-wild-ignore-table=my%.T2
> by
>  --replicate-ignore-table=mydb.T2
>
>>
>> === added file 'mysql-test/suite/rpl/t/rpl_ignore_table_filter.test'
>> --- a/mysql-test/suite/rpl/t/rpl_ignore_table_filter.test    
>> 1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_ignore_table_filter.test    
>> 2010-08-13 07:33:56 +0000
>> @@ -0,0 +1,12 @@
>> +#
>> +# Bug #51639
>> +# This test verifies that 'ignore table' filter will follow
>> +# the setting of lower_case_table_name to be case sensitive
>> +# when setting lower_case_table_name=0
>> +#
>> +
>> +-- source include/master-slave.inc
>> +-- source include/have_binlog_format_statement.inc
>> +
>> +-- source extra/rpl_tests/rpl_filters.test +
>>
>> === added file 'mysql-test/suite/rpl/t/rpl_rewrite_db_filter-master.opt'
>> --- a/mysql-test/suite/rpl/t/rpl_rewrite_db_filter-master.opt    
>> 1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_rewrite_db_filter-master.opt    
>> 2010-08-13 07:33:56 +0000
>> @@ -0,0 +1 @@
>> +--lower_case_table_names=1
>>
>> === added file 'mysql-test/suite/rpl/t/rpl_rewrite_db_filter-slave.opt'
>> --- a/mysql-test/suite/rpl/t/rpl_rewrite_db_filter-slave.opt    
>> 1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_rewrite_db_filter-slave.opt    
>> 2010-08-13 07:33:56 +0000
>> @@ -0,0 +1 @@
>> +--replicate-rewrite-db=MYDB->rewrite --lower_case_table_names=1
>>
>> === added file 'mysql-test/suite/rpl/t/rpl_rewrite_db_filter.test'
>> --- a/mysql-test/suite/rpl/t/rpl_rewrite_db_filter.test    1970-01-01 
>> 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_rewrite_db_filter.test    2010-08-13 
>> 07:33:56 +0000
>> @@ -0,0 +1,17 @@
>> +#
>> +# Bug #51639
>> +# This test verifies that 'rewrite db' filter will follow
>> +# the setting of lower_case_table_name to be case insensitive
>> +# when setting lower_case_table_name > 0
>> +#
>> +
>> +-- source include/master-slave.inc
>> +-- source include/have_binlog_format_statement.inc
>> +connection slave;
>> +CREATE DATABASE rewrite;
>> +
>> +connection master;
>> +-- source extra/rpl_tests/rpl_filters.test
>> +
>> +connection slave;
>> +DROP DATABASE rewrite;
>>
>> === modified file 'sql/mysqld.cc'
>> --- a/sql/mysqld.cc    2010-07-16 21:00:50 +0000
>> +++ b/sql/mysqld.cc    2010-08-13 07:33:56 +0000
>> @@ -3514,6 +3514,13 @@ You should consider changing lower_case_
>>              files_charset_info :
>>              &my_charset_bin);
>>  
>> +  /*
>> +    Build do_table and ignore_table rules to hush
>> +    after the resetting of table_alias_charset
>> +  */
>> +  rpl_filter->build_do_table_hash();
>> +  rpl_filter->build_ignore_table_hash();
>
> (1) Please make these functions return 1 on on error, and check the 
> return values of these function calls.
>
>> +
>>    return 0;
>>  }
>>  
>>
>> === modified file 'sql/rpl_filter.cc'
>> --- a/sql/rpl_filter.cc    2010-07-08 21:42:23 +0000
>> +++ b/sql/rpl_filter.cc    2010-08-13 07:33:56 +0000
>> @@ -24,7 +24,8 @@
>>  
>>  Rpl_filter::Rpl_filter() :    table_rules_on(0), do_table_inited(0), 
>> ignore_table_inited(0),
>> -  wild_do_table_inited(0), wild_ignore_table_inited(0)
>> +  wild_do_table_inited(0), wild_ignore_table_inited(0),
>> +  do_table_hash_inited(0), ignore_table_hash_inited(0)
>>  {
>>    do_db.empty();
>>    ignore_db.empty();
>> @@ -34,10 +35,14 @@ Rpl_filter::Rpl_filter() :  
>>  Rpl_filter::~Rpl_filter()  {
>> -  if (do_table_inited) -    my_hash_free(&do_table);
>> +  if (do_table_hash_inited)
>> +    my_hash_free(&do_table_hash);
>> +  if (ignore_table_hash_inited)
>> +    my_hash_free(&ignore_table_hash);
>> +  if (do_table_inited)
>> +    free_string_array(&do_table);
>>    if (ignore_table_inited)
>> -    my_hash_free(&ignore_table);
>> +    free_string_array(&ignore_table);
>>    if (wild_do_table_inited)
>>      free_string_array(&wild_do_table);
>>    if (wild_ignore_table_inited)
>> @@ -91,7 +96,7 @@ Rpl_filter::tables_ok(const char* db, TA
>>  {
>>    bool some_tables_updating= 0;
>>    DBUG_ENTER("Rpl_filter::tables_ok");
>> -  +
>>    for (; tables; tables= tables->next_global)
>>    {
>>      char hash_key[2*NAME_LEN+2];
>> @@ -104,14 +109,14 @@ Rpl_filter::tables_ok(const char* db, TA
>>      end= strmov(hash_key, tables->db ? tables->db : db);
>>      *end++= '.';
>>      len= (uint) (strmov(end, tables->table_name) - hash_key);
>> -    if (do_table_inited) // if there are any do's
>> +    if (do_table_hash_inited) // if there are any do's
>>      {
>> -      if (my_hash_search(&do_table, (uchar*) hash_key, len))
>> +      if (my_hash_search(&do_table_hash, (uchar*) hash_key, len))
>>      DBUG_RETURN(1);
>>      }
>> -    if (ignore_table_inited) // if there are any ignores
>> +    if (ignore_table_hash_inited) // if there are any ignores
>>      {
>> -      if (my_hash_search(&ignore_table, (uchar*) hash_key, len))
>> +      if (my_hash_search(&ignore_table_hash, (uchar*) hash_key, len))
>>      DBUG_RETURN(0);      }
>>      if (wild_do_table_inited && @@ -167,8 +172,13 @@ 
>> Rpl_filter::db_ok(const char* db)
>>  
>>      while ((tmp=it++))
>>      {
>> -      if (!strcmp(tmp->ptr, db))
>> -    DBUG_RETURN(1); // match
>> +      /*
>> +        Filters will follow the setting of lower_case_table_name
>> +        to be case sensitive when setting lower_case_table_name=0.
>> +        Otherwise they will be case insensitive.
>> +      */
>> +      if (!my_strcasecmp(table_alias_charset, tmp->ptr, db))
>> +        DBUG_RETURN(1); // match
>>      }
>>      DBUG_RETURN(0);
>>    }
>> @@ -179,8 +189,13 @@ Rpl_filter::db_ok(const char* db)
>>  
>>      while ((tmp=it++))
>>      {
>> -      if (!strcmp(tmp->ptr, db))
>> -    DBUG_RETURN(0); // match
>> +      /*
>> +        Filters will follow the setting of lower_case_table_name
>> +        to be case sensitive when setting lower_case_table_name=0.
>> +        Otherwise they will be case insensitive.
>> +      */
>> +      if (!my_strcasecmp(table_alias_charset, tmp->ptr, db))
>> +        DBUG_RETURN(0); // match
>>      }
>>      DBUG_RETURN(1);
>>    }
>> @@ -255,23 +270,23 @@ Rpl_filter::is_on()
>>  }
>>  
>>  
>> -int +int
>>  Rpl_filter::add_do_table(const char* table_spec)  {
>>    DBUG_ENTER("Rpl_filter::add_do_table");
>>    if (!do_table_inited)
>> -    init_table_rule_hash(&do_table, &do_table_inited);
>> +    init_table_rule_array(&do_table, &do_table_inited);
>>    table_rules_on= 1;
>>    DBUG_RETURN(add_table_rule(&do_table, table_spec));
>>  }
>> -   
>> -int +
>> +int
>>  Rpl_filter::add_ignore_table(const char* table_spec)  {
>>    DBUG_ENTER("Rpl_filter::add_ignore_table");
>>    if (!ignore_table_inited)
>> -    init_table_rule_hash(&ignore_table, &ignore_table_inited);
>> +    init_table_rule_array(&ignore_table, &ignore_table_inited);
>>    table_rules_on= 1;
>>    DBUG_RETURN(add_table_rule(&ignore_table, table_spec));
>>  }
>> @@ -284,7 +299,7 @@ Rpl_filter::add_wild_do_table(const char
>>    if (!wild_do_table_inited)
>>      init_table_rule_array(&wild_do_table, &wild_do_table_inited);
>>    table_rules_on= 1;
>> -  DBUG_RETURN(add_wild_table_rule(&wild_do_table, table_spec));
>> +  DBUG_RETURN(add_table_rule(&wild_do_table, table_spec));
>>  }
>>     
>> @@ -295,7 +310,7 @@ Rpl_filter::add_wild_ignore_table(const    if 
>> (!wild_ignore_table_inited)
>>      init_table_rule_array(&wild_ignore_table, 
>> &wild_ignore_table_inited);
>>    table_rules_on= 1;
>> -  DBUG_RETURN(add_wild_table_rule(&wild_ignore_table, table_spec));
>> +  DBUG_RETURN(add_table_rule(&wild_ignore_table, table_spec));
>>  }
>>  
>>  
>> @@ -307,15 +322,62 @@ Rpl_filter::add_db_rewrite(const char* f
>>  }
>>  
>>  
>> -int -Rpl_filter::add_table_rule(HASH* h, const char* table_spec)
>> +/*
>> +  Build do_table rules to HASH from DYNAMIC_ARRAY for quick search
>> +*/
>> +
>> +void
>> +Rpl_filter::build_do_table_hash()
>> +{
>> +  DBUG_ENTER("Rpl_filter::build_do_table_hash");
>> +  init_table_rule_hash(&do_table_hash, &do_table_hash_inited);
>
> (3) init_table_rule_hash can be called inside the if below. It's an 
> optimization when do_table_inited is false.
>
>> +
>> +  if (do_table_inited)
>> +  {
>> +    for (uint i= 0; i < do_table.elements; i++)
>> +    {
>> +      TABLE_RULE_ENT* e;
>> +      get_dynamic(&do_table, (uchar*)&e, i);
>> +      add_table_rule_to_hash(&do_table_hash, e->db, e->key_len);
>
> (1) add_table_rule_to_hash() can fail, please check the return value 
> and return error from this function if add_table_rule_hash() fails.
>
>> +    }
>> +  }
>> +
>> +  DBUG_VOID_RETURN;
>> +}
>> +
>> +
>> +/*
>> +  Build ignore_table rules to HASH from DYNAMIC_ARRAY for quick search
>> +*/
>> +
>> +void
>> +Rpl_filter::build_ignore_table_hash()
>> +{
>> +  DBUG_ENTER("Rpl_filter::build_ignore_table_hash");
>> +  init_table_rule_hash(&ignore_table_hash, &ignore_table_hash_inited);
>> +
>> +  if (ignore_table_inited)
>> +  {
>> +    for (uint i= 0; i < ignore_table.elements; i++)
>> +    {
>> +      TABLE_RULE_ENT* e;
>> +      get_dynamic(&ignore_table, (uchar*)&e, i);
>> +      add_table_rule_to_hash(&ignore_table_hash, e->db, e->key_len);
>> +    }
>> +  }
>> +
>> +  DBUG_VOID_RETURN;
>> +}
>> +
>> +
>
> (2) Can you add a comment for add_table_rule_to_hash here?
>
>> +int
>> +Rpl_filter::add_table_rule_to_hash(HASH* h, const char* table_spec, 
>> uint len)
>>  {
>>    const char* dot = strchr(table_spec, '.');
>>    if (!dot) return 1;
>>    // len is always > 0 because we know the there exists a '.'
>> -  uint len = (uint)strlen(table_spec);
>>    TABLE_RULE_ENT* e = (TABLE_RULE_ENT*)my_malloc(sizeof(TABLE_RULE_ENT)
>> -                         + len, MYF(MY_WME));
>> +                                                 + len, MYF(MY_WME));
>>    if (!e) return 1;
>>    e->db= (char*)e + sizeof(TABLE_RULE_ENT);
>>    e->tbl_name= e->db + (dot - table_spec) + 1;
>> @@ -327,11 +389,11 @@ Rpl_filter::add_table_rule(HASH* h, cons
>>  
>>  
>>  /*
>> -  Add table expression with wildcards to dynamic array
>> +  Add table expression to dynamic array
>>  */
>>  
>> -int -Rpl_filter::add_wild_table_rule(DYNAMIC_ARRAY* a, const char* 
>> table_spec)
>> +int
>> +Rpl_filter::add_table_rule(DYNAMIC_ARRAY* a, const char* table_spec)
>>  {
>>    const char* dot = strchr(table_spec, '.');
>>    if (!dot) return 1;
>> @@ -387,11 +449,11 @@ void free_table_ent(void* a)
>>  }
>>  
>>  
>> -void +void
>>  Rpl_filter::init_table_rule_hash(HASH* h, bool* h_inited)
>>  {
>> -  my_hash_init(h, system_charset_info,TABLE_RULE_HASH_SIZE,0,0,
>> -        get_table_key, free_table_ent, 0);
>> +  my_hash_init(h, table_alias_charset, TABLE_RULE_HASH_SIZE,0,0,
>> +            get_table_key, free_table_ent, 0);
>>    *h_inited = 1;
>>  }
>>  
>> @@ -410,12 +472,17 @@ Rpl_filter::find_wild(DYNAMIC_ARRAY *a,  {
>>    uint i;
>>    const char* key_end= key + len;
>> -  +
>>    for (i= 0; i < a->elements; i++)
>>    {
>>      TABLE_RULE_ENT* e ;
>>      get_dynamic(a, (uchar*)&e, i);
>> -    if (!my_wildcmp(system_charset_info, key, key_end, +    /*
>> +      Filters will follow the setting of lower_case_table_name
>> +      to be case sensitive when setting lower_case_table_name=0.
>> +      Otherwise they will be case insensitive.
>> +    */
>> +    if (!my_wildcmp(table_alias_charset, key, key_end,
>>              (const char*)e->db,
>>              (const char*)(e->db + e->key_len),
>>              '\\',wild_one,wild_many))
>> @@ -440,36 +507,6 @@ Rpl_filter::free_string_array(DYNAMIC_AR
>>  }
>>  
>>  
>> -/*
>> -  Builds a String from a HASH of TABLE_RULE_ENT. Cannot be used for 
>> any other -  hash, as it assumes that the hash entries are 
>> TABLE_RULE_ENT.
>> -
>> -  SYNOPSIS
>> -    table_rule_ent_hash_to_str()
>> -    s               pointer to the String to fill
>> -    h               pointer to the HASH to read
>> -
>> -  RETURN VALUES
>> -    none
>> -*/
>> -
>> -void -Rpl_filter::table_rule_ent_hash_to_str(String* s, HASH* h, 
>> bool inited)
>> -{
>> -  s->length(0);
>> -  if (inited)
>> -  {
>> -    for (uint i= 0; i < h->records; i++)
>> -    {
>> -      TABLE_RULE_ENT* e= (TABLE_RULE_ENT*) my_hash_element(h, i);
>> -      if (s->length())
>> -        s->append(',');
>> -      s->append(e->db,e->key_len);
>> -    }
>> -  }
>> -}
>> -
>> -
>>  void  Rpl_filter::table_rule_ent_dynamic_array_to_str(String* s, 
>> DYNAMIC_ARRAY* a,
>>                                                  bool inited)
>> @@ -492,14 +529,14 @@ Rpl_filter::table_rule_ent_dynamic_array
>>  void
>>  Rpl_filter::get_do_table(String* str)
>>  {
>> -  table_rule_ent_hash_to_str(str, &do_table, do_table_inited);
>> +  table_rule_ent_dynamic_array_to_str(str, &do_table, do_table_inited);
>>  }
>>  
>>  
>>  void
>>  Rpl_filter::get_ignore_table(String* str)
>>  {
>> -  table_rule_ent_hash_to_str(str, &ignore_table, ignore_table_inited);
>> +  table_rule_ent_dynamic_array_to_str(str, &ignore_table, 
>> ignore_table_inited);
>>  }
>>  
>>  
>> @@ -527,7 +564,12 @@ Rpl_filter::get_rewrite_db(const char* d
>>  
>>    while ((tmp=it++))
>>    {
>> -    if (!strcmp(tmp->key, db))
>> +    /*
>> +      Filters will follow the setting of lower_case_table_name
>> +      to be case sensitive when setting lower_case_table_name=0.
>> +      Otherwise they will be case insensitive.
>> +    */
>> +    if (!my_strcasecmp(table_alias_charset, tmp->key, db))
>>      {
>>        *new_len= strlen(tmp->val);
>>        return tmp->val;
>>
>> === modified file 'sql/rpl_filter.h'
>> --- a/sql/rpl_filter.h    2010-07-02 18:15:21 +0000
>> +++ b/sql/rpl_filter.h    2010-08-13 07:33:56 +0000
>> @@ -55,6 +55,8 @@ public:
>>    bool is_on();
>>  
>>    /* Setters - add filtering rules */
>> +  void build_do_table_hash();
>> +  void build_ignore_table_hash();
>>  
>>    int add_do_table(const char* table_spec);
>>    int add_ignore_table(const char* table_spec);
>> @@ -83,28 +85,33 @@ public:
>>  private:
>>    bool table_rules_on;
>>  
>> -  void init_table_rule_hash(HASH* h, bool* h_inited);
>>    void init_table_rule_array(DYNAMIC_ARRAY* a, bool* a_inited);
>> +  void init_table_rule_hash(HASH* h, bool* h_inited);
>>  
>> -  int add_table_rule(HASH* h, const char* table_spec);
>> -  int add_wild_table_rule(DYNAMIC_ARRAY* a, const char* table_spec);
>> +  int add_table_rule(DYNAMIC_ARRAY* a, const char* table_spec);
>> +  int add_table_rule_to_hash(HASH* h, const char* table_spec, uint 
>> len);
>>  
>>    void free_string_array(DYNAMIC_ARRAY *a);
>>  
>> -  void table_rule_ent_hash_to_str(String* s, HASH* h, bool inited);
>>    void table_rule_ent_dynamic_array_to_str(String* s, DYNAMIC_ARRAY* a,
>>                                             bool inited);
>>    TABLE_RULE_ENT* find_wild(DYNAMIC_ARRAY *a, const char* key, int 
>> len);
>>  
>>    /*
>> -    Those 4 structures below are uninitialized memory unless the
>> +    Those 6 structures below are uninitialized memory unless the
>>      corresponding *_inited variables are "true".
>>    */
>> -  HASH do_table;
>> -  HASH ignore_table;
>> +  /* For quick search */
>> +  HASH do_table_hash;
>> +  HASH ignore_table_hash;
>> +
>> +  DYNAMIC_ARRAY do_table;
>> +  DYNAMIC_ARRAY ignore_table;
>>    DYNAMIC_ARRAY wild_do_table;
>>    DYNAMIC_ARRAY wild_ignore_table;
>>  
>> +  bool do_table_hash_inited;
>> +  bool ignore_table_hash_inited;
>>    bool do_table_inited;
>>    bool ignore_table_inited;
>>    bool wild_do_table_inited;
>>
>>
>>
>> ------------------------------------------------------------------------
>>
>>
>

Thread
bzr commit into mysql-next-mr-bugfixing branch (Dao-Gang.Qu:3310) Bug#51639Dao-Gang.Qu13 Aug
  • Re: bzr commit into mysql-next-mr-bugfixing branch (Dao-Gang.Qu:3310)Bug#51639Sven Sandberg13 Aug
    • Please review Bug#51639Daogang Qu19 Aug