Commit 5bace3f0 authored by Martin Hansson's avatar Martin Hansson

Bug#53859: Valgrind: opt_sum_query(TABLE_LIST*, List<Item>&,

Item*) at opt_sum.cc:305
      
Queries applying MIN/MAX functions to indexed columns are
optimized to read directly from the index if all key parts
of the index preceding the aggregated key part are bound to
constants by the WHERE clause. A prefix length is also
produced, equal to the total length of the bound key
parts. If the aggregated column itself is bound to a
constant, however, it is also included in the prefix.

Such full search keys are read as closed intervals for
reasons beyond the scope of this bug. However, the procedure
missed one case where a key part meant for use as range
endpoint was being overwritten with a NULL value destined
for equality checking. In this case the key part was
overwritten but the range flag remained, causing open
interval reading to be performed.

Bug was fixed by adding more stringent checking to the
search key building procedure (matching_cond) and never
allow overwrites of range predicates with non-range
predicates.

An assertion was added to make sure open intervals are never
used with full search keys.
parent 53b88296
...@@ -2767,4 +2767,17 @@ SELECT MIN( a ) FROM t1 WHERE a IS NULL; ...@@ -2767,4 +2767,17 @@ SELECT MIN( a ) FROM t1 WHERE a IS NULL;
MIN( a ) MIN( a )
NULL NULL
DROP TABLE t1; DROP TABLE t1;
#
# Bug#53859: Valgrind: opt_sum_query(TABLE_LIST*, List<Item>&, Item*) at
# opt_sum.cc:305
#
CREATE TABLE t1 ( a INT, KEY (a) );
INSERT INTO t1 VALUES (1), (2), (3);
SELECT MIN( a ) AS min_a
FROM t1
WHERE a > 1 AND a IS NULL
ORDER BY min_a;
min_a
NULL
DROP TABLE t1;
End of 5.1 tests End of 5.1 tests
...@@ -1085,4 +1085,19 @@ INSERT INTO t1 VALUES (1), (2), (3); ...@@ -1085,4 +1085,19 @@ INSERT INTO t1 VALUES (1), (2), (3);
--source include/min_null_cond.inc --source include/min_null_cond.inc
DROP TABLE t1; DROP TABLE t1;
--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, KEY (a) );
INSERT INTO t1 VALUES (1), (2), (3);
SELECT MIN( a ) AS min_a
FROM t1
WHERE a > 1 AND a IS NULL
ORDER BY min_a;
DROP TABLE t1;
--echo End of 5.1 tests --echo End of 5.1 tests
...@@ -142,7 +142,10 @@ static int get_index_min_value(TABLE *table, TABLE_REF *ref, ...@@ -142,7 +142,10 @@ static int get_index_min_value(TABLE *table, TABLE_REF *ref,
1) We have only MIN() and the argument column is nullable, or 1) We have only MIN() and the argument column is nullable, or
2) there is a > predicate on it, nullability is irrelevant. 2) there is a > predicate on it, nullability is irrelevant.
We need to scan the next bigger record first. We need to scan the next bigger record first.
Open interval is not used if the search key involves the last keypart,
and it would not work.
*/ */
DBUG_ASSERT(prefix_len < ref->key_length);
error= table->file->index_read_map(table->record[0], error= table->file->index_read_map(table->record[0],
ref->key_buff, ref->key_buff,
make_prev_keypart_map(ref->key_parts), make_prev_keypart_map(ref->key_parts),
...@@ -591,18 +594,19 @@ static bool matching_cond(bool max_fl, TABLE_REF *ref, KEY *keyinfo, ...@@ -591,18 +594,19 @@ static bool matching_cond(bool max_fl, TABLE_REF *ref, KEY *keyinfo,
key_part_map *key_part_used, uint *range_fl, key_part_map *key_part_used, uint *range_fl,
uint *prefix_len) uint *prefix_len)
{ {
DBUG_ENTER("matching_cond");
if (!cond) if (!cond)
return 1; DBUG_RETURN(TRUE);
Field *field= field_part->field; Field *field= field_part->field;
if (!(cond->used_tables() & field->table->map)) if (!(cond->used_tables() & field->table->map))
{ {
/* Condition doesn't restrict the used table */ /* Condition doesn't restrict the used table */
return 1; DBUG_RETURN(TRUE);
} }
if (cond->type() == Item::COND_ITEM) if (cond->type() == Item::COND_ITEM)
{ {
if (((Item_cond*) cond)->functype() == Item_func::COND_OR_FUNC) if (((Item_cond*) cond)->functype() == Item_func::COND_OR_FUNC)
return 0; DBUG_RETURN(FALSE);
/* AND */ /* AND */
List_iterator_fast<Item> li(*((Item_cond*) cond)->argument_list()); List_iterator_fast<Item> li(*((Item_cond*) cond)->argument_list());
...@@ -611,13 +615,13 @@ static bool matching_cond(bool max_fl, TABLE_REF *ref, KEY *keyinfo, ...@@ -611,13 +615,13 @@ static bool matching_cond(bool max_fl, TABLE_REF *ref, KEY *keyinfo,
{ {
if (!matching_cond(max_fl, ref, keyinfo, field_part, item, if (!matching_cond(max_fl, ref, keyinfo, field_part, item,
key_part_used, range_fl, prefix_len)) key_part_used, range_fl, prefix_len))
return 0; DBUG_RETURN(FALSE);
} }
return 1; DBUG_RETURN(TRUE);
} }
if (cond->type() != Item::FUNC_ITEM) if (cond->type() != Item::FUNC_ITEM)
return 0; // Not operator, can't optimize DBUG_RETURN(FALSE); // Not operator, can't optimize
bool eq_type= 0; // =, <=> or IS NULL bool eq_type= 0; // =, <=> or IS NULL
bool is_null_safe_eq= FALSE; // The operator is NULL safe, e.g. <=> bool is_null_safe_eq= FALSE; // The operator is NULL safe, e.g. <=>
...@@ -651,7 +655,7 @@ static bool matching_cond(bool max_fl, TABLE_REF *ref, KEY *keyinfo, ...@@ -651,7 +655,7 @@ static bool matching_cond(bool max_fl, TABLE_REF *ref, KEY *keyinfo,
eq_type= 1; eq_type= 1;
break; break;
default: default:
return 0; // Can't optimize function DBUG_RETURN(FALSE); // Can't optimize function
} }
Item *args[3]; Item *args[3];
...@@ -659,11 +663,11 @@ static bool matching_cond(bool max_fl, TABLE_REF *ref, KEY *keyinfo, ...@@ -659,11 +663,11 @@ static bool matching_cond(bool max_fl, TABLE_REF *ref, KEY *keyinfo,
/* Test if this is a comparison of a field and constant */ /* Test if this is a comparison of a field and constant */
if (!simple_pred((Item_func*) cond, args, &inv)) if (!simple_pred((Item_func*) cond, args, &inv))
return 0; DBUG_RETURN(FALSE);
if (!is_null_safe_eq && !is_null && if (!is_null_safe_eq && !is_null &&
(args[1]->is_null() || (between && args[2]->is_null()))) (args[1]->is_null() || (between && args[2]->is_null())))
return FALSE; DBUG_RETURN(FALSE);
if (inv && !eq_type) if (inv && !eq_type)
less_fl= 1-less_fl; // Convert '<' -> '>' (etc) less_fl= 1-less_fl; // Convert '<' -> '>' (etc)
...@@ -675,14 +679,14 @@ static bool matching_cond(bool max_fl, TABLE_REF *ref, KEY *keyinfo, ...@@ -675,14 +679,14 @@ static bool matching_cond(bool max_fl, TABLE_REF *ref, KEY *keyinfo,
{ {
if (part > field_part) if (part > field_part)
return 0; // Field is beyond the tested parts DBUG_RETURN(FALSE); // Field is beyond the tested parts
if (part->field->eq(((Item_field*) args[0])->field)) if (part->field->eq(((Item_field*) args[0])->field))
break; // Found a part of the key for the field break; // Found a part of the key for the field
} }
bool is_field_part= part == field_part; bool is_field_part= part == field_part;
if (!(is_field_part || eq_type)) if (!(is_field_part || eq_type))
return 0; DBUG_RETURN(FALSE);
key_part_map org_key_part_used= *key_part_used; key_part_map org_key_part_used= *key_part_used;
if (eq_type || between || max_fl == less_fl) if (eq_type || between || max_fl == less_fl)
...@@ -702,6 +706,17 @@ static bool matching_cond(bool max_fl, TABLE_REF *ref, KEY *keyinfo, ...@@ -702,6 +706,17 @@ static bool matching_cond(bool max_fl, TABLE_REF *ref, KEY *keyinfo,
*key_part_used|= (key_part_map) 1 << (part - keyinfo->key_part); *key_part_used|= (key_part_map) 1 << (part - keyinfo->key_part);
} }
if (org_key_part_used == *key_part_used &&
/*
The current search key is not being extended with a new key part. This
means that the a condition is added a key part for which there was a
previous condition. We can only overwrite such key parts in some special
cases, e.g. a > 2 AND a > 1 (here range_fl must be set to something). In
all other cases the WHERE condition is always false anyway.
*/
(eq_type || *range_fl == 0))
DBUG_RETURN(FALSE);
if (org_key_part_used != *key_part_used || if (org_key_part_used != *key_part_used ||
(is_field_part && (is_field_part &&
(between || eq_type || max_fl == less_fl) && !cond->val_int())) (between || eq_type || max_fl == less_fl) && !cond->val_int()))
...@@ -747,11 +762,11 @@ static bool matching_cond(bool max_fl, TABLE_REF *ref, KEY *keyinfo, ...@@ -747,11 +762,11 @@ static bool matching_cond(bool max_fl, TABLE_REF *ref, KEY *keyinfo,
{ {
if ((!is_null && !cond->val_int()) || if ((!is_null && !cond->val_int()) ||
(is_null && !test(part->field->is_null()))) (is_null && !test(part->field->is_null())))
return 0; // Impossible test DBUG_RETURN(FALSE); // Impossible test
} }
else if (is_field_part) else if (is_field_part)
*range_fl&= ~(max_fl ? NO_MIN_RANGE : NO_MAX_RANGE); *range_fl&= ~(max_fl ? NO_MIN_RANGE : NO_MAX_RANGE);
return 1; DBUG_RETURN(TRUE);
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment