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.

9 files changed, 182 insertions, 34 deletions

diff --git a/mysql-test/r/ps.result b/mysql-test/r/ps.result
index d73dd03fc57..080187cfa7b 100644
--- a/mysql-test/r/ps.result
+++ b/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.
diff --git a/mysql-test/t/ps.test b/mysql-test/t/ps.test
index d0f31087c8f..5b2e37ecc94 100644
--- a/mysql-test/t/ps.test
+++ b/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.
diff --git a/sql/item.cc b/sql/item.cc
index 95ff5462fad..ac0658224b3 100644
--- a/sql/item.cc
+++ b/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 &&
diff --git a/sql/item.h b/sql/item.h
index 514c31c2d74..003f264fc7d 100644
--- a/sql/item.h
+++ b/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);
 };
 
 /*
diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc
index 34170124cd7..02b34a4d672 100644
--- a/sql/item_cmpfunc.cc
+++ b/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);
 }
diff --git a/sql/item_func.cc b/sql/item_func.cc
index d3458103f6e..c7f2048b9dc 100644
--- a/sql/item_func.cc
+++ b/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);
     }
diff --git a/sql/item_row.cc b/sql/item_row.cc
index f5c8d511025..d7591b9865d 100644
--- a/sql/item_row.cc
+++ b/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);
 }
diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc
index 4fc7340a3b0..eb5964c6e21 100644
--- a/sql/item_strfunc.cc
+++ b/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("));
diff --git a/sql/item_strfunc.h b/sql/item_strfunc.h
index 488dc20b063..e501ccce687 100644
--- a/sql/item_strfunc.h
+++ b/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);
 };