| List: | Commits | « Previous MessageNext Message » | |
| From: | Marc Alff | Date: | August 7 2008 7:17pm |
| Subject: | Re: bzr commit into mysql-6.0-wl2110-review branch (marc.alff:2675) WL#2110 | ||
| View as plain text | |||
Hi Davi See my comments below. Some points are still to be discussed, and others have been addressed already. Davi Arnaut wrote: > Hi Marc, > > Here goes some initial review comments. I guess that some bits are > missing and I need to take a more careful look at this patch. Let's > first discuss the issues that I raise here. > > Marc Alff wrote: > >> #At file:///home/malff/BZR-TREE/mysql-6.0-wl2110-review-part2/ >> >> 2675 Marc Alff 2008-07-22 >> WL#2110 (Stored Procedures: Implement SIGNAL) >> >> Formal review part 2/10: Core >> >> This patch contains the core implementation of the SIGNAL and RESIGNAL >> statements. >> In particular, >> - SQL_condition is a new class used to represent a SQL condition >> - SIGNAL / RESIGNAL is implemented in sql_signal.cc >> - raising conditions (my_message_sql, push_warning), >> and the SQL diagnostics area are changed. >> added: >> sql/sql_signal.cc >> modified: >> sql/derror.cc >> sql/mysql_priv.h >> sql/mysqld.cc >> sql/share/errmsg.txt >> sql/sql_class.cc >> sql/sql_class.h >> sql/sql_error.cc >> sql/sql_error.h >> sql/sql_insert.cc >> >> === modified file 'sql/derror.cc' >> --- a/sql/derror.cc 2008-04-09 00:56:49 +0000 >> +++ b/sql/derror.cc 2008-07-23 00:25:11 +0000 >> @@ -121,7 +121,7 @@ Please install the latest version of thi >> } >> >> /* TODO: Convert the character set to server system character set */ >> - if (!get_charset(head[30],MYF(MY_WME))) >> + if (!(error_message_charset_info= get_charset(head[30],MYF(MY_WME)))) >> { >> sql_print_error("Character set #%d is not supported for messagefile '%s'", >> (int)head[30],name); >> >> === modified file 'sql/mysql_priv.h' >> --- a/sql/mysql_priv.h 2008-07-09 07:12:43 +0000 >> +++ b/sql/mysql_priv.h 2008-07-23 00:25:11 +0000 >> @@ -152,6 +152,10 @@ char* query_table_status(THD *thd,const >> extern CHARSET_INFO *system_charset_info, *files_charset_info ; >> extern CHARSET_INFO *national_charset_info, *table_alias_charset; >> >> +/** >> + Character set of the buildin error messages loaded from errmsg.sys. >> +*/ >> +extern CHARSET_INFO *error_message_charset_info; >> >> enum Derivation >> { >> @@ -789,6 +793,7 @@ typedef my_bool (*qc_engine_callback)(TH >> uint key_length, >> ulonglong *engine_data); >> #include "sql_string.h" >> +#include "sql_fixstring.h" >> #include "sql_list.h" >> #include "sql_map.h" >> #include "my_decimal.h" >> >> === modified file 'sql/mysqld.cc' >> --- a/sql/mysqld.cc 2008-07-09 07:12:43 +0000 >> +++ b/sql/mysqld.cc 2008-07-23 00:25:11 +0000 >> @@ -664,6 +664,7 @@ MY_BITMAP temp_pool; >> CHARSET_INFO *system_charset_info, *files_charset_info ; >> CHARSET_INFO *national_charset_info, *table_alias_charset; >> CHARSET_INFO *character_set_filesystem; >> +CHARSET_INFO *error_message_charset_info= NULL; >> > > No need to set to NULL. > > Are you actually asking me to leave this variable as a random pointer instead ? No change. >> >> MY_LOCALE *my_default_lc_time_names; >> >> @@ -1897,7 +1898,10 @@ void close_connection(THD *thd, uint err >> if ((vio= thd->net.vio) != 0) >> { >> if (errcode) >> - net_send_error(thd, errcode, ER(errcode)); /* purecov: inspected */ >> + { >> + const char* sqlstate= mysql_errno_to_sqlstate(errcode); >> + net_send_error(thd, errcode, ER(errcode), sqlstate); /* purecov: inspected > */ >> + } >> vio_close(vio); /* vio is freed in delete thd */ >> } >> if (lock) >> @@ -2947,8 +2951,6 @@ extern "C" void my_message_sql(uint erro >> void my_message_sql(uint error, const char *str, myf MyFlags) >> { >> THD *thd; >> - MYSQL_ERROR::enum_warning_level level; >> - sql_print_message_func func; >> DBUG_ENTER("my_message_sql"); >> DBUG_PRINT("error", ("error: %u message: '%s'", error, str)); >> /* >> @@ -2956,106 +2958,44 @@ void my_message_sql(uint error, const ch >> will be fixed >> DBUG_ASSERT(error != 0); >> */ >> + >> + /* >> + TODO: ME_JUST_INFO and ME_JUST_WARNING are back doors used in >> + storage/maria to print to the server log files. >> + my_error/my_message_sql should not be used for this, since we >> + don't want the exception handlers / SIGNAL / RESIGNAL to interfere >> + with the error handling in this case. >> + Instead, the server should expose a clean interface to the storage >> + engines for printing into the server logs (sql/log.cc), >> + and the storage engine should call that interface (see ma_message_end_user) >> + */ >> + >> + /* >> + Flags for printing to the logs are mutually exclusive >> + */ >> + DBUG_ASSERT(!(MyFlags & ME_JUST_INFO) || >> + !(MyFlags & ME_JUST_WARNING) || >> + !(MyFlags & ME_NOREFRESH)); >> + >> + >> > > Please go there and remove this hideous stuff. I have a hard time > trying to understand why it was done this way since they already use > server internals, why didn't they use sql_print_information..?! > > I think we can push this first. > > SIGNAL / RESIGNAL will go into 6.1 The cleanup of Maria should happen before that, I will enter a separate bug report for this. >> if (MyFlags & ME_JUST_INFO) >> { >> - level= MYSQL_ERROR::WARN_LEVEL_NOTE; >> - func= sql_print_information; >> - } >> - else if (MyFlags & ME_JUST_WARNING) >> - { >> - level= MYSQL_ERROR::WARN_LEVEL_WARN; >> - func= sql_print_warning; >> + sql_print_information("%s: %s", my_progname, str); >> + DBUG_VOID_RETURN; >> } >> - else >> + >> + if (MyFlags & ME_JUST_WARNING) >> { >> - level= MYSQL_ERROR::WARN_LEVEL_ERROR; >> - func= sql_print_error; >> + sql_print_warning("%s: %s", my_progname, str); >> + DBUG_VOID_RETURN; >> } >> >> if ((thd= current_thd)) >> { >> - if (MyFlags & ME_FATALERROR) >> - thd->is_fatal_error= 1; >> - >> -#ifdef BUG_36098_FIXED >> - mysql_audit_general(thd,MYSQL_AUDIT_GENERAL_ERROR,error,my_time(0), >> - 0,0,str,str ? strlen(str) : 0, >> - thd->query,thd->query_length, >> - thd->variables.character_set_client, >> - thd->row_count); >> -#endif >> - >> - >> - /* >> - TODO: There are two exceptions mechanism (THD and sp_rcontext), >> - this could be improved by having a common stack of handlers. >> - */ >> - if (thd->handle_error(error, str, level)) >> - DBUG_VOID_RETURN; >> - >> - if (level == MYSQL_ERROR::WARN_LEVEL_WARN) >> - push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN, error, str); >> - if (level != MYSQL_ERROR::WARN_LEVEL_ERROR) >> - goto to_error_log; >> - >> - thd->is_slave_error= 1; // needed to catch query errors during > replication >> - >> - /* >> - thd->lex->current_select == 0 if lex structure is not inited >> - (not query command (COM_QUERY)) >> - */ >> - if (thd->lex->current_select && >> - thd->lex->current_select->no_error && > !thd->is_fatal_error) >> - { >> - DBUG_PRINT("error", >> - ("Error converted to warning: current_select: no_error %d " >> - "fatal_error: %d", >> - (thd->lex->current_select ? >> - thd->lex->current_select->no_error : 0), >> - (int) thd->is_fatal_error)); >> - } >> - else >> - { >> - if (! thd->main_da.is_error()) // Return only first message >> - { >> - if (error == 0) >> - error= ER_UNKNOWN_ERROR; >> - if (str == NULL) >> - str= ER(error); >> - thd->main_da.set_error_status(thd, error, str); >> - } >> - query_cache_abort(&thd->query_cache_tls); >> - } >> - /* >> - If a continue handler is found, the error message will be cleared >> - by the stored procedures code. >> - */ >> - if (!thd->is_fatal_error && thd->spcont && >> - ! (MyFlags & ME_NO_SP_HANDLER) && >> - thd->spcont->handle_error(error, MYSQL_ERROR::WARN_LEVEL_ERROR, > thd)) >> - { >> - /* >> - Do not push any warnings, a handled error must be completely >> - silenced. >> - */ >> - DBUG_VOID_RETURN; >> - } >> - >> - if (!thd->is_fatal_error && !thd->no_warnings_for_error > && >> - !(MyFlags & ME_NO_WARNING_FOR_ERROR)) >> - { >> - /* >> - Suppress infinite recursion if there a memory allocation error >> - inside push_warning. >> - */ >> - thd->no_warnings_for_error= TRUE; >> - push_warning(thd, MYSQL_ERROR::WARN_LEVEL_ERROR, error, str); >> - thd->no_warnings_for_error= FALSE; >> - } >> + thd->raise_error(error, str, MyFlags); >> > > Good. > > >> } >> -to_error_log: >> - if (!thd || (MyFlags & ME_NOREFRESH)) >> - (*func)("%s: %s", my_progname_short, str); /* purecov: inspected */ >> + if (!thd || MyFlags & ME_NOREFRESH) >> + sql_print_error("%s: %s", my_progname, str); /* purecov: inspected */ >> DBUG_VOID_RETURN; >> } >> >> @@ -3223,6 +3163,7 @@ SHOW_VAR com_status_vars[]= { >> {"replace", (char*) offsetof(STATUS_VAR, com_stat[(uint) > SQLCOM_REPLACE]), SHOW_LONG_STATUS}, >> {"replace_select", (char*) offsetof(STATUS_VAR, com_stat[(uint) > SQLCOM_REPLACE_SELECT]), SHOW_LONG_STATUS}, >> {"reset", (char*) offsetof(STATUS_VAR, com_stat[(uint) > SQLCOM_RESET]), SHOW_LONG_STATUS}, >> + {"resignal", (char*) offsetof(STATUS_VAR, com_stat[(uint) > SQLCOM_RESIGNAL]), SHOW_LONG_STATUS}, >> {"restore", (char*) offsetof(STATUS_VAR, com_stat[(uint) > SQLCOM_RESTORE]), SHOW_LONG_STATUS}, >> {"revoke", (char*) offsetof(STATUS_VAR, com_stat[(uint) > SQLCOM_REVOKE]), SHOW_LONG_STATUS}, >> {"revoke_all", (char*) offsetof(STATUS_VAR, com_stat[(uint) > SQLCOM_REVOKE_ALL]), SHOW_LONG_STATUS}, >> @@ -3231,6 +3172,7 @@ SHOW_VAR com_status_vars[]= { >> {"savepoint", (char*) offsetof(STATUS_VAR, com_stat[(uint) > SQLCOM_SAVEPOINT]), SHOW_LONG_STATUS}, >> {"select", (char*) offsetof(STATUS_VAR, com_stat[(uint) > SQLCOM_SELECT]), SHOW_LONG_STATUS}, >> {"set_option", (char*) offsetof(STATUS_VAR, com_stat[(uint) > SQLCOM_SET_OPTION]), SHOW_LONG_STATUS}, >> + {"signal", (char*) offsetof(STATUS_VAR, com_stat[(uint) > SQLCOM_SIGNAL]), SHOW_LONG_STATUS}, >> {"show_authors", (char*) offsetof(STATUS_VAR, com_stat[(uint) > SQLCOM_SHOW_AUTHORS]), SHOW_LONG_STATUS}, >> {"show_binlog_events", (char*) offsetof(STATUS_VAR, com_stat[(uint) > SQLCOM_SHOW_BINLOG_EVENTS]), SHOW_LONG_STATUS}, >> {"show_binlogs", (char*) offsetof(STATUS_VAR, com_stat[(uint) > SQLCOM_SHOW_BINLOGS]), SHOW_LONG_STATUS}, >> @@ -4932,6 +4874,8 @@ void create_thread_to_handle_connection( >> handle_one_connection, >> (void*) thd))) >> { >> + const char* sqlstate; >> + >> /* purecov: begin inspected */ >> DBUG_PRINT("error", >> ("Can't create thread to handle request (error %d)", >> @@ -4948,7 +4892,8 @@ void create_thread_to_handle_connection( >> /* Can't use my_error() since store_globals has not been called. */ >> my_snprintf(error_message_buff, sizeof(error_message_buff), >> ER(ER_CANT_CREATE_THREAD), error); >> - net_send_error(thd, ER_CANT_CREATE_THREAD, error_message_buff); >> + sqlstate= mysql_errno_to_sqlstate(ER_CANT_CREATE_THREAD); >> + net_send_error(thd, ER_CANT_CREATE_THREAD, error_message_buff, sqlstate); >> (void) pthread_mutex_lock(&LOCK_thread_count); >> close_connection(thd,0,0); >> delete thd; >> >> === modified file 'sql/share/errmsg.txt' >> --- a/sql/share/errmsg.txt 2008-07-09 07:12:43 +0000 >> +++ b/sql/share/errmsg.txt 2008-07-23 00:25:11 +0000 >> @@ -6372,3 +6372,28 @@ ER_BACKUP_OBTAIN_NAME_LOCK_FAILED >> eng "Restore failed to obtain the name locks on the tables." >> ER_BACKUP_RELEASE_NAME_LOCK_FAILED >> eng "Restore failed to release the name locks on the tables." >> + >> +ER_DUP_SIGNAL_SET 42000 >> + eng "Duplicate condition information item: %s" >> > > I think it's better to use a more descriptive message. How about > something like: "The condition information item '%s' was specified > more than once". In any case, don't forget to add apostrophe between > the conversion specification. > > The above applies for all error messages added here. > > Fixed %s -> '%s' in error texts. Now, if we want to change the error wording itself, I will wait for other opinions before making any change. >> + >> +ER_SIGNAL_WARN 01000 >> + eng "Unhandled user-defined warning" >> + >> +ER_SIGNAL_NOT_FOUND 02000 >> + eng "Unhandled user-defined not found" >> + >> +ER_SIGNAL_EXCEPTION HY000 >> + eng "Unhandled user-defined exception" >> + >> +ER_RESIGNAL_NO_HANDLER 0K000 >> + eng "RESIGNAL when handler not active" >> + >> +ER_SIGNAL_BAD_CONDITION_TYPE >> + eng "SIGNAL/RESIGNAL can only use a CONDITION defined with SQLSTATE" >> + >> +WARN_COND_ITEM_TRUNCATED >> + eng "Data truncated for condition item %s" >> + >> +ER_COND_ITEM_TOO_LONG >> + eng "Data too long for condition item %s" >> + >> > > >> === modified file 'sql/sql_class.cc' >> --- a/sql/sql_class.cc 2008-07-14 22:06:19 +0000 >> +++ b/sql/sql_class.cc 2008-07-23 00:25:11 +0000 >> @@ -206,8 +206,9 @@ bool foreign_key_prefix(Key *a, Key *b) >> bool >> Reprepare_observer::report_error(THD *thd) >> { >> - my_error(ER_NEED_REPREPARE, MYF(ME_NO_WARNING_FOR_ERROR|ME_NO_SP_HANDLER)); >> - >> + /* No handler, no error in the condition area */ >> + thd->main_da.set_error_status(thd, ER_NEED_REPREPARE, >> + ER(ER_NEED_REPREPARE), "HY000"); >> m_invalidated= TRUE; >> >> return TRUE; >> @@ -370,6 +371,16 @@ char *thd_security_context(THD *thd, cha >> return thd->strmake(str.ptr(), str.length()); >> } >> >> +void Diagnostics_stmt_area::clear() >> +{ >> + m_number= 0; >> + m_more= FALSE; >> + m_rowcount= 0; >> + >> + warn_list.empty(); >> + bzero((char*) warn_count, sizeof(warn_count)); >> > > memset > > Fixed >> +} >> + >> /** >> Clear this diagnostics area. >> >> @@ -385,6 +396,7 @@ Diagnostics_area::reset_diagnostics_area >> /** Don't take chances in production */ >> m_message[0]= '\0'; >> m_sql_errno= 0; >> + memset(m_sqlstate, 0, sizeof(m_sqlstate)); >> m_server_status= 0; >> m_affected_rows= 0; >> m_last_insert_id= 0; >> @@ -396,7 +408,6 @@ Diagnostics_area::reset_diagnostics_area >> DBUG_VOID_RETURN; >> } >> >> - >> /** >> Set OK status -- ends commands that do not return a >> result set, e.g. INSERT/UPDATE/DELETE. >> @@ -425,6 +436,12 @@ Diagnostics_area::set_ok_status(THD *thd >> else >> m_message[0]= '\0'; >> m_status= DA_OK; >> + >> + if (! m_stmt_area.is_read_only()) >> + { >> + m_stmt_area.m_number++; >> + m_stmt_area.m_rowcount= m_affected_rows; >> + } >> DBUG_VOID_RETURN; >> } >> >> @@ -463,9 +480,18 @@ Diagnostics_area::set_eof_status(THD *th >> */ >> >> void >> -Diagnostics_area::set_error_status(THD *thd, uint sql_errno_arg, >> +Diagnostics_area::set_default_error_status(THD *thd, uint sql_errno_arg, >> const char *message_arg) >> { >> + const char* sqlstate= mysql_errno_to_sqlstate(sql_errno_arg); >> + set_error_status(thd, sql_errno_arg, message_arg, sqlstate); >> +} >> + >> +void >> +Diagnostics_area::set_error_status(THD *thd, uint sql_errno_arg, >> + const char *message_arg, >> + const char *sqlstate) >> > > Hum, can we have also a inline set_error_status that retrieves the > error message using ER(sql_errno_arg)? > > calling set_error_status directly instead of reporting a condition with my_error() or thd->raise_xxx() is not a normal path. I would rather not provide helpers that encourage developers to do the wrong thing. >> +{ >> DBUG_ENTER("set_error_status"); >> /* >> Only allowed to report error if has not yet reported a success >> @@ -483,8 +509,9 @@ Diagnostics_area::set_error_status(THD * >> #endif >> >> m_sql_errno= sql_errno_arg; >> + strncpy(m_sqlstate, sqlstate, SQLSTATE_LENGTH); >> + m_sqlstate[SQLSTATE_LENGTH]= '\0'; >> strmake(m_message, message_arg, sizeof(m_message)-1); >> - >> m_status= DA_ERROR; >> DBUG_VOID_RETURN; >> } >> @@ -528,6 +555,7 @@ THD::THD() >> bootstrap(0), >> derived_tables_processing(FALSE), >> spcont(NULL), >> + end_partial_result_set(FALSE), >> m_lip(NULL), >> /* >> @todo The following is a work around for online backup and the DDL blocker. >> @@ -536,6 +564,7 @@ THD::THD() >> when the DDL blocker is engaged. >> */ >> DDL_exception(FALSE), >> + m_tmp_syn(NULL), >> #if defined(ENABLED_DEBUG_SYNC) >> debug_sync_control(0), >> #endif /* defined(ENABLED_DEBUG_SYNC) */ >> @@ -666,12 +695,11 @@ void THD::push_internal_handler(Internal >> } >> >> >> -bool THD::handle_error(uint sql_errno, const char *message, >> - MYSQL_ERROR::enum_warning_level level) >> +bool THD::handle_condition(const SQL_condition *cond) >> { >> if (m_internal_handler) >> { >> - return m_internal_handler->handle_error(sql_errno, message, level, > this); >> + return m_internal_handler->handle_condition(this, cond); >> } >> >> return FALSE; // 'FALSE', as per coding style >> @@ -684,6 +712,222 @@ void THD::pop_internal_handler() >> m_internal_handler= NULL; >> } >> >> +void THD::raise_error(uint code, const char *str, myf MyFlags) >> +{ >> + SQL_condition cond(this->mem_root); >> + cond.set(this, code, str, MYSQL_ERROR::WARN_LEVEL_ERROR, MyFlags); >> + >> > > Drop spurious new line (you do the same in other places..) > > Changed (and in other places also) >> + raise_condition(& cond); >> +} >> + >> +void THD::raise_error_printf(uint code, const char *format, myf MyFlags, ...) >> +{ >> + va_list args; >> + char ebuff[ERRMSGSIZE+20]; >> + >> + >> > > Drop spurious new line. > > Changed >> + DBUG_ENTER("THD::raise_error_printf"); >> + DBUG_PRINT("my", ("nr: %d MyFlags: %d errno: %d Format: %s", >> + code, MyFlags, errno, format)); >> + >> + va_start(args, MyFlags); >> + (void) my_vsnprintf (ebuff, sizeof(ebuff), format, args); >> > > Drop (void). > > Changed >> + va_end(args); >> + >> + SQL_condition cond(this->mem_root); >> + cond.set(this, code, ebuff, MYSQL_ERROR::WARN_LEVEL_ERROR, MyFlags); >> + >> > > Drop spurious new line. Same applies for above. > > Changed >> + raise_condition(& cond); >> + DBUG_VOID_RETURN; >> +} >> + >> +void THD::raise_warning(uint code, const char *msg) >> +{ >> + SQL_condition cond(this->mem_root); >> + cond.set(this, code, msg, MYSQL_ERROR::WARN_LEVEL_WARN, MYF(0)); >> + >> + raise_condition(& cond); >> +} >> + >> +void THD::raise_warning_printf(uint code, const char *format, ...) >> +{ >> + va_list args; >> + char ebuff[ERRMSGSIZE+20]; >> + >> + >> + DBUG_ENTER("THD::raise_warning_printf"); >> + DBUG_PRINT("enter", ("warning: %u", code)); >> + >> + va_start(args, format); >> + my_vsnprintf(ebuff, sizeof(ebuff), format, args); >> + va_end(args); >> + >> + SQL_condition cond(this->mem_root); >> + cond.set(this, code, ebuff, MYSQL_ERROR::WARN_LEVEL_WARN, MYF(0)); >> + >> + raise_condition(& cond); >> + DBUG_VOID_RETURN; >> +} >> + >> +void THD::raise_note(uint code, const char *msg) >> +{ >> + DBUG_ENTER("THD::raise_note"); >> + DBUG_PRINT("enter", ("code: %d, msg: %s", code, msg)); >> + >> + if (!(this->options & OPTION_SQL_NOTES)) >> + DBUG_VOID_RETURN; >> + >> + SQL_condition cond(this->mem_root); >> + cond.set(this, code, msg, MYSQL_ERROR::WARN_LEVEL_NOTE, MYF(0)); >> + >> + raise_condition(& cond); >> + DBUG_VOID_RETURN; >> +} >> + >> +void THD::raise_note_printf(uint code, const char *format, ...) >> +{ >> + va_list args; >> + char ebuff[ERRMSGSIZE+20]; >> + >> + >> + DBUG_ENTER("THD::raise_note_printf"); >> + DBUG_PRINT("enter",("code: %u", code)); >> + >> + if (!(this->options & OPTION_SQL_NOTES)) >> + DBUG_VOID_RETURN; >> + >> + va_start(args, format); >> + my_vsnprintf(ebuff, sizeof(ebuff), format, args); >> + va_end(args); >> + >> + SQL_condition cond(this->mem_root); >> + cond.set(this, code, ebuff, MYSQL_ERROR::WARN_LEVEL_NOTE, MYF(0)); >> + >> + raise_condition(& cond); >> + DBUG_VOID_RETURN; >> +} >> + >> +void THD::raise_condition(const SQL_condition *cond) >> +{ >> + const char* msg= cond->get_message_text(); >> + >> + DBUG_ENTER("THD::raise_condition"); >> + >> + if (!(this->options & OPTION_SQL_NOTES) && >> + (cond->m_level == MYSQL_ERROR::WARN_LEVEL_NOTE)) >> + DBUG_VOID_RETURN; >> + >> + if (query_id != warn_id && !spcont) >> + mysql_reset_errors(this, 0); >> + >> + switch (cond->m_level) >> + { >> + case MYSQL_ERROR::WARN_LEVEL_NOTE: >> + case MYSQL_ERROR::WARN_LEVEL_WARN: >> + got_warning= 1; >> + break; >> + case MYSQL_ERROR::WARN_LEVEL_ERROR: >> + if (cond->m_flags & ME_FATALERROR) >> + is_fatal_error= 1; >> + break; >> + default: >> + DBUG_ASSERT(FALSE); >> + } >> + >> + if (handle_condition(cond)) >> + DBUG_VOID_RETURN; >> + >> + if (cond->m_level == MYSQL_ERROR::WARN_LEVEL_ERROR) >> + { >> + is_slave_error= 1; // needed to catch query errors during replication >> + >> + /* >> + thd->lex->current_select == 0 if lex structure is not inited >> + (not query command (COM_QUERY)) >> + */ >> + if (lex->current_select && >> + lex->current_select->no_error && !is_fatal_error) >> + { >> + DBUG_PRINT("error", >> + ("Error converted to warning: current_select: no_error %d " >> + "fatal_error: %d", >> + (lex->current_select ? >> + lex->current_select->no_error : 0), >> + (int) is_fatal_error)); >> > > This message doesn't make much sense... > > I totally agree with that. However, this was existing code in my_message_sql() that got moved: this code has been left unchanged. Cleaning how the implementation of select interfere with the error/warning component is an unrelated task. No change. >> + } >> + else if (! main_da.is_error()) >> + main_da.set_error_status(this, cond->m_sql_errno, >> + msg, cond->get_sqlstate()); >> + } >> + >> + /* >> + If a continue handler is found, the error message will be cleared >> + by the stored procedures code. >> + */ >> + if (!is_fatal_error && spcont && >> + spcont->handle_condition(this, cond)) >> + { >> + /* >> + Do not push any warnings, a handled error must be completely >> + silenced. >> + */ >> + DBUG_VOID_RETURN; >> + } >> + >> + /* Un-handled conditions */ >> + >> + raise_condition_no_handler(cond); >> + DBUG_VOID_RETURN; >> +} >> + >> +void THD::raise_condition_no_handler(const SQL_condition *cond) >> +{ >> + DBUG_ENTER("THD::raise_condition_no_handler"); >> + >> + query_cache_abort(& query_cache_tls); >> + >> + /* FIXME: broken special case */ >> + if (no_warnings_for_error && (cond->m_level == > MYSQL_ERROR::WARN_LEVEL_ERROR)) >> + DBUG_VOID_RETURN; >> + >> +#ifdef BUG_36098_FIXED >> + mysql_audit_general(thd, MYSQL_AUDIT_GENERAL_ERROR, error, my_time(0), >> + 0, 0, msg, msg ? strlen(msg) : 0, >> + query, query_length, >> + variables.character_set_client, >> + row_count); >> > > Hum.. this used to be before the various checks. Are you sure you > can move it? > > Given that there is controversy, I just removed the reference of mysql_audit_general from this code, to avoid giving the false impression that the fix is only to remove the #ifdef. The code was not compiling anyway due to changes, and more analysis should be done when mysql_audit_general is properly implemented. >> +#endif >> + >> + if (! main_da.m_stmt_area.is_read_only()) >> + { >> + MYSQL_ERROR::enum_warning_level level; >> + >> + /* FIXME: broken special case, waiting for bug#36777 */ >> + level= (cond->m_broken_caller) ? >> + MYSQL_ERROR::WARN_LEVEL_ERROR : cond->m_level; >> + >> + if (main_da.m_stmt_area.warn_list.elements < variables.max_error_count) >> + { >> + /* We have to use warn_root, as mem_root is freed after each query */ >> + SQL_condition *stored_cond; >> + stored_cond= SQL_condition::deep_copy(this, & warn_root, cond); >> > > I get the impression that we should just drop warn_root. It doesn't > make much sense and complicates things. SQL_condition is the type of > object that begs for a simple slab.. > > Things are not as simple. SQL_condition are allocated - in the warn_root, when they are not caught anywhere - in a stored procedure sp_head::mem_root, when caught in a stored procedure handler (see patch 5/10) To remove warn_root and use malloc/free instead, malloc/free would have to be used in stored procedures also, which is yet another story. The MEM_ROOT implementation might be ugly, but it also provide a reasonably robust way to prevent memory leaks, that calling alloc/free does not provide. No change. >> + if (stored_cond) >> + { >> + stored_cond->m_level= level; >> + main_da.m_stmt_area.warn_list.push_back(stored_cond, & warn_root); >> + main_da.m_stmt_area.m_number++; >> + } >> + } >> + else >> + main_da.m_stmt_area.m_more= TRUE; >> + >> + main_da.m_stmt_area.warn_count[(uint) level]++; >> + } >> + >> + total_warn_count++; >> + DBUG_VOID_RETURN; >> +} >> + >> extern "C" >> void *thd_alloc(MYSQL_THD thd, unsigned int size) >> { >> @@ -766,8 +1010,8 @@ void THD::init(void) >> TL_WRITE_LOW_PRIORITY : >> TL_WRITE); >> session_tx_isolation= (enum_tx_isolation) variables.tx_isolation; >> - warn_list.empty(); >> - bzero((char*) warn_count, sizeof(warn_count)); >> + main_da.reset_diagnostics_area(); >> + main_da.m_stmt_area.clear(); >> total_warn_count= 0; >> update_charset(); >> reset_current_stmt_binlog_row_based(); >> @@ -1566,9 +1810,8 @@ bool select_send::send_fields(List<Item> >> void select_send::abort() >> { >> DBUG_ENTER("select_send::abort"); >> - if (is_result_set_started && thd->spcont && >> - thd->spcont->find_handler(thd, thd->main_da.sql_errno(), >> - MYSQL_ERROR::WARN_LEVEL_ERROR)) >> + >> + if (is_result_set_started && thd->spcont) >> { >> /* >> We're executing a stored procedure, have an open result >> @@ -1580,7 +1823,7 @@ void select_send::abort() >> otherwise the client will hang due to the violation of the >> client/server protocol. >> */ >> - thd->protocol->end_partial_result_set(thd); >> + thd->end_partial_result_set= TRUE; >> } >> DBUG_VOID_RETURN; >> } >> >> === modified file 'sql/sql_class.h' >> --- a/sql/sql_class.h 2008-07-14 22:06:19 +0000 >> +++ b/sql/sql_class.h 2008-07-23 00:25:11 +0000 >> @@ -78,6 +78,7 @@ class Slave_log_event; >> class sp_rcontext; >> class sp_cache; >> class Lex_input_stream; >> +class Tmp_syn; >> class Rows_log_event; >> >> enum enum_enable_or_disable { LEAVE_AS_IS, ENABLE, DISABLE }; >> @@ -303,6 +304,119 @@ struct Query_cache_tls >> Query_cache_tls() :first_query_block(NULL) {} >> }; >> >> +/* SIGNAL / RESIGNAL / GET DIAGNOSTICS */ >> + >> +#define FIRST_DIAG_SET_PROPERTY 0 >> +#define LAST_DIAG_SET_PROPERTY 11 >> + >> +#define FIRST_DIAG_PROPERTY 0 >> +#define LAST_DIAG_PROPERTY 28 >> > > These defines are not necessary, use a const. > > Removed >> +/** >> + This enumeration list all the condition item names of a condition in the >> + SQL condition area. >> +*/ >> +typedef enum enum_diag_condition_item_name >> +{ >> + /* >> + Conditions that can be set by the user (SIGNAL/RESIGNAL), >> + and by the server implementation (THD::raise_ER_XXX). >> + */ >> + >> + DIAG_CLASS_ORIGIN= 0, >> > > FIRST_DIAG_SET_PROPERTY = DIAG_CLASS_ORIGIN > > Fixed >> + DIAG_SUBCLASS_ORIGIN= 1, >> + DIAG_CONSTRAINT_CATALOG= 2, >> + DIAG_CONSTRAINT_SCHEMA= 3, >> + DIAG_CONSTRAINT_NAME= 4, >> + DIAG_CATALOG_NAME= 5, >> + DIAG_SCHEMA_NAME= 6, >> + DIAG_TABLE_NAME= 7, >> + DIAG_COLUMN_NAME= 8, >> + DIAG_CURSOR_NAME= 9, >> + DIAG_MESSAGE_TEXT= 10, >> + DIAG_MYSQL_ERRNO= 11, >> > > LAST_DIAG_SET_PROPERTY = DIAG_MYSQL_ERRNO > > Fixed >> + >> + /* >> + Conditions that can be set only by the server implementation >> + (THD::raise_ER_XXX). >> + */ >> + >> + DIAG_CONDITION_IDENTIFIER= 12, >> + DIAG_CONDITION_NUMBER= 13, >> + DIAG_CONNECTION_NAME= 14, >> + DIAG_MESSAGE_LENGTH= 15, >> + DIAG_MESSAGE_OCTET_LENGTH= 16, >> + DIAG_PARAMETER_MODE= 17, >> + DIAG_PARAMETER_NAME= 18, >> + DIAG_PARAMETER_ORDINAL_POSITION= 19, >> + DIAG_RETURNED_SQLSTATE= 20, >> + DIAG_ROUTINE_CATALOG= 21, >> + DIAG_ROUTINE_NAME= 22, >> + DIAG_ROUTINE_SCHEMA= 23, >> + DIAG_SERVER_NAME= 24, >> + DIAG_SPECIFIC_NAME= 25, >> + DIAG_TRIGGER_CATALOG= 26, >> + DIAG_TRIGGER_NAME= 27, >> + DIAG_TRIGGER_SCHEMA= 28, >> +} Diag_condition_item_name; >> + >> +/** >> + Name of each diagnostic condition item. >> + This array is indexed by Diag_condition_item_name. >> +*/ >> +extern const LEX_STRING Diag_condition_item_names[]; >> > > I couldn't find any other uses for this.. > > Removed the unused DIAG_xxx. These condition items are not strictly required for SIGNAL / RESIGNAL, they are only used for GET DIAGNOSTICS. Diag_condition_item_names is used in sql_yacc.yy, when raising the error ER_DUP_SIGNAL_SET. >> + >> +/** >> + Set_signal_information is a container used in the parsed tree to represent >> + the collection of assignments to condition items in the SIGNAL and RESIGNAL >> + statements. >> +*/ >> +class Set_signal_information >> +{ >> +public: >> + /** Constructor. */ >> + Set_signal_information(); >> + >> + /** Copy constructor. */ >> + Set_signal_information(const Set_signal_information& set); >> + >> + /** Destructor. */ >> + ~Set_signal_information() >> + {} >> + >> + /** Clear all items. */ >> + void clear(); >> + >> + /** >> + For each contition item assignment, m_item[] contains the parsed tree >> + that represents the expression assigned, if any. >> + m_item[] is an array indexed by Diag_condition_item_name. >> + */ >> + Item *m_item[LAST_DIAG_SET_PROPERTY+1]; >> +}; >> + >> + >> +/** >> + Parser temporary internal state, used during syntax analysis. >> +*/ >> +/* FIXME: Waiting for Bug#35577 to be merged, to use Yacc_state */ >> +class Tmp_syn >> +{ >> +public: >> + Tmp_syn() >> + : m_set_signal_info() >> + {} >> + >> + ~Tmp_syn() >> + {} >> + >> + /** >> + Fragments of parsed tree, >> + used during the parsing of SIGNAL and RESIGNAL. >> + */ >> + Set_signal_information m_set_signal_info; >> +}; >> + >> #include "sql_lex.h" /* Must be here */ >> >> class Delayed_insert; >> @@ -1073,6 +1187,202 @@ enum enum_thread_type >> >> >> /** >> + Representation of a SQL condition. >> + A SQL condition can be a completion condition (note, warning), >> + or an exception contition (error, not found). >> +*/ >> +class SQL_condition : public Sql_alloc >> +{ >> +public: >> + /** >> + Constructor. >> + @param mem_root The memory root to use for the condition items >> + of this condition >> + */ >> + SQL_condition(MEM_ROOT *mem_root); >> + >> + /** Destructor. */ >> + ~SQL_condition() >> + {} >> + >> + /** >> + Deep copy (static method). >> + Builds a copy of a condition using a given memory root. >> + 'Deep copy' is useful to propagate SQL conditions raised from a short >> + lived runtime environment to a parent execution environment with a longer >> + life cycle. >> + For example, when proc_p1() calls proc_p2(), an exception raised in >> + proc_p2() should be copied when caught in proc_p1(), >> + before destroying the proc_p2() memory root. >> + @param thd the current thread. >> + @param mem_root the memory root to use for memory allocation. >> + @param cond the condition to copy. >> + @return the duplicated condition. >> + */ >> + static SQL_condition* deep_copy(THD *thd, MEM_ROOT *mem_root, >> + const SQL_condition *cond); >> + >> + /** >> + Deep copy (instance method). >> + @param cond the condition to copy. >> + */ >> + void deep_copy(const SQL_condition *cond); >> + >> + >> + /** >> + Set this condition area with a fixed message text. >> + @param thd the current thread. >> + @param code the error number for this condition. >> + @param str the message text for this condition. >> + @param level the error level for this condition. >> + @param MyFlags additional flags. >> + */ >> + void set(THD *thd, uint code, const char *str, >> + MYSQL_ERROR::enum_warning_level level, myf MyFlags); >> > > Hum, I think it would be better to only set public > set_warning/error/note interfaces and make this one private. > > This is a public method of SQL_condition, used by the SIGNAL implementation (SQLCOM_xxx). How to raise a condition (SIGNAL, among others) and what a condition is (SQL_condition) are two separate classes as it should be. SIGNAL is more generic, and accesses a more generic set method(), but nothing is exposed in this method that can't be done otherwise, so there is no breaking of the encapsulation here. No change. >> + /** >> + Set this condition area with formatting of the message text. >> + @param thd the current thread. >> + @param code the error number for this condition. >> + @param str the message text printf format for this condition. >> + @param level the error level for this condition. >> + @param MyFlags additional flags. >> + */ >> + void set_printf(THD *thd, uint code, const char *str, >> + MYSQL_ERROR::enum_warning_level level, myf MyFlags, ...); >> + >> + /** >> + Set the condition message test. >> + @param str Message text, expressed in the character set derived from >> + the server --language option >> + */ >> + void set_builtin_message_text(const char* str); >> + >> + /** >> + Get the MESSAGE_TEXT of this condition. >> + @return the message text. >> + */ >> + const char* get_message_text() const; >> + >> + /** >> + Get the MESSAGE_OCTET_LENGTH of this condition. >> + @return the length in bytes of the message text. >> + */ >> + int get_message_octet_length() const; >> + >> + /** Set the SQLSTATE of this condition. */ >> + void set_sqlstate(const char* sqlstate); >> + >> + /** >> + Get the SQLSTATE of this condition. >> + @return the sql state. >> + */ >> + const char* get_sqlstate() const >> + { return m_returned_sqlstate; } >> + >> +public: >> + /** SQL CLASS_ORIGIN condition item. */ >> + UTF8String64 m_class_origin; >> + >> + /** SQL SUBCLASS_ORIGIN condition item. */ >> + UTF8String64 m_subclass_origin; >> + >> + /** SQL CONSTRAINT_CATALOG condition item. */ >> + UTF8String64 m_constraint_catalog; >> + >> + /** SQL CONSTRAINT_SCHEMA condition item. */ >> + UTF8String64 m_constraint_schema; >> + >> + /** SQL CONSTRAINT_NAME condition item. */ >> + UTF8String64 m_constraint_name; >> + >> + /** SQL CATALOG_NAME condition item. */ >> + UTF8String64 m_catalog_name; >> + >> + /** SQL SCHEMA_NAME condition item. */ >> + UTF8String64 m_schema_name; >> + >> + /** SQL TABLE_NAME condition item. */ >> + UTF8String64 m_table_name; >> + >> + /** SQL COLUMN_NAME condition item. */ >> + UTF8String64 m_column_name; >> + >> + /** SQL CURSOR_NAME condition item. */ >> + UTF8String64 m_cursor_name; >> > > Hum.. I personally don't like this proliferation of string > parameters. Can't we make this stuff smarter? encapsulating it in > classes and using a array. This also needlessly bloats the class and > we shouldn't keep things that aren't necessary. > > Let's discuss this more in-depth on IRC. > > Discussed on IRC privately. I'll wait to see if you have a practical proposal, to evaluate which solution is best. For the record: setting the condition area can be done by mostly 2 paths: - in a generic way, using the SIGNAL statement, - in a non generic way, when the internal implementation of the server raises a condition (in practice, any call to my_error(), push_warning(), or thd->raise_ER_XXX()). Changing the code to be more "generic" or more "specific" is only doing a favor to 1 path at the expense of the other, so looking at 1 code fragment alone to make a conclusion can only lead to wrong conclusions. No change. >> +private: >> + /** Message text, expressed in the character set implied by --language. */ >> + String m_message_text; >> + >> +public: >> + /** MySQL extension, MYSQL_ERRNO condition item. */ >> + int m_sql_errno; >> + >> + /** SQL CONDITION_IDENTIFIER condition item. */ >> + UTF8String64 m_condition_identifier; >> + >> + /** SQL CONNECTION_NAME condition item. */ >> + UTF8String64 m_connection_name; >> + >> + /** SQL PARAMETER_MODE condition item. */ >> + UTF8String64 m_parameter_mode; >> + >> + /** SQL PARAMETER_NAME condition item. */ >> + UTF8String64 m_parameter_name; >> + >> + /** SQL PARAMETER_ORDINAL_POSITION condition item. */ >> + int m_parameter_ordinal_position; >> + >> +private: >> + /** >> + SQL RETURNED_SQLSTATE condition item. >> + This member is always NUL terminated. >> + */ >> + char m_returned_sqlstate[SQLSTATE_LENGTH+1]; >> + >> +public: >> + /** SQL ROUTINE_CATALOG condition item. */ >> + UTF8String64 m_routine_catalog; >> + >> + /** SQL ROUTINE_NAME condition item. */ >> + UTF8String64 m_routine_name; >> + >> + /** SQL ROUTINE_SCHEMA condition item. */ >> + UTF8String64 m_routine_schema; >> + >> + /** SQL SERVER_NAME condition item. */ >> + UTF8String64 m_server_name; >> + >> + /** SQL SPECIFIC_NAME condition item. */ >> + UTF8String64 m_specific_name; >> + >> + /** SQL TRIGGER_CATALOG condition item. */ >> + UTF8String64 m_trigger_catalog; >> + >> + /** SQL TRIGGER_NAME condition item. */ >> + UTF8String64 m_trigger_name; >> + >> + /** SQL TRIGGER_SCHEMA condition item. */ >> + UTF8String64 m_trigger_schema; >> + >> + /** Severity (error, warning, note) of this condition. */ >> + MYSQL_ERROR::enum_warning_level m_level; >> + >> + /** Additional flags. */ >> + myf m_flags; >> + >> + /** Memory root to use to hold condition item values. */ >> + MEM_ROOT *m_mem_root; >> + >> + /* >> + FIXME: push_warning() called with a level of ERROR: >> + waiting for bug#36777 to be merged to remove this. >> + */ >> + bool m_broken_caller; >> +}; >> + >> +/** >> This class represents the interface for internal error handlers. >> Internal error handlers are exception handlers used by the server >> implementation. >> @@ -1085,12 +1395,12 @@ protected: >> >> public: >> /** >> - Handle an error condition. >> + Handle a sql condition. >> This method can be implemented by a subclass to achieve any of the >> following: >> - - mask an error internally, prevent exposing it to the user, >> - - mask an error and throw another one instead. >> - When this method returns true, the error condition is considered >> + - mask a warning/error internally, prevent exposing it to the user, >> + - mask a warning/error and throw another one instead. >> + When this method returns true, the sql condition is considered >> 'handled', and will not be propagated to upper layers. >> It is the responsability of the code installing an internal handler >> to then check for trapped conditions, and implement logic to recover >> @@ -1104,15 +1414,102 @@ public: >> before removing it from the exception stack with >> <code>THD::pop_internal_handler()</code>. >> >> - @param sql_errno the error number >> - @param level the error level >> @param thd the calling thread >> - @return true if the error is handled >> + @param cond the condition raised. >> + @return true if the condition is handled >> + */ >> + virtual bool handle_condition(THD *thd, const SQL_condition *cond) = 0; >> +}; >> + >> + >> +/** >> + Statement diagnostics area. >> +*/ >> +class Diagnostics_stmt_area >> +{ >> +public: >> + /** Constructor. */ >> + Diagnostics_stmt_area() >> + : m_number(0), >> + m_more(FALSE), >> + m_command_function_cmd(SQLCOM_END), >> + m_dynamic_function_cmd(SQLCOM_END), >> + m_rowcount(0), >> + warn_list(), >> + m_read_only(FALSE) >> + { >> + bzero((char*) warn_count, sizeof(warn_count)); >> > > memset > > Fixed >> + } >> + >> + /** Destructor. */ >> + ~Diagnostics_stmt_area() >> + {} >> + >> + /** Clear the statement area. */ >> + void clear(); >> + >> + /** >> + Set the read only status for this statement area. >> + This is a priviledged operation, reserved for the implementation of >> + diagnostics related statements, to enforce that the statement area is >> + left untouched during execution. >> + The diagnostics statements are: >> + - SHOW WARNINGS >> + - SHOW ERRORS >> + - GET DIAGNOSTICS >> + @param read_only the read only property to set >> + */ >> + void set_read_only(bool read_only) >> + { m_read_only= read_only; } >> + >> + /** >> + Read only status. >> + @return the read only property >> + */ >> + bool is_read_only() const >> + { return m_read_only; } >> + >> + /** SQL 'NUMBER' statement information item. */ >> + uint m_number; >> + >> + /** SQL 'MORE' statement information item. */ >> + bool m_more; >> > > Use? > > Removed. This also was a left over from the GET DIAGNOSTICS prototype, that was missed during the final clean-up. >> + >> + /** >> + Internal command of a statement. >> + The following statement information items: >> + - SQL 'COMMAND_FUNCTION' >> + - SQL 'COMMAND_FUNCTION_CODE' >> + derive from this attribute. >> + */ >> + enum_sql_command m_command_function_cmd; >> + >> + /** >> + Internal command of a statement, >> + for dynamic statements. >> + The following statement information items: >> + - SQL 'DYNAMIC_FUNCTION' >> + - SQL 'DYNAMIC_FUNCTION_CODE' >> + derive from this attribute. >> + */ >> + enum_sql_command m_dynamic_function_cmd; >> + >> + /** SQL 'ROWCOUNT' statement information item. */ >> + ha_rows m_rowcount; >> > > I haven't been able to find a meaningful use for m_rowcount, > m_number, etc in any of the patches. Is something missing or I > missed something? > > Removed. >> + /** >> + Condition area. >> + */ >> + List <SQL_condition> warn_list; >> + >> + /** >> + Counters for errors/warnings/notes in the condition area. >> */ >> - virtual bool handle_error(uint sql_errno, >> - const char *message, >> - MYSQL_ERROR::enum_warning_level level, >> - THD *thd) = 0; >> + uint warn_count[(uint) MYSQL_ERROR::WARN_LEVEL_END]; >> + >> +private: >> + /** Read only status. */ >> + bool m_read_only; >> }; >> >> >> @@ -1148,7 +1545,10 @@ public: >> ulonglong last_insert_id_arg, >> const char *message); >> void set_eof_status(THD *thd); >> - void set_error_status(THD *thd, uint sql_errno_arg, const char *message_arg); >> + void set_default_error_status(THD *thd, uint sql_errno_arg, >> + const char *message_arg); >> + void set_error_status(THD *thd, uint sql_errno_arg, const char *message_arg, >> + const char* sqlstate); >> >> void disable_status(); >> >> @@ -1167,6 +1567,9 @@ public: >> uint sql_errno() const >> { DBUG_ASSERT(m_status == DA_ERROR); return m_sql_errno; } >> >> + const char* get_sqlstate() const >> + { DBUG_ASSERT(m_status == DA_ERROR); return m_sqlstate; } >> + >> uint server_status() const >> { >> DBUG_ASSERT(m_status == DA_OK || m_status == DA_EOF); >> @@ -1196,6 +1599,8 @@ private: >> */ >> uint m_sql_errno; >> >> + char m_sqlstate[SQLSTATE_LENGTH+1]; >> + >> /** >> Copied from thd->server_status when the diagnostics area is assigned. >> We need this member as some places in the code use the following pattern: >> @@ -1224,12 +1629,11 @@ private: >> */ >> ulonglong m_last_insert_id; >> /** The total number of warnings. */ >> - uint m_total_warn_count; >> + uint m_total_warn_count; >> enum_diagnostics_status m_status; >> - /** >> - @todo: the following THD members belong here: >> - - warn_list, warn_count, >> - */ >> + >> +public: >> + Diagnostics_stmt_area m_stmt_area; >> }; >> >> /** >> @@ -1752,15 +2156,7 @@ public: >> table_map used_tables; >> USER_CONN *user_connect; >> CHARSET_INFO *db_charset; >> - /* >> - FIXME: this, and some other variables like 'count_cuted_fields' >> - maybe should be statement/cursor local, that is, moved to Statement >> - class. With current implementation warnings produced in each prepared >> - statement/cursor settle here. >> - */ >> - List <MYSQL_ERROR> warn_list; >> - uint warn_count[(uint) MYSQL_ERROR::WARN_LEVEL_END]; >> - uint total_warn_count; >> + uint total_warn_count; >> Diagnostics_area main_da; >> #if defined(ENABLED_PROFILING) >> PROFILING profiling; >> @@ -1872,6 +2268,8 @@ public: >> my_bool tablespace_op; /* This is TRUE in DISCARD/IMPORT TABLESPACE */ >> >> sp_rcontext *spcont; // SP runtime context >> + bool end_partial_result_set; >> + >> sp_cache *sp_proc_cache; >> sp_cache *sp_func_cache; >> >> @@ -1934,6 +2332,12 @@ public: >> */ >> my_bool DDL_exception; // Allow some DDL if there is an exception >> >> + /** >> + Parser internal temporary state, used during syntax parsing. >> + This member is only valid during parsing. >> + */ >> + Tmp_syn *m_tmp_syn; >> + >> Locked_tables_list locked_tables_list; >> >> #ifdef WITH_PARTITION_STORAGE_ENGINE >> @@ -2355,19 +2759,90 @@ public: >> void push_internal_handler(Internal_error_handler *handler); >> >> /** >> - Handle an error condition. >> - @param sql_errno the error number >> - @param level the error level >> + Handle a sql condition. >> + @param cond the sql condition to handle. >> @return true if the error is handled >> */ >> - virtual bool handle_error(uint sql_errno, const char *message, >> - MYSQL_ERROR::enum_warning_level level); >> + virtual bool handle_condition(const SQL_condition *cond); >> >> /** >> Remove the error handler last pushed. >> */ >> void pop_internal_handler(); >> >> + /** >> + Raise an ER_NO_SUCH_TABLE exception condition. >> + @param db the schema name of the missing table >> + @param table the table name of the missing table >> + */ >> + void raise_ER_NO_SUCH_TABLE(const char* db, const char* table) >> + { >> + SQL_condition cond(this->mem_root); >> + cond.set_printf(this, ER_NO_SUCH_TABLE, ER(ER_NO_SUCH_TABLE), >> + MYSQL_ERROR::WARN_LEVEL_ERROR, MYF(0), >> + db, table); >> + cond.m_schema_name.set(db); >> + cond.m_table_name.set(table); >> + raise_condition(& cond); >> + } >> + >> + /** >> + Raise an exception condition, with a fixed message. >> + @param code the MYSQL_ERRNO error code of the error >> + @param str the MESSAGE_TEXT of the error >> + @param MyFlags additional flags >> + */ >> + void raise_error(uint code, const char *str, myf MyFlags); >> + >> + /** >> + Raise an exception condition, with a formatted message. >> + @param code the MYSQL_ERRNO error code of the error >> + @param format the MESSAGE_TEXT printf format of the error >> + @param MyFlags additional flags >> + */ >> + void raise_error_printf(uint code, const char *format, myf MyFlags, ...); >> + >> + /** >> + Raise a completion condition (warning), with a fixed message. >> + @param code the MYSQL_ERRNO error code of the warning >> + @param str the MESSAGE_TEXT of the warning >> + */ >> + void raise_warning(uint code, const char *str); >> + >> + /** >> + Raise a completion condition (warning), with a formatted message. >> + @param code the MYSQL_ERRNO error code of the warning >> + @param format the MESSAGE_TEXT printf format of the warning >> + */ >> + void raise_warning_printf(uint code, const char *format, ...); >> + >> + /** >> + Raise a completion condition (note), with a fixed message. >> + @param code the MYSQL_ERRNO error code of the note >> + @param str the MESSAGE_TEXT of the note >> + */ >> + void raise_note(uint code, const char *str); >> + >> + /** >> + Raise a completion condition (note), with a formatted message. >> + @param code the MYSQL_ERRNO error code of the note >> + @param format the MESSAGE_TEXT printf format of the note >> + */ >> + void raise_note_printf(uint code, const char *format, ...); >> + >> + /** >> + Raise a generic SQL condition. >> + @param cond the condition to raise >> + */ >> + void raise_condition(const SQL_condition *cond); >> + >> + /** >> + Raise a generic SQL condition, without activation any SQL condition >> + handlers. >> + @param cond the condition to raise >> + */ >> + void raise_condition_no_handler(const SQL_condition *cond); >> + >> private: >> /** The current internal error handler for this thread, or NULL. */ >> Internal_error_handler *m_internal_handler; >> >> === modified file 'sql/sql_error.cc' >> --- a/sql/sql_error.cc 2008-02-19 12:59:19 +0000 >> +++ b/sql/sql_error.cc 2008-07-23 00:25:11 +0000 >> @@ -74,14 +74,17 @@ void MYSQL_ERROR::set_msg(THD *thd, cons >> void mysql_reset_errors(THD *thd, bool force) >> { >> DBUG_ENTER("mysql_reset_errors"); >> + >> + if (thd->main_da.m_stmt_area.is_read_only()) >> + DBUG_VOID_RETURN; >> + >> > > Cool. > > >> if (thd->query_id != thd->warn_id || force) >> { >> thd->warn_id= thd->query_id; >> + thd->main_da.m_stmt_area.clear(); >> free_root(&thd->warn_root,MYF(0)); >> - bzero((char*) thd->warn_count, sizeof(thd->warn_count)); >> if (force) >> thd->total_warn_count= 0; >> - thd->warn_list.empty(); >> thd->row_count= 1; // by default point to row 1 >> } >> DBUG_VOID_RETURN; >> @@ -97,68 +100,40 @@ void mysql_reset_errors(THD *thd, bool f >> level Severity of warning (note, warning, error ...) >> code Error number >> msg Clear error message >> - >> - RETURN >> - pointer on MYSQL_ERROR object >> */ >> >> -MYSQL_ERROR *push_warning(THD *thd, MYSQL_ERROR::enum_warning_level level, >> - uint code, const char *msg) >> +void push_warning(THD *thd, MYSQL_ERROR::enum_warning_level level, >> + uint code, const char *msg) >> { >> - MYSQL_ERROR *err= 0; >> DBUG_ENTER("push_warning"); >> DBUG_PRINT("enter", ("code: %d, msg: %s", code, msg)); >> >> - if (level == MYSQL_ERROR::WARN_LEVEL_NOTE && >> - !(thd->options & OPTION_SQL_NOTES)) >> - DBUG_RETURN(0); >> - >> - if (thd->query_id != thd->warn_id && !thd->spcont) >> - mysql_reset_errors(thd, 0); >> - thd->got_warning= 1; >> - >> - /* Abort if we are using strict mode and we are not using IGNORE */ >> - if ((int) level >= (int) MYSQL_ERROR::WARN_LEVEL_WARN && >> - thd->really_abort_on_warning()) >> + switch(level) >> { >> - /* Avoid my_message() calling push_warning */ >> - bool no_warnings_for_error= thd->no_warnings_for_error; >> - sp_rcontext *spcont= thd->spcont; >> - >> - thd->no_warnings_for_error= 1; >> - thd->spcont= NULL; >> - >> - thd->killed= THD::KILL_BAD_DATA; >> - my_message(code, msg, MYF(0)); >> - >> - thd->spcont= spcont; >> - thd->no_warnings_for_error= no_warnings_for_error; >> - /* Store error in error list (as my_message() didn't do it) */ >> - level= MYSQL_ERROR::WARN_LEVEL_ERROR; >> - } >> - >> - if (thd->handle_error(code, msg, level)) >> - DBUG_RETURN(NULL); >> - >> - if (thd->spcont && >> - thd->spcont->handle_error(code, level, thd)) >> + case MYSQL_ERROR::WARN_LEVEL_NOTE: >> + thd->raise_note(code, msg); >> + break; >> + case MYSQL_ERROR::WARN_LEVEL_WARN: >> + thd->raise_warning(code, msg); >> + break; >> + case MYSQL_ERROR::WARN_LEVEL_ERROR: >> { >> - DBUG_RETURN(NULL); >> + SQL_condition cond(thd->mem_root); >> + cond.set(thd, code, msg, MYSQL_ERROR::WARN_LEVEL_WARN, MYF(0)); >> + /* FIXME: broken calling code */ >> + cond.m_broken_caller= TRUE; >> + thd->raise_condition(& cond); >> + break; >> } >> - query_cache_abort(&thd->query_cache_tls); >> - >> >> - if (thd->warn_list.elements < thd->variables.max_error_count) >> - { >> - /* We have to use warn_root, as mem_root is freed after each query */ >> - if ((err= new (&thd->warn_root) MYSQL_ERROR(thd, code, level, msg))) >> - thd->warn_list.push_back(err, &thd->warn_root); >> + default: >> + DBUG_ASSERT(FALSE); >> } >> - thd->warn_count[(uint) level]++; >> - thd->total_warn_count++; >> - DBUG_RETURN(err); >> + >> + DBUG_VOID_RETURN; >> } >> > > I think we should drop push_warning all together.. > > In the long term, I agree with that. At the minimum, this will avoid calling _current_thd again and again. When replaced with thd->raise_WARN_XXX(), bugs in the printf format arguments will be reduced also. I think that however, there are good reasons to keep push_warning for now: a) grep push_warning sql/*.cc | wc -l --> 238 At least removing calls to push_warning should be a separate task / patch b) To merge code from 6.0 to 6.1, keeping the push_warning API around would be most welcome for maintenance, otherwise every single bug fix raising a warning will cause rework after a merge. >> + >> /* >> Push the warning/error to error list if there is still room in the list >> >> @@ -211,10 +186,12 @@ const LEX_STRING warning_level_names[]= >> }; >> >> bool mysqld_show_warnings(THD *thd, ulong levels_to_show) >> -{ >> +{ >> List<Item> field_list; >> DBUG_ENTER("mysqld_show_warnings"); >> >> + DBUG_ASSERT(thd->main_da.m_stmt_area.is_read_only()); >> + >> field_list.push_back(new Item_empty_string("Level", 7)); >> field_list.push_back(new Item_return_int("Code",4, MYSQL_TYPE_LONG)); >> field_list.push_back(new Item_empty_string("Message",MYSQL_ERRMSG_SIZE)); >> @@ -223,7 +200,7 @@ bool mysqld_show_warnings(THD *thd, ulon >> Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF)) >> DBUG_RETURN(TRUE); >> >> - MYSQL_ERROR *err; >> + SQL_condition *cond; >> SELECT_LEX *sel= &thd->lex->select_lex; >> SELECT_LEX_UNIT *unit= &thd->lex->unit; >> ha_rows idx= 0; >> @@ -231,24 +208,30 @@ bool mysqld_show_warnings(THD *thd, ulon >> >> unit->set_limit(sel); >> >> - List_iterator_fast<MYSQL_ERROR> it(thd->warn_list); >> - while ((err= it++)) >> + List_iterator_fast<SQL_condition> > it(thd->main_da.m_stmt_area.warn_list); >> + while ((cond= it++)) >> { >> /* Skip levels that the user is not interested in */ >> - if (!(levels_to_show & ((ulong) 1 << err->level))) >> + if (!(levels_to_show & ((ulong) 1 << cond->m_level))) >> continue; >> if (++idx <= unit->offset_limit_cnt) >> continue; >> if (idx > unit->select_limit_cnt) >> break; >> protocol->prepare_for_resend(); >> - protocol->store(warning_level_names[err->level].str, >> - warning_level_names[err->level].length, system_charset_info); >> - protocol->store((uint32) err->code); >> - protocol->store(err->msg, strlen(err->msg), system_charset_info); >> + protocol->store(warning_level_names[cond->m_level].str, >> + warning_level_names[cond->m_level].length, >> + system_charset_info); >> + protocol->store((uint32) cond->m_sql_errno); >> + protocol->store(cond->get_message_text(), >> + cond->get_message_octet_length(), >> + system_charset_info); >> if (protocol->write()) >> DBUG_RETURN(TRUE); >> } >> my_eof(thd); >> + >> + thd->main_da.m_stmt_area.set_read_only(FALSE); >> + >> DBUG_RETURN(FALSE); >> } >> >> === modified file 'sql/sql_error.h' >> --- a/sql/sql_error.h 2008-06-12 19:04:52 +0000 >> +++ b/sql/sql_error.h 2008-07-23 00:25:11 +0000 >> @@ -35,8 +35,8 @@ private: >> void set_msg(THD *thd, const char *msg_arg); >> }; >> >> -MYSQL_ERROR *push_warning(THD *thd, MYSQL_ERROR::enum_warning_level level, >> - uint code, const char *msg); >> +void push_warning(THD *thd, MYSQL_ERROR::enum_warning_level level, >> + uint code, const char *msg); >> void push_warning_printf(THD *thd, MYSQL_ERROR::enum_warning_level level, >> uint code, const char *format, ...); >> void mysql_reset_errors(THD *thd, bool force); >> >> === modified file 'sql/sql_insert.cc' >> --- a/sql/sql_insert.cc 2008-07-14 14:56:49 +0000 >> +++ b/sql/sql_insert.cc 2008-07-23 00:25:11 +0000 >> @@ -2314,8 +2314,8 @@ pthread_handler_t handle_delayed_insert( >> if (my_thread_init()) >> { >> /* Can't use my_error since store_globals has not yet been called */ >> - thd->main_da.set_error_status(thd, ER_OUT_OF_RESOURCES, >> - ER(ER_OUT_OF_RESOURCES)); >> + thd->main_da.set_default_error_status(thd, ER_OUT_OF_RESOURCES, >> + ER(ER_OUT_OF_RESOURCES)); >> goto end; >> } >> #endif >> @@ -2325,8 +2325,8 @@ pthread_handler_t handle_delayed_insert( >> if (init_thr_lock() || thd->store_globals()) >> { >> /* Can't use my_error since store_globals has perhaps failed */ >> - thd->main_da.set_error_status(thd, ER_OUT_OF_RESOURCES, >> - ER(ER_OUT_OF_RESOURCES)); >> + thd->main_da.set_default_error_status(thd, ER_OUT_OF_RESOURCES, >> + ER(ER_OUT_OF_RESOURCES)); >> thd->fatal_error(); >> goto err; >> } >> >> === added file 'sql/sql_signal.cc' >> --- a/sql/sql_signal.cc 1970-01-01 00:00:00 +0000 >> +++ b/sql/sql_signal.cc 2008-07-23 00:25:11 +0000 >> @@ -0,0 +1,651 @@ >> +/* Copyright (C) 2008 Sun Microsystems, Inc >> + >> + This program is free software; you can redistribute it and/or modify >> + it under the terms of the GNU General Public License as published by >> + the Free Software Foundation; version 2 of the License. >> + >> + This program is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + GNU General Public License for more details. >> + >> + You should have received a copy of the GNU General Public License >> + along with this program; if not, write to the Free Software >> + Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ >> + >> +#include "mysql_priv.h" >> +#include "sp_head.h" >> +#include "sp_pcontext.h" >> +#include "sp_rcontext.h" >> +#include "sql_signal.h" >> + >> +/* >> + The parser accepts any error code (desired) >> + The runtime internally supports any error code (desired) >> + The client server protocol is limited to 16 bits error codes (restriction) >> + Enforcing the 65535 limit in the runtime until the protocol can change. >> +*/ >> +#define MAX_MYSQL_ERRNO UINT_MAX16 >> + >> +const LEX_STRING Diag_condition_item_names[]= >> +{ >> + { C_STRING_WITH_LEN("CLASS_ORIGIN") }, >> + { C_STRING_WITH_LEN("SUBCLASS_ORIGIN") }, >> + { C_STRING_WITH_LEN("CONSTRAINT_CATALOG") }, >> + { C_STRING_WITH_LEN("CONSTRAINT_SCHEMA") }, >> + { C_STRING_WITH_LEN("CONSTRAINT_NAME") }, >> + { C_STRING_WITH_LEN("CATALOG_NAME") }, >> + { C_STRING_WITH_LEN("SCHEMA_NAME") }, >> + { C_STRING_WITH_LEN("TABLE_NAME") }, >> + { C_STRING_WITH_LEN("COLUMN_NAME") }, >> + { C_STRING_WITH_LEN("CURSOR_NAME") }, >> + { C_STRING_WITH_LEN("MESSAGE_TEXT") }, >> + { C_STRING_WITH_LEN("MYSQL_ERRNO") }, >> + >> + { C_STRING_WITH_LEN("CONDITION_IDENTIFIER") }, >> + { C_STRING_WITH_LEN("CONDITION_NUMBER") }, >> + { C_STRING_WITH_LEN("CONNECTION_NAME") }, >> + { C_STRING_WITH_LEN("MESSAGE_LENGTH") }, >> + { C_STRING_WITH_LEN("MESSAGE_OCTET_LENGTH") }, >> + { C_STRING_WITH_LEN("PARAMETER_MODE") }, >> + { C_STRING_WITH_LEN("PARAMETER_NAME") }, >> + { C_STRING_WITH_LEN("PARAMETER_ORDINAL_POSITION") }, >> + { C_STRING_WITH_LEN("RETURNED_SQLSTATE") }, >> + { C_STRING_WITH_LEN("ROUTINE_CATALOG") }, >> + { C_STRING_WITH_LEN("ROUTINE_NAME") }, >> + { C_STRING_WITH_LEN("ROUTINE_SCHEMA") }, >> + { C_STRING_WITH_LEN("SERVER_NAME") }, >> + { C_STRING_WITH_LEN("SPECIFIC_NAME") }, >> + { C_STRING_WITH_LEN("TRIGGER_CATALOG") }, >> + { C_STRING_WITH_LEN("TRIGGER_NAME") }, >> + { C_STRING_WITH_LEN("TRIGGER_SCHEMA") } >> +}; >> + >> +const LEX_STRING Diag_statement_item_names[]= >> +{ >> + { C_STRING_WITH_LEN("NUMBER") }, >> + { C_STRING_WITH_LEN("MORE") }, >> + { C_STRING_WITH_LEN("COMMAND_FUNCTION") }, >> + { C_STRING_WITH_LEN("COMMAND_FUNCTION_CODE") }, >> + { C_STRING_WITH_LEN("DYNAMIC_FUNCTION") }, >> + { C_STRING_WITH_LEN("DYNAMIC_FUNCTION_CODE") }, >> + { C_STRING_WITH_LEN("ROW_COUNT") }, >> + { C_STRING_WITH_LEN("TRANSACTIONS_COMMITTED") }, >> + { C_STRING_WITH_LEN("TRANSACTIONS_ROLLED_BACK") }, >> + { C_STRING_WITH_LEN("TRANSACTION_ACTIVE") } >> +}; >> + >> + >> +SQL_condition::SQL_condition(MEM_ROOT *mem_root) >> + : Sql_alloc(), >> + m_class_origin(mem_root), >> + m_subclass_origin(mem_root), >> + m_constraint_catalog(mem_root), >> + m_constraint_schema(mem_root), >> + m_constraint_name(mem_root), >> + m_catalog_name(mem_root), >> + m_schema_name(mem_root), >> + m_table_name(mem_root), >> + m_column_name(mem_root), >> + m_cursor_name(mem_root), >> + m_message_text(), >> + m_sql_errno(0), >> + m_condition_identifier(mem_root), >> + m_connection_name(mem_root), >> + m_parameter_mode(mem_root), >> + m_parameter_name(mem_root), >> + m_parameter_ordinal_position(0), >> + m_routine_catalog(mem_root), >> + m_routine_name(mem_root), >> + m_routine_schema(mem_root), >> + m_server_name(mem_root), >> + m_specific_name(mem_root), >> + m_trigger_catalog(mem_root), >> + m_trigger_name(mem_root), >> + m_trigger_schema(mem_root), >> + m_level(MYSQL_ERROR::WARN_LEVEL_ERROR), >> + m_flags(0), >> + m_mem_root(mem_root), >> + m_broken_caller(FALSE) >> > > Ugh.. we definitely need to improve this. Let's discuss it. > > I don't see a problem here. The code is brutal but explicit and robust. A SQL_condition *has* many attributes, per the requirements. Each attributes might have a different type. String attributes might have different length. Replacing: - String m_a - String m_b by - String m_array[2] won't allocate less memory, and will just create a need for more indexing and more set() / get() helpers, to create a final result where some complexity and indirections are added, making it not necessarily easier to read / maintain. No change, waiting for a concrete proposal to evaluate (see previous comments). >> +{ >> + memset(m_returned_sqlstate, 0, sizeof(m_returned_sqlstate)); >> +} >> + >> +SQL_condition * >> +SQL_condition::deep_copy(THD *thd, MEM_ROOT *mem_root, >> + const SQL_condition *cond) >> +{ >> + SQL_condition *copy= new (mem_root) SQL_condition(mem_root); >> + if (copy) >> + copy->deep_copy(cond); >> + return copy; >> +} >> + >> +void >> +SQL_condition::deep_copy(const SQL_condition *cond) >> +{ >> + memcpy(m_returned_sqlstate, cond->m_returned_sqlstate, >> + sizeof(m_returned_sqlstate)); >> + >> + if (cond->m_message_text.length()) >> + { >> + const char* copy; >> + >> + copy= strdup_root(m_mem_root, cond->m_message_text.ptr()); >> + m_message_text.set(copy, cond->m_message_text.length(), >> + error_message_charset_info); >> + } >> + else >> + m_message_text.length(0); >> + >> + DBUG_ASSERT(! m_message_text.is_alloced()); >> + >> + m_class_origin.copy(& cond->m_class_origin); >> + m_subclass_origin.copy(& cond->m_subclass_origin); >> + m_constraint_catalog.copy(& cond->m_constraint_catalog); >> + m_constraint_schema.copy(& cond->m_constraint_schema); >> + m_constraint_name.copy(& cond->m_constraint_name); >> + m_catalog_name.copy(& cond->m_catalog_name); >> + m_schema_name.copy(& cond->m_schema_name); >> + m_table_name.copy(& cond->m_table_name); >> + m_column_name.copy(& cond->m_column_name); >> + m_cursor_name.copy(& cond->m_cursor_name); >> + m_sql_errno= cond->m_sql_errno; >> + m_condition_identifier.copy(& cond->m_condition_identifier); >> + m_connection_name.copy(& cond->m_connection_name); >> + m_parameter_mode.copy(& cond->m_parameter_mode); >> + m_parameter_name.copy(& cond->m_parameter_name); >> + m_parameter_ordinal_position= cond->m_parameter_ordinal_position; >> + m_routine_catalog.copy(& cond->m_routine_catalog); >> + m_routine_name.copy(& cond->m_routine_name); >> + m_routine_schema.copy(& cond->m_routine_schema); >> + m_server_name.copy(& cond->m_server_name); >> + m_specific_name.copy(& cond->m_specific_name); >> + m_trigger_catalog.copy(& cond->m_trigger_catalog); >> + m_trigger_name.copy(& cond->m_trigger_name); >> + m_trigger_schema.copy(& cond->m_trigger_schema); >> + m_level= cond->m_level; >> + m_flags= cond->m_flags; >> + m_broken_caller= cond->m_broken_caller; >> +} >> + >> +void >> +SQL_condition::set_printf(THD *thd, uint code, const char *str, >> + MYSQL_ERROR::enum_warning_level level, >> + myf flags, ...) >> +{ >> + va_list args; >> + char ebuff[ERRMSGSIZE+20]; >> + >> + >> + DBUG_ENTER("SQL_condition::set_printf"); >> + >> + va_start(args, flags); >> + (void) my_vsnprintf (ebuff, sizeof(ebuff), str, args); >> > > Drop (void) > > Fixed >> + va_end(args); >> + >> + set(thd, code, ebuff, level, flags); >> + >> + DBUG_VOID_RETURN; >> +} >> + >> +void >> +SQL_condition::set(THD *thd, uint code, const char *str, >> + MYSQL_ERROR::enum_warning_level level, myf flags) >> +{ >> + const char* sqlstate; >> + >> + if (code == 0) >> + code= ER_UNKNOWN_ERROR; >> > > Hum.. is there a valid use case for this? > > This is an existing work around from my_message_sql. It's needed until bugs like - Bug #36768 partition_info::check_partition_info() reports mal formed warnings are fixed, so that an assert can be used instead. >> + if (str == NULL) >> + str= ER(code); >> > > Good. > > >> + m_sql_errno= code; >> + >> + sqlstate= mysql_errno_to_sqlstate(m_sql_errno); >> + memcpy(m_returned_sqlstate, sqlstate, SQLSTATE_LENGTH); >> + m_returned_sqlstate[SQLSTATE_LENGTH]= '\0'; >> + >> + set_builtin_message_text(str); >> + if ((level == MYSQL_ERROR::WARN_LEVEL_WARN) && >> + thd->really_abort_on_warning()) >> + { >> + /* >> + FIXME: >> + push_warning and strict SQL_MODE case. >> + */ >> + m_level= MYSQL_ERROR::WARN_LEVEL_ERROR; >> + thd->killed= THD::KILL_BAD_DATA; >> + } >> + else >> + m_level= level; >> + m_flags= flags; >> +} >> + >> +void >> +SQL_condition::set_builtin_message_text(const char* str) >> +{ >> + const char* copy; >> + >> + copy= strdup_root(m_mem_root, str); >> + m_message_text.set(copy, strlen(copy), error_message_charset_info); >> + DBUG_ASSERT(! m_message_text.is_alloced()); >> +} >> + >> +const char* >> +SQL_condition::get_message_text() const >> +{ >> + return m_message_text.ptr(); >> +} >> + >> +int >> +SQL_condition::get_message_octet_length() const >> +{ >> + return m_message_text.length(); >> +} >> + >> +void >> +SQL_condition::set_sqlstate(const char* sqlstate) >> +{ >> + memcpy(m_returned_sqlstate, sqlstate, SQLSTATE_LENGTH); >> + m_returned_sqlstate[SQLSTATE_LENGTH]= '\0'; >> +} >> + >> +Set_signal_information::Set_signal_information() >> +{ >> + int i; >> + for (i= FIRST_DIAG_SET_PROPERTY; >> + i <= LAST_DIAG_SET_PROPERTY; >> + i++) >> + { >> + m_item[i]= NULL; >> + } >> > > Replace this with clear(); > > Fixed >> +} >> + >> +Set_signal_information::Set_signal_information( >> + const Set_signal_information& set) >> +{ >> + int i; >> + for (i= FIRST_DIAG_SET_PROPERTY; >> + i <= LAST_DIAG_SET_PROPERTY; >> + i++) >> + { >> + m_item[i]= set.m_item[i]; >> + } >> +} >> + >> +void Set_signal_information::clear() >> +{ >> + int i; >> + for (i= FIRST_DIAG_SET_PROPERTY; >> + i <= LAST_DIAG_SET_PROPERTY; >> + i++) >> + { >> + m_item[i]= NULL; >> + } >> +} >> + >> +int Abstract_signal::eval_sqlcode_sqlstate(THD *thd, SQL_condition *cond) >> +{ >> + DBUG_ASSERT(m_cond); >> + DBUG_ASSERT(cond); >> + >> + /* >> + SIGNAL is restricted in sql_yacc.yy to only signal SQLSTATE conditions >> + */ >> + DBUG_ASSERT(m_cond->type == sp_cond_type::state); >> + >> + cond->m_sql_errno= 0; >> + cond->set_sqlstate(m_cond->sqlstate); >> + >> + return 0; >> +} >> + >> +int Abstract_signal::eval_defaults(THD *thd, SQL_condition *cond) >> +{ >> + const char* sqlstate= cond->get_sqlstate(); >> + >> + DBUG_ASSERT((sqlstate[0] != '0') || (sqlstate[1] != '0')); >> + >> + if ((sqlstate[0] == '0') && (sqlstate[1] == '1')) >> + { >> + /* SQLSTATE class "01": warning */ >> + cond->set_builtin_message_text(ER(ER_SIGNAL_WARN)); >> + cond->m_level= MYSQL_ERROR::WARN_LEVEL_WARN; >> + if (cond->m_sql_errno == 0) >> + cond->m_sql_errno= ER_SIGNAL_WARN; >> + } >> + else if ((sqlstate[0] == '0') && (sqlstate[1] == '2')) >> + { >> + /* SQLSTATE class "02": not found */ >> + cond->set_builtin_message_text(ER(ER_SIGNAL_NOT_FOUND)); >> + cond->m_level= MYSQL_ERROR::WARN_LEVEL_ERROR; >> + if (cond->m_sql_errno == 0) >> + cond->m_sql_errno= ER_SIGNAL_NOT_FOUND; >> + } >> + else >> + { >> + cond->set_builtin_message_text(ER(ER_SIGNAL_EXCEPTION)); >> + cond->m_level= MYSQL_ERROR::WARN_LEVEL_ERROR; >> + if (cond->m_sql_errno == 0) >> + cond->m_sql_errno= ER_SIGNAL_EXCEPTION; >> + } >> + >> + return 0; >> +} >> + >> +int assign_condition_item(const char* name, THD *thd, Item *set, >> + UTF8String64 *ci) >> +{ >> + String str_value; >> + String *str; >> + >> + DBUG_ENTER("assign_condition_item"); >> + >> + if (set->is_null()) >> + { >> + my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), name, "NULL"); >> + DBUG_RETURN(1); >> + } >> + >> + str= set->val_str(& str_value); >> + ci->set(str); >> + if (ci->is_truncated()) >> + { >> + if (thd->variables.sql_mode & (MODE_STRICT_TRANS_TABLES | >> + MODE_STRICT_ALL_TABLES)) >> + { >> + my_error(ER_COND_ITEM_TOO_LONG, MYF(0), name); >> + DBUG_RETURN(1); >> + } >> + >> + push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_WARN, >> + WARN_COND_ITEM_TRUNCATED, >> + ER(WARN_COND_ITEM_TRUNCATED), >> + name); >> + } >> + >> + DBUG_RETURN(0); >> +} >> + >> + >> +int Abstract_signal::eval_signal_informations(THD *thd, SQL_condition *cond) >> +{ >> + Item *set; >> + String str_value; >> + String *str; >> + int i; >> + int result= 1; >> + >> + DBUG_ENTER("Abstract_signal::eval_signal_informations"); >> + >> + for (i= FIRST_DIAG_SET_PROPERTY; >> + i <= LAST_DIAG_SET_PROPERTY; >> + i++) >> + { >> + set= m_set_signal_information.m_item[i]; >> + if (set) >> + { >> + if (! set->fixed) >> + { >> + if (set->fix_fields(thd, & set)) >> + goto end; >> + m_set_signal_information.m_item[i]= set; >> + } >> + } >> + } >> + >> + set= m_set_signal_information.m_item[DIAG_CLASS_ORIGIN]; >> + if (set != NULL) >> + { >> + if (assign_condition_item("CLASS_ORIGIN", thd, set, >> + & cond->m_class_origin)) >> + goto end; >> + } >> + >> + set= m_set_signal_information.m_item[DIAG_SUBCLASS_ORIGIN]; >> + if (set != NULL) >> + { >> + if (assign_condition_item("SUBCLASS_ORIGIN", thd, set, >> + & cond->m_subclass_origin)) >> + goto end; >> + } >> + >> + set= m_set_signal_information.m_item[DIAG_CONSTRAINT_CATALOG]; >> + if (set != NULL) >> + { >> + if (assign_condition_item("CONSTRAINT_CATALOG", thd, set, >> + & cond->m_constraint_catalog)) >> + goto end; >> + } >> + >> + set= m_set_signal_information.m_item[DIAG_CONSTRAINT_SCHEMA]; >> + if (set != NULL) >> + { >> + if (assign_condition_item("CONSTRAINT_SCHEMA", thd, set, >> + & cond->m_constraint_schema)) >> + goto end; >> + } >> + >> + set= m_set_signal_information.m_item[DIAG_CONSTRAINT_NAME]; >> + if (set != NULL) >> + { >> + if (assign_condition_item("CONSTRAINT_NAME", thd, set, >> + & cond->m_constraint_name)) >> + goto end; >> + } >> + >> + set= m_set_signal_information.m_item[DIAG_CATALOG_NAME]; >> + if (set != NULL) >> + { >> + if (assign_condition_item("CATALOG_NAME", thd, set, >> + & cond->m_catalog_name)) >> + goto end; >> + } >> + >> + set= m_set_signal_information.m_item[DIAG_SCHEMA_NAME]; >> + if (set != NULL) >> + { >> + if (assign_condition_item("SCHEMA_NAME", thd, set, >> + & cond->m_schema_name)) >> + goto end; >> + } >> + >> + set= m_set_signal_information.m_item[DIAG_TABLE_NAME]; >> + if (set != NULL) >> + { >> + if (assign_condition_item("TABLE_NAME", thd, set, >> + & cond->m_table_name)) >> + goto end; >> + } >> + >> + set= m_set_signal_information.m_item[DIAG_COLUMN_NAME]; >> + if (set != NULL) >> + { >> + if (assign_condition_item("COLUMN_NAME", thd, set, >> + & cond->m_column_name)) >> + goto end; >> + } >> + >> + set= m_set_signal_information.m_item[DIAG_CURSOR_NAME]; >> + if (set != NULL) >> + { >> + if (assign_condition_item("CURSOR_NAME", thd, set, >> + & cond->m_cursor_name)) >> + goto end; >> + } >> > > Ugh. The variables could also be a array as m_item and we would just > need to loop, index it, and assign as needed. We already have arrays > containing the names, index, etc. > > As discussed on IRC, the array would need a *different* index only on string attributes and not on every attributes, and create complexity elsewhere. I tried that earlier, and this leads to : - a "generic loop" that calls an assign_item function, - an assign item function that consist of a giant switch case for each item. The code posted in the patch corresponds to this loop + switch unrolled, and it's much better IMO. No change, waiting for a concrete proposal to be evaluated. >> + set= m_set_signal_information.m_item[DIAG_MESSAGE_TEXT]; >> + if (set != NULL) >> + { >> + if (set->is_null()) >> + { >> + my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), "MESSAGE_TEXT", "NULL"); >> + goto end; >> + } >> + /* >> + Enforce that SET MESSAGE_TEXT = <value> evaluates the value >> + as VARCHAR(128) CHARACTER SET UTF8. >> + */ >> + UTF8String128 utf8_text(thd->mem_root); >> > > I can't find UTF8String128 anywhere.. > > It's in sql/sql_fixstring.h, in patch 8/10 >> + str= set->val_str(& str_value); >> + utf8_text.set(str->ptr(), (size_t) str->length(), str->charset()); >> + >> + if (utf8_text.is_truncated()) >> + { >> + if (thd->variables.sql_mode & (MODE_STRICT_TRANS_TABLES | >> + MODE_STRICT_ALL_TABLES)) >> + { >> + my_error(ER_COND_ITEM_TOO_LONG, MYF(0), "MESSAGE_TEXT"); >> + goto end; >> + } >> + >> + push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_WARN, >> + WARN_COND_ITEM_TRUNCATED, >> + ER(WARN_COND_ITEM_TRUNCATED), >> + "MESSAGE_TEXT"); >> + } >> + >> + /* >> + Convert to the character set used with --language. >> + This code should be removed when WL#751 is implemented. >> + */ >> + String converted_text; >> + converted_text.set_charset(error_message_charset_info); >> + converted_text.append(utf8_text.ptr(), utf8_text.length(), >> + (CHARSET_INFO *) utf8_text.charset()); >> + cond->set_builtin_message_text(converted_text.c_ptr_safe()); >> + } >> + >> + set= m_set_signal_information.m_item[DIAG_MYSQL_ERRNO]; >> + if (set != NULL) >> + { >> + if (set->is_null()) >> + { >> + my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), "MYSQL_ERRNO", "NULL"); >> + goto end; >> + } >> + longlong code= set->val_int(); >> + if ((code <= 0) || (code > MAX_MYSQL_ERRNO)) >> + { >> + str= set->val_str(& str_value); >> + my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), >> + "MYSQL_ERRNO", str->c_ptr_safe()); >> + goto end; >> + } >> + cond->m_sql_errno= (int) code; >> + } >> + >> + /* >> + The various item->val_xxx() methods don't return an error code, >> + but flag thd in case of failure. >> + */ >> + if (! thd->is_error()) >> + result= 0; >> + >> +end: >> + for (i= FIRST_DIAG_SET_PROPERTY; >> + i <= LAST_DIAG_SET_PROPERTY; >> + i++) >> + { >> + set= m_set_signal_information.m_item[i]; >> + if (set) >> + { >> + if (set->fixed) >> + set->cleanup(); >> + } >> + } >> + >> + DBUG_RETURN(result); >> +} >> + >> +int Abstract_signal::raise_condition(THD *thd, SQL_condition *cond) >> +{ >> + int result= 1; >> + >> + DBUG_ENTER("Abstract_signal::raise_condition"); >> + >> + DBUG_ASSERT(m_lex->query_tables == NULL); >> + >> + if (m_cond != NULL) >> + { >> + if (eval_sqlcode_sqlstate(thd, cond)) >> + DBUG_RETURN(result); >> + >> + if (eval_defaults(thd, cond)) >> + DBUG_RETURN(result); >> + } >> + >> + if (eval_signal_informations(thd, cond)) >> + DBUG_RETURN(result); >> + >> + /* SIGNAL should not signal WARN_LEVEL_NOTE */ >> + DBUG_ASSERT((cond->m_level == MYSQL_ERROR::WARN_LEVEL_WARN) || >> + (cond->m_level == MYSQL_ERROR::WARN_LEVEL_ERROR)); >> + >> + thd->raise_condition(cond); >> + >> + if (cond->m_level == MYSQL_ERROR::WARN_LEVEL_WARN) >> + { >> + my_ok(thd); >> + result= 0; >> + } >> + >> + DBUG_RETURN(result); >> +} >> + >> +int SQLCOM_signal::execute(THD *thd) >> +{ >> + int result= 1; >> + SQL_condition cond(thd->mem_root); >> + >> + DBUG_ENTER("SQLCOM_signal::execute"); >> + >> + thd->main_da.reset_diagnostics_area(); >> + thd->row_count_func= 0; >> + mysql_reset_errors(thd, TRUE); >> + >> + result= raise_condition(thd, &cond); >> + >> + DBUG_RETURN(result); >> +} >> + >> + >> +int SQLCOM_resignal::execute(THD *thd) >> +{ >> + SQL_condition *signaled; >> + int result= 1; >> + >> + DBUG_ENTER("SQLCOM_resignal::execute"); >> + >> + if (! thd->spcont || ! (signaled= thd->spcont->raised_condition())) >> + { >> + my_error(ER_RESIGNAL_NO_HANDLER, MYF(0)); >> + DBUG_RETURN(result); >> + } >> + >> + if (m_cond == NULL) >> + { >> + /* RESIGNAL without signal_value */ >> + result= raise_condition(thd, signaled); >> + DBUG_RETURN(result); >> + } >> + >> + /* RESIGNAL with signal_value */ >> + >> + /* Make room for 2 conditions */ >> + while ((thd->main_da.m_stmt_area.warn_list.elements > 0) && >> + ((thd->main_da.m_stmt_area.warn_list.elements + 2) >> + > thd->variables.max_error_count)) >> + { >> + thd->main_da.m_stmt_area.warn_list.pop(); >> + thd->main_da.m_stmt_area.m_more= TRUE; >> + } >> + >> + thd->raise_condition_no_handler(signaled); >> + >> + SQL_condition new_cond(thd->mem_root); >> + new_cond.deep_copy(signaled); >> + result= raise_condition(thd, & new_cond); >> + DBUG_RETURN(result); >> +} >> + >> In addition to the changes from this review, I also implemented a change in sql_signal.h / sql_signal.cc to implement the behavior documented yesterday in the RESIGNAL work log (WL#2265). RESIGNAL passed architecture review, RESIGNAL is now code complete, so that this code review should cover both SIGNAL and RESIGNAL. Regards, -- Marc.
| Thread | ||
|---|---|---|
| • bzr commit into mysql-6.0-wl2110-review branch (marc.alff:2675) WL#2110 | Marc Alff | 23 Jul |
| • Re: bzr commit into mysql-6.0-wl2110-review branch (marc.alff:2675)WL#2110 | Davi Arnaut | 6 Aug |
| • Re: bzr commit into mysql-6.0-wl2110-review branch (marc.alff:2675)WL#2110 | Marc Alff | 7 Aug |
| • Re: bzr commit into mysql-6.0-wl2110-review branch (marc.alff:2675)WL#2110 | Davi Arnaut | 8 Aug |
| • Re: bzr commit into mysql-6.0-wl2110-review branch (marc.alff:2675)WL#2110 | Marc Alff | 14 Aug |
© 1995-2008 MySQL AB, 2008-2009 Sun Microsystems, Inc.
Page generated in 1.529 sec. using MySQL 5.1.14-beta-log