Re: #37656 [Pnd]: lower_case_table_names=1 doesn't convert database names in replicated statements

[Alexander Barkov]

Hi Luis,


Luís Soares wrote:
> Hi Bar,
>   Please find some comments below.
>  
>   Also, I can read you had some concerns about "unexpected consequences"
> on the bug report. Can you recall what were these?

Can you please remind when we discusses unexpected consequences?

> 
> Regards,
> Luís
> 
> On Thu, 2009-07-09 at 10:00 +0500, Alexander Barkov wrote: 
>> Hi Luís,
>>
>> Luís Soares wrote:
>>> Hi Bar,
>>>
>>>
>>> On Wed, 2009-07-08 at 18:52 +0500, Alexander Barkov wrote:
>>>> Hi Luis,
>>>>
>>>>
>>>>> MySQL
>>>>>
>>>>> Logout / Profile
>>>>>
>>>>>     * Developer Zone
>>>>>     * Lists Home
>>>>>     * FAQ
>>>>>
>>>>>
>>>>> List:	Commits	« Previous MessageNext Message »
>>>>> From:	Luis Soares 	Date:	July 3 2009 12:24pm
>>>>> Subject:	bzr commit into mysql-5.4 branch (luis.soares:2804)
> Bug#37656
>>>>> View as plain text  
>>>>>
>>>>> #At
> file:///home/lsoares/Workspace/mysql-server/bugfix/b37656/mysql-azalea-bugfixing/
>>>>> based on revid:alik@stripped
>>>>>
>>>>>  2804 Luis Soares	2009-07-03
>>>>>       BUG#37656: lower_case_table_names=1 doesn't convert database
>>>>>                  names in replicated statements
>>>>>       
>>>>>       This bug revealed itself while using case sensitive
> filesystems
>>>>>       and exhibited two symptoms:
>>>>>       
>>>>>         1. If setting lower_case_table_names=1 on the slave, but not
> on
>>>>>            the master, this setting will not convert database name
> in
>>>>>            replicated statements, ultimately breaking replication;
>>>>>       
>>>>>         2. The same problem for symptom 1. surfaced in RBR, but this
>>>>>            time for table names, as these would not be converted to
>>>>>            lower case for row based replication events.
>>>>>       
>>>>>       Symptom 1. is addressed by conditionally converting to lower
> case,
>>>>>       database name on Query_log_event constructor and
>>>>>       Load_log_event::do_apply_event.
>>>>>       
>>>>>       Symptom 2. is addressed by conditionally converting to lower
> case
>>>>>       database name and table name when processing
> Table_map_log_event.
>>>>>       
>>>>>       On top of these two fixes, this patch also provides
> functionality
>>>>>       to automatically turn into down case user defined replication
>>>>>       filtering rules. For example, if lower_case_table_names=1 is
> used
>>>>>       simultaneously with any replication filtering rule, say
>>>>>       --replicate-do-db=TEST, then all rules are automatically and
>>>>>       implicitly translated to lower case, in this example it would
>>>>>       turn do-db rule into --replicate-do-db=test. This was
>>>>>       accomplished by extending the Rpl_filter class with a
>>>>>       to_lower_case public method that gets called on mysqld startup
> if
>>>>>       lower_case_table_names=1 is set.
>>>>>      @ sql/log_event.cc
>>>>>         Changed Query_log_event constructor,
> Load_log_event::do_apply_event
>>>>>         and Table_map_log_event::do_apply_event to automatically turn
> database
>>>>>         and table names to down case.
>>>>>      @ sql/mysqld.cc
>>>>>         Added check to mysqld option initialization so that whenever
> 
>>>>>         lower_case_table_names==1 filtering rules are turned into
> lower
>>>>>         case.
>>>>>      @ sql/rpl_filter.cc
>>>>>         Extended Rpl_filter class by adding a to_lower_case public
> method
>>>>>         that turns all internal filter rules into lower case. Used
> in
>>>>>         mysqld.cc
>>>>>      @ sql/rpl_filter.h
>>>>>         Added to_lower_case(CHARSET_INFO * cs_info); to Rpl_filter
>>>>>         interface.
>>>>>
>>>>>     added:
>>>>>       mysql-test/extra/rpl_tests/rpl_lower_case_table_names.test
>>>>>       mysql-test/suite/rpl/r/rpl_lower_case_table_names_do_db.result
>>>>>      
> mysql-test/suite/rpl/r/rpl_lower_case_table_names_do_table.result
>>>>>      
> mysql-test/suite/rpl/r/rpl_lower_case_table_names_rewrite_db.result
>>>>>      
> mysql-test/suite/rpl/t/rpl_lower_case_table_names_do_db-slave.opt
>>>>>       mysql-test/suite/rpl/t/rpl_lower_case_table_names_do_db.test
>>>>>      
> mysql-test/suite/rpl/t/rpl_lower_case_table_names_do_table-slave.opt
>>>>>      
> mysql-test/suite/rpl/t/rpl_lower_case_table_names_do_table.test
>>>>>      
> mysql-test/suite/rpl/t/rpl_lower_case_table_names_rewrite_db-slave.opt
>>>>>      
> mysql-test/suite/rpl/t/rpl_lower_case_table_names_rewrite_db.test
>>>>>     modified:
>>>>>       sql/log_event.cc
>>>>>       sql/mysqld.cc
>>>>>       sql/rpl_filter.cc
>>>>>       sql/rpl_filter.h
>>>>> === added file
> 'mysql-test/extra/rpl_tests/rpl_lower_case_table_names.test'
>>>>> ---
> a/mysql-test/extra/rpl_tests/rpl_lower_case_table_names.test	1970-01-01 00:00:00 +0000
>>>>> +++
> b/mysql-test/extra/rpl_tests/rpl_lower_case_table_names.test	2009-07-03 10:23:57 +0000
>>>>> @@ -0,0 +1,79 @@
>>>>> +# BUG#37656
>>>>> +#
>>>>> +#  This test aims at checking whether lower_case_table_names=1
> option works
>>>>> +#  for database names and table names (on row mode logging).
>>>>> +#
>>>>> +#  This test checks the following (when lower_case_table_names=1 is
> set on slave):
>>>>> +#    (i) creating a database on upper case on master results in
> lower case
>>>>> +#        database name on slave
>>>>> +#   (ii) creating tables with upper case names on master results in
> lower case
>>>>> +#        table names on slave
>>>>> +#  (iii) loading data infile into capitalized table name on master
> replicates to
>>>>> +#        lower case table name on slave
>>>>> +#   (iv) Propagating changes from upper case table names on into
> correspondent 
>>>>> +#        lower case table names on slave works.
>>>>> +
>>>>> +
>>>>> +# setup: create database and tables
>>>>> +-- echo ******** [ MASTER ] ********
>>>>> +-- let $dbname_upper= BUG_37656
>>>>> +-- let $dbname_lower= `SELECT LOWER('$dbname_upper')`
>>>>> +-- eval CREATE DATABASE $dbname_upper
>>>>> +-- eval use $dbname_upper
>>>>> +
>>>>> +# assert: database names are in upper case in master and lower
>>>>> +#         case in slave
>>>>> +-- eval show databases like '$dbname_upper'
>>>>> +sync_slave_with_master;
>>>>> +-- echo ******** [ SLAVE ] ********
>>>>> +--eval show databases like '$dbname_lower'
>>>>> +
>>>>> +-- connection master
>>>>> +-- echo ******** [ MASTER ] ********
>>>>> +CREATE TABLE T1 (a int);
>>>>> +-- eval CREATE TABLE T2 (b int) ENGINE=$engine
>>>>> +CREATE TABLE T3 (txt TEXT);
>>>>> +
>>>>> +# assert: that tables exist on master with upper case names
>>>>> +show tables;
>>>>> +
>>>>> +# assert: that tables exist on slave but with lower case names
>>>>> +-- sync_slave_with_master
>>>>> +-- echo ******** [ SLAVE ] ********
>>>>> +-- eval use $dbname_lower
>>>>> +show tables;
>>>>> +
>>>>> +# action: fill data into tables
>>>>> +-- connection master
>>>>> +-- echo ******** [ MASTER ] ********
>>>>> +-- eval use $dbname_upper
>>>>> +INSERT INTO T1 VALUES (1);
>>>>> +INSERT INTO T2 VALUES (1);
>>>>> +use test;
>>>>> +-- eval INSERT INTO $dbname_upper.T1 VALUES (2)
>>>>> +-- eval INSERT INTO $dbname_upper.T2 VALUES (2)
>>>>> +-- eval LOAD DATA INFILE '../../std_data/words.dat' INTO TABLE
> $dbname_upper.T3
>>>>> +
>>>>> +# assert: lower case tables on lower case database on slave
>>>>> +#         get updates from upper case tables on upper case
>>>>> +#         database on master
>>>>> +-- sync_slave_with_master
>>>>> +-- echo ******** [ SLAVE ] ********
>>>>> +
>>>>> +-- let $diff_table_1=master:$dbname_upper.T1
>>>>> +-- let $diff_table_2=slave:$dbname_lower.t1
>>>>> +-- source include/diff_tables.inc
>>>>> +
>>>>> +-- let $diff_table_1=master:$dbname_upper.T2
>>>>> +-- let $diff_table_2=slave:$dbname_lower.t2
>>>>> +-- source include/diff_tables.inc
>>>>> +
>>>>> +-- let $diff_table_1=master:$dbname_upper.T3
>>>>> +-- let $diff_table_2=slave:$dbname_lower.t3
>>>>> +-- source include/diff_tables.inc
>>>>> +
>>>>> +# clean up
>>>>> +-- connection master
>>>>> +-- echo ******** [ MASTER ] ********
>>>>> +-- eval DROP DATABASE $dbname_upper
>>>>> +-- sync_slave_with_master
>>>>>
>>>>> === added file
> 'mysql-test/suite/rpl/r/rpl_lower_case_table_names_do_db.result'
>>>>> ---
> a/mysql-test/suite/rpl/r/rpl_lower_case_table_names_do_db.result	1970-01-01 00:00:00
>>>>> +0000
>>>>> +++
> b/mysql-test/suite/rpl/r/rpl_lower_case_table_names_do_db.result	2009-07-03 10:23:57
>>>>> +0000
>>>>> @@ -0,0 +1,56 @@
>>>>> +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;
>>>>> +SHOW DATABASES;
>>>>> +Database
>>>>> +information_schema
>>>>> +mtr
>>>>> +mysql
>>>>> +test
>>>>> +CREATE TABLE T1 (a int);
>>>>> +INSERT INTO T1 VALUES (1);
>>>>> +Comparing tables master:test.T1 and slave:test.t1
>>>>> +DROP TABLE T1;
>>>>> +******** [ MASTER ] ********
>>>>> +CREATE DATABASE BUG_37656;
>>>>> +use BUG_37656;
>>>>> +show databases like 'BUG_37656';
>>>>> +Database (BUG_37656)
>>>>> +BUG_37656
>>>>> +******** [ SLAVE ] ********
>>>>> +show databases like 'bug_37656';
>>>>> +Database (bug_37656)
>>>>> +bug_37656
>>>>> +******** [ MASTER ] ********
>>>>> +CREATE TABLE T1 (a int);
>>>>> +CREATE TABLE T2 (b int) ENGINE=InnoDB;
>>>>> +CREATE TABLE T3 (txt TEXT);
>>>>> +show tables;
>>>>> +Tables_in_BUG_37656
>>>>> +T1
>>>>> +T2
>>>>> +T3
>>>>> +******** [ SLAVE ] ********
>>>>> +use bug_37656;
>>>>> +show tables;
>>>>> +Tables_in_bug_37656
>>>>> +t1
>>>>> +t2
>>>>> +t3
>>>>> +******** [ MASTER ] ********
>>>>> +use BUG_37656;
>>>>> +INSERT INTO T1 VALUES (1);
>>>>> +INSERT INTO T2 VALUES (1);
>>>>> +use test;
>>>>> +INSERT INTO BUG_37656.T1 VALUES (2);
>>>>> +INSERT INTO BUG_37656.T2 VALUES (2);
>>>>> +LOAD DATA INFILE '../../std_data/words.dat' INTO TABLE
> BUG_37656.T3;
>>>>> +******** [ SLAVE ] ********
>>>>> +Comparing tables master:BUG_37656.T1 and slave:bug_37656.t1
>>>>> +Comparing tables master:BUG_37656.T2 and slave:bug_37656.t2
>>>>> +Comparing tables master:BUG_37656.T3 and slave:bug_37656.t3
>>>>> +******** [ MASTER ] ********
>>>>> +DROP DATABASE BUG_37656;
>>>>>
>>>>> === added file
> 'mysql-test/suite/rpl/r/rpl_lower_case_table_names_do_table.result'
>>>>> ---
> a/mysql-test/suite/rpl/r/rpl_lower_case_table_names_do_table.result	1970-01-01
>>>>> 00:00:00 +0000
>>>>> +++
> b/mysql-test/suite/rpl/r/rpl_lower_case_table_names_do_table.result	2009-07-03
>>>>> 10:23:57 +0000
>>>>> @@ -0,0 +1,46 @@
>>>>> +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;
>>>>> +******** [ MASTER ] ********
>>>>> +CREATE DATABASE BUG_37656;
>>>>> +use BUG_37656;
>>>>> +show databases like 'BUG_37656';
>>>>> +Database (BUG_37656)
>>>>> +BUG_37656
>>>>> +******** [ SLAVE ] ********
>>>>> +show databases like 'bug_37656';
>>>>> +Database (bug_37656)
>>>>> +bug_37656
>>>>> +******** [ MASTER ] ********
>>>>> +CREATE TABLE T1 (a int);
>>>>> +CREATE TABLE T2 (b int) ENGINE=InnoDB;
>>>>> +CREATE TABLE T3 (txt TEXT);
>>>>> +show tables;
>>>>> +Tables_in_BUG_37656
>>>>> +T1
>>>>> +T2
>>>>> +T3
>>>>> +******** [ SLAVE ] ********
>>>>> +use bug_37656;
>>>>> +show tables;
>>>>> +Tables_in_bug_37656
>>>>> +t1
>>>>> +t2
>>>>> +t3
>>>>> +******** [ MASTER ] ********
>>>>> +use BUG_37656;
>>>>> +INSERT INTO T1 VALUES (1);
>>>>> +INSERT INTO T2 VALUES (1);
>>>>> +use test;
>>>>> +INSERT INTO BUG_37656.T1 VALUES (2);
>>>>> +INSERT INTO BUG_37656.T2 VALUES (2);
>>>>> +LOAD DATA INFILE '../../std_data/words.dat' INTO TABLE
> BUG_37656.T3;
>>>>> +******** [ SLAVE ] ********
>>>>> +Comparing tables master:BUG_37656.T1 and slave:bug_37656.t1
>>>>> +Comparing tables master:BUG_37656.T2 and slave:bug_37656.t2
>>>>> +Comparing tables master:BUG_37656.T3 and slave:bug_37656.t3
>>>>> +******** [ MASTER ] ********
>>>>> +DROP DATABASE BUG_37656;
>>>>>
>>>>> === added file
> 'mysql-test/suite/rpl/r/rpl_lower_case_table_names_rewrite_db.result'
>>>>> ---
> a/mysql-test/suite/rpl/r/rpl_lower_case_table_names_rewrite_db.result	1970-01-01
>>>>> 00:00:00 +0000
>>>>> +++
> b/mysql-test/suite/rpl/r/rpl_lower_case_table_names_rewrite_db.result	2009-07-03
>>>>> 10:23:57 +0000
>>>>> @@ -0,0 +1,26 @@
>>>>> +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;
>>>>> +SET SQL_LOG_BIN=0;
>>>>> +CREATE DATABASE B37656;
>>>>> +SET SQL_LOG_BIN=1;
>>>>> +CREATE DATABASE bug37656;
>>>>> +USE B37656;
>>>>> +CREATE TABLE T1 (a int);
>>>>> +INSERT INTO T1 VALUES (1);
>>>>> +### assertion: master contains capitalized case table
>>>>> +SHOW TABLES;
>>>>> +Tables_in_B37656
>>>>> +T1
>>>>> +use bug37656;
>>>>> +### assertion: slave contains lowered case table
>>>>> +SHOW TABLES;
>>>>> +Tables_in_bug37656
>>>>> +t1
>>>>> +### assertion: master and slave tables do not differ
>>>>> +Comparing tables master:B37656.T1 and slave:bug37656.t1
>>>>> +DROP DATABASE B37656;
>>>>> +DROP DATABASE bug37656;
>>>>>
>>>>> === added file
> 'mysql-test/suite/rpl/t/rpl_lower_case_table_names_do_db-slave.opt'
>>>>> ---
> a/mysql-test/suite/rpl/t/rpl_lower_case_table_names_do_db-slave.opt	1970-01-01
>>>>> 00:00:00 +0000
>>>>> +++
> b/mysql-test/suite/rpl/t/rpl_lower_case_table_names_do_db-slave.opt	2009-07-03
>>>>> 10:23:57 +0000
>>>>> @@ -0,0 +1 @@
>>>>> +--replicate-do-db=BuG_37656 --replicate-do-db=TESt
> --lower-case-table-names=1
>>>>>
>>>>> === added file
> 'mysql-test/suite/rpl/t/rpl_lower_case_table_names_do_db.test'
>>>>> ---
> a/mysql-test/suite/rpl/t/rpl_lower_case_table_names_do_db.test	1970-01-01 00:00:00
>>>>> +0000
>>>>> +++
> b/mysql-test/suite/rpl/t/rpl_lower_case_table_names_do_db.test	2009-07-03 10:23:57
>>>>> +0000
>>>>> @@ -0,0 +1,41 @@
>>>>> +# BUG#37656
>>>>> +#  
>>>>> +#  For details look into
> extra/rpl_tests/rpl_lower_case_table_names.test
>>>>> +# 
>>>>> +#  Apart from the assertions present in the included file, this test
> also does
>>>>> +#  an extra assertion. Check below.
>>>>> +
>>>>> +-- source include/master-slave.inc
>>>>> +-- source include/have_innodb.inc
>>>>> +-- source include/not_windows.inc
>>>>> +
>>>>> +# assertion: assert that test.T1 replicates to test.t1 despite
>>>>> +#            --replicate-do-db=TESt is set (meaning that names of
> databases
>>>>> +#            are lowered case when using lower_case_table_names=1)
>>>>> +#
>>>>> +#            The same check is done for the included file, because
>>>>> +#            --replicate-db-db=BuG_37656 is set
>>>>> +
>>>>> +SHOW DATABASES;
>>>>> +CREATE TABLE T1 (a int);
>>>>> +INSERT INTO T1 VALUES (1);
>>>>> +
>>>>> +-- sync_slave_with_master
>>>>> +
>>>>> +-- let $diff_table_1=master:test.T1
>>>>> +-- let $diff_table_2=slave:test.t1
>>>>> +-- source include/diff_tables.inc
>>>>> +
>>>>> +-- connection master
>>>>> +
>>>>> +DROP TABLE T1;
>>>>> +
>>>>> +-- sync_slave_with_master
>>>>> +
>>>>> +-- connection master
>>>>> +
>>>>> +# check test details inside
> extra/rpl_tests/rpl_lower_case_table_names.test
>>>>> +#
>>>>> +
>>>>> +-- let $engine=InnoDB
>>>>> +-- source extra/rpl_tests/rpl_lower_case_table_names.test
>>>>>
>>>>> === added file
> 'mysql-test/suite/rpl/t/rpl_lower_case_table_names_do_table-slave.opt'
>>>>> ---
> a/mysql-test/suite/rpl/t/rpl_lower_case_table_names_do_table-slave.opt	1970-01-01
>>>>> 00:00:00 +0000
>>>>> +++
> b/mysql-test/suite/rpl/t/rpl_lower_case_table_names_do_table-slave.opt	2009-07-03
>>>>> 10:23:57 +0000
>>>>> @@ -0,0 +1 @@
>>>>> +--replicate-do-table=bug_37656.t1 --replicate-do-table=bug_37656.T2
>>>>> --replicate-do-table=Bug_37656.t3 --lower-case-table-names=1
>>>>>
>>>>> === added file
> 'mysql-test/suite/rpl/t/rpl_lower_case_table_names_do_table.test'
>>>>> ---
> a/mysql-test/suite/rpl/t/rpl_lower_case_table_names_do_table.test	1970-01-01 00:00:00
>>>>> +0000
>>>>> +++
> b/mysql-test/suite/rpl/t/rpl_lower_case_table_names_do_table.test	2009-07-03 10:23:57
>>>>> +0000
>>>>> @@ -0,0 +1,17 @@
>>>>> +# BUG#37656
>>>>> +#  
>>>>> +#  For details look into
> extra/rpl_tests/rpl_lower_case_table_names.test
>>>>> +# 
>>>>> +#  Apart from the assertions present in the included file, this test
> also:
>>>>> +#
>>>>> +#   (i) implicitly, checks that filter rules are turned to lower
> case
>>>>> +#       when lower_case_table_names=1 is used. Check -slave.opt file
> for 
>>>>> +#       rule definition.
>>>>> +#  
>>>>> +
>>>>> +-- source include/master-slave.inc
>>>>> +-- source include/have_innodb.inc
>>>>> +-- source include/not_windows.inc
>>>>> +
>>>>> +-- let $engine=InnoDB
>>>>> +-- source extra/rpl_tests/rpl_lower_case_table_names.test
>>>>>
>>>>> === added file
> 'mysql-test/suite/rpl/t/rpl_lower_case_table_names_rewrite_db-slave.opt'
>>>>> ---
> a/mysql-test/suite/rpl/t/rpl_lower_case_table_names_rewrite_db-slave.opt	1970-01-01
>>>>> 00:00:00 +0000
>>>>> +++
> b/mysql-test/suite/rpl/t/rpl_lower_case_table_names_rewrite_db-slave.opt	2009-07-03
>>>>> 10:23:57 +0000
>>>>> @@ -0,0 +1 @@
>>>>> +--lower-case-table-names=1
> "--replicate-rewrite-db=b37656->BuG37656"
>>>>>
>>>>> === added file
> 'mysql-test/suite/rpl/t/rpl_lower_case_table_names_rewrite_db.test'
>>>>> ---
> a/mysql-test/suite/rpl/t/rpl_lower_case_table_names_rewrite_db.test	1970-01-01
>>>>> 00:00:00 +0000
>>>>> +++
> b/mysql-test/suite/rpl/t/rpl_lower_case_table_names_rewrite_db.test	2009-07-03
>>>>> 10:23:57 +0000
>>>>> @@ -0,0 +1,58 @@
>>>>> +# BUG#37656
>>>>> +#
>>>>> +# DESCRIPTION
>>>>> +#
>>>>> +#
>>>>> +#  This test case is tests whether replication works properly when
>>>>> +#  slave is configured with --lower-case-table-names=1 and
> replication
>>>>> +#  rewrite rules are in effect.
>>>>> +#
>>>>> +#  It checks four issues:
>>>>> +# 
>>>>> +#   (i) master contains capitalized table name
>>>>> +#
>>>>> +#  (ii) slave contains lowered case table name
>>>>> +#
>>>>> +# (iii) master and slave tables do not differ
>>>>> +#
>>>>> +#  (iv) implicitly, checks that rewrite rules are turned to
> lowercase
>>>>> +#       when lower_case_table_names=1 on slave. Check -slave.opt
> file
>>>>> +#       for rewrite rule: b37656->BuG37656
>>>>> +
>>>>> +-- source include/master-slave.inc
>>>>> +-- source include/not_windows.inc
>>>>> +
>>>>> +SET SQL_LOG_BIN=0;
>>>>> +CREATE DATABASE B37656;
>>>>> +SET SQL_LOG_BIN=1;
>>>>> +
>>>>> +-- connection slave
>>>>> +CREATE DATABASE bug37656;
>>>>> +
>>>>> +-- connection master
>>>>> +USE B37656;
>>>>> +CREATE TABLE T1 (a int);
>>>>> +INSERT INTO T1 VALUES (1);
>>>>> +
>>>>> +-- echo ### assertion: master contains capitalized case table
>>>>> +SHOW TABLES;
>>>>> +
>>>>> +-- sync_slave_with_master
>>>>> +
>>>>> +use bug37656;
>>>>> +
>>>>> +-- echo ### assertion: slave contains lowered case table
>>>>> +SHOW TABLES;
>>>>> +
>>>>> +
>>>>> +-- echo ### assertion: master and slave tables do not differ
>>>>> +let $diff_table_1=master:B37656.T1;
>>>>> +let $diff_table_2=slave:bug37656.t1;
>>>>> +
>>>>> +-- source include/diff_tables.inc
>>>>> +
>>>>> +-- connection master
>>>>> +DROP DATABASE B37656;
>>>>> +
>>>>> +-- sync_slave_with_master
>>>>> +DROP DATABASE bug37656;
>>>>>
>>>>> === modified file 'sql/log_event.cc'
>>>>> --- a/sql/log_event.cc	2009-06-09 16:53:34 +0000
>>>>> +++ b/sql/log_event.cc	2009-07-03 10:23:57 +0000
>>>>> @@ -2939,6 +2939,8 @@ int Query_log_event::do_apply_event(Rela
>>>>>  int Query_log_event::do_apply_event(Relay_log_info const *rli,
>>>>>                                        const char *query_arg, uint32
> q_len_arg)
>>>>>  {
>>>>> +  char db_buf[NAME_LEN + 1];
>>>>> +  strmov(db_buf, db);
>>>>>    LEX_STRING new_db;
>>>>>    int expected_error,actual_error= 0;
>>>>>    /*
>>>>> @@ -2950,7 +2952,9 @@ int Query_log_event::do_apply_event(Rela
>>>>>    */
>>>>>    thd->catalog= catalog_len ? (char *) catalog : (char *)"";
>>>>>    new_db.length= db_len;
>>>>> -  new_db.str= (char *) rpl_filter->get_rewrite_db(db,
> &new_db.length);
>>>>> +  if (lower_case_table_names == 1)
>>>>> +    my_casedn_str(system_charset_info, db_buf);
>>>>> +  new_db.str= (char *) rpl_filter->get_rewrite_db(db_buf,
> &new_db.length);
>>>> when lower_case_table_names is not 1, then you do unnecessary
>>>> copying to db_buf.
>>>>
>>>>
>>>> I suggest to do something like this:
>>>>
>>>> char db_buf[NAME_LEN +1];
>>>> ...
>>>> if (lower_case_table_names == 1)
>>>> {
>>>>    strmov(db_buf, db);
>>>>    new_db.str= (char *) rpl_filter->get_rewrite_db(db_buf,
> &new_db.length);
>>>> }
>>>> else
>>>>    new_db.str= (char *) rpl_filter->get_rewrite_db(db,
> &new_db.length);
>>>>
>>>>
>> I made a closer look into the patch
>> and found some duplicate code.
>>
>> Perhaps a better idea is even to add a new method:
>>
>>
>> Log_event::set_rewrite_db()
>> {
>>    char lcase_db_buf[NAME_LEN +1];
>>    LEX_STRING new_db;
>>    new_db.length= db_len;
>>    if (lower_case_table_names == 1)
>>    {
>>      strmov(lcase_db_buf, db);
>>      my_casedn_str(system_charset_info, lcase_db_buf);
>>      newdb.str= lcase_db_buf;
>>    }
>>    else
>>    {
>>      newdb.str= db;
>>    }
>>    new_db.str= (char*) rpl_filter->get_rewrite_db(newdb.str,
>>                                                   &new_db.length);
>>    thd->set_db(new_db.str, new_db.length);
>> }
> 
> Both Load_log_event and Query_log_event derive from Log_event. 
> However, Log_event has no reference to db_len and db, these are
> properties defined in Load_log_event and Query_log_event (separately).
> So one cannot just define a top level method to aggregate
> functionality. 
> 
> OTOH, we could, declare a static function in log_event.cc that would
> remove the duplicate portion of code and would pretty much serve the
> same purpose of the Log_event::set_rewrite_db. Although I am not very
> keen on this approach, it would look something like (I renamed
> set_rewrite_db to set_thd_db - I think this name is more appropriate):
> 
> static void set_thd_db(THD *thd, const char *db, uint32 db_len)
> {
>   char lcase_db_buf[NAME_LEN +1]; 
>   LEX_STRING new_db;
>   new_db.length= db_len;
>   if (lower_case_table_names == 1)
>   {
>     strmov(lcase_db_buf, db); 
>     my_casedn_str(system_charset_info, lcase_db_buf);
>     new_db.str= lcase_db_buf;
>   }
>   else 
>     new_db.str= (char*) db;
>   new_db.str= (char*) rpl_filter->get_rewrite_db(new_db.str,
> &new_db.length);
>   thd->set_db(new_db.str, new_db.length);
> }


I agree that using a static function is not a good approach.

It seems that  Load_log_event and Query_log_event are missing
an intermediary class on top of them, which can have at least
the following common properties and methods:


class Log_event_with_db: public Log_event
{
public:
   const char *db;
   uint32 db_len;
   const char *get_db() { return db; };
   ulong thread_id;
   ulong slave_proxy_id;
   virtual bool is_valid();
   void set_thd_db(THD *thd, const char *db, uint32 db_len);
   /*
      Perhaps we even don't need the last two parameters
      in the above method.
   */
}


Then both Load_log_event and Query_log_event can derive from
Log_event_with_db.


> 
> Then in the {Query,Load}_log_event we would have the following:

Agree, using Log_evet_with_db::set_thd_db() instead of
a static function set_thd_db().

> 
> @@ -2939,9 +2958,6 @@
> int Query_log_event::do_apply_event(Relay_log_info const *rli,
>                                        const char *query_arg, uint32
> q_len_arg)
> {
> -  char db_buf[NAME_LEN + 1];
> -  strmov(db_buf, db);
> -  LEX_STRING new_db;
>    int expected_error,actual_error= 0;
>    /*
>      Colleagues: please never free(thd->catalog) in MySQL. This would
> @@ -2951,11 +2967,7 @@
>      you.
>    */
>    thd->catalog= catalog_len ? (char *) catalog : (char *)"";
> -  new_db.length= db_len;
> -  if (lower_case_table_names == 1)
> -    my_casedn_str(system_charset_info, db_buf);
> -  new_db.str= (char *) rpl_filter->get_rewrite_db(db_buf,
> &new_db.length);
> -  thd->set_db(new_db.str, new_db.length);       /* allocates a copy of
> 'db' */
> +  set_thd_db(thd, db, db_len);
>    thd->variables.auto_increment_increment= auto_increment_increment;
>    thd->variables.auto_increment_offset=    auto_increment_offset;
> 
> @@ -4449,14 +4461,7 @@
> int Load_log_event::do_apply_event(NET* net, Relay_log_info const *rli,
>                                     bool use_rli_only_for_errors)
> {
> -  char db_buf[NAME_LEN + 1];
> -  strmov(db_buf, db);
> -  LEX_STRING new_db;
> -  new_db.length= db_len;
> -  if (lower_case_table_names == 1)
> -    my_casedn_str(system_charset_info, db_buf);
> -  new_db.str= (char *) rpl_filter->get_rewrite_db(db_buf,
> &new_db.length);
> -  thd->set_db(new_db.str, new_db.length);
> +  set_thd_db(thd, db, db_len);
> 
>> This will help to avoid unnecessary strmov() when
>> lower_case_table_names is 0, and also help to get
>> rid of duplicate code in
>> Query_log_event::do_apply_event() and Load_log_event::do_apply_event().
>>
>>
>> So instead of
>>
>>    char db_buf[NAME_LEN + 1];
>>    strmov(db_buf, db);
>>    LEX_STRING new_db;
>>    int expected_error,actual_error= 0;
>>    thd->catalog= catalog_len ? (char *) catalog : (char *)"";
>>    new_db.length= db_len;
>>    if (lower_case_table_names == 1)
>>      my_casedn_str(system_charset_info, db_buf);
>>    new_db.str= (char *) rpl_filter->get_rewrite_db(db_buf,
>>                                                    &new_db.length);
>>    thd->set_db(new_db.str, new_db.length); /* allocates a copy of 'db' */
>>
>>
>> you could just do:
>>
>>    thd->catalog= catalog_len ? (char *) catalog : (char *)"";
>>    set_rewrite_db();
>>
>>
>> in both Query_log_event::do_apply_event() and 
>> Load_log_event::do_apply_event().
>>
>>
>> Please also find one comment below:
>>
>>
>>>> And in many other places below.
>>>>
>>>>
>>>> Otherwise the patch looks fine for me.
>>>>
>>>> I have ticked the checkbox.
>>> Thanks for the input I will check this. Also, I was mostly concerned
>>> about the charset issue which you provided very good feedback, again
>>> thanks.
>>>
>>>>>    thd->set_db(new_db.str, new_db.length);       /* allocates a
> copy of 'db' */
>>>>>    thd->variables.auto_increment_increment=
> auto_increment_increment;
>>>>>    thd->variables.auto_increment_offset=   
> auto_increment_offset;
>>>>> @@ -4445,9 +4449,13 @@ void Load_log_event::set_fields(const ch
>>>>>  int Load_log_event::do_apply_event(NET* net, Relay_log_info const
> *rli,
>>>>>                                     bool use_rli_only_for_errors)
>>>>>  {
>>>>> +  char db_buf[NAME_LEN + 1];
>>>>> +  strmov(db_buf, db);
>>>>>    LEX_STRING new_db;
>>>>>    new_db.length= db_len;
>>>>> -  new_db.str= (char *) rpl_filter->get_rewrite_db(db,
> &new_db.length);
>>>>> +  if (lower_case_table_names == 1)
>>>>> +    my_casedn_str(system_charset_info, db_buf);
>>>>> +  new_db.str= (char *) rpl_filter->get_rewrite_db(db_buf,
> &new_db.length);
>>>>>    thd->set_db(new_db.str, new_db.length);
>>>>>    DBUG_ASSERT(thd->query == 0);
>>>>>    thd->query_length= 0;                         // Should not be
> needed
>>>>> @@ -4505,9 +4513,13 @@ int Load_log_event::do_apply_event(NET* 
>>>>>     
> thd->warning_info->opt_clear_warning_info(thd->query_id);
>>>>>  
>>>>>      TABLE_LIST tables;
>>>>> +    char table_buf[NAME_LEN + 1];
>>>>> +    strmov(table_buf, table_name);
>>>>> +    if (lower_case_table_names == 1)
>>>>> +      my_casedn_str(system_charset_info, table_buf);
>>>>>      bzero((char*) &tables,sizeof(tables));
>>>>>      tables.db= thd->strmake(thd->db, thd->db_length);
>>>>> -    tables.alias = tables.table_name = (char*) table_name;
>>>>> +    tables.alias = tables.table_name = table_buf;
>>>>>      tables.lock_type = TL_WRITE;
>>>>>      tables.updating= 1;
>>>>>  
>>>>> @@ -8051,7 +8063,7 @@ Table_map_log_event::~Table_map_log_even
>>>>>  int Table_map_log_event::do_apply_event(Relay_log_info const *rli)
>>>>>  {
>>>>>    RPL_TABLE_LIST *table_list;
>>>>> -  char *db_mem, *tname_mem;
>>>>> +  char *db_mem, *tname_mem, *ptr;
>>>>>    MDL_request *mdl_request;
>>>>>    size_t dummy_len;
>>>>>    void *memory;
>>>>> @@ -8078,8 +8090,18 @@ int Table_map_log_event::do_apply_event(
>>>>>    table_list->next_global= table_list->next_local= 0;
>>>>>    table_list->table_id= m_table_id;
>>>>>    table_list->updating= 1;
>>>>> -  strmov(table_list->db, rpl_filter->get_rewrite_db(m_dbnam,
> &dummy_len));
>>>>> +  strmov(table_list->db, m_dbnam);
>>>>>    strmov(table_list->table_name, m_tblnam);
>>>>> +  if (lower_case_table_names == 1)
>>>>> +  {
>>>>> +    my_casedn_str(system_charset_info, table_list->db);
>>>>> +    my_casedn_str(system_charset_info, table_list->table_name);
>>>>> +  }
>>>>> +
>>>>> +  /* rewrite rules changed the database. */
>>>>> +  if ((ptr= (char*) rpl_filter->get_rewrite_db(table_list->db,
> &dummy_len)) != 
>>>>> +      table_list->db)
>>>>> +    strmov(table_list->db, ptr);
>>>>>    mdl_request->init(0, table_list->db,
> table_list->table_name);
>>>>>    table_list->mdl_request= mdl_request;
>>>>>  
>>>>>
>>>>> === modified file 'sql/mysqld.cc'
>>>>> --- a/sql/mysqld.cc	2009-06-17 07:30:19 +0000
>>>>> +++ b/sql/mysqld.cc	2009-07-03 10:23:57 +0000
>>>>> @@ -3696,6 +3696,12 @@ You should consider changing lower_case_
>>>>>        (test_if_case_insensitive(mysql_real_data_home) == 1);
>>>>>    }
>>>>>  
>>>>> +  if (lower_case_table_names == 1)
>>>>> +  {
>>>>> +    rpl_filter->to_lower_case(system_charset_info);
>>>>> +    binlog_filter->to_lower_case(system_charset_info);
>>>>> +  }
>>>>> +
>>>>>    /* Reset table_alias_charset, now that lower_case_table_names is
> set. */
>>>>>    table_alias_charset= (lower_case_table_names ?
>>>>>  			files_charset_info :
>>>>>
>>>>> === modified file 'sql/rpl_filter.cc'
>>>>> --- a/sql/rpl_filter.cc	2009-04-29 03:50:14 +0000
>>>>> +++ b/sql/rpl_filter.cc	2009-07-03 10:23:57 +0000
>>>>> @@ -299,7 +299,9 @@ Rpl_filter::add_wild_ignore_table(const 
>>>>>  void
>>>>>  Rpl_filter::add_db_rewrite(const char* from_db, const char* to_db)
>>>>>  {
>>>>> -  i_string_pair *db_pair = new i_string_pair(from_db, to_db);
>>>>> +  char *from_db_cp= my_strdup(from_db, MYF(MY_WME |
> ME_FATALERROR));
>>>>> +  char *to_db_cp= my_strdup(to_db, MYF(MY_WME | ME_FATALERROR));
>>>>> +  i_string_pair *db_pair = new i_string_pair(from_db_cp, to_db_cp);
>> I can see new my_strdups, but didn't find new my_frees.
>> Please make sure that the space allocated in my_strdups is
>> freed.
> 
> AFAIK, there are no methods for removing filters. Also, filters are set
> on server startup and valid throughout the entire server lifetime. Just
> to be sure, I have made a valgrind run on the proposed tests (which
> exercise this part of the code) and no leak was found. 

Sorry, I'm not strong with replication filters.
Just new allocs without frees looked suspicios.
Good that there are no leaks found.


> 
> However, I did bump into an unexpected "invalid read" in ctype-utf8.c.
> This was caused while making replicate-do-table rules downcase (which
> hold not null terminated string), thence my_casedn_str_utf8mb3 loop
> would read beyond size of string.
> 
> 16 ==7492== Invalid read of size 1
> 17 ==7492==    at 0xB2F396: my_casedn_str_utf8mb3 (ctype-utf8.c:2436)
> 18 ==7492==    by 0x8C8FD6: Rpl_filter::to_lower_case(charset_info_st*,
> st_hash*) (rpl_filter.cc:573)
> (...)
> 
> And that seems fixable with something like the following (I am testing
> atm):
> 
> === modified file 'sql/rpl_filter.cc'
> --- sql/rpl_filter.cc 2009-07-03 10:23:57 +0000
> +++ sql/rpl_filter.cc 2009-07-09 15:27:21 +0000
> @@ -314,12 +314,12 @@
>    // 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 + 1, MYF(MY_WME));
>    if (!e) return 1;
>    e->db= (char*)e + sizeof(TABLE_RULE_ENT);
>    e->tbl_name= e->db + (dot - table_spec) + 1;
>    e->key_len= len;
> -  memcpy(e->db, table_spec, len);
> +  memcpy(e->db, table_spec, len + 1 /* '\0' */); 
> 
>    return my_hash_insert(h, (uchar*)e);

This change looks correct,
we need an extra byte for the terminating '\0'.


> }
> 
> 
>>>>>    rewrite_db.push_back(db_pair);
>>>>>  }
>>>>>  
>>>>> @@ -348,7 +350,8 @@ void
>>>>>  Rpl_filter::add_do_db(const char* table_spec)
>>>>>  {
>>>>>    DBUG_ENTER("Rpl_filter::add_do_db");
>>>>> -  i_string *db = new i_string(table_spec);
>>>>> +  char *tbl_spec_cp= my_strdup(table_spec, MYF(MY_WME |
> ME_FATALERROR));
>>>>> +  i_string *db = new i_string(tbl_spec_cp);
>>>>>    do_db.push_back(db);
>>>>>    DBUG_VOID_RETURN;
>>>>>  }
>>>>> @@ -358,7 +361,8 @@ void
>>>>>  Rpl_filter::add_ignore_db(const char* table_spec)
>>>>>  {
>>>>>    DBUG_ENTER("Rpl_filter::add_ignore_db");
>>>>> -  i_string *db = new i_string(table_spec);
>>>>> +  char *tbl_spec_cp= my_strdup(table_spec, MYF(MY_WME |
> ME_FATALERROR));
>>>>> +  i_string *db = new i_string(tbl_spec_cp);
>>>>>    ignore_db.push_back(db);
>>>>>    DBUG_VOID_RETURN;
>>>>>  }
>>>>> @@ -546,3 +550,72 @@ Rpl_filter::get_ignore_db()
>>>>>  {
>>>>>    return &ignore_db;
>>>>>  }
>>>>> +
>>>>> +void 
>>>>> +Rpl_filter::to_lower_case(CHARSET_INFO *cs_info, DYNAMIC_ARRAY* a)
>>>>> +{
>>>>> +  for (uint i= 0; i < a->elements; i++)
>>>>> +  {
>>>>> +    TABLE_RULE_ENT* e;
>>>>> +    get_dynamic(a, (uchar*)&e, i);
>>>>> +    my_casedn_str(cs_info, e->db);
>>>>> +    my_casedn_str(cs_info, e->tbl_name);
>>>>> +  }
>>>>> +}
>>>>> +
>>>>> +void 
>>>>> +Rpl_filter::to_lower_case(CHARSET_INFO *cs_info, HASH* h)
>>>>> +{
>>>>> +  for (uint i= 0; i < h->records; i++)
>>>>> +  {
>>>>> +    TABLE_RULE_ENT* e= (TABLE_RULE_ENT*) my_hash_element(h, i);
>>>>> +    my_casedn_str(cs_info, e->db);
>>>>> +    my_casedn_str(cs_info, e->tbl_name);
>>>>> +  }
>>>>> +}
>>>>> +
>>>>> +void 
>>>>> +Rpl_filter::to_lower_case(CHARSET_INFO *cs_info,
> I_List<i_string>* l)
>>>>> +{
>>>>> +  I_List_iterator<i_string> it(*l);
>>>>> +  i_string *tmp= NULL;
>>>>> +
>>>>> +  while ((tmp=it++))
>>>>> +    my_casedn_str(cs_info, const_cast<char *>(tmp->ptr));
>>>>> +}
>>>>> +
>>>>> +void
>>>>> +Rpl_filter::to_lower_case(CHARSET_INFO * cs_info)
>>>>> +{
>>>>> +  // rewrite_db
>>>>> +  if (!rewrite_db.is_empty())
>>>>> +  {
>>>>> +    I_List_iterator<i_string_pair> it(rewrite_db);
>>>>> +    i_string_pair* tmp= NULL;
>>>>> +
>>>>> +    while ((tmp=it++))
>>>>> +    {
>>>>> +      my_casedn_str(cs_info, const_cast<char
> *>(tmp->key));
>>>>> +      my_casedn_str(cs_info, const_cast<char
> *>(tmp->val));
>>>>> +    }
>>>>> +  }
>>>>> +
>>>>> +  if (!do_db.is_empty())
>>>>> +    to_lower_case(cs_info, &do_db);
>>>>> +
>>>>> +  if (!ignore_db.is_empty())
>>>>> +    to_lower_case(cs_info, &ignore_db);
>>>>> +
>>>>> +  if (do_table_inited)
>>>>> +    to_lower_case(cs_info, &do_table);
>>>>> +
>>>>> +  if (ignore_table_inited)
>>>>> +    to_lower_case(cs_info, &ignore_table);
>>>>> +
>>>>> +  if (wild_do_table_inited)
>>>>> +    to_lower_case(cs_info, &wild_do_table);
>>>>> +
>>>>> +  if (wild_ignore_table_inited)
>>>>> +    to_lower_case(cs_info, &wild_ignore_table);
>>>>> +
>>>>> +}
>>>>>
>>>>> === modified file 'sql/rpl_filter.h'
>>>>> --- a/sql/rpl_filter.h	2007-05-10 09:59:39 +0000
>>>>> +++ b/sql/rpl_filter.h	2009-07-03 10:23:57 +0000
>>>>> @@ -74,6 +74,13 @@ public:
>>>>>    I_List<i_string>* get_do_db();
>>>>>    I_List<i_string>* get_ignore_db();
>>>>>  
>>>>> +  /* 
>>>>> +    To lower case method.
>>>>> +
>>>>> +    This method will down case all filter contents.
>>>>> +  */
>>>>> +  void to_lower_case(CHARSET_INFO * cs_info);
>>>>> +
>>>>>  private:
>>>>>    bool table_rules_on;
>>>>>  
>>>>> @@ -90,6 +97,10 @@ private:
>>>>>                                             bool inited);
>>>>>    TABLE_RULE_ENT* find_wild(DYNAMIC_ARRAY *a, const char* key, int
> len);
>>>>>  
>>>>> +  void to_lower_case(CHARSET_INFO *cs_info, I_List<i_string>*
> l);
>>>>> +  void to_lower_case(CHARSET_INFO *cs_info, HASH* h);
>>>>> +  void to_lower_case(CHARSET_INFO *cs_info, DYNAMIC_ARRAY* a);
>>>>> +
>>>>>    /*
>>>>>      Those 4 structures below are uninitialized memory unless the
>>>>>      corresponding *_inited variables are "true".
>>>>>
>>>>>
>>>>> Attachment: [text/bzr-bundle]
> bzr/luis.soares@stripped
>>>>>
>>>>> Thread
>>>>> • bzr commit into mysql-5.4 branch (luis.soares:2804)
> Bug#37656	Luis Soares	3 Jul 2009
>>>>>   • Re: bzr commit into mysql-5.4 branch (luis.soares:2804)
> Bug#37656	He Zhenxing	6 Jul 2009
>>>>>
>>>>> © 1995-2008 MySQL AB, 2008-2009 Sun Microsystems, Inc.
>>>>>
>>>>>     * Online Shop
>>>>>     * Privacy Policy
>>>>>     * Legal
>>>>>     * Contact Us
>>>>>     * No Software Patents!
>>>>>
>>>>>
>>>>> Page generated in 1.258 sec. using MySQL 5.1.14-beta-log
>>