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.

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>&, 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>&, 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<Item> 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);  
 }