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
