List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:July 29 2010 11:03pm
Subject:Re: bzr commit into mysql-trunk-runtime branch (alik:3158) Bug#5889
Bug#9857 Bug#23032 Bug#36185
View as plain text  
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.
Thread
bzr commit into mysql-trunk-runtime branch (alik:3158) Bug#5889Bug#9857 Bug#23032 Bug#36185Alexander Nozdrin30 Jul
  • Re: bzr commit into mysql-trunk-runtime branch (alik:3158) Bug#5889Bug#9857 Bug#23032 Bug#36185Davi Arnaut30 Jul