List:Commits« Previous MessageNext Message »
From:Daogang Qu Date:October 9 2010 12:19am
Subject:Re: Please review the updated patch for bug51639
View as plain text  
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.
>
> 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
>>>>
>>>
>>
>
>

Thread
Please review the updated patch for bug51639Daogang Qu28 Sep
  • Re: Please review the updated patch for bug51639Alexander Barkov30 Sep
    • Re: Please review the updated patch for bug51639Daogang Qu8 Oct
      • Re: Please review the updated patch for bug51639Alexander Barkov8 Oct
        • Re: Please review the updated patch for bug51639Daogang Qu9 Oct
          • Re: Please review the updated patch for bug51639Alexander Barkov12 Oct
          • Re: Please review the updated patch for bug51639Alexander Barkov12 Oct
            • Re: Please review the updated patch for bug51639Daogang Qu13 Oct
              • Re: Please review the updated patch for bug51639Alexander Barkov13 Oct