A fix and test case for Bug#10729 "mysql_stmt_attr_set

CURSOR_TYPE_READ_ONLY". The bug was that we (me) don't perform proper
cleanups of the prepared statement when done fetching from a cursor.
Another patch.


sql/mysql_priv.h:
  Rename reset_stmt_for_execute to init_stmt_before_use (to correspond to
  cleanup_stmt_and_thd_after_use).
sql/sp_head.cc:
  Rename.
sql/sql_prepare.cc:
  Move common cleanup code to a cleanup function, call it when we close
  a cursor.
sql/sql_select.cc:
  Cleanup.
sql/sql_select.h:
  No need for init_thd, this code has been inlined in Cursor::open.
tests/mysql_client_test.c:
  Add a test case for Bug#10729 "mysql_stmt_attr_set CURSOR_TYPE_READ_ONLY"
  (problem reusing a prepared statemnt if there was a cursor)

sql/mysql_priv.h

@@ -834,7 +834,7 @@ void mysql_stmt_fetch(THD *thd, char *packet, uint packet_length);
 void mysql_stmt_free(THD *thd, char *packet);
 void mysql_stmt_reset(THD *thd, char *packet);
 void mysql_stmt_get_longdata(THD *thd, char *pos, ulong packet_length);
-void reset_stmt_for_execute(THD *thd, LEX *lex);
+void reinit_stmt_before_use(THD *thd, LEX *lex);
 void init_stmt_after_parse(THD*, LEX*);
 
 /* sql_handler.cc */

sql/sp_head.cc

@@ -1355,7 +1355,7 @@ sp_lex_keeper::reset_lex_and_exec_core(THD *thd, uint *nextp,
            implemented at the same time as ability not to store LEX for
            instruction if it is not really used.
   */
-  reset_stmt_for_execute(thd, m_lex);
+  reinit_stmt_before_use(thd, m_lex);
 
   /*
     If requested check whenever we have access to tables in LEX's table list

sql/sql_prepare.cc

@@ -1656,6 +1656,17 @@ static bool init_param_array(Prepared_statement *stmt)
   return FALSE;
 }
 
+
+/* Cleanup PS after execute/prepare and restore THD state */
+
+static void cleanup_stmt_and_thd_after_use(Statement *stmt, THD *thd)
+{
+  stmt->lex->unit.cleanup();
+  cleanup_items(stmt->free_list);
+  thd->rollback_item_tree_changes();
+  thd->cleanup_after_query();
+}
+
 /*
   Given a query string with parameter markers, create a Prepared Statement
   from it and send PS info back to the client.
@@ -1760,12 +1771,9 @@ bool mysql_stmt_prepare(THD *thd, char *packet, uint packet_length,
     thd->lex->sphead= NULL;
   }
   lex_end(lex);
-  lex->unit.cleanup();
   close_thread_tables(thd);
+  cleanup_stmt_and_thd_after_use(stmt, thd);
   thd->restore_backup_statement(stmt, &thd->stmt_backup);
-  cleanup_items(stmt->free_list);
-  thd->rollback_item_tree_changes();
-  thd->cleanup_after_query();
   thd->current_arena= thd;
 
   if (error)
@@ -1808,10 +1816,10 @@ void init_stmt_after_parse(THD *thd, LEX *lex)
 
 /* Reinit prepared statement/stored procedure before execution */
 
-void reset_stmt_for_execute(THD *thd, LEX *lex)
+void reinit_stmt_before_use(THD *thd, LEX *lex)
 {
   SELECT_LEX *sl= lex->all_selects_list;
-  DBUG_ENTER("reset_stmt_for_execute");
+  DBUG_ENTER("reinit_stmt_before_use");
 
   if (lex->empty_field_list_on_rset)
   {
@@ -2007,7 +2015,7 @@ void mysql_stmt_execute(THD *thd, char *packet, uint packet_length)
   thd->stmt_backup.set_statement(thd);
   thd->set_statement(stmt);
   thd->current_arena= stmt;
-  reset_stmt_for_execute(thd, stmt->lex);
+  reinit_stmt_before_use(thd, stmt->lex);
   /* From now cursors assume that thd->mem_root is clean */
   if (expanded_query.length() &&
       alloc_query(thd, (char *)expanded_query.ptr(),
@@ -2031,17 +2039,18 @@ void mysql_stmt_execute(THD *thd, char *packet, uint packet_length)
 
   if (cursor && cursor->is_open())
   {
+    /*
+      It's safer if we grab THD state after mysql_execute_command is
+      finished and not in Cursor::open(), because currently the call to
+      Cursor::open is buried deep in JOIN::exec of the top level join.
+    */
     cursor->init_from_thd(thd);
-    cursor->state= stmt->state;
   }
   else
   {
-    thd->lex->unit.cleanup();
-    cleanup_items(stmt->free_list);
+    close_thread_tables(thd);
+    cleanup_stmt_and_thd_after_use(stmt, thd);
     reset_stmt_params(stmt);
-    close_thread_tables(thd);                   /* to close derived tables */
-    thd->rollback_item_tree_changes();
-    thd->cleanup_after_query();
   }
 
   thd->set_statement(&thd->stmt_backup);
@@ -2119,7 +2128,7 @@ static void execute_stmt(THD *thd, Prepared_statement *stmt,
 {
   DBUG_ENTER("execute_stmt");
 
-  reset_stmt_for_execute(thd, stmt->lex);
+  reinit_stmt_before_use(thd, stmt->lex);
 
   if (expanded_query->length() &&
       alloc_query(thd, (char *)expanded_query->ptr(),
@@ -2141,14 +2150,11 @@ static void execute_stmt(THD *thd, Prepared_statement *stmt,
   if (!(specialflag & SPECIAL_NO_PRIOR))
     my_pthread_setprio(pthread_self(), WAIT_PRIOR);
 
-  thd->lex->unit.cleanup();
-  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
+  cleanup_stmt_and_thd_after_use(stmt, thd);
+  reset_stmt_params(stmt);
   thd->set_statement(&thd->stmt_backup);
-  thd->cleanup_after_query();
+  thd->current_arena= thd;
 
   if (stmt->state == Item_arena::PREPARED)
     stmt->state= Item_arena::EXECUTED;
@@ -2171,7 +2177,7 @@ void mysql_stmt_fetch(THD *thd, char *packet, uint packet_length)
   /* assume there is always place for 8-16 bytes */
   ulong stmt_id= uint4korr(packet);
   ulong num_rows= uint4korr(packet+4);
-  Statement *stmt;
+  Prepared_statement *stmt;
   DBUG_ENTER("mysql_stmt_fetch");
 
   if (!(stmt= find_prepared_statement(thd, stmt_id, "mysql_stmt_fetch")))
@@ -2185,7 +2191,6 @@ void mysql_stmt_fetch(THD *thd, char *packet, uint packet_length)
 
   thd->current_arena= stmt;
   thd->set_n_backup_statement(stmt, &thd->stmt_backup);
-  stmt->cursor->init_thd(thd);
 
   if (!(specialflag & SPECIAL_NO_PRIOR))
     my_pthread_setprio(pthread_self(), QUERY_PRIOR);
@@ -2197,11 +2202,16 @@ void mysql_stmt_fetch(THD *thd, char *packet, uint packet_length)
   if (!(specialflag & SPECIAL_NO_PRIOR))
     my_pthread_setprio(pthread_self(), WAIT_PRIOR);
 
-  /* Restore THD state */
-  stmt->cursor->reset_thd(thd);
   thd->restore_backup_statement(stmt, &thd->stmt_backup);
   thd->current_arena= thd;
 
+  if (!stmt->cursor->is_open())
+  {
+    /* We're done with the fetch: reset PS for next execution */
+    cleanup_stmt_and_thd_after_use(stmt, thd);
+    reset_stmt_params(stmt);
+  }
+
   DBUG_VOID_RETURN;
 }
 

sql/sql_select.cc

@@ -1709,10 +1709,11 @@ Cursor::init_from_thd(THD *thd)
     We need to save and reset thd->mem_root, otherwise it'll be freed
     later in mysql_parse.
 
-    We can't just change the thd->mem_root here as we want to keep the things
-    that is already allocated in thd->mem_root for Cursor::fetch()
+    We can't just change the thd->mem_root here as we want to keep the
+    things that are already allocated in thd->mem_root for Cursor::fetch()
   */
   main_mem_root=  *thd->mem_root;
+  state= thd->current_arena->state;
   /* Allocate new memory root for thd */
   init_sql_alloc(thd->mem_root,
                  thd->variables.query_alloc_block_size,
@@ -1735,24 +1736,6 @@ Cursor::init_from_thd(THD *thd)
     What problems can we have with it if cursor is open?
     TODO: must be fixed because of the prelocked mode.
   */
-  /*
-    TODO: grab thd->free_list here?
-  */
-}
-
-
-void
-Cursor::init_thd(THD *thd)
-{
-  DBUG_ASSERT(thd->derived_tables == 0);
-  thd->derived_tables= derived_tables;
-
-  DBUG_ASSERT(thd->open_tables == 0);
-  thd->open_tables= open_tables;
-
-  DBUG_ASSERT(thd->lock== 0);
-  thd->lock= lock;
-  thd->query_id= query_id;
 }
 
 
@@ -1828,6 +1811,13 @@ Cursor::fetch(ulong num_rows)
   DBUG_ENTER("Cursor::fetch");
   DBUG_PRINT("enter",("rows: %lu", num_rows));
 
+  DBUG_ASSERT(thd->derived_tables == 0 && thd->open_tables == 0 &&
+              thd->lock == 0);
+
+  thd->derived_tables= derived_tables;
+  thd->open_tables= open_tables;
+  thd->lock= lock;
+  thd->query_id= query_id;
   /* save references to memory, allocated during fetch */
   thd->set_n_backup_item_arena(this, &thd->stmt_backup);
 
@@ -1846,6 +1836,9 @@ Cursor::fetch(ulong num_rows)
 #endif
 
   thd->restore_backup_item_arena(this, &thd->stmt_backup);
+  DBUG_ASSERT(thd->free_list == 0);
+  reset_thd(thd);
+
   if (error == NESTED_LOOP_CURSOR_LIMIT)
   {
     /* Fetch limit worked, possibly more rows are there */
@@ -1886,8 +1879,8 @@ Cursor::close()
     join->cleanup();
     delete join;
   }
-  /* XXX: Another hack: closing tables used in the cursor */
   {
+    /* XXX: Another hack: closing tables used in the cursor */
     DBUG_ASSERT(lock || open_tables || derived_tables);
 
     TABLE *tmp_derived_tables= thd->derived_tables;

sql/sql_select.h

@@ -386,8 +386,6 @@ public:
   /* Temporary implementation as now we replace THD state by value */
   /* Save THD state into cursor */
   void init_from_thd(THD *thd);
-  /* Restore THD from cursor to continue cursor execution */
-  void init_thd(THD *thd);
   /* bzero cursor state in THD */
   void reset_thd(THD *thd);
 

tests/mysql_client_test.c

@@ -13145,6 +13145,67 @@ static void test_bug9643()
   myquery(rc);
 }
 
+/*
+  Check that proper cleanups are done for prepared statement when
+  fetching thorugh a cursor.
+*/
+
+static void test_bug10729()
+{
+  MYSQL_STMT *stmt;
+  MYSQL_BIND bind[1];
+  char a[21];
+  int rc;
+  const char *stmt_text;
+  int i= 0;
+  char *name_array[3]= { "aaa", "bbb", "ccc" };
+  ulong type;
+
+  myheader("test_bug10729");
+
+  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";
+  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)
+    {
+      DIE_UNLESS(strcmp(a, name_array[row_no]) == 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
 */
@@ -13377,6 +13438,7 @@ static struct my_tests_st my_tests[]= {
   { "test_bug9520", test_bug9520 },
   { "test_bug9478", test_bug9478 },
   { "test_bug9643", test_bug9643 },
+  { "test_bug10729", test_bug10729 },
   { 0, 0 }
 };