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

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

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