List:Commits« Previous MessageNext Message »
From:Evgeny Potemkin Date:September 1 2010 1:16pm
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch
(jorgen.loland:3264) Bug#53562
View as plain text  
IMHO it's not worth issuing two different warnings, "Cannot do ref or range 
access..." is enough. Moreover, I would add only one combined warning like 
"Can't do ref or range ... due to type or collation conversion". It gives a clue 
and this is our ultimate goal for this bug. For a complex (or weird) query 
EXPLAIN output could be overloaded with these warnings and thus instead of 
making things easier we'll make them complex.

Regards, Evgen.

On 09/01/10 08:51, Jorgen Loland wrote:
> I see your point. How about this:
>
> 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 do ref access on index 'PRIMARY' due to type conversion on
> field 'url'
> Warning 1708 Cannot do range access on index 'PRIMARY' due to type conversion on
> field 'url'
> Note 1003 select `test`.`t1`.`url` AS `url` from `test`.`t1` where
> (`test`.`t1`.`url` = 1)
>
> 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 do range access on index 'PRIMARY' due to type conversion on
> field 'url'
> Note 1003 select `test`.`t1`.`url` AS `url` from `test`.`t1` where
> (`test`.`t1`.`url` > 3)
>
> 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 1709 Cannot do range access on index 'PRIMARY' due to collation mismatch
> on field 'url'
> Note 1003 select `test`.`t1`.`url` AS `url` from `test`.`t1` where
> (`test`.`t1`.`url` > <cache>(('3' collate latin1_german2_ci)))
>
> With this, we have warnings for
> * ref(_or_null): "Cannot do ref access..."
> * range: "Cannot do range access"
>
> I don't think fulltext, ALL, index* will need warnings.
>
> On 08/31/2010 07:39 PM, Evgeny Potemkin wrote:
>> IMHO, it worth to generalize the bug report to "it's hard to understand
>> why an index isn't used" and an example with a query not using ref access.
>> In the code you can see that not only ref access is affected. See:
>>
>> mysql> explain extended select * from t1 where f1 >'999'\G
>> *************************** 1. row ***************************
>> id: 1
>> select_type: SIMPLE
>> table: t1
>> type: range
>> possible_keys: ci1
>> key: ci1
>> key_len: 3
>> ref: NULL
>> rows: 1
>> filtered: 100.00
>> Extra: Using where; Using index
>> 1 row in set, 1 warning (0.00 sec)
>>
>> mysql> explain extended select * from t1 where f1 >999\G
>> *************************** 1. row ***************************
>> id: 1
>> select_type: SIMPLE
>> table: t1
>> type: index
>> possible_keys: ci1
>> key: ci1
>> key_len: 8
>> ref: NULL
>> rows: 1076
>> filtered: 100.00
>> Extra: Using where; Using index
>> 1 row in set, 1 warning (0.00 sec)
>>
>>
>> As you can see 'range' is _much_ better: 1 row vs 700+ rows (1076 is
>> InnoDB's estimate).
>> It would be wrong to limit this info to ref access only.
>>
>> Regards, Evgen.
>>
>>
>> On 08/31/10 12:04, Jorgen Loland wrote:
>>> Evgen,
>>>
>>> I disagree. In the case below, the index is not used for lookup but for
>>> scanning. That's perfectly fine and the reason why the text is
>>> explicit about
>>> "for lookup".
>>>
>>> On 08/31/2010 05:28 PM, Evgeny Potemkin wrote:
>>>> Hi Jorgen,
>>>>
>>>> The fix is good, but there is a room for improvement. Consider the
>>>> example (it would be good if you include it into your test case):
>>>>
>>>> CREATE TABLE t1 (f1 char(3) PRIMARY KEY, f2 int);
>>>> create table t2(f1 int);
>>>> insert into t2 values (1),(2),(3),(4),(5),(6),(7),(8),(9);
>>>> insert into t1(f1,f2) select A.f1 + B.f1*10 + C.f1*100, A.f1 + B.f1*10 +
>>>> C.f1*100 from t2 A,t2 B,t2 C;
>>>> mysql> EXPLAIN EXTENDED SELECT * FROM t1 WHERE f1=333 and f1 >=
> '333'
>>>> and f1 <444;
>>>>
> +----+-------------+-------+-------+---------------+---------+---------+------+------+----------+-------------+
>>>>
>>>>
>>>>
>>>>
>>>> | id | select_type | table | type | possible_keys | key | key_len | ref
>>>> | rows | filtered | Extra |
>>>>
> +----+-------------+-------+-------+---------------+---------+---------+------+------+----------+-------------+
>>>>
>>>>
>>>>
>>>>
>>>> | 1 | SIMPLE | t1 | range | PRIMARY | PRIMARY | 3 | NULL | 546 | 100.00
>>>> | Using where |
>>>>
> +----+-------------+-------+-------+---------------+---------+---------+------+------+----------+-------------+
>>>>
>>>>
>>>>
>>>>
>>>> 1 row in set, 2 warnings (0.00 sec)
>>>>
>>>> mysql> show warnings;
>>>>
> +---------+------+-------------------------------------------------------------------------------------
>>>>
>>>>
>>>>
>>>>
>>>> | Level | Code | Message
>>>>
> +---------+------+-------------------------------------------------------------------------------------
>>>>
>>>>
>>>>
>>>>
>>>> | Warning | 1708 | Can't use index 'PRIMARY' for lookups due to type
>>>> conversion on field 'f1'
>>>> [skip]
>>>> 2 rows in set (0.00 sec)
>>>>
>>>> And there is 2 ways to fix it:
>>>> 1) (Easy) Correct warning message to be like "Type conversion on '%s'
>>>> could prevent index lookups on index '%s'".
>>>> So it could prevent, but the case when index is used is still possible.
>>>> 2) (Harder) Implement more comprehensive check - first, mark keys which
>>>> we "couldn't use", after all keys are gathered compare it with possible
>>>> keys,
>>>> if marked keys are absent in possible keys then type conversion actually
>>>> prevented marked keys usage.
>>>>
>>>> I personally prefer #1 since it gives a clue on why an index isn't used
>>>> and if the key is still used it points to a potential problem. Also,
>>>> it's much easier and error-proof.
>>>>
>>>> Regards, Evgen.
>>>>
>>>>
>>>> On 08/31/10 07:08, Jorgen Loland wrote:
>>>>> #At
> file:///export/home/jl208045/mysql/mysql-next-mr-bugfixing-53562/
>>>>> based on
> revid:jorgen.loland@stripped
>>>>>
>>>>> 3264 Jorgen Loland 2010-08-31
>>>>> BUG#53562: EXPLAIN statement should hint when index is not used
>>>>> due to type conversion
>>>>>
>>>>> An index may not be used for lookup 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 for lookup, 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 lookup, 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/share/errmsg-utf8.txt
>>>>> Added ER_WARN_INDEX_NO_LOOKUP_CAST and
>>>>> ER_WARN_INDEX_NO_LOOKUP_COLLATION for use when an index cannot
>>>>> be used for lookup due to type conversion or collation mismatch.
>>>>> @ sql/sql_select.cc
>>>>> Make EXPLAIN EXTENDED issue a warning if an index cannot be used
>>>>> for lookup due to type conversion of collation mismatch.
>>>>>
>>>>> modified:
>>>>> mysql-test/r/explain.result
>>>>> mysql-test/t/explain.test
>>>>> 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-08-31 07:08:36 +0000
>>>>> @@ -324,4 +324,43 @@ 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');
>>>>> +
>>>>> +# Normally, primary key lookup 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 2 100.00 Using where;
> Using
>>>>> index
>>>>> +Warnings:
>>>>> +Warning 1708 Can't use index 'PRIMARY' for lookups due to type
>>>>> 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 2 100.00 Using where;
> Using
>>>>> index
>>>>> +Warnings:
>>>>> +Warning 1709 Can't use index 'PRIMARY' for lookups due to collation
>>>>> mismatch on field 'url'
>>>>> +Note 1003 select `test`.`t1`.`url` AS `url` from `test`.`t1` where
>>>>> (`test`.`t1`.`url` =<cache>(('1' 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-08-31 07:08:36 +0000
>>>>> @@ -279,4 +279,30 @@ 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');
>>>>> +
>>>>> +--echo
>>>>> +--echo # Normally, primary key lookup 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
>>>>> +DROP TABLE t1;
>>>>> +
>>>>> +--echo # End BUG#53562
>>>>> +
>>>>> --echo End of 6.0 tests.
>>>>>
>>>>> === 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-08-31 07:08:36 +0000
>>>>> @@ -6387,3 +6387,7 @@ 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_NO_LOOKUP_CAST
>>>>> + eng "Can't use index '%-.64s' for lookups due to type conversion
> on
>>>>> field '%-.64s'"
>>>>> +ER_WARN_INDEX_NO_LOOKUP_COLLATION
>>>>> + eng "Can't use index '%-.64s' for lookups due to collation
> mismatch
>>>>> 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-08-31 07:08:36 +0000
>>>>> @@ -5159,6 +5159,41 @@ static uint get_semi_join_select_list_in
>>>>> }
>>>>>
>>>>> /**
>>>>> + If EXPLAIN EXTENDED, add a warning for each index that cannot be
>>>>> + used for lookup 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
>>>>> + @param field Field used in comparision
>>>>> + @param cant_use_indexes Indexes that cannot be used for lookup
>>>>> + @param errorno The error message number to use in the warning
>>>>> + */
>>>>> +static void
>>>>> +warn_index_not_applicable(THD *thd, const Field *field,
>>>>> + const key_map cant_use_index, const uint errorno)
>>>>> +{
>>>>> + 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,
>>>>> + errorno,
>>>>> + ER(errorno),
>>>>> + 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,11 @@ 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,
>>>>> + ER_WARN_INDEX_NO_LOOKUP_CAST);
>>>>> return;
>>>>> + }
>>>>> }
>>>>> else
>>>>> {
>>>>> @@ -5294,7 +5334,11 @@ 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,
>>>>> + ER_WARN_INDEX_NO_LOOKUP_COLLATION);
>>>>> return;
>>>>> + }
>>>>> }
>>>>> }
>>>>> }
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>
>
Thread
bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3264) Bug#53562Jorgen Loland31 Aug
  • Re: bzr commit into mysql-next-mr-bugfixing branch(jorgen.loland:3264) Bug#53562Evgeny Potemkin31 Aug
    • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3264)Bug#53562Jorgen Loland31 Aug
      • Re: bzr commit into mysql-next-mr-bugfixing branch(jorgen.loland:3264) Bug#53562Evgeny Potemkin31 Aug
        • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3264)Bug#53562Jorgen Loland1 Sep
          • Re: bzr commit into mysql-next-mr-bugfixing branch(jorgen.loland:3264) Bug#53562Evgeny Potemkin1 Sep