bk commit into 5.0 tree (konstantin:1.1981) BUG#10736

[konstantin]

Below is the list of changes that have just been committed into a local
5.0 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.1981 05/06/28 03:02:55 konstantin@stripped +3 -0
  A fix and a test case for Bug#10736 "mysql_stmt_attr_set 
  CURSOR_TYPE_READ_ONLY select within select".
  The bug was caused by the reset of thd->mem_root to thd->main_mem_root in 
  Item_subselect::exec, which in turn triggered too early free_root() for
  data which was needed on subsequent fetches from a cursor.
  This reset also caused a memory leak in stored procedures, as 
  subsequent executions of instructions containing a subselect
  were allocating memory in thd->main_mem_root, which is not freed
  until the end of the entire SP, instead of the per-call mem_root,
  which is freed in the end of execution of the instruction.

  tests/mysql_client_test.c
    1.129 05/06/28 03:02:50 konstantin@stripped +59 -0
    A test case for Bug#10736 "mysql_stmt_attr_set CURSOR_TYPE_READ_ONLY
     select within select"

  sql/opt_range.cc
    1.176 05/06/28 03:02:49 konstantin@stripped +53 -28
    In get_mm_leaf restore the original mem_root of the thd which has
    been temporarily reset with the quick select mem_root earlier: if 
    thd->mem_root points to the QUICK...::mem_root, the memory allocated
    in JOIN::exec during evaluation of a subquery gets freed too early.

  sql/item_subselect.cc
    1.112 05/06/28 03:02:49 konstantin@stripped +0 -7
    Don't try to protect subqueries from the code that assumes that
    it can reset thd->mem_root and get away with it: it's responsibility
    of the caller to ensure that no assumption about the life span
    of the allocated memory made by the called code is broken.
    Besides, this didn't work well with cursors and stored procedures, 
    where the runtime memory root is not the same as &thd->main_mem_root.

# 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:	/opt/local/work/mysql-5.0-10736

--- 1.175/sql/opt_range.cc	2005-06-23 18:23:41 +04:00
+++ 1.176/sql/opt_range.cc	2005-06-28 03:02:49 +04:00
@@ -325,7 +325,7 @@
   TABLE *table;
   KEY_PART *key_parts,*key_parts_end;
   KEY_PART *key[MAX_KEY]; /* First key parts of keys used in the query */
-  MEM_ROOT *mem_root;
+  MEM_ROOT *mem_root, *old_root;
   table_map prev_tables,read_tables,current_table;
   uint baseflag, max_key_part, range_count;
 
@@ -1665,7 +1665,7 @@
   keys_to_use.intersect(head->keys_in_use_for_query);
   if (!keys_to_use.is_clear_all())
   {
-    MEM_ROOT *old_root,alloc;
+    MEM_ROOT alloc;
     SEL_TREE *tree= NULL;
     KEY_PART *key_parts;
     KEY *key_info;
@@ -1680,6 +1680,7 @@
     param.table=head;
     param.keys=0;
     param.mem_root= &alloc;
+    param.old_root= thd->mem_root;
     param.needed_reg= &needed_reg;
     param.imerge_cost_buff_size= 0;
 
@@ -1695,7 +1696,6 @@
       DBUG_RETURN(0);				// Can't use range
     }
     key_parts= param.key_parts;
-    old_root= thd->mem_root;
     thd->mem_root= &alloc;
 
     /*
@@ -1845,7 +1845,7 @@
       }
     }
 
-    thd->mem_root= old_root;
+    thd->mem_root= param.old_root;
 
     /* If we got a read plan, create a quick select from it. */
     if (best_trp)
@@ -1860,7 +1860,7 @@
 
   free_mem:
     free_root(&alloc,MYF(0));			// Return memory & allocator
-    thd->mem_root= old_root;
+    thd->mem_root= param.old_root;
     thd->no_errors=0;
   }
 
@@ -3695,24 +3695,38 @@
 {
   uint maybe_null=(uint) field->real_maybe_null();
   bool optimize_range;
-  SEL_ARG *tree;
+  SEL_ARG *tree= 0;
+  MEM_ROOT *alloc= param->mem_root;
   char *str;
   DBUG_ENTER("get_mm_leaf");
 
+  /*
+    We need to restore the runtime mem_root of the thread in this
+    function becuase it evaluates the value of its argument, while
+    the argument can be any, e.g. a subselect. The subselect
+    items, in turn, assume that all the memory allocated during
+    the evaluation has the same life span as the item itself.
+    TODO: opt_range.cc should not reset thd->mem_root at all.
+  */
+  param->thd->mem_root= param->old_root;
   if (!value)					// IS NULL or IS NOT NULL
   {
     if (field->table->maybe_null)		// Can't use a key on this
-      DBUG_RETURN(0);
+      goto end;
     if (!maybe_null)				// Not null field
-      DBUG_RETURN(type == Item_func::ISNULL_FUNC ? &null_element : 0);
-    if (!(tree=new SEL_ARG(field,is_null_string,is_null_string)))
-      DBUG_RETURN(0);		// out of memory
+    {
+      if (type == Item_func::ISNULL_FUNC)
+        tree= &null_element;
+      goto end;
+    }
+    if (!(tree=new (alloc) SEL_ARG(field,is_null_string,is_null_string)))
+      goto end;                                 // out of memory
     if (type == Item_func::ISNOTNULL_FUNC)
     {
       tree->min_flag=NEAR_MIN;		    /* IS NOT NULL ->  X > NULL */
       tree->max_flag=NO_MAX_RANGE;
     }
-    DBUG_RETURN(tree);
+    goto end;
   }
 
   /*
@@ -3732,7 +3746,7 @@
       key_part->image_type == Field::itRAW &&
       ((Field_str*)field)->charset() != conf_func->compare_collation() &&
       !(conf_func->compare_collation()->state & MY_CS_BINSORT))
-    DBUG_RETURN(0);
+    goto end;
 
   optimize_range= field->optimize_range(param->real_keynr[key_part->key],
                                         key_part->part);
@@ -3746,9 +3760,12 @@
     uint field_length= field->pack_length()+maybe_null;
 
     if (!optimize_range)
-      DBUG_RETURN(0);				// Can't optimize this
+      goto end;
     if (!(res= value->val_str(&tmp)))
-      DBUG_RETURN(&null_element);
+    {
+      tree= &null_element;
+      goto end;
+    }
 
     /*
       TODO:
@@ -3761,7 +3778,7 @@
       res= &tmp;
     }
     if (field->cmp_type() != STRING_RESULT)
-      DBUG_RETURN(0);				// Can only optimize strings
+      goto end;                                 // Can only optimize strings
 
     offset=maybe_null;
     length=key_part->store_length;
@@ -3786,8 +3803,8 @@
 	field_length= length;
     }
     length+=offset;
-    if (!(min_str= (char*) alloc_root(param->mem_root, length*2)))
-      DBUG_RETURN(0);
+    if (!(min_str= (char*) alloc_root(alloc, length*2)))
+      goto end;
 
     max_str=min_str+length;
     if (maybe_null)
@@ -3802,20 +3819,21 @@
 			      min_str+offset, max_str+offset,
 			      &min_length, &max_length);
     if (like_error)				// Can't optimize with LIKE
-      DBUG_RETURN(0);
+      goto end;
 
     if (offset != maybe_null)			// BLOB or VARCHAR
     {
       int2store(min_str+maybe_null,min_length);
       int2store(max_str+maybe_null,max_length);
     }
-    DBUG_RETURN(new SEL_ARG(field,min_str,max_str));
+    tree= new (alloc) SEL_ARG(field, min_str, max_str);
+    goto end;
   }
 
   if (!optimize_range &&
       type != Item_func::EQ_FUNC &&
       type != Item_func::EQUAL_FUNC)
-    DBUG_RETURN(0);				// Can't optimize this
+    goto end;                                   // Can't optimize this
 
   /*
     We can't always use indexes when comparing a string index to a number
@@ -3824,21 +3842,22 @@
   if (field->result_type() == STRING_RESULT &&
       value->result_type() != STRING_RESULT &&
       field->cmp_type() != value->result_type())
-    DBUG_RETURN(0);
+    goto end;
 
   if (value->save_in_field_no_warnings(field, 1) < 0)
   {
     /* This happens when we try to insert a NULL field in a not null column */
-    DBUG_RETURN(&null_element);			// cmp with NULL is never TRUE
+    tree= &null_element;                        // cmp with NULL is never TRUE
+    goto end;
   }
-  str= (char*) alloc_root(param->mem_root, key_part->store_length+1);
+  str= (char*) alloc_root(alloc, key_part->store_length+1);
   if (!str)
-    DBUG_RETURN(0);
+    goto end;
   if (maybe_null)
     *str= (char) field->is_real_null();		// Set to 1 if null
   field->get_key_image(str+maybe_null, key_part->length, key_part->image_type);
-  if (!(tree=new SEL_ARG(field,str,str)))
-    DBUG_RETURN(0);		// out of memory
+  if (!(tree=new (alloc) SEL_ARG(field, str, str)))
+    goto end;                                   // out of memory
 
   /*
     Check if we are comparing an UNSIGNED integer with a negative constant.
@@ -3862,10 +3881,13 @@
       if (type == Item_func::LT_FUNC || type == Item_func::LE_FUNC)
       {
         tree->type= SEL_ARG::IMPOSSIBLE;
-        DBUG_RETURN(tree);
+        goto end;
       }
       if (type == Item_func::GT_FUNC || type == Item_func::GE_FUNC)
-        DBUG_RETURN(0);
+      {
+        tree= 0;
+        goto end;
+      }
     }
   }
 
@@ -3928,6 +3950,9 @@
   default:
     break;
   }
+
+end:
+  param->thd->mem_root= param->mem_root;
   DBUG_RETURN(tree);
 }
 

--- 1.111/sql/item_subselect.cc	2005-06-23 20:31:30 +04:00
+++ 1.112/sql/item_subselect.cc	2005-06-28 03:02:49 +04:00
@@ -194,15 +194,8 @@
 bool Item_subselect::exec()
 {
   int res;
-  MEM_ROOT *old_root= thd->mem_root;
 
-  /*
-    As this is execution, all objects should be allocated through the main
-    mem root
-  */
-  thd->mem_root= &thd->main_mem_root;
   res= engine->exec();
-  thd->mem_root= old_root;
 
   if (engine_changed)
   {

--- 1.128/tests/mysql_client_test.c	2005-06-20 15:38:09 +04:00
+++ 1.129/tests/mysql_client_test.c	2005-06-28 03:02:50 +04:00
@@ -13332,6 +13332,64 @@
   mysql_close(mysql1);
 }
 
+
+/* Bug#10736: cursors and subqueries, memroot management */
+
+static void test_bug10736()
+{
+  MYSQL_STMT *stmt;
+  MYSQL_BIND bind[1];
+  char a[21];
+  int rc;
+  const char *stmt_text;
+  int i= 0;
+  ulong type;
+
+  myheader("test_bug10736");
+
+  mysql_query(mysql, "drop table if exists t1");
+  mysql_query(mysql, "create table t1 (id integer not null primary key,"
+                                      "name VARCHAR(20) NOT NULL)");
+  rc= mysql_query(mysql, "insert into t1 (id, name) values "
+                         "(1, 'aaa'), (2, 'bbb'), (3, 'ccc')");
+  myquery(rc);
+
+  stmt= mysql_stmt_init(mysql);
+
+  type= (ulong) CURSOR_TYPE_READ_ONLY;
+  rc= mysql_stmt_attr_set(stmt, STMT_ATTR_CURSOR_TYPE, (void*) &type);
+  check_execute(stmt, rc);
+  stmt_text= "select name from t1 where name=(select name from t1 where id=2)";
+  rc= mysql_stmt_prepare(stmt, stmt_text, strlen(stmt_text));
+  check_execute(stmt, rc);
+
+  bzero(bind, sizeof(bind));
+  bind[0].buffer_type= MYSQL_TYPE_STRING;
+  bind[0].buffer= (void*) a;
+  bind[0].buffer_length= sizeof(a);
+  mysql_stmt_bind_result(stmt, bind);
+
+  for (i= 0; i < 3; i++)
+  {
+    int row_no= 0;
+    rc= mysql_stmt_execute(stmt);
+    check_execute(stmt, rc);
+    while ((rc= mysql_stmt_fetch(stmt)) == 0)
+    {
+      if (!opt_silent)
+        printf("%d: %s\n", row_no, a);
+      ++row_no;
+    }
+    DIE_UNLESS(rc == MYSQL_NO_DATA);
+  }
+  rc= mysql_stmt_close(stmt);
+  DIE_UNLESS(rc == 0);
+
+  rc= mysql_query(mysql, "drop table t1");
+  myquery(rc);
+}
+
+
 /*
   Read and parse arguments and MySQL options from my.cnf
 */
@@ -13567,6 +13625,7 @@
   { "test_bug10729", test_bug10729 },
   { "test_bug11111", test_bug11111 },
   { "test_bug9992", test_bug9992 },
+  { "test_bug10736", test_bug10736 },
   { 0, 0 }
 };