From 0823afc8bf160e03ce722c5e3cef21024b63b99a Mon Sep 17 00:00:00 2001 From: Martin Hansson Date: Fri, 11 Jun 2010 09:38:29 +0200 Subject: [PATCH] Bug#53859: Valgrind: opt_sum_query(TABLE_LIST*, List&, 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. --- mysql-test/r/group_min_max.result | 13 ++++++++++ mysql-test/t/group_min_max.test | 15 +++++++++++ sql/opt_sum.cc | 41 +++++++++++++++++++++---------- 3 files changed, 56 insertions(+), 13 deletions(-) diff --git a/mysql-test/r/group_min_max.result b/mysql-test/r/group_min_max.result index 01f27a498ef..6fef66b9d93 100644 --- a/mysql-test/r/group_min_max.result +++ b/mysql-test/r/group_min_max.result @@ -2767,4 +2767,17 @@ SELECT MIN( a ) FROM t1 WHERE a IS NULL; MIN( a ) NULL DROP TABLE t1; +# +# Bug#53859: Valgrind: opt_sum_query(TABLE_LIST*, List&, 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 diff --git a/mysql-test/t/group_min_max.test b/mysql-test/t/group_min_max.test index b06f0c09fc5..8ab7e1c9cb4 100644 --- a/mysql-test/t/group_min_max.test +++ b/mysql-test/t/group_min_max.test @@ -1085,4 +1085,19 @@ INSERT INTO t1 VALUES (1), (2), (3); --source include/min_null_cond.inc DROP TABLE t1; +--echo # +--echo # Bug#53859: Valgrind: opt_sum_query(TABLE_LIST*, List&, 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 diff --git a/sql/opt_sum.cc b/sql/opt_sum.cc index 666485fcfa2..b20a0c4fcbe 100644 --- a/sql/opt_sum.cc +++ b/sql/opt_sum.cc @@ -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 2) there is a > predicate on it, nullability is irrelevant. 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], ref->key_buff, make_prev_keypart_map(ref->key_parts), @@ -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, uint *prefix_len) { + DBUG_ENTER("matching_cond"); if (!cond) - return 1; + DBUG_RETURN(TRUE); Field *field= field_part->field; if (!(cond->used_tables() & field->table->map)) { /* Condition doesn't restrict the used table */ - return 1; + DBUG_RETURN(TRUE); } if (cond->type() == Item::COND_ITEM) { if (((Item_cond*) cond)->functype() == Item_func::COND_OR_FUNC) - return 0; + DBUG_RETURN(FALSE); /* AND */ List_iterator_fast li(*((Item_cond*) cond)->argument_list()); @@ -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, key_part_used, range_fl, prefix_len)) - return 0; + DBUG_RETURN(FALSE); } - return 1; + DBUG_RETURN(TRUE); } 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 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, eq_type= 1; break; default: - return 0; // Can't optimize function + DBUG_RETURN(FALSE); // Can't optimize function } Item *args[3]; @@ -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 */ if (!simple_pred((Item_func*) cond, args, &inv)) - return 0; + DBUG_RETURN(FALSE); if (!is_null_safe_eq && !is_null && (args[1]->is_null() || (between && args[2]->is_null()))) - return FALSE; + DBUG_RETURN(FALSE); if (inv && !eq_type) less_fl= 1-less_fl; // Convert '<' -> '>' (etc) @@ -675,14 +679,14 @@ static bool matching_cond(bool max_fl, TABLE_REF *ref, KEY *keyinfo, { 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)) break; // Found a part of the key for the field } bool is_field_part= part == field_part; if (!(is_field_part || eq_type)) - return 0; + DBUG_RETURN(FALSE); key_part_map org_key_part_used= *key_part_used; if (eq_type || between || max_fl == less_fl) @@ -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); } + 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 || (is_field_part && (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, { if ((!is_null && !cond->val_int()) || (is_null && !test(part->field->is_null()))) - return 0; // Impossible test + DBUG_RETURN(FALSE); // Impossible test } else if (is_field_part) *range_fl&= ~(max_fl ? NO_MIN_RANGE : NO_MAX_RANGE); - return 1; + DBUG_RETURN(TRUE); }