Hi Alik,
Some initial review comments.
On 7/29/10 7:08 PM, Alexander Nozdrin wrote:
> #At file:///mnt/raid/alik/MySQL/bzr/00/bug23032/mysql-trunk-bugfixing-bug23032/ based
> on revid:alik@stripped
>
> 3158 Alexander Nozdrin 2010-07-30
> This patch fixes the following bugs:
> - Bug#5889: Exit handler for a warning doesn't hide the warning in
> trigger
> - Bug#9857: Stored procedures: handler for sqlwarning ignored
> - Bug#23032: Handlers declared in a SP do not handle warnings generated
> in sub-SP
> - Bug#36185: Incorrect precedence for warning and exception handlers
>
[..]
> modified:
> mysql-test/r/signal.result
> mysql-test/r/signal_demo3.result
> mysql-test/r/sp-big.result
> mysql-test/r/sp-bugs.result
> mysql-test/r/sp-code.result
> mysql-test/r/sp-error.result
> mysql-test/r/sp.result
> mysql-test/r/sp_trans.result
> mysql-test/r/strict.result
> mysql-test/r/view.result
> mysql-test/suite/funcs_1/r/innodb_storedproc_02.result
> mysql-test/suite/funcs_1/r/memory_storedproc_02.result
> mysql-test/suite/funcs_1/r/myisam_storedproc_02.result
> mysql-test/suite/funcs_1/r/storedproc.result
> mysql-test/suite/rpl/r/rpl_row_sp005.result
> mysql-test/suite/rpl/r/rpl_row_sp006_InnoDB.result
> mysql-test/suite/rpl/r/rpl_row_trig003.result
> mysql-test/t/signal.test
> mysql-test/t/sp-error.test
> mysql-test/t/sp.test
> sql/sp_head.cc
> sql/sp_pcontext.h
> sql/sp_rcontext.cc
> sql/sp_rcontext.h
> sql/sql_class.cc
> sql/sql_class.h
> sql/sql_error.cc
> sql/sql_error.h
> sql/sql_signal.cc
Please write per-file messages. I need to understand the context.
[..]
Took only a brief look over the test cases. Please look for typos (I
think i saw a caugt or something like).
Please move the test cases that uses the debug session variable to
sp-code.test (which has a have_debug tag).
> === modified file 'sql/sp_head.cc'
> --- a/sql/sp_head.cc 2010-07-29 12:32:11 +0000
> +++ b/sql/sp_head.cc 2010-07-29 22:08:04 +0000
> @@ -1076,6 +1076,76 @@ void sp_head::recursion_level_error(THD
>
>
> /**
> + Find an SQL handler for any warning or error after execution of a store
store -> stored
[..]
> +
> + @param thd thread handle
> + @param ctx runtime context of the stored routine
> +*/
> +
> +static void
> +find_handler_after_execution(THD *thd, sp_rcontext *ctx)
> +{
> + if (thd->is_error() &&
> + ctx->find_handler(thd,
> + thd->stmt_da->sql_errno(),
> + thd->stmt_da->get_sqlstate(),
> + MYSQL_ERROR::WARN_LEVEL_ERROR,
> + thd->stmt_da->message()))
> + {
> + return;
> + }
Please, no need for return. Stick to a simple if/else.
> +
> + if (thd->warning_info->statement_warn_count())
> + {
> + List_iterator<MYSQL_ERROR> it(thd->warning_info->warn_list());
> + MYSQL_ERROR *err;
> + while ((err= it++))
> + {
> + if (err->get_level() != MYSQL_ERROR::WARN_LEVEL_WARN&&
> + err->get_level() != MYSQL_ERROR::WARN_LEVEL_NOTE)
> + continue;
> +
> + if (ctx->find_handler(thd,
> + err->get_sql_errno(),
> + err->get_sqlstate(),
> + err->get_level(),
> + err->get_message_text()))
> + {
> + return;
> + }
> + }
> + }
> +}
> +
> +
> +/**
> Execute the routine. The main instruction jump loop is there.
> Assume the parameters already set.
> @todo
[..]
> - if (ctx)
> + if (!thd->is_fatal_error&& !thd->killed_errno())
> {
> - uint handler_index;
> + /*
> + Find SQL handler in the appropriate RT-contexts:
> + - warnings can be handled by SQL handlers within
> + the current scope only;
> + - errors can be handled by any SQL handler from outer scope.
>
> - switch (ctx->found_handler(& hip,& handler_index)) {
> - case SP_HANDLER_NONE:
> - break;
> - case SP_HANDLER_CONTINUE:
> - thd->restore_active_arena(&execute_arena,&backup_arena);
> - thd->set_n_backup_active_arena(&execute_arena,&backup_arena);
> - ctx->push_hstack(i->get_cont_dest());
> - /* Fall through */
> - default:
> - if (ctx->end_partial_result_set)
> - thd->protocol->end_partial_result_set(thd);
> - ip= hip;
> - err_status= FALSE;
> - ctx->clear_handler();
> - ctx->enter_handler(hip, handler_index);
> - thd->clear_error();
> - thd->is_fatal_error= 0;
> - thd->killed= THD::NOT_KILLED;
> - thd->mysys_var->abort= 0;
> - continue;
> - }
> + Update warning_info.
> + */
> + find_handler_after_execution(thd, ctx);
Update?
[..]
> === modified file 'sql/sp_pcontext.h'
> --- a/sql/sp_pcontext.h 2010-03-31 14:05:33 +0000
> +++ b/sql/sp_pcontext.h 2010-07-29 22:08:04 +0000
> @@ -332,13 +332,6 @@ public:
> int
> push_cond(LEX_STRING *name, sp_cond_type_t *val);
>
> - inline void
> - pop_cond(uint num)
> - {
> - while (num--)
> - pop_dynamic(&m_conds);
> - }
> -
> sp_cond_type_t *
> find_cond(LEX_STRING *name, my_bool scoped=0);
>
>
> === modified file 'sql/sp_rcontext.cc'
> --- a/sql/sp_rcontext.cc 2010-07-27 12:42:36 +0000
> +++ b/sql/sp_rcontext.cc 2010-07-29 22:08:04 +0000
[..]
>
> === modified file 'sql/sql_error.cc'
> --- a/sql/sql_error.cc 2010-07-20 11:57:02 +0000
> +++ b/sql/sql_error.cc 2010-07-29 22:08:04 +0000
> @@ -494,14 +494,6 @@ void Warning_info::clear_warning_info(ul
> m_current_row_for_warning= 1; /* Start counting from the first row */
> }
>
> -void Warning_info::reserve_space(THD *thd, uint count)
> -{
> - /* Make room for count conditions */
> - while ((m_warn_list.elements> 0)&&
> - ((m_warn_list.elements + count)> thd->variables.max_error_count))
> - m_warn_list.pop();
> -}
> -
> /**
> Append warnings only if the original contents of the routine
> warning info was replaced.
> @@ -570,6 +562,12 @@ MYSQL_ERROR *Warning_info::push_warning(
> return cond;
> }
>
> +void Warning_info::remove_warning(THD *thd, const MYSQL_ERROR *warning)
> +{
> + m_warn_count[warning->get_level()]--;
> + m_statement_warn_count--;
> +}
> +
Remove.
> /*
> Push the warning to error list if there is still room in the list
>
>
> === modified file 'sql/sql_error.h'
> --- a/sql/sql_error.h 2010-03-31 14:05:33 +0000
> +++ b/sql/sql_error.h 2010-07-29 22:08:04 +0000
> @@ -153,8 +153,8 @@ private:
> Representation of a SQL condition.
> A SQL condition can be a completion condition (note, warning),
> or an exception condition (error, not found).
> - @note This class is named MYSQL_ERROR instead of SQL_condition for historical
> reasons,
> - to facilitate merging code with previous releases.
> + @note This class is named MYSQL_ERROR instead of SQL_condition for
> + historical reasons, to facilitate merging code with previous releases.
> */
> class MYSQL_ERROR : public Sql_alloc
> {
> @@ -471,24 +471,14 @@ public:
>
> ulong statement_warn_count() const { return m_statement_warn_count; }
>
> - /**
> - Reserve some space in the condition area.
> - This is a privileged operation, reserved for the RESIGNAL implementation,
> - as only the RESIGNAL statement is allowed to remove conditions from
> - the condition area.
> - For other statements, new conditions are not added to the condition
> - area once the condition area is full.
> - @param thd The current thread
> - @param count The number of slots to reserve
> - */
> - void reserve_space(THD *thd, uint count);
> -
> /** Add a new condition to the current list. */
> MYSQL_ERROR *push_warning(THD *thd,
> uint sql_errno, const char* sqlstate,
> MYSQL_ERROR::enum_warning_level level,
> const char* msg);
>
> + void remove_warning(THD *thd, const MYSQL_ERROR *warning);
> +
Remove.
> /**
> Set the read only status for this statement area.
> This is a privileged operation, reserved for the implementation of
>
> === modified file 'sql/sql_signal.cc'
> --- a/sql/sql_signal.cc 2010-05-14 18:11:25 +0000
> +++ b/sql/sql_signal.cc 2010-07-29 22:08:04 +0000
> @@ -499,18 +499,6 @@ bool Resignal_statement::execute(THD *th
> }
>
> /* RESIGNAL with signal_value */
> -
> - /* Make room for 2 conditions */
> - thd->warning_info->reserve_space(thd, 2);
> -
> - MYSQL_ERROR *raised= NULL;
> - raised= thd->raise_condition_no_handler(signaled->get_sql_errno(),
> - signaled->get_sqlstate(),
> - signaled->get_level(),
> - signaled->get_message_text());
> - if (raised)
> - raised->copy_opt_attributes(signaled);
> -
> result= raise_condition(thd, signaled);
>
Please write a per-file message describing this change.
