From: Alexander Barkov
Date: October 12 2010 11:00am
Subject: Re: Please review the updated patch for bug51639
List-Archive: http://lists.mysql.com/commits/120547
Message-Id: <4CB43FE3.3090104@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Hi Daogang,

Daogang Qu wrote:
> 2010-10-08 18:54, Alexander Barkov wrote:
>> Hi Daogang,
>>
>> Daogang Qu wrote:
>>> Hi Bar,
>>> Thanks for the review. See reply in-line.
>>> Please review:
>>> http://lists.mysql.com/commits/120340
>>>
>>> 2010-09-30 20:49, Alexander Barkov wrote:
>>>> Hi Daogang,
>>>>
>>>>
>>>> Daogang Qu wrote:
>>>>> Hi Bar,
>>>>> Please review the updated patch:
>>>>> http://lists.mysql.com/commits/118275
>>>>
>>>> my_strcasecmp() in these two chunks is not correct.
>>>>
>>>> @@ -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);
>>>>    }
>>>>
>>>> This comparison will always be case insensitive,
>>> No. It will be case sensitive when setting lower_case_table_name=0.
>>> The 'rpl_do_db_filter.test' will verify the point. Please check.
>>
>> I am sorry for confusion. You're right:
>> - In case of lower_case_table_names=0 it will
>> call my_strcasecmp_bin() which then calls strcmp(),
>> which is case sensitive.
>> - In case of lower_case_table_name=1 it will
>> call my_strcasecmp_utf8() which is case insensitive.
>> Everything is correct here.
>>
>> (last time I checked my_strcasecmp(&my_charset_utf8_bin,a,b)
>> instead of my_strcasecmp(&my_charset_bin,a,b)
>> in a mistake).
> No problem.
>>
>>
>>>
>>>> even if lower_case_table_name=0. This is not what we need.
>>>> You need to use my_strnncoll() instead.
>>>>
>>>>
>>>> Please also add a test covering this.
>>>> You need .opt file with these settings:
>>>>
>>>> --replicate-do-db=MYDB --lower-case-table-names=0
>>> Added in 'rpl_do_db_filter.test'
>>>>
>>>> This test will check that nothing is replicated,
>>>> because database name is "mydb" which is not equal to MYDB
>>>> when lower_case_table_name=0.
>>>>
>>>> Currently this combination is not covered.
>>>>
>>>>>
>>>>> Could you please add a test for testing new collation
>>>>> for supporting the case insensitive but accent sensitive?
>>>>
>>>> It will be very similar to how you test case sensitivity.
>>>> Just use some accented letters in db or table names in
>>>> SQL queries. For example,
>>>>
>>>> ý  in database name: mýdb
>>>> ţ  in table name: ţ1
>>>>
>>>> Use non-accented letters in .opt file.
>>>>
>>>> This is to make sure that filters recognize
>>>> that "mydb" and "mýdb" are different database names,
>>>> and that "t1" and "ţ1" are different table names.
>>>>
>>>> no matter what lower-case-table-name is.
>>> I tested the non-accented letters in rpl_do_table_filter-slave.opt file.
>>> But it didn't work as expected. The filters still will be case 
>>> insensitive
>>> but non-accent sensitive when setting lower_case_table_name>0.
>>> Please double check the definition of 'my_charset_utf8_tolower_ci'.
>>> Thanks!
>>
>> The problem is likely that
>> my_strcasecmp_utf8('mydb', 'mýdb')
>> my_strcasecmpsp_utf8('mydb', 'mýdb')
>>
>> return TRUE for my_charset_utf8_tolower_ci for some reasons,
>> for the above strings, or get wrong parameters.
>> The idea is to set break-points in these functions and check.
>>
>> I'll compile your patch later, and write a separate letter.


I tested these functions and they seem to work fine.
I exposed utf8_tolower_ci to SQL level by adding this patch:

=== modified file 'mysys/charset-def.c'
--- mysys/charset-def.c	2010-07-08 10:57:38 +0000
+++ mysys/charset-def.c	2010-10-12 10:27:56 +0000
@@ -258,6 +258,7 @@ my_bool init_compiled_charsets(myf flags

  #ifdef HAVE_CHARSET_utf8
    add_compiled_collation(&my_charset_utf8_general_ci);
+  add_compiled_collation(&my_charset_utf8_tolower_ci);
    add_compiled_collation(&my_charset_utf8_bin);
  #ifdef HAVE_UTF8_GENERAL_CS
    add_compiled_collation(&my_charset_utf8_general_cs);


Now I can use "COLLATE utf8_tolower_ci" in SQL syntax.


mysql> drop table if exists t1;
mysql> create table t1 (a varchar(16) character set utf8 collate 
utf8_tolower_ci);
mysql> insert into t1 values
('mýdb'),('mydb'),('MYDB'),('MÝDB'),('ţ1'),('t1'),('T1'),('Ţ1');

-- make sure we get correct HEX utf8 values
mysql> select hex(a), a from t1;
+------------+-------+
| hex(a)     | a     |
+------------+-------+
| 6DC3BD6462 | mýdb  |
| 6D796462   | mydb  |
| 4D594442   | MYDB  |
| 4DC39D4442 | MÝDB  |
| C5A331     | ţ1    |
| 7431       | t1    |
| 5431       | T1    |
| C5A231     | Ţ1    |
+------------+-------+
8 rows in set (0.00 sec)

-- check GROUP BY is accent sensitive
mysql> select group_concat(a), count(*) from t1 group by a;
+-----------------+----------+
| group_concat(a) | count(*) |
+-----------------+----------+
| mydb,MYDB       |        2 |
| mýdb,MÝDB       |        2 |
| t1,T1           |        2 |
| ţ1,Ţ1           |        2 |
+-----------------+----------+
4 rows in set (0.00 sec)

-- check comparison is accent sensitive
mysql> select * from t1 where a='mydb';
+------+
| a    |
+------+
| mydb |
| MYDB |
+------+
2 rows in set (0.00 sec)

-- check comparison is accent sensitive
mysql> select * from t1 where a='mýdb';
+-------+
| a     |
+-------+
| mýdb  |
| MÝDB  |
+-------+
2 rows in set (0.00 sec)


So the collation code works as expected.

Now checking mtr --gdb.

>>
>> Is there a way to run slave under "gdb" in mysql-test?
> 
> If './mtr rpl_do_table_filter --gdb'  does not work, use --manual-gdb 
> follow the following four steps:
> 
> 1.
> ./mtr rpl_do_table_filter --mysqld=--log-bin=/var/tmp/masterdg-bin 
> --record --manual-gdb --testcase-timeout=1000
> 
> 2. open and rewrite var/tmp/gdbinit.mysqld.2
> vi var/tmp/gdbinit.mysqld.2
> 
> Disable the break point in the file as following:
> set args --defaults-group-suffix=.1 
> --defaults-file=/home/daogangqu/mysql/bzrwork/bug34739/mysql-5.1-bugteam/mysql-test/var/my.cnf 
> --log-output=file --gdb --loose-skip-innodb 
> --log-bin=/var/tmp/masterdg-bin --binlog-format=statement --core-file 
> --loose-debug-sync-timeout=300
> #break mysql_parse
> #commands 1
> #disable 1
> #end
> #run
> 
> 3. Run the following command copied from the result of first step.
> like this:
> libtool --mode execute gdb -cd 
> /home/daogangqu/mysql/bzrwork/bug34739/mysql-5.1-bugteam/mysql-test -x 
> /home/daogangqu/mysql/bzrwork/bug34739/mysql-5.1-bugteam/mysql-test/var/tmp/gdbinit.mysqld.1 
> /home/daogangqu/mysql/bzrwork/bug34739/mysql-5.1-bugteam/sql/mysqld
> 
> (libtool --mode execute) is added extra.
> 
> 
> 4. Set breakpoint and run it in gdb
> 
> Best Regards,
> 
> Daogang
>>
>>>
>>> Best Regards,
>>>
>>> Daogang
>>>>
>>>>
>>>>
>>>>>
>>>>> Best Regards,
>>>>>
>>>>> Daogang
>>>>>
>>>>
>>>
>>
>>
>