Hi Marc,
Some comments below.
Marc Alff wrote:
>
> 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 ?
The bss segment is initialized to zero at runtime.
>>>
>>> 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.
OK.
<snip>
>>
> 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.
OK, consult one of the documentation guys.
>>> +
>>> +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 meant something like:
void THD::raise_error(uint code, int flags)
{
set status -> ER(code)
}
The point is that almost all use cases passes the standard error
message of the error code (ER(code)) and thus we waste resources
(pushing to stack or a register) having to deal with a parameter
that we can figure out later down the path.
You already do something similar where you check if the message_ar
is NULL.
<snip>
>>> +#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.
I just think that a per-thread slab would be much better. This way
we could "free" objects right away and re-use then, with the same
safety of our MEM_ROOT implementation.
<snip>
>>> + 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.
You haven't explained why it can't use
raise_warning/raise_error/raise_whatever..
If signal is the only "abuser", make it a friend class. What I
desire is to avoid the proliferation of this interface and enforce
that the interface that should be generally used is raise_warning,
raise_error, etc.
<snip>
>>> + 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.
>
Please put a comment there then.
Regards,
-- Davi Arnaut
