List:Commits« Previous MessageNext Message »
From:Martin Hansson Date:June 3 2010 8:45am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (martin.hansson:3387)
Bug#53859
View as plain text  
Hi Sergey,

new patch ready.

Sergey Glukhov wrote:
> Hi Martin,....
>>
>> +--echo #
>> +--echo # Bug#53859: Valgrind: opt_sum_query(TABLE_LIST*, 
>> List<Item>&, Item*) at
>> +--echo # opt_sum.cc:305
>> +--echo #
>> +CREATE TABLE t1 ( a INT, b INT, KEY (b) );
>> +INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
>> +
>> +--echo # Should not give valgrid warnings.
>> +SELECT MIN( t1a.b ) AS field1
>> +FROM t1 t1a LEFT JOIN t1 t1b USING( a )
>> +WHERE t1a.b>  1 AND t1a.b IS NULL
>> +ORDER BY field1;
>> +
>> +DROP TABLE t1;
>> +
>>   --echo End of 5.1 tests
> It's not necessary to use LEFT JOIN here,
> The following SELECT is enough:
>
> SELECT MIN( t1a.b ) AS field1
> FROM t1 t1a
> WHERE t1a.b > 1 AND t1a.b IS NULL
> ORDER BY field1;
>
> Check also reverse order of conditions
> (which does not work with your patch properly).
Thank you for that. You've discovered a problem I didn't think of. The 
problem I was solving was that we may use a search key which is the full 
length of the index, i.e. if index is (a, b), then in a normal use of 
matching_cond we might search for <a_value>. But the key can also be 
<a_value, b_value> and in this case we must make sure not to look at the 
next value in the read tuple.

You proved that this can only happen for closed intervals and hence we 
don't need a special case in get_index_min_value. But I put in 
DBUG_ASSERT to make sure it doesn't happen.

It can only happen for closed intervals because for some reason equality 
keys such as <1, 1>

SELECT MIN(kp_b) WHERE kp_a = 1 AND kp_b = 1;

are read as closed intervals, which surprises me, since if there is no 
<1, 1> tuple in the database we will get the next  one, which could be 
<100, 100> and only later is the tuple rejected (well, directly 
afterwards to be fair). Maybe someone actually fixed a similar bug for = 
instead of IS NULL this way? But nevermind, I'm only here to fix a bug. :-)

You discovered another problem: That we may actually overwrite the 
search key if we have multiple conditions on the same keypart: a > 1 AND 
a IS NULL. This will cause the a keypart first to be 1 and then NULL, 
hence the condition a > 1 is forgotten. I modified your fix to check for 
this, but in one place only. I'm not to fond of nesting if statements 3 
levels deep.

>>
>>
>> === modified file 'sql/opt_sum.cc'
>> --- a/sql/opt_sum.cc    2010-05-12 16:10:33 +0000
>> +++ b/sql/opt_sum.cc    2010-05-28 07:42:14 +0000
>> @@ -126,7 +126,18 @@ static int get_index_min_value(TABLE *ta
>>         key. If read fails, and we're looking for a MIN() value for a
>>         nullable column, test if there is an exact match for the key.
>>       */
>> -    if (!(range_fl&  NEAR_MIN))
>> +    if (prefix_len == ref->key_length)
>> +      /*
>> +         In the special case when the argument to the MIN function 
>> is also
>> +         bound to a constant, i.e. SELECT MIN(a) ... WHERE a 
>> =<constant>, the
>> +         prefix is the entire ref key and all we need to do is try 
>> to read
>> +         once using that key.
>> +      */
>> +      error= table->file->index_read_map(table->record[0],
>> +                                         ref->key_buff,
>> +                                         
>> make_prev_keypart_map(ref->key_parts),
>> +                                         HA_READ_KEY_EXACT);
>> +    else if (!(range_fl&  NEAR_MIN))
>>         /*
>>            Closed interval: Either The MIN argument is non-nullable, or
>>            we have a>= predicate for the MIN argument.
> It seems to me that problem here is in matching_cond() function
> (which is called from find_key_for_maxmin()) which can not
> determine impossible condition.
> Re matching_cond(),
> we twice process the same key_part. At first step we store
> appropriate value into the field, at second step we can check
> if this value corresponds with current operation.
This is because matching_cond iterates (recursively) over the AND 
branches. So a keypart may be visited twice.
>
> Please also check the fix with RQG.
Sure, which grammar and where can I find it?

Best Regards

Martin
Thread
bzr commit into mysql-5.1-bugteam branch (martin.hansson:3387) Bug#53859Martin Hansson28 May
Re: bzr commit into mysql-5.1-bugteam branch (martin.hansson:3387)Bug#53859Sergey Glukhov2 Jun
  • Re: bzr commit into mysql-5.1-bugteam branch (martin.hansson:3387)Bug#53859Martin Hansson3 Jun