Hi Marc,
Marc Alff wrote:
> #At file:///home/malff/BZR-TREE/mysql-6.0-wl2110-review-part5/
>
> 2675 Marc Alff 2008-07-22
> WL#2110 (Stored Procedures: Implement SIGNAL)
>
> Formal review part 5/10: Stored procedures
>
> This patch implements changes in stored procedures, to support RESIGNAL.
> In particular, a SQL condition caught by a stored procedure SQL HANDLER
> is preserved, so it can be re-thrown using the RESIGNAL statement in the
> HANDLER.
> modified:
> sql/item.cc
> sql/sp_head.cc
> sql/sp_head.h
> sql/sp_pcontext.cc
> sql/sp_pcontext.h
> sql/sp_rcontext.cc
> sql/sp_rcontext.h
>
> === modified file 'sql/item.cc'
> --- a/sql/item.cc 2008-07-14 14:56:49 +0000
> +++ b/sql/item.cc 2008-07-23 00:27:12 +0000
> @@ -1106,7 +1106,7 @@ Item_splocal::Item_splocal(const LEX_STR
> Item *
> Item_splocal::this_item()
> {
> - DBUG_ASSERT(m_sp == m_thd->spcont->sp);
> + DBUG_ASSERT(m_sp == m_thd->spcont->m_sp);
All this is unnecessary noise. There isn't a compelling reason to rename it.
<snip>
>
> === modified file 'sql/sp_head.cc'
> --- a/sql/sp_head.cc 2008-06-28 11:00:59 +0000
> +++ b/sql/sp_head.cc 2008-07-23 00:27:12 +0000
> @@ -1263,9 +1263,9 @@ sp_head::execute(THD *thd)
> */
> if (ctx)
> {
> - uint hf;
> + uint handler_index;
>
> - switch (ctx->found_handler(&hip, &hf)) {
> + switch (ctx->found_handler(& hip, & handler_index)) {
> case SP_HANDLER_NONE:
> break;
> case SP_HANDLER_CONTINUE:
> @@ -1274,16 +1274,20 @@ sp_head::execute(THD *thd)
> ctx->push_hstack(i->get_cont_dest());
> /* Fall through */
> default:
> + if (thd->end_partial_result_set)
> + thd->protocol->end_partial_result_set(thd);
> ip= hip;
> err_status= FALSE;
> ctx->clear_handler();
> - ctx->enter_handler(hip);
> + 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;
> }
> +
> + thd->end_partial_result_set= FALSE;
> }
> } while (!err_status && !thd->killed &&
> !thd->is_fatal_error);
>
> @@ -1525,17 +1529,13 @@ sp_head::execute_trigger(THD *thd,
> init_sql_alloc(&call_mem_root, MEM_ROOT_BLOCK_SIZE, 0);
> thd->set_n_backup_active_arena(&call_arena, &backup_arena);
>
> - if (!(nctx= new sp_rcontext(m_pcont, 0, octx)) ||
> + if (!(nctx= new sp_rcontext(this, m_pcont, 0, octx)) ||
> nctx->init(thd))
> {
> err_status= TRUE;
> goto err_with_cleanup;
> }
>
> -#ifndef DBUG_OFF
> - nctx->sp= this;
> -#endif
> -
> thd->spcont= nctx;
>
> err_status= execute(thd);
> @@ -1642,7 +1642,7 @@ sp_head::execute_function(THD *thd, Item
> init_sql_alloc(&call_mem_root, MEM_ROOT_BLOCK_SIZE, 0);
> thd->set_n_backup_active_arena(&call_arena, &backup_arena);
>
> - if (!(nctx= new sp_rcontext(m_pcont, return_value_fld, octx)) ||
> + if (!(nctx= new sp_rcontext(this, m_pcont, return_value_fld, octx)) ||
> nctx->init(thd))
> {
> thd->restore_active_arena(&call_arena, &backup_arena);
> @@ -1658,10 +1658,6 @@ sp_head::execute_function(THD *thd, Item
> */
> thd->restore_active_arena(&call_arena, &backup_arena);
>
> -#ifndef DBUG_OFF
> - nctx->sp= this;
> -#endif
> -
> /* Pass arguments. */
> for (arg_no= 0; arg_no < argcount; arg_no++)
> {
> @@ -1846,32 +1842,26 @@ sp_head::execute_procedure(THD *thd, Lis
> save_spcont= octx= thd->spcont;
> if (! octx)
> { // Create a temporary old context
> - if (!(octx= new sp_rcontext(m_pcont, NULL, octx)) ||
> + if (!(octx= new sp_rcontext(this, m_pcont, NULL, octx)) ||
> octx->init(thd))
> {
> delete octx; /* Delete octx if it was init() that failed. */
> DBUG_RETURN(TRUE);
> }
>
> -#ifndef DBUG_OFF
> - octx->sp= 0;
> -#endif
> thd->spcont= octx;
>
> /* set callers_arena to thd, for upper-level function to work */
> thd->spcont->callers_arena= thd;
> }
>
> - if (!(nctx= new sp_rcontext(m_pcont, NULL, octx)) ||
> + if (!(nctx= new sp_rcontext(this, m_pcont, NULL, octx)) ||
> nctx->init(thd))
> {
> delete nctx; /* Delete nctx if it was init() that failed. */
> thd->spcont= save_spcont;
> DBUG_RETURN(TRUE);
> }
> -#ifndef DBUG_OFF
> - nctx->sp= this;
> -#endif
>
> if (params > 0)
> {
> @@ -3210,7 +3200,7 @@ sp_instr_hpush_jump::execute(THD *thd, u
> sp_cond_type_t *p;
>
> while ((p= li++))
> - thd->spcont->push_handler(p, m_ip+1, m_type, m_frame);
> + thd->spcont->push_handler(p, m_ip+1, m_type);
>
> *nextp= m_dest;
> DBUG_RETURN(0);
>
> === modified file 'sql/sp_head.h'
> --- a/sql/sp_head.h 2008-07-07 23:15:08 +0000
> +++ b/sql/sp_head.h 2008-07-23 00:27:12 +0000
> @@ -152,8 +152,9 @@ class sp_head :private Query_arena
> sp_head(const sp_head &); /**< Prevent use of these */
> void operator=(sp_head &);
>
> - MEM_ROOT main_mem_root;
> public:
> + MEM_ROOT main_mem_root;
> +
Hum... I personally don't like it, more comments and a suggestion on
this below.
> /** Possible values of m_flags */
> enum {
> HAS_RETURN= 1, // For FUNCTIONs only: is set if has RETURN
>
> === modified file 'sql/sp_pcontext.cc'
> --- a/sql/sp_pcontext.cc 2008-04-09 00:56:49 +0000
> +++ b/sql/sp_pcontext.cc 2008-07-23 00:27:12 +0000
> @@ -51,7 +51,7 @@ sp_cond_check(LEX_STRING *sqlstate)
> (c < 'A' || 'Z' < c))
> return FALSE;
> }
> - if (strcmp(sqlstate->str, "00000") == 0)
> + if (strncmp(sqlstate->str, "00", 2) == 0)
> return FALSE;
> return TRUE;
> }
>
> === modified file 'sql/sp_pcontext.h'
> --- a/sql/sp_pcontext.h 2007-06-10 10:43:57 +0000
> +++ b/sql/sp_pcontext.h 2008-07-23 00:27:12 +0000
> @@ -71,7 +71,7 @@ typedef struct sp_label
> typedef struct sp_cond_type
> {
> enum { number, state, warning, notfound, exception } type;
> - char sqlstate[6];
> + char sqlstate[SQLSTATE_LENGTH+1];
> uint mysqlerr;
> } sp_cond_type_t;
>
>
> === modified file 'sql/sp_rcontext.cc'
> --- a/sql/sp_rcontext.cc 2008-01-23 22:36:57 +0000
> +++ b/sql/sp_rcontext.cc 2008-07-23 00:27:12 +0000
> @@ -29,10 +29,11 @@
> #include "sp_pcontext.h"
>
>
> -sp_rcontext::sp_rcontext(sp_pcontext *root_parsing_ctx,
> +sp_rcontext::sp_rcontext(sp_head *sp, sp_pcontext *root_parsing_ctx,
> Field *return_value_fld,
> sp_rcontext *prev_runtime_ctx)
> - :m_root_parsing_ctx(root_parsing_ctx),
> + :m_sp(sp),
> + m_root_parsing_ctx(root_parsing_ctx),
> m_var_table(0),
> m_var_items(0),
> m_return_value_fld(return_value_fld),
> @@ -68,27 +69,37 @@ sp_rcontext::~sp_rcontext()
>
> bool sp_rcontext::init(THD *thd)
> {
> + uint i;
> + uint handler_count= m_root_parsing_ctx->max_handler_index();
> +
> in_sub_stmt= thd->in_sub_stmt;
>
> if (init_var_table(thd) || init_var_items())
> return TRUE;
>
> - return
> + if (
> !(m_handler=
> - (sp_handler_t*)thd->alloc(m_root_parsing_ctx->max_handler_index() *
> - sizeof(sp_handler_t))) ||
> + (sp_handler_t*)thd->alloc(handler_count * sizeof(sp_handler_t))) ||
> + !(m_raised_conditions=
> + (SQL_condition**)thd->alloc(handler_count * sizeof(SQL_condition*))) ||
> !(m_hstack=
> - (uint*)thd->alloc(m_root_parsing_ctx->max_handler_index() *
> - sizeof(uint))) ||
> + (uint*)thd->alloc(handler_count * sizeof(uint))) ||
> !(m_in_handler=
> - (uint*)thd->alloc(m_root_parsing_ctx->max_handler_index() *
> - sizeof(uint))) ||
> + (sp_active_handler_t*)thd->alloc(handler_count *
> + sizeof(sp_active_handler_t))) ||
> !(m_cstack=
> (sp_cursor**)thd->alloc(m_root_parsing_ctx->max_cursor_index() *
> sizeof(sp_cursor*))) ||
> !(m_case_expr_holders=
> (Item_cache**)thd->calloc(m_root_parsing_ctx->get_num_case_exprs() *
> - sizeof (Item_cache*)));
> + sizeof (Item_cache*)))
> + )
> + return TRUE;
> +
> + for (i = 0; i < handler_count; i++)
> + m_raised_conditions[i]= NULL;
Use thd::calloc to allocate m_raised_conditions.
> +
> + return FALSE;
> }
>
>
> @@ -194,13 +205,15 @@ sp_rcontext::set_return_value(THD *thd,
> */
>
> bool
> -sp_rcontext::find_handler(THD *thd, uint sql_errno,
> - MYSQL_ERROR::enum_warning_level level)
> +sp_rcontext::find_handler(THD *thd, const SQL_condition *cond)
> {
> if (m_hfound >= 0)
> return 1; // Already got one
>
> - const char *sqlstate= mysql_errno_to_sqlstate(sql_errno);
> + uint sql_errno= cond->m_sql_errno;
> + MYSQL_ERROR::enum_warning_level level= cond->m_level;
> +
> + const char *sqlstate= cond->get_sqlstate();
> int i= m_hcount, found= -1;
>
> /*
> @@ -220,7 +233,7 @@ sp_rcontext::find_handler(THD *thd, uint
>
> /* Check active handlers, to avoid invoking one recursively */
> while (j--)
> - if (m_in_handler[j] == m_handler[i].handler)
> + if (m_in_handler[j].ip == m_handler[i].handler)
> break;
> if (j >= 0)
> continue; // Already executing this handler
> @@ -264,10 +277,16 @@ sp_rcontext::find_handler(THD *thd, uint
> */
> if (m_prev_runtime_ctx && IS_EXCEPTION_CONDITION(sqlstate) &&
> level == MYSQL_ERROR::WARN_LEVEL_ERROR)
> - return m_prev_runtime_ctx->find_handler(thd, sql_errno, level);
> + return m_prev_runtime_ctx->find_handler(thd, cond);
Good.
> return FALSE;
> }
> +
> m_hfound= found;
> +
> + SQL_condition *raised;
> + raised= SQL_condition::deep_copy(thd, & m_sp->main_mem_root, cond) ;
Can't we use the permanent arena of the statement for this?
> + m_raised_conditions[m_hfound]= raised;
Assert that m_hfound always fits.
> +
> return TRUE;
> }
>
> @@ -293,22 +312,22 @@ sp_rcontext::find_handler(THD *thd, uint
> FALSE if no handler was found.
> */
> bool
> -sp_rcontext::handle_error(uint sql_errno,
> - MYSQL_ERROR::enum_warning_level level,
> - THD *thd)
> +sp_rcontext::handle_condition(THD *thd, const SQL_condition *cond)
> {
> - MYSQL_ERROR::enum_warning_level elevated_level= level;
> -
> -
> /* Depending on the sql_mode of execution,
> warnings may be considered errors */
> - if ((level == MYSQL_ERROR::WARN_LEVEL_WARN) &&
> + if ((cond->m_level == MYSQL_ERROR::WARN_LEVEL_WARN) &&
> thd->really_abort_on_warning())
> {
> - elevated_level= MYSQL_ERROR::WARN_LEVEL_ERROR;
> + SQL_condition elevated_cond(thd->mem_root);
> + elevated_cond.set(thd, cond->m_sql_errno,
> + cond->get_message_text(),
> + MYSQL_ERROR::WARN_LEVEL_ERROR,
> + MYF(0));
> + return find_handler(thd, & elevated_cond);
> }
>
> - return find_handler(thd, sql_errno, elevated_level);
> + return find_handler(thd, cond);
> }
>
> void
> @@ -335,7 +354,7 @@ sp_rcontext::pop_cursors(uint count)
> }
>
> void
> -sp_rcontext::push_handler(struct sp_cond_type *cond, uint h, int type, uint f)
> +sp_rcontext::push_handler(struct sp_cond_type *cond, uint h, int type)
Please, for this and other trivial cleanups, commit separately and push
earlier.
> {
> DBUG_ENTER("sp_rcontext::push_handler");
> DBUG_ASSERT(m_hcount < m_root_parsing_ctx->max_handler_index());
> @@ -343,7 +362,6 @@ sp_rcontext::push_handler(struct sp_cond
> m_handler[m_hcount].cond= cond;
> m_handler[m_hcount].handler= h;
> m_handler[m_hcount].type= type;
> - m_handler[m_hcount].foffset= f;
> m_hcount+= 1;
>
> DBUG_PRINT("info", ("m_hcount: %d", m_hcount));
> @@ -382,11 +400,13 @@ sp_rcontext::pop_hstack()
> }
>
> void
> -sp_rcontext::enter_handler(int hid)
> +sp_rcontext::enter_handler(uint hip, uint hindex)
> {
> DBUG_ENTER("sp_rcontext::enter_handler");
> DBUG_ASSERT(m_ihsp < m_root_parsing_ctx->max_handler_index());
> - m_in_handler[m_ihsp++]= hid;
> + m_in_handler[m_ihsp].ip= hip;
> + m_in_handler[m_ihsp].index= hindex;
> + m_ihsp++;
> DBUG_PRINT("info", ("m_ihsp: %d", m_ihsp));
> DBUG_VOID_RETURN;
> }
> @@ -396,11 +416,32 @@ sp_rcontext::exit_handler()
> {
> DBUG_ENTER("sp_rcontext::exit_handler");
> DBUG_ASSERT(m_ihsp);
> + uint hindex= m_in_handler[m_ihsp-1].index;
> + DBUG_ASSERT(m_raised_conditions[hindex]);
> + delete m_raised_conditions[hindex];
> + m_raised_conditions[hindex]= NULL;
> m_ihsp-= 1;
> DBUG_PRINT("info", ("m_ihsp: %d", m_ihsp));
> DBUG_VOID_RETURN;
> }
>
> +SQL_condition*
> +sp_rcontext::raised_condition() const
> +{
> + if (m_ihsp > 0)
> + {
> + uint hindex= m_in_handler[m_ihsp - 1].index;
> + SQL_condition *raised= m_raised_conditions[hindex];
> + DBUG_ASSERT(raised);
> + return raised;
> + }
> +
> + if (m_prev_runtime_ctx)
> + return m_prev_runtime_ctx->raised_condition();
> +
> + return NULL;
> +}
> +
>
> int
> sp_rcontext::set_variable(THD *thd, uint var_idx, Item **value)
>
> === modified file 'sql/sp_rcontext.h'
> --- a/sql/sp_rcontext.h 2008-01-23 20:26:41 +0000
> +++ b/sql/sp_rcontext.h 2008-07-23 00:27:12 +0000
> @@ -34,12 +34,21 @@ class sp_instr_cpush;
>
> typedef struct
> {
> + /** Condition caught by this HANDLER. */
> struct sp_cond_type *cond;
> - uint handler; // Location of handler
> + /** Location (instruction pointer) of the handler code. */
> + uint handler;
> + /** Handler type (EXIT, CONTINUE). */
> int type;
> - uint foffset; // Frame offset for the handlers declare level
> } sp_handler_t;
>
> +typedef struct
> +{
> + /** Instruction pointer of the active handler. */
> + uint ip;
> + /** Handler index of the active handler. */
> + uint index;
> +} sp_active_handler_t;
>
> /*
> This class is a runtime context of a Stored Routine. It is used in an
> @@ -75,16 +84,10 @@ class sp_rcontext : public Sql_alloc
> */
> Query_arena *callers_arena;
>
> -#ifndef DBUG_OFF
> - /*
> - The routine for which this runtime context is created. Used for checking
> - if correct runtime context is used for variable handling.
> - */
> - sp_head *sp;
> -#endif
> + sp_head *m_sp;
>
> - sp_rcontext(sp_pcontext *root_parsing_ctx, Field *return_value_fld,
> - sp_rcontext *prev_runtime_ctx);
> + sp_rcontext(sp_head *sp, sp_pcontext *root_parsing_ctx,
> + Field *return_value_fld, sp_rcontext *prev_runtime_ctx);
> bool init(THD *thd);
Even in debug build, not for all cases the sp_head argument would be
passed. Also, to prevent any future abuse, I would prefer that we pass
the memory root and give it a explicit name like cond_mem_root.
-- Davi
