Re: bzr commit into mysql-5.1-bugteam branch (v.narayanan:3197) Bug#47762

[Kevin Lewis]

V Narayanan,

I have been trying to debug this recently as a way to start understanding the server code.
 While I was looking at it, it was assigned to you and you seem to have a fix that works. 
I agree that the possible -1 result of item->save_in_field() needs to be passed through
store_val_in_field(), just like it is in  Item::save_in_field_no_warnings().  This allows
matching_cond() can fail, so that the NOT NULL PRIMARY KEY is not used as an optimized
search index in opt_sum_query().  So opt_sum_query() returns 0 to JOIN::optimize()
meaning that no error was found to prevent further processing, but not all items could be
resolved by the index.  This causes JOIN::optimize() to go on and call
make_join_statistics(). 

When  make_join_statistics() calls get_quick_record_count() which calls
SQL_SELECT::test_quick_select(), a call is made to get_mm_tree(), which eventually calls
the same item->save_in_field().  This returns -1 again, this time to
Item::save_in_field_no_warnings(), and returned up the stack back to
SQL_SELECT::test_quick_select().  Here, the (tree->type == SEL_TREE::IMPOSSIBLE)
because it could not put a NULL into a NOT NULL field for searching purposes.

In get_quick_record_count(), this is set; table->reginfo.impossible_range=1;  So
JOIN::optimize eventually sets zero_result_cause= "no matching row in const table"; and
the query returns this;
mysql> SELECT MIN(a) FROM t1 WHERE a = NULL;
+--------+
| MIN(a) |
+--------+
|   NULL |
+--------+
1 row in set (0.00 sec)

It is interesting that without the MIN(), the result is this;
mysql> SELECT a FROM t1 WHERE a = NULL;
Empty set (0.00 sec)

But if our customers are used to this difference in behavior, I guess it is not a big
deal.

All that analysis above was done as much for my benefit as yours, since I'm sure you saw
all that. But it is interesting to note that the failure to find any matching records was
based on the fact that the field was a NOT NULL field.  I wondered what would happen if
the key was not a primary key and was nullable.  The following test show mostly the
correct result, but they get there by different paths.

However, WHERE a > NULL does not work so well. . .

mysql> CREATE TABLE t2 (a int(11), key k2 (a)) engine innod
Query OK, 0 rows affected (0.08 sec)

mysql> INSERT INTO t2 VALUES (1),(2);
Query OK, 2 rows affected (0.03 sec)
Records: 2  Duplicates: 0  Warnings: 0

mysql> SELECT a FROM t2 WHERE a = NULL;
Empty set (0.00 sec)

mysql> SELECT MIN(a) FROM t2 WHERE a = NULL;
+--------+
| MIN(a) |
+--------+
|   NULL |
+--------+
1 row in set (0.00 sec)

mysql> SELECT a FROM t2 WHERE a <> NULL;
Empty set (0.00 sec)

mysql> SELECT MIN(a) FROM t2 WHERE a <> NULL;
+--------+
| MIN(a) |
+--------+
|   NULL |
+--------+
1 row in set (0.00 sec)

mysql> SELECT a FROM t2 WHERE a > NULL;
Empty set (0.00 sec)

mysql> SELECT MIN(a) FROM t2 WHERE a > NULL;
+--------+
| MIN(a) |
+--------+
|      1 |
+--------+
1 row in set (0.00 sec)

mysql> SELECT a FROM t2 WHERE a < NULL;
Empty set (0.00 sec)

mysql> SELECT MIN(a) FROM t2 WHERE a < NULL;
+--------+
| MIN(a) |
+--------+
|   NULL |
+--------+
1 row in set (0.00 sec)

mysql> SELECT a FROM t2 WHERE a between NULL and 10;
Empty set (0.00 sec)

mysql> SELECT MIN(a) FROM t2 WHERE a between NULL and 10;
+--------+
| MIN(a) |
+--------+
|   NULL |
+--------+
1 row in set (0.00 sec)


If
   SELECT a FROM t2 WHERE a > NULL; 
fails with Empty set (since a > NULL is meaningless), then   
   SELECT MIN(a) FROM t2 WHERE a > NULL;
should also fail.  I don't know if you want to open a new bug for this, or continue
working on this with the same bug.  But this is still a problem, I think.

I hope my analysis has been helpful to you.

Kevin Lewis,  
formerly Falcon Team Lead


V Narayanan wrote:
> Hi Jorgen,
> 
> I had already cscoped for all uses of store_val_in_field. I understand 
> this situation.
> If you had noticed the earlier code error was a bool value, and it was 
> being assigned
> 0, 1 and -1 (error= item->save_in_field(field, 1); ). error correctly 
> translated -1 to true.
> Hence I felt confident to use it.
> 
> Also I have run all the tests. All of them passed for 5.1.
> 
> Narayanan
> 
> Jørgen Løland wrote:
>> I discovered one more thing:
>>
>> store_val_in_field is used in another location that may break by the 
>> suggested three-value return:
>>
>> sql_select.cc#test_if_ref()
>> return !store_val_in_field(field, right_item, CHECK_FIELD_WARN);
>>
>> V Narayanan wrote:
>>> #At 
>>>
> file:///home/narayanan/Work/mysql_checkouts/shared_repository_directory/mysql-5.1-bugteam-47762/
> 
>>> based on revid:joro@stripped
>>>
>>>  3197 V Narayanan    2009-10-30
>>>       Bug#47762 Incorrect result from MIN() when WHERE tests NOT NULL 
>>> column for NULL
>>>             The lookup of a NULL value in a column having a NOT NULL
>>>       index was resulting in wrong query result. This was happening
>>>       due to the return value, from the function that stored the
>>>       field value, being ignored.
>>>      @ sql/opt_sum.cc
>>>         Bug#47762 Incorrect result from MIN() when WHERE tests NOT 
>>> NULL column for NULL
>>>                 Ensure that the error (or not) from the 
>>> store_val_in_field
>>>         function is used while determining the usability of the index
>>>         (determined by the return value of the matching_cond function).
>>>      @ sql/sql_select.cc
>>>         Bug#47762 Incorrect result from MIN() when WHERE tests NOT 
>>> NULL column for NULL
>>>                 Ensure that all the three cases of no error, conversion
>>>         and error are reported by the store_val_in_field function.
>>>      @ sql/sql_select.h
>>>         Bug#47762 Incorrect result from MIN() when WHERE tests NOT 
>>> NULL column for NULL
>>>                 The store_val_in_field function returns a int instead of
>>>         a bool so that it can handle all the three cases, namely,
>>>         error, conversion and no error.
>>>
>>>     modified:
>>>       sql/opt_sum.cc
>>>       sql/sql_select.cc
>>>       sql/sql_select.h
>>> === modified file 'sql/opt_sum.cc'
>>> --- a/sql/opt_sum.cc    2009-10-14 08:46:50 +0000
>>> +++ b/sql/opt_sum.cc    2009-10-30 12:24:33 +0000
>>> @@ -599,6 +599,12 @@ static bool matching_cond(bool max_fl, T
>>>                            key_part_map *key_part_used, uint *range_fl,
>>>                            uint *prefix_len)
>>>  {
>>> +  /*
>>> +    contains the error that may occur when trying to store a
>>> +    field value
>>> +  */
>>> +  int store_value_in_field_error;
>>> +
>>>    if (!cond)
>>>      return 1;
>>>    Field *field= field_part->field;
>>> @@ -723,8 +729,10 @@ static bool matching_cond(bool max_fl, T
>>>      }
>>>      else
>>>      {
>>> -      store_val_in_field(part->field, args[between && max_fl ? 2
> : 1],
>>> -                         CHECK_FIELD_IGNORE);
>>> +      store_value_in_field_error=
>>> +                         store_val_in_field(part->field,
>>> +                                            args[between && max_fl ?
> 
>>> 2 : 1],
>>> +                                            CHECK_FIELD_IGNORE);
>>>        if (part->null_bit)          *key_ptr++= (uchar) 
>>> test(part->field->is_null());
>>>        part->field->get_key_image(key_ptr, part->length,
> Field::itRAW);
>>> @@ -751,6 +759,14 @@ static bool matching_cond(bool max_fl, T
>>>    }
>>>    else if (is_field_part)
>>>      *range_fl&= ~(max_fl ? NO_MIN_RANGE : NO_MAX_RANGE);
>>> +  +  /*
>>> +    If an error has occurred when trying to store the value
>>> +    return 0 to indicate failure
>>> +  */
>>> +  if (store_value_in_field_error == -1)
>>> +    return 0;
>>> +
>>>    return 1;   }
>>>  
>>>
>>> === modified file 'sql/sql_select.cc'
>>> --- a/sql/sql_select.cc    2009-10-22 09:40:15 +0000
>>> +++ b/sql/sql_select.cc    2009-10-30 12:24:33 +0000
>>> @@ -5735,13 +5735,15 @@ get_store_key(THD *thd, KEYUSE *keyuse,    
>>> This function is only called for const items on fields which are keys.
>>>  
>>>    @return
>>> -    returns 1 if there was some conversion made when the field was 
>>> stored.
>>> +    returns  0 to signify no error or conversion
>>> +    returns  1 if there was some conversion made when the field was 
>>> stored.
>>> +    returns -1 to signify error
>>>  */
>>>  
>>> -bool
>>> +int
>>>  store_val_in_field(Field *field, Item *item, enum_check_fields 
>>> check_flag)
>>>  {
>>> -  bool error;
>>> +  int error;
>>>    TABLE *table= field->table;
>>>    THD *thd= table->in_use;
>>>    ha_rows cuted_fields=thd->cuted_fields;
>>> @@ -5758,7 +5760,19 @@ store_val_in_field(Field *field, Item *i
>>>    error= item->save_in_field(field, 1);
>>>    thd->count_cuted_fields= old_count_cuted_fields;
>>>    dbug_tmp_restore_column_map(table->write_set, old_map);
>>> -  return error || cuted_fields != thd->cuted_fields;
>>> +
>>> +  /*
>>> +    In the value being returned we handle three cases
>>> +    1)  0 meaning no error or truncation has occurred
>>> +    2)  1 meaning there has been a truncation
>>> +    3) -1 meaning there is an error when trying to store the value
>>> +  */
>>> +  if (error)
>>> +    return error;
>>> +  else if (cuted_fields != thd->cuted_fields)
>>> +    return 1;
>>> +  +  return 0;
>>>  }
>>>  
>>>  
>>>
>>> === modified file 'sql/sql_select.h'
>>> --- a/sql/sql_select.h    2009-10-14 08:46:50 +0000
>>> +++ b/sql/sql_select.h    2009-10-30 12:24:33 +0000
>>> @@ -558,7 +558,7 @@ extern const char *join_type_str[];
>>>  void TEST_join(JOIN *join);
>>>  
>>>  /* Extern functions in sql_select.cc */
>>> -bool store_val_in_field(Field *field, Item *val, enum_check_fields 
>>> check_flag);
>>> +int store_val_in_field(Field *field, Item *val, enum_check_fields 
>>> check_flag);
>>>  TABLE *create_tmp_table(THD *thd,TMP_TABLE_PARAM *param,List<Item> 
>>> &fields,
>>>              ORDER *group, bool distinct, bool save_sum_fields,
>>>              ulonglong select_options, ha_rows rows_limit,
>>>
>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>>
>>
>>
> 
>