bk commit into 4.1 tree (konstantin:1.2188) BUG#9096

[konstantin]

Below is the list of changes that have just been committed into a local
4.1 repository of kostja. When kostja does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet
  1.2188 05/04/16 19:23:11 konstantin@stripped +5 -0
  A fix and test case for Bug#9096 "select doesn't return all matched 
  records if prepared statements is used".
  This fix changes equality evaluation method of basic constants from
  by-name to by-value, thus effectively enabling use of parameter markers
  in some optimizations (constants propagation, evaluation of possible
  keys for query).

  sql/item_func.cc
    1.239 05/04/16 19:23:06 konstantin@stripped +5 -4
    Remove casts to Item_real/Item_int which didn't take
    into account Item_param (the casts were added in 4.1.8).

  sql/item.h
    1.177 05/04/16 19:23:06 konstantin@stripped +31 -11
    Change declaration of basic_const_item(): now it also widens its 
    argument from const Item * to Item * if the argument is a basic constant.
    Declare eq() for all basic constatns, as long as now they 
    are compared by value, not by name. Each constant needs its own
    comparison method.
    Declarations of Item_param methods needed to fully emulate 
    a basic constant when parameter value is set.

  sql/item.cc
    1.199 05/04/16 19:23:06 konstantin@stripped +98 -2
    Implement by-value equality evaluation of basic constants.
    This is needed to work with Item_param values. Until now
    Item_param was compared with other items by its name, which is always "?".
    The bug at hand showed up when an integer
    constant was created from one parameter marker (with value 200887 and
     name "?") and then compared by-name with another parameter marker
    (with value 860 and name "?"). True returned by this comparison resulted
    in a wrong table access method used to evaluate the query.
    Implement Item_param methods needed to emulate "basic constant" mode at 
    full.

  mysql-test/t/ps.test
    1.32 05/04/16 19:23:06 konstantin@stripped +26 -0
    A short test case for Bug#9096 "select doesn't return all matched records if
     prepared statements is used". The is enough to reproduce the
    glitch in update_ref_and_keys causing the bug to occur.

  mysql-test/r/ps.result
    1.33 05/04/16 19:23:06 konstantin@stripped +25 -0
    Test results for the test case for Bug#9096

# This is a BitKeeper patch.  What follows are the unified diffs for the
# set of deltas contained in the patch.  The rest of the patch, the part
# that BitKeeper cares about, is below these diffs.
# User:	konstantin
# Host:	dragonfly.local
# Root:	/media/sda1/mysql/mysql-4.1-9096

--- 1.198/sql/item.cc	2005-04-10 00:39:17 -07:00
+++ 1.199/sql/item.cc	2005-04-16 19:23:06 -07:00
@@ -200,8 +200,13 @@
 
 bool Item::eq(const Item *item, bool binary_cmp) const
 {
+  /*
+    Note, that this is never TRUE if item is a Item_param:
+    for all basic constants we have special checks, and Item_param's
+    type() can be only among basic constant types.
+  */
   return type() == item->type() && name && item->name &&
-    !my_strcasecmp(system_charset_info,name,item->name);
+         !my_strcasecmp(system_charset_info,name,item->name);
 }
 
 
@@ -254,7 +259,7 @@
 
 bool Item_string::eq(const Item *item, bool binary_cmp) const
 {
-  if (type() == item->type())
+  if (type() == item->type() && item->basic_const_item())
   {
     if (binary_cmp)
       return !stringcmp(&str_value, &item->str_value);
@@ -1356,6 +1361,65 @@
   return 0;
 }
 
+
+Item *Item_param::basic_const_item() const
+{
+  if (state == NO_VALUE || state == TIME_VALUE)
+    return 0;
+  return (Item*) this;
+}
+
+Item *
+Item_param::new_item()
+{
+  /* see comments in the header file */
+  switch (state) {
+  case NULL_VALUE:
+    return new Item_null(name);
+  case INT_VALUE:
+    return new Item_int(name, value.integer, max_length);
+  case REAL_VALUE:
+    return new Item_real(name, value.real, decimals, max_length);
+  case STRING_VALUE:
+  case LONG_DATA_VALUE:
+    return new Item_string(name, str_value.c_ptr_quick(), str_value.length(),
+                           str_value.charset());
+  case TIME_VALUE:
+    break;
+  case NO_VALUE:
+  default:
+    DBUG_ASSERT(0);
+  };
+  return 0;
+}
+
+
+bool
+Item_param::eq(const Item *arg, bool binary_cmp) const
+{
+  Item *item= arg->basic_const_item();
+  if (!basic_const_item() || !item || item->type() != type())
+    return FALSE;
+
+  switch (state) {
+  case NULL_VALUE:
+    return TRUE;
+  case INT_VALUE:
+    return value.integer == item->val_int() &&
+           unsigned_flag == item->unsigned_flag;
+  case REAL_VALUE:
+    return value.real == item->val();
+  case STRING_VALUE:
+  case LONG_DATA_VALUE:
+    if (binary_cmp)
+      return !stringcmp(&str_value, &item->str_value);
+    return !sortcmp(&str_value, &item->str_value, collation.collation);
+  default:
+    break;
+  }
+  return FALSE;
+}
+
 /* End of Item_param related */
 
 
@@ -1937,6 +2001,17 @@
   return field->store(nr);
 }
 
+
+bool Item_int::eq(const Item *arg, bool binary_cmp) const
+{
+  Item *item= arg->basic_const_item();
+  /* No need to check for null value as basic constant can't be NULL */
+  if (item && item->type() == type())
+    return item->val_int() == value && item->unsigned_flag ==
unsigned_flag;
+  return FALSE;
+}
+
+
 Item_num *Item_uint::neg()
 {
   return new Item_real(name, - ((double) value), 0, max_length);
@@ -1951,6 +2026,15 @@
   return field->store(nr);
 }
 
+
+bool Item_real::eq(const Item *arg, bool binary_cmp) const
+{
+  Item *item= arg->basic_const_item();
+  if (item && item->type() == type())
+    return item->val() == value;
+  return FALSE;
+}
+
 /****************************************************************************
 ** varbinary item
 ** In string context this is a binary string
@@ -2016,6 +2100,18 @@
   return error;
 }
 
+
+bool Item_varbinary::eq(const Item *arg, bool binary_cmp) const
+{
+  Item *item= arg->basic_const_item();
+  if (item && item->type() == type())
+  {
+    if (binary_cmp)
+      return !stringcmp(&str_value, &item->str_value);
+    return !sortcmp(&str_value, &item->str_value, collation.collation);
+  }
+  return FALSE;
+}
 
 /*
   Pack data in buffer for sending

--- 1.176/sql/item.h	2005-03-31 00:47:30 -08:00
+++ 1.177/sql/item.h	2005-04-16 19:23:06 -07:00
@@ -233,10 +233,11 @@
   */
   virtual table_map not_null_tables() const { return used_tables(); }
   /*
-    Returns true if this is a simple constant item like an integer, not
-    a constant expression
+    Returns non-const pointer to this item if this is a simple constant item
+    like an integer and not a constant expression, 0 otherwise.
+    Used in the optimizer to propagate basic constants.
   */
-  virtual bool basic_const_item() const { return 0; }
+  virtual Item *basic_const_item() const { return 0; }
   /* cloning of constant items (0 if it is not const) */
   virtual Item *new_item() { return 0; }
   virtual cond_result eq_cmp_result() const { return COND_OK; }
@@ -463,7 +464,7 @@
   enum_field_types field_type() const   { return MYSQL_TYPE_NULL; }
   // to prevent drop fixed flag (no need parent cleanup call)
   void cleanup() {}
-  bool basic_const_item() const { return 1; }
+  Item *basic_const_item() const { return (Item*) this; }
   Item *new_item() { return new Item_null(name); }
   bool is_null() { return 1; }
   void print(String *str) { str->append("NULL", 4); }
@@ -581,7 +582,6 @@
 
   bool convert_str_value(THD *thd);
 
-  Item *new_item() { return new Item_param(pos_in_query); }
   /*
     If value for parameter was not set we treat it as non-const
     so noone will use parameters value in fix_fields still
@@ -590,12 +590,29 @@
   virtual table_map used_tables() const
   { return state != NO_VALUE ? (table_map)0 : PARAM_TABLE_BIT; }
   void print(String *str) { str->append('?'); }
-  /* parameter never equal to other parameter of other item */
-  bool eq(const Item *item, bool binary_cmp) const { return 0; }
   bool is_null()
   { DBUG_ASSERT(state != NO_VALUE); return state == NULL_VALUE; }
+  Item *basic_const_item() const;
+  /*
+    This method is used to make a copy of a basic constant item when
+    propagating constants in the optimizer. The reason to create a new
+    item and not use the existing one is not precisely known (2005/04/16).
+    Probably we are trying to preserve tree structure of items, in other
+    words, avoid pointing at one item from two different nodes of the tree.
+    Return a new basic constant item if parameter value is a basic
+    constant, assert otherwise. This method is called only if
+    basic_const_item returned TRUE.
+  */
+  Item *new_item();
+  /*
+    Implement by-value equality evaluation if parameter value
+    is set and is a basic constant (integer, real or string).
+    Otherwise return FALSE.
+  */
+  bool eq(const Item *item, bool binary_cmp) const;
 };
 
+
 class Item_int :public Item_num
 {
 public:
@@ -616,12 +633,13 @@
   double val() { DBUG_ASSERT(fixed == 1); return (double) value; }
   String *val_str(String*);
   int save_in_field(Field *field, bool no_conversions);
-  bool basic_const_item() const { return 1; }
+  Item *basic_const_item() const { return (Item*) this; }
   Item *new_item() { return new Item_int(name,value,max_length); }
   // to prevent drop fixed flag (no need parent cleanup call)
   void cleanup() {}
   void print(String *str);
   Item_num *neg() { value= -value; return this; }
+  bool eq(const Item *, bool binary_cmp) const;
 };
 
 
@@ -672,11 +690,12 @@
     return (longlong) (value+(value > 0 ? 0.5 : -0.5));
   }
   String *val_str(String*);
-  bool basic_const_item() const { return 1; }
+  Item *basic_const_item() const { return (Item*) this; }
   // to prevent drop fixed flag (no need parent cleanup call)
   void cleanup() {}
   Item *new_item() { return new Item_real(name,value,decimals,max_length); }
   Item_num *neg() { value= -value; return this; }
+  bool eq(const Item *, bool binary_cmp) const;
 };
 
 
@@ -746,7 +765,7 @@
   int save_in_field(Field *field, bool no_conversions);
   enum Item_result result_type () const { return STRING_RESULT; }
   enum_field_types field_type() const { return MYSQL_TYPE_STRING; }
-  bool basic_const_item() const { return 1; }
+  Item *basic_const_item() const { return (Item*) this; }
   bool eq(const Item *item, bool binary_cmp) const;
   Item *new_item() 
   {
@@ -803,13 +822,14 @@
   double val()
     { DBUG_ASSERT(fixed == 1); return (double) Item_varbinary::val_int(); }
   longlong val_int();
-  bool basic_const_item() const { return 1; }
+  Item *basic_const_item() const { return (Item*) this; }
   String *val_str(String*) { DBUG_ASSERT(fixed == 1); return &str_value; }
   int save_in_field(Field *field, bool no_conversions);
   enum Item_result result_type () const { return STRING_RESULT; }
   enum_field_types field_type() const { return MYSQL_TYPE_STRING; }
   // to prevent drop fixed flag (no need parent cleanup call)
   void cleanup() {}
+  bool eq(const Item *item, bool binary_cmp) const;
 };
 
 

--- 1.238/sql/item_func.cc	2005-03-28 01:01:53 -08:00
+++ 1.239/sql/item_func.cc	2005-04-16 19:23:06 -07:00
@@ -790,10 +790,12 @@
     maximum number of bytes real or integer may require. Note that all 
     constants are non negative so we don't need to account for removed '-'.
     B) argument returns a string.
+    Use val() to get value as arg_type doesn't mean that item is
+    Item_int or Item_real due to existence of Item_param.
   */
   if (arg_result == STRING_RESULT || 
-      (arg_type == REAL_ITEM && ((Item_real*)args[0])->value >= 0) ||
-      (arg_type == INT_ITEM && ((Item_int*)args[0])->value > 0))
+      (arg_type == REAL_ITEM && args[0]->val() >= 0) ||
+      (arg_type == INT_ITEM && args[0]->val_int() > 0))
     max_length++;
 
   if (args[0]->result_type() == INT_RESULT)
@@ -809,8 +811,7 @@
       signed integers)
     */
     if (args[0]->type() != INT_ITEM ||
-	((ulonglong) ((Item_uint*) args[0])->value <=
-	 (ulonglong) LONGLONG_MIN))
+	(((ulonglong) args[0]->val_int()) <= (ulonglong) LONGLONG_MIN))
       hybrid_type= INT_RESULT;
   }
 }

--- 1.32/mysql-test/r/ps.result	2005-03-02 08:00:43 -08:00
+++ 1.33/mysql-test/r/ps.result	2005-04-16 19:23:06 -07:00
@@ -494,3 +494,28 @@
 FOUND_ROWS()
 2
 deallocate prepare stmt;
+drop table if exists t1;
+Warnings:
+Note	1051	Unknown table 't1'
+create table t1 (c1 int(11) not null, c2 int(11) not null,
+primary key  (c1,c2), key c2 (c2), key c1 (c1));
+insert into t1 values (200887, 860);
+insert into t1 values (200887, 200887);
+select * from t1 where (c1=200887 and c2=200887) or c2=860;
+c1	c2
[phone]
[phone]
+prepare stmt from
+"select * from t1 where (c1=200887 and c2=200887) or c2=860";
+execute stmt;
+c1	c2
[phone]
[phone]
+prepare stmt from
+"select * from t1 where (c1=200887 and c2=?) or c2=?";
+set @a=200887, @b=860;
+execute stmt using @a, @b;
+c1	c2
[phone]
[phone]
+deallocate prepare stmt;

--- 1.31/mysql-test/t/ps.test	2005-03-02 08:00:43 -08:00
+++ 1.32/mysql-test/t/ps.test	2005-04-16 19:23:06 -07:00
@@ -496,3 +496,29 @@
 execute stmt;                                                                   
 SELECT FOUND_ROWS();                                                            
 deallocate prepare stmt;
+
+#
+# Bug#9096 "select doesn't return all matched records if prepared statements
+# is used"
+# The bug was is bad co-operation of the optimizer's algorithm which determines
+# which keys can be used to execute a query, constants propagation
+# part of the optimizer and parameter markers used by prepared statements.
+
+drop table if exists t1;
+create table t1 (c1 int(11) not null, c2 int(11) not null,
+             primary key  (c1,c2), key c2 (c2), key c1 (c1));
+
+insert into t1 values (200887, 860);
+insert into t1 values (200887, 200887);
+
+select * from t1 where (c1=200887 and c2=200887) or c2=860;
+
+prepare stmt from
+"select * from t1 where (c1=200887 and c2=200887) or c2=860";
+execute stmt;
+prepare stmt from
+"select * from t1 where (c1=200887 and c2=?) or c2=?";
+set @a=200887, @b=860;
+# this query did not return all matching rows
+execute stmt using @a, @b;
+deallocate prepare stmt;