* Davi Arnaut <Davi.Arnaut@stripped> [08/12/10 21:34]:
> 2777 Davi Arnaut 2008-12-10
> Bug#36649: Condition area is not properly cleaned up after stored routine
> invocation
Please see some comments below.
> The problem is that the diagnostics area of a trigger is not
> isolated from the area of the statement that caused the trigger
> invocation. In MySQL terms, it means that warnings generated
> during the execution of the trigger are not removed from the
> "warning area" at the end of the execution.
>
> Before this fix, the rules for MySQL message list life cycle (see
> manual entry for SHOW WARNINGS) did not apply to statements
> inside stored programs:
>
> - The manual says that the list of messages is cleared by a
> statement that uses a table (any table). However, such
> statement, if run inside a stored program did not clear the
> message list.
> - The manual says that the list is cleared by a statement that
> generates a new error or a warning, but this was not the case
> with stored program statements either and is changed to be the
> case as well.
>
> In other words, after this fix, a statement has the same effect
> on the message list regardless of whether it's executed inside a
> stored program/sub-statement or not.
>
> This introduces an incompatible change:
>
> - before this fix, a, e.g. statement inside a trigger could
> never clear the global warning list
> - after this fix, a trigger that generates a warning or uses a
> table, clears the global warning list
Please add:
- however, when we leave a trigger or a function, the caller's
warning information is restored (see more on this below).
> === modified file 'sql/sp_head.cc'
> --- a/sql/sp_head.cc 2008-12-04 16:50:07 +0000
> +++ b/sql/sp_head.cc 2008-12-10 18:27:24 +0000
> @@ -502,7 +502,7 @@ sp_head::operator delete(void *ptr, size
> sp_head::sp_head()
> :Query_arena(&main_mem_root, INITIALIZED_FOR_SP),
> m_flags(0), m_recursion_level(0), m_next_cached_sp(0),
> - m_cont_level(0)
> + m_warning_info(0), m_cont_level(0)
> {
> const LEX_STRING str_reset= { NULL, 0 };
>
> @@ -1070,6 +1070,7 @@ sp_head::execute(THD *thd)
> Item_change_list old_change_list;
> String old_packet;
> Reprepare_observer *save_reprepare_observer= thd->m_reprepare_observer;
> + Warning_info *saved_warning_info;
>
> Object_creation_ctx *saved_creation_ctx;
>
> @@ -1120,6 +1121,9 @@ sp_head::execute(THD *thd)
> thd->is_slave_error= 0;
> old_arena= thd->stmt_arena;
>
> + /* Push a new warning information area. */
> + saved_warning_info= backup_warning_info(thd);
What's the point of having a member in sp_head
rather than declare it on stack? Keep in mind that sp_head
is a compiled body of a stored procedure and should not contain
runtime data.
Using a stack variable will also make it more obvious to the
reader what you're doing.
> +
> +static inline
> +void warn_info_copy(THD *thd, Warning_info *dest, Warning_info *src)
> +{
> + MYSQL_ERROR *err;
> + List_iterator_fast<MYSQL_ERROR> it(src->warn_list());
> + while ((err= it++))
> + dest->push_warning(thd, err->level, err->code, err->msg);
> +}
Please make it a helper member function of class Warning_info.
Please explain why it's void: it's considered tolerable to lose
a warning. Suggest to rename to Warning_info::append_warn_info,
since you are not performing a copy, but rather concat two lists.
> +Warning_info *
> +sp_head::backup_warning_info(THD *thd)
> +{
> + Warning_info *saved= thd->warning_info;
> + DBUG_ENTER("sp_head::backup_warning_info");
> + m_warning_info.clear_warning_info(saved->warn_id());
> + warn_info_copy(thd, &m_warning_info, saved);
> + thd->warning_info= &m_warning_info;
> + DBUG_RETURN(saved);
> +}
Suggest to inline this code in sp_head::execute, since all other
reset and backups there are inlined.
> +/**
> + Restore the caller's original warning information area.
> +
> + @param thd Thread context.
> + @param saved The saved warning information area.
> +*/
> +
> +void
> +sp_head::restore_warning_info(THD *thd, Warning_info *saved)
> +{
> + DBUG_ENTER("sp_head::restore_warning_info");
> + DBUG_ASSERT(saved);
> + if (saved->warn_id() != m_warning_info.warn_id())
This should be a method of Warning_info. It relies on too
many properties of its life cycle. Please call it something
like Warning_info::merge_with_routine_info().
Please add a comment explaining why you are performing this check
and how it works:
/*
If a routine body is empty, or if a routine did not generate
any warnings (thus warn_id() didn't change), do not duplicate
the contents our own contents by appending the contents
of the called routine. We know that the called routine
did not change its warning info.
On the other hand, if routine body is not empty, and
some statement in the routine generates a warning or uses tables,
warn_id() is guaranteed to have have changed.
In that case we know that the routine warning info contains
only new warnings, and thus we perform a copy.
In other words, this check means: append warnings only if
the original contents of the routine warning info was replaced.
*/
> + {
> + saved->opt_clear_warning_info(thd->query_id);
Please add a comment explaining why you call
opt_clear_warning_info():
If the routine invocation of the routine was a standalone
statement, rather than a sub-statement, in other words,
if it's a CALL of a procedure, rather than invocation of
a function or a trigger, we need to clear the current
contents of the caller's warning info. This is per
MySQL rules: if a statement generates a warning,
warnings from the previous statement are flushed.
Normally it's done in push_warning(). However, here we don't
use push_warning() to avoid invocation of condition handlers
or escalation of warnings to errors.
> + warn_info_copy(thd, saved, thd->warning_info);
> + }
> + m_warning_info.clear_warning_info(0);
If you have this as a stack variable, you don't need to do
that.
> + thd->warning_info= saved;
When converting this function into a method of Warning_info,
this bit should be moved outside the method.
> + DBUG_VOID_RETURN;
> +}
--
