From: konstantin Date: May 17 2008 9:51pm Subject: bk commit into 5.1 tree (kostja:1.2569) BUG#27430 List-Archive: http://lists.mysql.com/commits/46807 X-Bug: 27430 Message-Id: <20080517215128.AED1229007@vajra.local> Below is the list of changes that have just been committed into a local 5.1 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@stripped, 2008-05-18 01:51:18+04:00, kostja@bodhi.(none) +8 -0 Implement some code review fixes for the fix for Bug#27430 "Crash in subquery code when in PS and table DDL changed after PREPARE" include/my_sys.h@stripped, 2008-05-18 01:51:12+04:00, kostja@bodhi.(none) +3 -0 Add two new flags for my_error(). These flags help parameterize behavoiur of my_message_sql() sql/item.cc@stripped, 2008-05-18 01:51:12+04:00, kostja@bodhi.(none) +8 -4 Update comments. Fix a typo in Item_param::set_param_type_and_swap_value() sql/mysqld.cc@stripped, 2008-05-18 01:51:12+04:00, kostja@bodhi.(none) +3 -1 Implement two additional flags for my_error(): - if ME_NO_SP_HANDLER is specified, ignore stored procedure continue/ exit handlers - if ME_NO_WARNING_FOR_ERROR is specified, do not push warning sql/sql_base.cc@stripped, 2008-05-18 01:51:13+04:00, kostja@bodhi.(none) +14 -9 Update comments. Rename a few methods. sql/sql_class.h@stripped, 2008-05-18 01:51:13+04:00, kostja@bodhi.(none) +15 -3 Update and improve comments. sql/sql_prepare.cc@stripped, 2008-05-18 01:51:13+04:00, kostja@bodhi.(none) +40 -46 Update comments. Style changes. sql/table.h@stripped, 2008-05-18 01:51:13+04:00, kostja@bodhi.(none) +8 -8 Update comments. Style changes. Rename a few methods. tests/mysql_client_test.c@stripped, 2008-05-18 01:51:13+04:00, kostja@bodhi.(none) +1 -0 Zero the bind array, to follow C API requirements. diff -Nrup a/include/my_sys.h b/include/my_sys.h --- a/include/my_sys.h 2008-02-27 14:29:56 +03:00 +++ b/include/my_sys.h 2008-05-18 01:51:12 +04:00 @@ -90,6 +90,9 @@ extern int NEAR my_errno; /* Last error #define ME_COLOUR1 ((1 << ME_HIGHBYTE)) /* Possibly error-colours */ #define ME_COLOUR2 ((2 << ME_HIGHBYTE)) #define ME_COLOUR3 ((3 << ME_HIGHBYTE)) +#define ME_FATALERROR 1024 /* Fatal statement error */ +#define ME_NO_WARNING_FOR_ERROR 2048 /* Don't push a warning for error */ +#define ME_NO_SP_HANDLER 4096 /* Don't call stored routine error handlers */ /* Bits in last argument to fn_format */ #define MY_REPLACE_DIR 1 /* replace dir in name with 'dir' */ diff -Nrup a/sql/item.cc b/sql/item.cc --- a/sql/item.cc 2008-04-19 14:35:50 +04:00 +++ b/sql/item.cc 2008-05-18 01:51:12 +04:00 @@ -3165,15 +3165,19 @@ void Item_param::print(String *str, enum Preserve the original parameter types and values when re-preparing a prepared statement. - Copy parameter type information and conversion function - pointers from a parameter of the old statement to the - corresponding parameter of the new one. + @details Copy parameter type information and conversion + function pointers from a parameter of the old statement + to the corresponding parameter of the new one. Move parameter values from the old parameters to the new one. We simply "exchange" the values, which allows to save on allocation and character set conversion in case a parameter is a string or a blob/clob. + The old parameter gets the value of this one, which + ensures that all memory of this parameter is freed + correctly. + @param[in] src parameter item of the original prepared statement */ @@ -3187,7 +3191,7 @@ Item_param::set_param_type_and_swap_valu item_type= src->item_type; item_result_type= src->item_result_type; - collation.set(src->collation.collation); + collation.set(src->collation); maybe_null= src->maybe_null; null_value= src->null_value; max_length= src->max_length; diff -Nrup a/sql/mysqld.cc b/sql/mysqld.cc --- a/sql/mysqld.cc 2008-04-19 15:46:30 +04:00 +++ b/sql/mysqld.cc 2008-05-18 01:51:12 +04:00 @@ -2843,6 +2843,7 @@ int my_message_sql(uint error, const cha by the stored procedures code. */ if (thd->spcont && + ! (MyFlags & ME_NO_SP_HANDLER) && thd->spcont->handle_error(error, MYSQL_ERROR::WARN_LEVEL_ERROR, thd)) { /* @@ -2852,7 +2853,8 @@ int my_message_sql(uint error, const cha DBUG_RETURN(0); } - if (!thd->no_warnings_for_error) + if (!thd->no_warnings_for_error && + !(MyFlags & ME_NO_WARNING_FOR_ERROR)) { /* Suppress infinite recursion if there a memory allocation error diff -Nrup a/sql/sql_base.cc b/sql/sql_base.cc --- a/sql/sql_base.cc 2008-04-08 20:01:15 +04:00 +++ b/sql/sql_base.cc 2008-05-18 01:51:13 +04:00 @@ -3722,9 +3722,8 @@ void assign_new_table_id(TABLE_SHARE *sh Compare metadata versions of an element obtained from the table definition cache and its corresponding node in the parse tree. - If the new and the old values mismatch, invoke + @details If the new and the old values mismatch, invoke Metadata_version_observer. - At prepared statement prepare, all TABLE_LIST version values are NULL and we always have a mismatch. But there is no observer set in THD, and therefore no error is reported. Instead, we update @@ -3738,8 +3737,8 @@ void assign_new_table_id(TABLE_SHARE *sh @sa Execute_observer @sa check_prepared_statement() to see cases when an observer is installed - @sa TABLE_LIST::is_metadata_version_equal() - @sa TABLE_SHARE::get_metadata_version() + @sa TABLE_LIST::is_metadata_id_equal() + @sa TABLE_SHARE::get_metadata_id() @param[in] thd used to report errors @param[in,out] tables TABLE_LIST instance created by the parser @@ -3755,27 +3754,28 @@ bool check_and_update_table_version(THD *thd, TABLE_LIST *tables, TABLE_SHARE *table_share) { - if (! tables->is_metadata_version_equal(table_share)) + if (! tables->is_metadata_id_equal(table_share)) { if (thd->m_metadata_observer && - thd->m_metadata_observer->check_metadata_change(thd)) + thd->m_metadata_observer->report_error(thd)) { /* Version of the table share is different from the previous execution of the prepared statement, and it is unacceptable for this SQLCOM. Error has been reported. */ + DBUG_ASSERT(thd->is_error()); return TRUE; } - /* Always maintain the latest version */ - tables->set_metadata_version(table_share); + /* Always maintain the latest version and type */ + tables->set_metadata_id(table_share); } #if 0 #ifndef DBUG_OFF /* Spuriously reprepare each statement. */ if (thd->m_metadata_observer && thd->stmt_arena->is_reprepared == FALSE) { - thd->m_metadata_observer->check_metadata_change(thd); + thd->m_metadata_observer->report_error(thd); return TRUE; } #endif @@ -3828,6 +3828,10 @@ retry: if (share->is_view) { + /* + This table is a view. Validate its metadata version: in particular, + that it was a view when the statement was prepared. + */ if (check_and_update_table_version(thd, table_list, share)) goto err; if (table_list->i_s_requested_object & OPEN_TABLE_ONLY) @@ -4622,6 +4626,7 @@ int open_tables(THD *thd, TABLE_LIST **s } tables->table->grant= tables->grant; + /* Check and update metadata version of a base table. */ if (check_and_update_table_version(thd, tables, tables->table->s)) { result= -1; diff -Nrup a/sql/sql_class.h b/sql/sql_class.h --- a/sql/sql_class.h 2008-04-19 14:35:52 +04:00 +++ b/sql/sql_class.h 2008-05-18 01:51:13 +04:00 @@ -57,16 +57,15 @@ class Metadata_version_observer { -protected: - virtual ~Metadata_version_observer(); public: + virtual ~Metadata_version_observer(); /** Check if a change of metadata is OK. In future the signature of this method may be extended to accept the old and the new versions, but since currently the check is very simple, we only need the THD to report an error. */ - virtual bool check_metadata_change(THD *thd)= 0; + virtual bool report_error(THD *thd)= 0; }; @@ -2842,6 +2841,19 @@ public: #define CF_STATUS_COMMAND 4 #define CF_SHOW_TABLE_COMMAND 8 #define CF_WRITE_LOGS_COMMAND 16 +/** + Must be set for SQL statements that may contain + Item expressions and/or use joins and tables. + Indicates that the parse tree of such statement may + contain rule-based optimizations that depend on metadata + (i.e. number of columns in a table), and consequently + that the statement must be re-prepared whenever + referenced metadata changes. Must not be set for + statements that themselves change metadata, e.g. RENAME, + ALTER and other DDL, since otherwise will trigger constant + reprepare. Consequently, complex item expressions and + joins are currently prohibited in these statements. +*/ #define CF_REEXECUTION_FRAGILE 32 /* Functions in sql_class.cc */ diff -Nrup a/sql/sql_prepare.cc b/sql/sql_prepare.cc --- a/sql/sql_prepare.cc 2008-04-17 01:04:45 +04:00 +++ b/sql/sql_prepare.cc 2008-05-18 01:51:13 +04:00 @@ -124,7 +124,7 @@ public: class Execute_observer: public Metadata_version_observer { public: - virtual bool check_metadata_change(THD *thd); + virtual bool report_error(THD *thd); /** Set to TRUE if metadata of some used table has changed since prepare */ bool m_invalidated; }; @@ -137,14 +137,12 @@ public: */ bool -Execute_observer::check_metadata_change(THD *thd) +Execute_observer::report_error(THD *thd) { - bool save_thd_no_warnings_for_error= thd->no_warnings_for_error; - DBUG_ENTER("Execute_observer::notify_about_metadata_change"); + DBUG_ENTER("Execute_observer::report_error"); + + my_error(ER_NEED_REPREPARE, MYF(ME_NO_WARNING_FOR_ERROR|ME_NO_SP_HANDLER)); - thd->no_warnings_for_error= TRUE; - my_error(ER_NEED_REPREPARE, MYF(0)); - thd->no_warnings_for_error= save_thd_no_warnings_for_error; m_invalidated= TRUE; DBUG_RETURN(TRUE); } @@ -984,7 +982,7 @@ static bool emb_insert_params_with_log(P /** Setup data conversion routines using an array of parameter markers from the original prepared statement. - Move the parameter data of the original prepared + Swap the parameter data of the original prepared statement to the new one. Used only when we re-prepare a prepared statement. @@ -2844,7 +2842,7 @@ Select_fetch_protocol_binary::send_data( ****************************************************************************/ Prepared_statement::Prepared_statement(THD *thd_arg, Protocol *protocol_arg) - :Statement(0, &main_mem_root, + :Statement(NULL, &main_mem_root, INITIALIZED, ++thd_arg->statement_id_counter), thd(thd_arg), result(thd_arg), @@ -3153,38 +3151,36 @@ Prepared_statement::set_parameters(Strin uchar *packet, uchar *packet_end) { bool is_sql_ps= packet == NULL; + bool res; if (is_sql_ps) { /* SQL prepared statement */ - if (set_params_from_vars(this, thd->lex->prepared_stmt_params, - expanded_query)) - goto set_params_data_err; + res= set_params_from_vars(this, thd->lex->prepared_stmt_params, + expanded_query); } else if (param_count) { #ifndef EMBEDDED_LIBRARY uchar *null_array= packet; - if (setup_conversion_functions(this, &packet, packet_end) || - set_params(this, null_array, packet, packet_end, - expanded_query)) - goto set_params_data_err; + res= (setup_conversion_functions(this, &packet, packet_end) || + set_params(this, null_array, packet, packet_end, expanded_query)); #else - /* - In embedded library we re-install conversion routines each time - we set params, and also we don't need to parse packet. - So we do it in one function. - */ - if (set_params_data(this, expanded_query)) - goto set_params_data_err; + /* + In embedded library we re-install conversion routines each time + we set parameters, and also we don't need to parse packet. + So we do it in one function. + */ + res= set_params_data(this, expanded_query); #endif } - return FALSE; -set_params_data_err: - my_error(ER_WRONG_ARGUMENTS, MYF(0), - is_sql_ps ? "EXECUTE" : "mysql_stmt_execute"); - reset_stmt_params(this); - return TRUE; + if (res) + { + my_error(ER_WRONG_ARGUMENTS, MYF(0), + is_sql_ps ? "EXECUTE" : "mysql_stmt_execute"); + reset_stmt_params(this); + } + return res; } @@ -3249,6 +3245,7 @@ reexecute: if (sql_command_flags[lex->sql_command] & CF_REEXECUTION_FRAGILE) { + DBUG_ASSERT(thd->m_metadata_observer == NULL); thd->m_metadata_observer= &execute_observer; } @@ -3260,12 +3257,13 @@ reexecute: if (!(specialflag & SPECIAL_NO_PRIOR)) my_pthread_setprio(pthread_self(), WAIT_PRIOR); - thd->m_metadata_observer= 0; + thd->m_metadata_observer= NULL; if (error && !thd->is_fatal_error && !thd->killed && execute_observer.m_invalidated && reprepare_attempt++ < MAX_REPREPARE_ATTEMPTS) { + DBUG_ASSERT(thd->main_da.sql_errno() == ER_NEED_REPREPARE); thd->clear_error(); error= reprepare(); @@ -3299,44 +3297,40 @@ Prepared_statement::reprepare() LEX_STRING saved_cur_db_name= { saved_cur_db_name_buf, sizeof(saved_cur_db_name_buf) }; LEX_STRING stmt_db_name= { db, db_length }; - Prepared_statement *copy; bool cur_db_changed; - bool error= TRUE; - - status_var_increment(thd->status_var.com_stmt_reprepare); + bool error; - copy= new Prepared_statement(thd, &thd->protocol_text); + Prepared_statement copy(thd, &thd->protocol_text); - if (! copy) - return TRUE; + status_var_increment(thd->status_var.com_stmt_reprepare); if (mysql_opt_change_db(thd, &stmt_db_name, &saved_cur_db_name, TRUE, &cur_db_changed)) - goto end; + return TRUE; - error= (name.str && copy->set_name(&name) || - copy->prepare(query, query_length) || - validate_metadata(copy)); + error= (name.str && copy.set_name(&name) || + copy.prepare(query, query_length) || + validate_metadata(©)); if (cur_db_changed) mysql_change_db(thd, &saved_cur_db_name, TRUE); if (! error) { - swap_prepared_statement(copy); - swap_parameter_array(param_array, copy->param_array, param_count); + swap_prepared_statement(©); + swap_parameter_array(param_array, copy.param_array, param_count); #ifndef DBUG_OFF is_reprepared= TRUE; #endif /* - Clear possible warnigns during re-prepare, it has to be completely + Clear possible warnings during reprepare, it has to be completely transparent to the user. We use mysql_reset_errors() since there were no separate query id issued for re-prepare. + Sic: we can't simply silence warnings during reprepare, because if + it's failed, we need to return all the warnings to the user. */ mysql_reset_errors(thd, TRUE); } -end: - delete copy; return error; } diff -Nrup a/sql/table.h b/sql/table.h --- a/sql/table.h 2008-04-19 14:35:53 +04:00 +++ b/sql/table.h 2008-05-18 01:51:13 +04:00 @@ -478,10 +478,10 @@ typedef struct st_table_share Let's try to explain why and how this limited solution allows to validate prepared statements. - Firstly, spaces (in mathematical sense) of version numbers + Firstly, sets (in mathematical sense) of version numbers never intersect for different metadata types. Therefore, version id of a temporary table is never compared with - a version id of a view or a temporary table, and vice versa. + a version id of a view, and vice versa. Secondly, for base tables, we know that each DDL flushes the respective share from the TDC. This ensures that whenever @@ -530,11 +530,11 @@ typedef struct st_table_share with a base table, a base table is replaced with a temporary table and so on. - @sa TABLE_LIST::is_metadata_version_equal() + @sa TABLE_LIST::is_metadata_id_equal() */ ulong get_metadata_version() const { - return tmp_table == SYSTEM_TMP_TABLE || is_view ? 0 : table_map_id; + return (tmp_table == SYSTEM_TMP_TABLE || is_view) ? 0 : table_map_id; } } TABLE_SHARE; @@ -1340,7 +1340,7 @@ struct TABLE_LIST @sa check_and_update_table_version() */ inline - bool is_metadata_version_equal(TABLE_SHARE *s) const + bool is_metadata_id_equal(TABLE_SHARE *s) const { return (m_metadata_type == s->get_metadata_type() && m_metadata_version == s->get_metadata_version()); @@ -1353,7 +1353,7 @@ struct TABLE_LIST @sa check_and_update_table_version() */ inline - void set_metadata_version(TABLE_SHARE *s) + void set_metadata_id(TABLE_SHARE *s) { m_metadata_type= s->get_metadata_type(); m_metadata_version= s->get_metadata_version(); @@ -1369,9 +1369,9 @@ private: /* Remembered MERGE child def version. See top comment in ha_myisammrg.cc */ ulong child_def_version; - /** See comments for set_metadata_version() */ + /** See comments for set_metadata_id() */ enum enum_metadata_type m_metadata_type; - /** See comments for set_metadata_version() */ + /** See comments for set_metadata_id() */ ulong m_metadata_version; }; diff -Nrup a/tests/mysql_client_test.c b/tests/mysql_client_test.c --- a/tests/mysql_client_test.c 2008-04-19 14:35:55 +04:00 +++ b/tests/mysql_client_test.c 2008-05-18 01:51:13 +04:00 @@ -17418,6 +17418,7 @@ static void test_wl4166() verify_param_count(stmt, 7); + bzero(my_bind, sizeof(my_bind)); /* tinyint */ my_bind[0].buffer_type= MYSQL_TYPE_TINY; my_bind[0].buffer= (void *)&tiny_data;