Hi Davi
Thanks for your reply,
see details below.
Guilhem, see below ME_JUST_INFO and ME_JUST_WARNING.
Regards,
-- Marc
Davi Arnaut wrote:
> 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.
>>>>
(...)
>>>> === 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.
>
>
Ok, removed the set to NULL.
>>>> @@ -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.
>
>
The bug number is Bug#38781, and I added a comment referencing it.
>>>> @@ -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>
>
>
Ok, so your initial comment was about a THD::raise_error() helper,
but was not related to the Diagnostics_area class ...
Done:
Added THD::raise_error(code), THD::raise_warning(code) helpers for most
common cases, it's a good idea.
>>>> +#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.
>
>
I don't understand what you are suggesting here,
thd->warn_root is already a per-thread memory pool.
> <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.
>
>
Ok, so the real issue here is not SQL_condition itself, but the fact
that there are 2 public ways to raise a condition:
(a) - using high level THD helpers (preferred),
(b) - make calls to SQL_condition directly (lower level).
Hiding (b) and enforcing it can't be used is a good idea, I agree.
Done:
The SQL_condition class has been changed to have a "read-only" public
interface,
and only the implementation of SIGNAL/RESIGNAL/THD helpers is allowed
(friend) to raise or modify conditions.
In particular, attributes like SQL_condition::m_sql_code and
SQL_condition::m_level are now private,
and have public getters.
>>>> +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.
>
Fixed.
