A fix for Bug#5748 "Prepared statement with BETWEEN and bigint values

crashes mysqld": implementation for a generic item tree modifications
registry. Every item tree modification which should be rolled back for
subsequent execution of a prepared statement or stored procedure should
be saved in the registry. All such modifications are rolled back at once
during cleanup stage of PS.
Actual fix for the bug just adds a call to register modifications to
convert_constant_item.
Post review fixes implemented.


mysql-test/r/ps.result:
  A fix for bug#5748, test results fixed.
mysql-test/t/ps.test:
  A test case for Bug#5748 "Prepared statement with BETWEEN and bigint 
  values crashes mysqld"
sql/item.cc:
  Fix for Bug#5748 "Prepared statement with BETWEEN and bigint values
  crashes mysqld":
  First step in removing up item-specific cleanups: now all such
  tree modifications should be done using the genericm mechanism implemented
  in this changeset.
sql/item.h:
  Fix for Bug#5748 "Prepared statement with BETWEEN and bigint values
  crashes mysqld": no need for an item-specific change record any more.
sql/item_cmpfunc.cc:
  A fix for Bug#5748 "Prepared statement with BETWEEN and bigint 
  values crashes mysqld": register item tree transformation performed by
  convert_constant_item.
sql/sql_class.cc:
  Implementation for item tree transformations registry.
sql/sql_class.h:
  Declarations, necessary for the tree transformations registry.
sql/sql_parse.cc:
  Assert that the item tree transformations registry is not used for 
  conventional execution.
sql/sql_prepare.cc:
  Use of the item tree modifications registry in prepared statements:
  rollback all modifications in the end of statement prepare and execute.
  Also we now always set thd->current_arena to be able to determine that
  this is an execution of prepared statement inside the registry code.
tests/client_test.c:
  A typo fixed.

mysql-test/r/ps.result

@@ -297,3 +297,14 @@ execute stmt;
 'abc' like convert('abc' using utf8)
 1
 deallocate prepare stmt;
+create table t1 ( a bigint );
+prepare stmt from 'select a from t1 where a between ? and ?';
+set @a=1;
+execute stmt using @a, @a;
+a
+execute stmt using @a, @a;
+a
+execute stmt using @a, @a;
+a
+drop table t1;
+deallocate prepare stmt;

mysql-test/t/ps.test

@@ -314,3 +314,18 @@ prepare stmt from "select 'abc' like convert('abc' using utf8)";
 execute stmt;
 execute stmt;
 deallocate prepare stmt;
+
+# 
+# BUG#5748 "Prepared statement with BETWEEN and bigint values crashes
+# mysqld". Just another place where an item tree modification must be 
+# rolled back.
+# 
+create table t1 ( a bigint );
+prepare stmt from 'select a from t1 where a between ? and ?';
+set @a=1;
+execute stmt using @a, @a;
+execute stmt using @a, @a;
+execute stmt using @a, @a;
+drop table t1;
+deallocate prepare stmt;
+

sql/item.cc

@@ -106,7 +106,7 @@ void Item::print_item_w_name(String *str)
 Item_ident::Item_ident(const char *db_name_par,const char *table_name_par,
 		       const char *field_name_par)
   :orig_db_name(db_name_par), orig_table_name(table_name_par), 
-   orig_field_name(field_name_par), changed_during_fix_field(0), 
+   orig_field_name(field_name_par),
    db_name(db_name_par), table_name(table_name_par), 
    field_name(field_name_par), cached_field_index(NO_CACHED_FIELD_INDEX), 
    cached_table(0), depended_from(0)
@@ -120,7 +120,6 @@ Item_ident::Item_ident(THD *thd, Item_ident *item)
    orig_db_name(item->orig_db_name),
    orig_table_name(item->orig_table_name), 
    orig_field_name(item->orig_field_name),
-   changed_during_fix_field(0),
    db_name(item->db_name),
    table_name(item->table_name),
    field_name(item->field_name),
@@ -137,11 +136,6 @@ void Item_ident::cleanup()
 		       table_name, orig_table_name,
 		       field_name, orig_field_name));
   Item::cleanup();
-  if (changed_during_fix_field)
-  {
-    *changed_during_fix_field= this;
-    changed_during_fix_field= 0;
-  }
   db_name= orig_db_name; 
   table_name= orig_table_name;
   field_name= orig_field_name;
@@ -1340,10 +1334,10 @@ bool Item_field::fix_fields(THD *thd, TABLE_LIST *tables, Item **ref)
 
 	Item_ref *rf;
 	*ref= rf= new Item_ref(last->ref_pointer_array + counter,
-			       ref,
+			       0,
 			       (char *)table_name,
 			       (char *)field_name);
-	register_item_tree_changing(ref);
+        thd->register_item_tree_change(ref, this, &thd->mem_root);
 	if (!rf)
 	  return 1;
 	/*
@@ -1362,7 +1356,8 @@ bool Item_field::fix_fields(THD *thd, TABLE_LIST *tables, Item **ref)
 	if (last->having_fix_field)
 	{
 	  Item_ref *rf;
-	  *ref= rf= new Item_ref(ref, *ref,
+          thd->register_item_tree_change(ref, *ref, &thd->mem_root);
+	  *ref= rf= new Item_ref(ref, 0,
 				 (where->db[0]?where->db:0), 
 				 (char *)where->alias,
 				 (char *)field_name);
@@ -2003,7 +1998,7 @@ bool Item_ref::fix_fields(THD *thd,TABLE_LIST *tables, Item **reference)
 	Item_field* fld;
 	if (!((*reference)= fld= new Item_field(tmp)))
 	  return 1;
-	register_item_tree_changing(reference);
+	thd->register_item_tree_change(reference, this, &thd->mem_root);
 	mark_as_dependent(thd, last, thd->lex->current_select, fld);
 	return 0;
       }

sql/item.h

@@ -317,7 +317,6 @@ class Item_ident :public Item
   const char *orig_db_name;
   const char *orig_table_name;
   const char *orig_field_name;
-  Item **changed_during_fix_field;
 public:
   const char *db_name;
   const char *table_name;
@@ -340,8 +339,6 @@ class Item_ident :public Item
   Item_ident(THD *thd, Item_ident *item);
   const char *full_name() const;
   void cleanup();
-  void register_item_tree_changing(Item **ref)
-    { changed_during_fix_field= ref; }
   bool remove_dependence_processor(byte * arg);
 };
 

sql/item_cmpfunc.cc

@@ -145,7 +145,7 @@ void Item_func_not_all::print(String *str)
   1	Item was replaced with an integer version of the item
 */
 
-static bool convert_constant_item(Field *field, Item **item)
+static bool convert_constant_item(THD *thd, Field *field, Item **item)
 {
   if ((*item)->const_item())
   {
@@ -153,7 +153,10 @@ static bool convert_constant_item(Field *field, Item **item)
     {
       Item *tmp=new Item_int_with_ref(field->val_int(), *item);
       if (tmp)
+      {
+        thd->register_item_tree_change(item, *item, &thd->mem_root);
 	*item=tmp;
+      }
       return 1;					// Item was replaced
     }
   }
@@ -164,6 +167,7 @@ static bool convert_constant_item(Field *field, Item **item)
 void Item_bool_func2::fix_length_and_dec()
 {
   max_length= 1;				     // Function returns 0 or 1
+  THD *thd= current_thd;
 
   /*
     As some compare functions are generated after sql_yacc,
@@ -199,7 +203,6 @@ void Item_bool_func2::fix_length_and_dec()
         !coll.set(args[0]->collation, args[1]->collation, TRUE))
     {
       Item* conv= 0;
-      THD *thd= current_thd;
       Item_arena *arena= thd->current_arena, backup;
       strong= coll.strong;
       weak= strong ? 0 : 1;
@@ -245,7 +248,7 @@ void Item_bool_func2::fix_length_and_dec()
     Field *field=((Item_field*) args[0])->field;
     if (field->can_be_compared_as_longlong())
     {
-      if (convert_constant_item(field,&args[1]))
+      if (convert_constant_item(thd, field,&args[1]))
       {
 	cmp.set_cmp_func(this, tmp_arg, tmp_arg+1,
 			 INT_RESULT);		// Works for all types.
@@ -258,7 +261,7 @@ void Item_bool_func2::fix_length_and_dec()
     Field *field=((Item_field*) args[1])->field;
     if (field->can_be_compared_as_longlong())
     {
-      if (convert_constant_item(field,&args[0]))
+      if (convert_constant_item(thd, field,&args[0]))
       {
 	cmp.set_cmp_func(this, tmp_arg, tmp_arg+1,
 			 INT_RESULT); // Works for all types.
@@ -836,6 +839,7 @@ longlong Item_func_interval::val_int()
 void Item_func_between::fix_length_and_dec()
 {
    max_length= 1;
+   THD *thd= current_thd;
 
   /*
     As some compare functions are generated after sql_yacc,
@@ -858,9 +862,9 @@ void Item_func_between::fix_length_and_dec()
     Field *field=((Item_field*) args[0])->field;
     if (field->can_be_compared_as_longlong())
     {
-      if (convert_constant_item(field,&args[1]))
+      if (convert_constant_item(thd, field,&args[1]))
 	cmp_type=INT_RESULT;			// Works for all types.
-      if (convert_constant_item(field,&args[2]))
+      if (convert_constant_item(thd, field,&args[2]))
 	cmp_type=INT_RESULT;			// Works for all types.
     }
   }

sql/sql_class.cc

@@ -698,6 +698,54 @@ void THD::close_active_vio()
 #endif
 
 
+struct Item_change_record: public ilink
+{
+  Item **place;
+  Item *old_value;
+  /* Placement new was hidden by `new' in ilink (TODO: check): */
+  static void *operator new(unsigned int size, void *mem) { return mem; }
+};
+
+
+/*
+  Register an item tree tree transformation, performed by the query
+  optimizer. We need a pointer to runtime_memroot because it may be !=
+  thd->mem_root (due to possible set_n_backup_item_arena called for thd).
+*/
+
+void THD::nocheck_register_item_tree_change(Item **place, Item *old_value,
+                                            MEM_ROOT *runtime_memroot)
+{
+  Item_change_record *change;
+  /*
+    Now we use one node per change, which adds some memory overhead,
+    but still is rather fast as we use alloc_root for allocations.
+    A list of item tree changes of an average query should be short.
+  */
+  void *change_mem= alloc_root(runtime_memroot, sizeof(*change));
+  if (change_mem == 0)
+  {
+    fatal_error();
+    return;
+  }
+  change= new (change_mem) Item_change_record;
+  change->place= place;
+  change->old_value= old_value;
+  change_list.push_back(change);
+}
+
+
+void THD::rollback_item_tree_changes()
+{
+  I_List_iterator<Item_change_record> it(change_list);
+  Item_change_record *change;
+  while ((change= it++))
+    *change->place= change->old_value;
+  /* We can forget about changes memory: it's allocated in runtime memroot */
+  change_list.empty();
+}
+
+
 /*****************************************************************************
 ** Functions to provide a interface to select results
 *****************************************************************************/

sql/sql_class.h

@@ -461,6 +461,8 @@ class Item_arena
 
   inline bool is_stmt_prepare() const { return (int)state < (int)PREPARED; }
   inline bool is_first_stmt_execute() const { return state == PREPARED; }
+  inline bool is_conventional_execution() const
+  { return state == CONVENTIONAL_EXECUTION; }
   inline gptr alloc(unsigned int size) { return alloc_root(&mem_root,size); }
   inline gptr calloc(unsigned int size)
   {
@@ -640,6 +642,17 @@ class Statement_map
 };
 
 
+/*
+  A registry for item tree transformations performed during
+  query optimization. We register only those changes which require
+  a rollback to re-execute a prepared statement or stored procedure
+  yet another time.
+*/
+
+struct Item_change_record;
+typedef I_List<Item_change_record> Item_change_list;
+
+
 /*
   For each client connection we create a separate thread with THD serving as
   a thread/connection descriptor
@@ -807,6 +820,14 @@ class THD :public ilink,
 #ifdef SIGNAL_WITH_VIO_CLOSE
   Vio* active_vio;
 #endif
+  /*
+    This is to track items changed during execution of a prepared
+    statement/stored procedure. It's created by
+    register_item_tree_change() in memory root of THD, and freed in
+    rollback_item_tree_changes(). For conventional execution it's always 0.
+  */
+  Item_change_list change_list;
+
   /*
     Current prepared Item_arena if there one, or 0
   */
@@ -1031,6 +1052,16 @@ class THD :public ilink,
   }
   inline CHARSET_INFO *charset() { return variables.character_set_client; }
   void update_charset();
+
+  void register_item_tree_change(Item **place, Item *old_value,
+                                 MEM_ROOT *runtime_memroot)
+  {
+    if (!current_arena->is_conventional_execution())
+      nocheck_register_item_tree_change(place, old_value, runtime_memroot);
+  }
+  void nocheck_register_item_tree_change(Item **place, Item *old_value,
+                                             MEM_ROOT *runtime_memroot);
+  void rollback_item_tree_changes();
 };
 
 /* Flags for the THD::system_thread (bitmap) variable */

sql/sql_parse.cc

@@ -4056,6 +4056,7 @@ void mysql_parse(THD *thd, char *inBuf, uint length)
     }
     thd->proc_info="freeing items";
     thd->end_statement();
+    DBUG_ASSERT(thd->change_list.is_empty());
   }
   DBUG_VOID_RETURN;
 }

sql/sql_prepare.cc

@@ -1614,6 +1614,7 @@ int mysql_stmt_prepare(THD *thd, char *packet, uint packet_length,
   cleanup_items(stmt->free_list);
   close_thread_tables(thd);
   free_items(thd->free_list);
+  thd->rollback_item_tree_changes();
   thd->free_list= 0;
   thd->current_arena= thd;
 
@@ -1870,8 +1871,7 @@ static void execute_stmt(THD *thd, Prepared_statement *stmt,
     transformations of the query tree (i.e. negations elimination).
     This should be done permanently on the parse tree of this statement.
   */
-  if (stmt->state == Item_arena::PREPARED)
-    thd->current_arena= stmt;
+  thd->current_arena= stmt;
 
   if (!(specialflag & SPECIAL_NO_PRIOR))
     my_pthread_setprio(pthread_self(),QUERY_PRIOR);
@@ -1884,11 +1884,10 @@ static void execute_stmt(THD *thd, Prepared_statement *stmt,
   free_items(thd->free_list);
   thd->free_list= 0;
   if (stmt->state == Item_arena::PREPARED)
-  {
-    thd->current_arena= thd;
     stmt->state= Item_arena::EXECUTED;
-  }
+  thd->current_arena= thd;
   cleanup_items(stmt->free_list);
+  thd->rollback_item_tree_changes();
   reset_stmt_params(stmt);
   close_thread_tables(thd);                    // to close derived tables
   thd->set_statement(&thd->stmt_backup);

tests/client_test.c

@@ -8442,7 +8442,7 @@ static void test_subqueries_ref()
   int rc, i;
   const char *query= "SELECT a as ccc from t1 where a+1=(SELECT 1+ccc from t1 where ccc+1=a+1 and a=1)";
 
-  myheader("test_subquery_ref");
+  myheader("test_subqueries_ref");
 
   rc= mysql_query(mysql, "DROP TABLE IF EXISTS t1");
   myquery(rc);