List:Commits« Previous MessageNext Message »
From:Roy Lyseng Date:September 9 2010 8:23am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3267)
Bug#53562
View as plain text  
  Hi Jørgen,

fix is approved, but I have a couple of style issues below that you may consider.

Thanks,
Roy

On 01.09.10 15.04, Jorgen Loland wrote:
> #Atfile:///export/home/jl208045/mysql/mysql-next-mr-bugfixing-53562/  based
> onrevid:bar@stripped
>
>   3267 Jorgen Loland	2010-09-01
>        BUG#53562: EXPLAIN statement should hint when index is not used
>                   due to type conversion
>
>        An index may not be used for ref or range access if
>         a) a type conversion is needed to compare an indexed
>            field to a value, or
>         b) there is a collation mismatch between an indexed field
>            and the value compared to
>
>        Before, the index would not be used, but no hint
>        was given that this was the case. With this patch,
>        EXPLAIN EXTENDED will issue a warning for the cases above.
>
>        Note that even if an index cannot be used for ref or range
>        access, the index may still be scanned.
>       @ mysql-test/r/explain.result
>          Added test for BUG#53562
>       @ mysql-test/t/explain.test
>          Added test for BUG#53562
>       @ sql/opt_range.cc
>          Make EXPLAIN EXTENDED issue a warning if an index cannot be used
>          for range access due to type conversion of collation mismatch.
>       @ sql/share/errmsg-utf8.txt
>          Added ER_WARN_INDEX_NOT_APPLICABLE for use when an index cannot
>          be used for ref or range access due to type conversion or
>          collation mismatch.
>       @ sql/sql_select.cc
>          Make EXPLAIN EXTENDED issue a warning if an index cannot be used
>          for ref access due to type conversion of collation mismatch.
>
>      modified:
>        mysql-test/r/explain.result
>        mysql-test/t/explain.test
>        sql/opt_range.cc
>        sql/share/errmsg-utf8.txt
>        sql/sql_select.cc
> === modified file 'mysql-test/r/explain.result'
> --- a/mysql-test/r/explain.result	2010-08-26 12:02:59 +0000
> +++ b/mysql-test/r/explain.result	2010-09-01 13:04:01 +0000
> @@ -324,4 +324,77 @@ id	select_type	table	type	possible_keys	
>   25	UNION	NULL	NULL	NULL	NULL	NULL	NULL	NULL	No tables used
>   NULL	UNION
> RESULT	<union1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,...,25>	ALL	NULL	NULL	NULL	NULL	NULL	
>   # End BUG#30597
> +#
> +# BUG#53562: EXPLAIN statement should hint when
> +#            index is not used due to type conversion
> +#
> +CREATE TABLE t1 (url char(1) PRIMARY KEY);
> +INSERT INTO t1 VALUES ('1'),('2'),('3'),('4'),('5');
> +
> +# Normally, lookup access on primary key is done
> +EXPLAIN EXTENDED SELECT * FROM t1 WHERE url='1';
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
> +1	SIMPLE	t1	const	PRIMARY	PRIMARY	1	const	1	100.00	Using index
> +Warnings:
> +Note	1003	select '1' AS `url` from `test`.`t1` where 1
> +
> +# Test that index can't be used for lookup due to type conversion
> +# (comparing char and int)
> +SELECT * FROM t1 WHERE url=1;
> +url
> +1
> +EXPLAIN EXTENDED SELECT * FROM t1 WHERE url=1;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
> +1	SIMPLE	t1	index	PRIMARY	PRIMARY	1	NULL	5	100.00	Using where; Using index
> +Warnings:
> +Warning	1708	Cannot use ref access on index 'PRIMARY' due to type or collation
> conversion on field 'url'
> +Warning	1708	Cannot use range access on index 'PRIMARY' due to type or collation
> conversion on field 'url'
> +Note	1003	select `test`.`t1`.`url` AS `url` from `test`.`t1` where
> (`test`.`t1`.`url` = 1)
> +
> +# Test that index can't be used for lookup due to collation mismatch
> +SELECT * FROM t1 WHERE url='1' collate latin1_german2_ci;
> +url
> +1
> +EXPLAIN EXTENDED SELECT * FROM t1 WHERE url='1' collate latin1_german2_ci;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
> +1	SIMPLE	t1	index	PRIMARY	PRIMARY	1	NULL	5	100.00	Using where; Using index
> +Warnings:
> +Warning	1708	Cannot use ref access on index 'PRIMARY' due to type or collation
> conversion on field 'url'
> +Warning	1708	Cannot use range access on index 'PRIMARY' due to type or collation
> conversion on field 'url'
> +Note	1003	select `test`.`t1`.`url` AS `url` from `test`.`t1` where
> (`test`.`t1`.`url` =<cache>(('1' collate latin1_german2_ci)))
> +
> +# Normally, range access on primary key is done
> +EXPLAIN EXTENDED SELECT * FROM t1 WHERE url>'3';
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
> +1	SIMPLE	t1	range	PRIMARY	PRIMARY	1	NULL	3	100.00	Using where; Using index
> +Warnings:
> +Note	1003	select `test`.`t1`.`url` AS `url` from `test`.`t1` where
> (`test`.`t1`.`url`>  '3')
> +
> +# Test that range access on index can't be done due to type conversion
> +# (comparing char and int)
> +SELECT * FROM t1 WHERE url>3;
> +url
> +4
> +5
> +EXPLAIN EXTENDED SELECT * FROM t1 WHERE url>3;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
> +1	SIMPLE	t1	index	PRIMARY	PRIMARY	1	NULL	5	100.00	Using where; Using index
> +Warnings:
> +Warning	1708	Cannot use range access on index 'PRIMARY' due to type or collation
> conversion on field 'url'
> +Note	1003	select `test`.`t1`.`url` AS `url` from `test`.`t1` where
> (`test`.`t1`.`url`>  3)
> +
> +# Test that range access on index can't be done due to collation mismatch
> +SELECT * FROM t1 WHERE url>'3' collate latin1_german2_ci;
> +url
> +4
> +5
> +EXPLAIN EXTENDED SELECT * FROM t1 WHERE url>'3' collate latin1_german2_ci;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
> +1	SIMPLE	t1	index	PRIMARY	PRIMARY	1	NULL	5	100.00	Using where; Using index
> +Warnings:
> +Warning	1708	Cannot use range access on index 'PRIMARY' due to type or collation
> conversion on field 'url'
> +Note	1003	select `test`.`t1`.`url` AS `url` from `test`.`t1` where
> (`test`.`t1`.`url`>  <cache>(('3' collate latin1_german2_ci)))
> +
> +DROP TABLE t1;
> +# End BUG#53562
>   End of 6.0 tests.
>
> === modified file 'mysql-test/t/explain.test'
> --- a/mysql-test/t/explain.test	2010-08-26 12:02:59 +0000
> +++ b/mysql-test/t/explain.test	2010-09-01 13:04:01 +0000
> @@ -279,4 +279,43 @@ EXPLAIN
>
>   --echo # End BUG#30597
>
> +--echo #
> +--echo # BUG#53562: EXPLAIN statement should hint when
> +--echo #            index is not used due to type conversion
> +--echo #
> +
> +CREATE TABLE t1 (url char(1) PRIMARY KEY);
> +INSERT INTO t1 VALUES ('1'),('2'),('3'),('4'),('5');
> +
> +--echo
> +--echo # Normally, lookup access on primary key is done
> +EXPLAIN EXTENDED SELECT * FROM t1 WHERE url='1';
> +--echo
> +--echo # Test that index can't be used for lookup due to type conversion
> +--echo # (comparing char and int)
> +SELECT * FROM t1 WHERE url=1;
> +EXPLAIN EXTENDED SELECT * FROM t1 WHERE url=1;
> +--echo
> +--echo # Test that index can't be used for lookup due to collation mismatch
> +SELECT * FROM t1 WHERE url='1' collate latin1_german2_ci;
> +EXPLAIN EXTENDED SELECT * FROM t1 WHERE url='1' collate latin1_german2_ci;
> +
> +--echo
> +--echo # Normally, range access on primary key is done
> +EXPLAIN EXTENDED SELECT * FROM t1 WHERE url>'3';
> +--echo
> +--echo # Test that range access on index can't be done due to type conversion
> +--echo # (comparing char and int)
> +SELECT * FROM t1 WHERE url>3;
> +EXPLAIN EXTENDED SELECT * FROM t1 WHERE url>3;
> +--echo
> +--echo # Test that range access on index can't be done due to collation mismatch
> +SELECT * FROM t1 WHERE url>'3' collate latin1_german2_ci;
> +EXPLAIN EXTENDED SELECT * FROM t1 WHERE url>'3' collate latin1_german2_ci;
> +
> +--echo
> +DROP TABLE t1;
> +
> +--echo # End BUG#53562
> +
>   --echo End of 6.0 tests.
>
> === modified file 'sql/opt_range.cc'
> --- a/sql/opt_range.cc	2010-08-04 10:34:01 +0000
> +++ b/sql/opt_range.cc	2010-09-01 13:04:01 +0000
> @@ -5768,7 +5768,17 @@ get_mm_leaf(RANGE_OPT_PARAM *param, Item
>         ((Field_str*)field)->charset() !=
> conf_func->compare_collation()&&
>         !(conf_func->compare_collation()->state&  MY_CS_BINSORT&&
>           (type == Item_func::EQUAL_FUNC || type == Item_func::EQ_FUNC)))
> +  {
> +    if (param->thd->lex->describe&  DESCRIBE_EXTENDED)
> +      push_warning_printf(param->thd,
> +                          MYSQL_ERROR::WARN_LEVEL_WARN,
> +                          ER_WARN_INDEX_NOT_APPLICABLE,
> +                          ER(ER_WARN_INDEX_NOT_APPLICABLE),
> +                          "range",
> +                         
> field->table->key_info[param->real_keynr[key_part->key]].name,

Extends beyond column 80, see below for proposal.
> +                          field->field_name);
>       goto end;
> +  }
>
>     if (key_part->image_type == Field::itMBR)
>     {
> @@ -5889,7 +5899,17 @@ get_mm_leaf(RANGE_OPT_PARAM *param, Item
>     if (field->result_type() == STRING_RESULT&&
>         value->result_type() != STRING_RESULT&&
>         field->cmp_type() != value->result_type())
> +  {
> +    if (param->thd->lex->describe&  DESCRIBE_EXTENDED)
> +      push_warning_printf(param->thd,
> +                          MYSQL_ERROR::WARN_LEVEL_WARN,
> +                          ER_WARN_INDEX_NOT_APPLICABLE,
> +                          ER(ER_WARN_INDEX_NOT_APPLICABLE),
> +                          "range",
> +                         
> field->table->key_info[param->real_keynr[key_part->key]].name,
> +                          field->field_name);

This line extends well beyond column 80. My proposal is that you reformat as:

       push_warning_printf(
         param->thd,
         MYSQL_ERROR::WARN_LEVEL_WARN,
         ....,

but you may have a better idea...


>       goto end;
> +  }
>     /* For comparison purposes allow invalid dates like 2000-01-32 */
>     orig_sql_mode= field->table->in_use->variables.sql_mode;
>     if (value->real_item()->type() == Item::STRING_ITEM&&
>
> === modified file 'sql/share/errmsg-utf8.txt'
> --- a/sql/share/errmsg-utf8.txt	2010-07-29 14:15:38 +0000
> +++ b/sql/share/errmsg-utf8.txt	2010-09-01 13:04:01 +0000
> @@ -6387,3 +6387,5 @@ ER_TABLES_DIFFERENT_METADATA
>   ER_ROW_DOES_NOT_MATCH_PARTITION
>     eng "Found row that does not match the partition"
>     swe "Hittade rad som inte passar i partitionen"
> +ER_WARN_INDEX_NOT_APPLICABLE
> +  eng "Cannot use %-.64s access on index '%-.64s' due to type or collation
> conversion on field '%-.64s'"
>
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2010-08-26 13:54:59 +0000
> +++ b/sql/sql_select.cc	2010-09-01 13:04:01 +0000
> @@ -5159,6 +5159,41 @@ static uint get_semi_join_select_list_in
>   }
>
>   /**
Proposal: Add a "@brief" line here and a "@details section before this (very 
nice) comment.

> +   If EXPLAIN EXTENDED, add a warning for each index that cannot be
> +   used for ref access due to either type conversion or different
> +   collations on the field used for comparison
> +
> +   Example type conversion (char compared to int):
> +
> +   CREATE TABLE t1 (url char(1) PRIMARY KEY);
> +   SELECT * FROM t1 WHERE url=1;
> +
> +   Example different collations (danish vs german2):
> +
> +   CREATE TABLE t1 (url char(1) PRIMARY KEY) collate latin1_danish_ci;
> +   SELECT * FROM t1 WHERE url='1' collate latin1_german2_ci;
> +
> +   @param thd                 Thread for the connection that submitted the query
Lots of whitespace after text here...

> +   @param field               Field used in comparision
> +   @param cant_use_indexes    Indexes that cannot be used for lookup
> + */
> +static void
> +warn_index_not_applicable(THD *thd, const Field *field,
> +                          const key_map cant_use_index)
> +{
> +  if (thd->lex->describe&  DESCRIBE_EXTENDED)
> +    for (uint j=0 ; j<  field->table->s->keys ; j++)
> +      if (cant_use_index.is_set(j))
> +        push_warning_printf(thd,
> +                            MYSQL_ERROR::WARN_LEVEL_WARN,
> +                            ER_WARN_INDEX_NOT_APPLICABLE,
> +                            ER(ER_WARN_INDEX_NOT_APPLICABLE),
> +                            "ref",
> +                            field->table->key_info[j].name,
> +                            field->field_name);
> +}
> +
> +/**
>     Add a possible key to array of possible keys if it's usable as a key
>
>       @param key_fields      Pointer to add key, if usable
> @@ -5183,6 +5218,7 @@ add_key_field(KEY_FIELD **key_fields,uin
>                 Field *field, bool eq_func, Item **value, uint num_values,
>                 table_map usable_tables, SARGABLE_PARAM **sargables)
>   {
> +  DBUG_PRINT("info",("add_key_field for field %s",field->field_name));
>     uint exists_optimize= 0;
>     if (!(field->flags&  PART_KEY_FLAG))
>     {
> @@ -5284,7 +5320,10 @@ add_key_field(KEY_FIELD **key_fields,uin
>           if ((*value)->result_type() != STRING_RESULT)
>           {
>             if (field->cmp_type() != (*value)->result_type())
> +          {
> +            warn_index_not_applicable(stat->join->thd, field, possible_keys);
>               return;
> +          }
>           }
>           else
>           {
> @@ -5294,7 +5333,10 @@ add_key_field(KEY_FIELD **key_fields,uin
>             */
>             if (field->cmp_type() == STRING_RESULT&&
>                 ((Field_str*)field)->charset() != cond->compare_collation())
> +          {
> +            warn_index_not_applicable(stat->join->thd, field, possible_keys);
>               return;
> +          }
>           }
>         }
>       }
>
>
>
>



Thread
bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3267) Bug#53562Jorgen Loland1 Sep
  • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3267)Bug#53562Roy Lyseng9 Sep