From d0db70270c59c8def10980c76173b3072263ef25 Mon Sep 17 00:00:00 2001 From: unknown Date: Thu, 9 Jun 2005 18:17:45 +0400 Subject: [PATCH] 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 | 2 +- sql/sp_head.cc | 2 +- sql/sql_prepare.cc | 58 +++++++++++++++++++++--------------- sql/sql_select.cc | 35 +++++++++------------- sql/sql_select.h | 2 -- tests/mysql_client_test.c | 62 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 112 insertions(+), 49 deletions(-) diff --git a/sql/mysql_priv.h b/sql/mysql_priv.h index 4dca5e32c89..10ee5483d19 100644 --- a/sql/mysql_priv.h +++ b/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 */ diff --git a/sql/sp_head.cc b/sql/sp_head.cc index f3459e5b104..8ac9ed1a338 100644 --- a/sql/sp_head.cc +++ b/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 diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc index f1b3c69264c..37efd1d62b9 100644 --- a/sql/sql_prepare.cc +++ b/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; } diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 24b9d0b4713..099b7857af8 100644 --- a/sql/sql_select.cc +++ b/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; diff --git a/sql/sql_select.h b/sql/sql_select.h index 49ac58e913b..5351bbb13d3 100644 --- a/sql/sql_select.h +++ b/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); diff --git a/tests/mysql_client_test.c b/tests/mysql_client_test.c index 002076afd90..b6ca4996c70 100644 --- a/tests/mysql_client_test.c +++ b/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 } };