BUG#21166: Prepared statement causes signal 11 on second execution

Changes in an item tree done by optimizer weren't properly
registered and went unnoticed, which resulted in preliminary freeing
of used memory.


mysql-test/r/ps.result:
  Add result for bug#21166: Prepared statement causes signal 11
  on second execution.
mysql-test/t/ps.test:
  Add test case for bug#21166: Prepared statement causes signal 11
  on second execution.
sql/item.cc:
  Move Item::transform() and Item_default_value::transform() from
  item.h here and use THD::change_item_tree() instead of plain
  assignment.
  Change Item_field::set_no_const_sub() to be used with Item::walk()
  instead of Item::transform().
sql/item.h:
  Move definition of Item::transform() and Item_default_value::transform()
  to item.cc.
  Change Item::set_no_const_sub() to be used with Item::walk()
  instead of Item::transform().
sql/item_cmpfunc.cc:
  Use Item::walk() to execute Item::set_no_const_sub().
  Use THD::change_item_tree() instead of plain assignment.
sql/item_func.cc:
  Add assert and comment to Item_func::traverse_cond().
sql/item_row.cc:
  Use THD::change_item_tree() instead of plain assignment.
sql/item_strfunc.cc:
  Move Item_func_make_set::transform() from item_strfunc.h here and use
  THD::change_item_tree() instead of plain assignment.
sql/item_strfunc.h:
  Move definition of Item_func_make_set::transform() to item_strfunc.cc.

mysql-test/r/ps.result

@@ -1297,3 +1297,18 @@ ERROR 3D000: No database selected
 create temporary table t1 (i int);
 ERROR 3D000: No database selected
 use test;
+DROP TABLE IF EXISTS t1, t2, t3;
+CREATE TABLE t1 (i BIGINT, j BIGINT);
+CREATE TABLE t2 (i BIGINT);
+CREATE TABLE t3 (i BIGINT, j BIGINT);
+PREPARE stmt FROM "SELECT * FROM t1 JOIN t2 ON (t2.i = t1.i)
+                   LEFT JOIN t3 ON ((t3.i, t3.j) = (t1.i, t1.j))
+                   WHERE t1.i = ?";
+SET @a= 1;
+EXECUTE stmt USING @a;
+i	j	i	i	j
+EXECUTE stmt USING @a;
+i	j	i	i	j
+DEALLOCATE PREPARE stmt;
+DROP TABLE IF EXISTS t1, t2, t3;
+End of 5.0 tests.

mysql-test/t/ps.test

@@ -1329,4 +1329,33 @@ create temporary table t1 (i int);
 # Restore the old environemnt
 #
 use test;
-# End of 5.0 tests
+
+
+#
+# BUG#21166: Prepared statement causes signal 11 on second execution
+#
+# Changes in an item tree done by optimizer weren't properly
+# registered and went unnoticed, which resulted in preliminary freeing
+# of used memory.
+#
+--disable_warnings
+DROP TABLE IF EXISTS t1, t2, t3;
+--enable_warnings
+
+CREATE TABLE t1 (i BIGINT, j BIGINT);
+CREATE TABLE t2 (i BIGINT);
+CREATE TABLE t3 (i BIGINT, j BIGINT);
+
+PREPARE stmt FROM "SELECT * FROM t1 JOIN t2 ON (t2.i = t1.i)
+                   LEFT JOIN t3 ON ((t3.i, t3.j) = (t1.i, t1.j))
+                   WHERE t1.i = ?";
+
+SET @a= 1;
+EXECUTE stmt USING @a;
+EXECUTE stmt USING @a;
+
+DEALLOCATE PREPARE stmt;
+DROP TABLE IF EXISTS t1, t2, t3;
+
+
+--echo End of 5.0 tests.

sql/item.cc

@@ -420,6 +420,50 @@ void Item::rename(char *new_name)
 }
 
 
+/*
+  transform() - traverse item tree possibly transforming it (replacing
+                items)
+
+  SYNOPSIS
+    transform()
+      transformer    functor that performs transformation of a subtree
+      arg            opaque argument passed to the functor
+
+  DESCRIPTION
+    This function is designed to ease transformation of Item trees.
+
+    Re-execution note: every such transformation is registered for
+    rollback by THD::change_item_tree() and is rolled back at the end
+    of execution by THD::rollback_item_tree_changes().
+
+    Therefore:
+
+      - this function can not be used at prepared statement prepare
+        (in particular, in fix_fields!), as only permanent
+        transformation of Item trees are allowed at prepare.
+
+      - the transformer function shall allocate new Items in execution
+        memory root (thd->mem_root) and not anywhere else: allocated
+        items will be gone in the end of execution.
+
+    If you don't need to transform an item tree, but only traverse
+    it, please use Item::walk() instead.
+
+
+  RETURN
+    Returns pointer to the new subtree root.  THD::change_item_tree()
+    should be called for it if transformation took place, i.e. if
+    pointer to newly allocated item is returned.
+*/
+
+Item* Item::transform(Item_transformer transformer, byte *arg)
+{
+  DBUG_ASSERT(!current_thd->is_stmt_prepare());
+
+  return (this->*transformer)(arg);
+}
+
+
 Item_ident::Item_ident(Name_resolution_context *context_arg,
                        const char *db_name_arg,const char *table_name_arg,
 		       const char *field_name_arg)
@@ -3788,11 +3832,11 @@ Item *Item_field::equal_fields_propagator(byte *arg)
   See comments in Arg_comparator::set_compare_func() for details
 */
 
-Item *Item_field::set_no_const_sub(byte *arg)
+bool Item_field::set_no_const_sub(byte *arg)
 {
   if (field->charset() != &my_charset_bin)
     no_const_subst=1;
-  return this;
+  return FALSE;
 }
 
 
@@ -5294,6 +5338,30 @@ int Item_default_value::save_in_field(Field *field_arg, bool no_conversions)
 }
 
 
+/* 
+   This method like the walk method traverses the item tree, but at
+   the same time it can replace some nodes in the tree
+*/ 
+Item *Item_default_value::transform(Item_transformer transformer, byte *args)
+{
+  DBUG_ASSERT(!current_thd->is_stmt_prepare());
+
+  Item *new_item= arg->transform(transformer, args);
+  if (!new_item)
+    return 0;
+
+  /*
+    THD::change_item_tree() should be called only if the tree was
+    really transformed, i.e. when a new item has been created.
+    Otherwise we'll be allocating a lot of unnecessary memory for
+    change records at each execution.
+  */
+  if (arg != new_item)
+    current_thd->change_item_tree(&arg, new_item);
+  return (this->*transformer)(args);
+}
+
+
 bool Item_insert_value::eq(const Item *item, bool binary_cmp) const
 {
   return item->type() == INSERT_VALUE_ITEM &&

sql/item.h

@@ -734,10 +734,7 @@ public:
     return (this->*processor)(arg);
   }
 
-  virtual Item* transform(Item_transformer transformer, byte *arg)
-  {
-    return (this->*transformer)(arg);
-  }
+  virtual Item* transform(Item_transformer transformer, byte *arg);
 
    virtual void traverse_cond(Cond_traverser traverser,
                               void *arg, traverse_order order)
@@ -755,7 +752,7 @@ public:
   virtual bool is_expensive_processor(byte *arg) { return 0; }
 
   virtual Item *equal_fields_propagator(byte * arg) { return this; }
-  virtual Item *set_no_const_sub(byte *arg) { return this; }
+  virtual bool set_no_const_sub(byte *arg) { return FALSE; }
   virtual Item *replace_equal_field(byte * arg) { return this; }
 
   /*
@@ -1255,7 +1252,7 @@ public:
   }
   Item_equal *find_item_equal(COND_EQUAL *cond_equal);
   Item *equal_fields_propagator(byte *arg);
-  Item *set_no_const_sub(byte *arg);
+  bool set_no_const_sub(byte *arg);
   Item *replace_equal_field(byte *arg);
   inline uint32 max_disp_length() { return field->max_length(); }
   Item_field *filed_for_view_update() { return this; }
@@ -2116,18 +2113,7 @@ public:
       (this->*processor)(args);
   }
 
-  /* 
-     This method like the walk method traverses the item tree, but
-     at the same time it can replace some nodes in the tree
-  */ 
-  Item *transform(Item_transformer transformer, byte *args)
-  {
-    Item *new_item= arg->transform(transformer, args);
-    if (!new_item)
-      return 0;
-    arg= new_item;
-    return (this->*transformer)(args);
-  }
+  Item *transform(Item_transformer transformer, byte *args);
 };
 
 /*

sql/item_cmpfunc.cc

@@ -500,8 +500,8 @@ int Arg_comparator::set_compare_func(Item_bool_func2 *item, Item_result type)
         which would be transformed to:
         WHERE col= 'j'
       */
-      (*a)->transform(&Item::set_no_const_sub, (byte*) 0);
-      (*b)->transform(&Item::set_no_const_sub, (byte*) 0);
+      (*a)->walk(&Item::set_no_const_sub, (byte*) 0);
+      (*b)->walk(&Item::set_no_const_sub, (byte*) 0);
     }
     break;
   }
@@ -2753,6 +2753,8 @@ bool Item_cond::walk(Item_processor processor, byte *arg)
 
 Item *Item_cond::transform(Item_transformer transformer, byte *arg)
 {
+  DBUG_ASSERT(!current_thd->is_stmt_prepare());
+
   List_iterator<Item> li(list);
   Item *item;
   while ((item= li++))
@@ -2760,8 +2762,15 @@ Item *Item_cond::transform(Item_transformer transformer, byte *arg)
     Item *new_item= item->transform(transformer, arg);
     if (!new_item)
       return 0;
+
+    /*
+      THD::change_item_tree() should be called only if the tree was
+      really transformed, i.e. when a new item has been created.
+      Otherwise we'll be allocating a lot of unnecessary memory for
+      change records at each execution.
+    */
     if (new_item != item)
-      li.replace(new_item);
+      current_thd->change_item_tree(li.ref(), new_item);
   }
   return Item_func::transform(transformer, arg);
 }
@@ -4006,6 +4015,8 @@ bool Item_equal::walk(Item_processor processor, byte *arg)
 
 Item *Item_equal::transform(Item_transformer transformer, byte *arg)
 {
+  DBUG_ASSERT(!current_thd->is_stmt_prepare());
+
   List_iterator<Item_field> it(fields);
   Item *item;
   while ((item= it++))
@@ -4013,8 +4024,15 @@ Item *Item_equal::transform(Item_transformer transformer, byte *arg)
     Item *new_item= item->transform(transformer, arg);
     if (!new_item)
       return 0;
+
+    /*
+      THD::change_item_tree() should be called only if the tree was
+      really transformed, i.e. when a new item has been created.
+      Otherwise we'll be allocating a lot of unnecessary memory for
+      change records at each execution.
+    */
     if (new_item != item)
-      it.replace((Item_field *) new_item);
+      current_thd->change_item_tree((Item **) it.ref(), new_item);
   }
   return Item_func::transform(transformer, arg);
 }

sql/item_func.cc

@@ -258,6 +258,8 @@ void Item_func::traverse_cond(Cond_traverser traverser,
 
 Item *Item_func::transform(Item_transformer transformer, byte *argument)
 {
+  DBUG_ASSERT(!current_thd->is_stmt_prepare());
+
   if (arg_count)
   {
     Item **arg,**arg_end;
@@ -266,6 +268,13 @@ Item *Item_func::transform(Item_transformer transformer, byte *argument)
       Item *new_item= (*arg)->transform(transformer, argument);
       if (!new_item)
 	return 0;
+
+      /*
+        THD::change_item_tree() should be called only if the tree was
+        really transformed, i.e. when a new item has been created.
+        Otherwise we'll be allocating a lot of unnecessary memory for
+        change records at each execution.
+      */
       if (*arg != new_item)
         current_thd->change_item_tree(arg, new_item);
     }

sql/item_row.cc

@@ -154,12 +154,22 @@ bool Item_row::walk(Item_processor processor, byte *arg)
 
 Item *Item_row::transform(Item_transformer transformer, byte *arg)
 {
+  DBUG_ASSERT(!current_thd->is_stmt_prepare());
+
   for (uint i= 0; i < arg_count; i++)
   {
     Item *new_item= items[i]->transform(transformer, arg);
     if (!new_item)
       return 0;
-    items[i]= new_item;
+
+    /*
+      THD::change_item_tree() should be called only if the tree was
+      really transformed, i.e. when a new item has been created.
+      Otherwise we'll be allocating a lot of unnecessary memory for
+      change records at each execution.
+    */
+    if (items[i] != new_item)
+      current_thd->change_item_tree(&items[i], new_item);
   }
   return (this->*transformer)(arg);
 }

sql/item_strfunc.cc

@@ -2051,6 +2051,26 @@ String *Item_func_make_set::val_str(String *str)
 }
 
 
+Item *Item_func_make_set::transform(Item_transformer transformer, byte *arg)
+{
+  DBUG_ASSERT(!current_thd->is_stmt_prepare());
+
+  Item *new_item= item->transform(transformer, arg);
+  if (!new_item)
+    return 0;
+
+  /*
+    THD::change_item_tree() should be called only if the tree was
+    really transformed, i.e. when a new item has been created.
+    Otherwise we'll be allocating a lot of unnecessary memory for
+    change records at each execution.
+  */
+  if (item != new_item)
+    current_thd->change_item_tree(&item, new_item);
+  return Item_str_func::transform(transformer, arg);
+}
+
+
 void Item_func_make_set::print(String *str)
 {
   str->append(STRING_WITH_LEN("make_set("));

sql/item_strfunc.h

@@ -475,14 +475,7 @@ public:
     return item->walk(processor, arg) ||
       Item_str_func::walk(processor, arg);
   }
-  Item *transform(Item_transformer transformer, byte *arg)
-  {
-    Item *new_item= item->transform(transformer, arg);
-    if (!new_item)
-      return 0;
-    item= new_item;
-    return Item_str_func::transform(transformer, arg);
-  }
+  Item *transform(Item_transformer transformer, byte *arg);
   void print(String *str);
 };