From: Alexander Barkov
Date: October 12 2010 1:28pm
Subject: Re: Please review the updated patch for bug51639
List-Archive: http://lists.mysql.com/commits/120606
Message-Id: <4CB46270.2080206@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.
>>
>> 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:



mtr --gdb did not work for me.

I tried to add some debug output.
The relevant piece of "bzr diff rpl_filter.cc" now
looks like this in my tree (notice DBUG_PRINT commands):


@@ -104,22 +109,35 @@ 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
+    DBUG_PRINT("info", ("tables_ok:checking '%.*s'", len, hash_key));
+    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_PRINT("info", ("tables_ok:1:do_table found '%.*s'", len, 
hash_key));
  	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_PRINT("info", ("tables_ok:0:ignore_table found '%.*s'", 
len, hash_key));
  	DBUG_RETURN(0);
+      }
      }
      if (wild_do_table_inited &&
  	find_wild(&wild_do_table, hash_key, len))
+    {
+      DBUG_PRINT("info", ("tables_ok:1:wild_do_table found '%.*s'", 
len, hash_key));
        DBUG_RETURN(1);
+    }
      if (wild_ignore_table_inited &&
  	find_wild(&wild_ignore_table, hash_key, len))
+    {
+      DBUG_PRINT("info", ("tables_ok:0:wild_ignore_table found '%.*s'", 
len, hash_key));
        DBUG_RETURN(0);
+    }
    }

    /*
@@ -128,8 +146,14 @@ Rpl_filter::tables_ok(const char* db, TA
      If no explicit rule found and there was a do list, do not replicate.
      If there was no do list, go ahead
    */
+  DBUG_PRINT("info", ("tables_ok:%d:return some_tables_updating=%d 
do_table_array_inited=%d wild_do_table_inited=%d",
+             some_tables_updating && !do_table_array_inited &&
+             !wild_do_table_inited,
+             some_tables_updating,
+             do_table_array_inited,
+             wild_do_table_inited));



Now I run these commands:

./mtr --debug rpl_do_table_filter
grep "tables_ok:" var/log/mysqld.2.trace

and get this output from grep:

T@7    : | | | | | | info: tables_ok:checking 'mydb.t1'
T@7    : | | | | | | info: tables_ok:1:return some_tables_updating=1 
do_table_array_inited=0 wild_do_table_inited=0
T@7    : | | | | | | info: tables_ok:checking 'mydb.t2'
T@7    : | | | | | | info: tables_ok:1:do_table found 'mydb.t2'
T@7    : | | | | | | info: tables_ok:checking 'mydb.t1'
T@7    : | | | | | | info: tables_ok:1:return some_tables_updating=1 
do_table_array_inited=0 wild_do_table_inited=0
T@7    : | | | | | | info: tables_ok:checking 'mydb.t2'
T@7    : | | | | | | info: tables_ok:1:do_table found 'mydb.t2'


 From which I conclude that tables_ok() never returns 1 because
of wrong collation behaviour.

Collation correctly finds 'mydb.t2' in do_table.
Collation correctly skips 'mydb.t1'.

But then it returns 1 for 'mydb.t1' anyway,
in the very end of the function, in this line:

   DBUG_RETURN(some_tables_updating &&
               !do_table_array_inited && !wild_do_table_inited);



Perhaps this is something related to this change in your patch:

    DBUG_RETURN(some_tables_updating &&
-              !do_table_inited && !wild_do_table_inited);
+              !do_table_array_inited && !wild_do_table_inited);
  }



> 
> 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
>>>>>
>>>>
>>>
>>
>>
>