List:Commits« Previous MessageNext Message »
From:Konstantin Osipov Date:April 8 2008 10:18am
Subject:Re: bk commit into 6.0 tree (davi:1.2622) BUG#23032
View as plain text  
* Davi Arnaut <davi@stripped> [08/04/07 22:42]:

>   include/my_sys.h@stripped, 2008-04-07 14:29:38-03:00, davi@stripped +9 -11
>     Removed unused/useless return value from error functions
>     and remove the unused MYSYS_PROGRAM_USES_CURSES.
>   mysys/my_error.c@stripped, 2008-04-07 14:29:39-03:00, davi@stripped +8 -10
>     Remove unused/unecesasry return value from error setting
>     and handling functions.
> 
>   mysys/my_messnc.c@stripped, 2008-04-07 14:29:39-03:00, davi@stripped +2 -2
>     Drop unnecessary return value.
>   mysys/my_static.c@stripped, 2008-04-07 14:29:39-03:00, davi@stripped +2 -2
>     Drop unused/uncessary return value from error handling
>     functions.
> 
>   sql/backup/error.h@stripped, 2008-04-07 14:29:40-03:00, davi@stripped +2 -3
>     my_printf_error doesn't returns nothing now.

This part (related to changes in the return value) is approved.
Please push it separately if you can.

>  call mysqltest.sp1();
>  my-col
>  1
> -Warnings:
> -Note	1051	Unknown table 't1'
>  select database();
>  database()
>  test
> diff -Nrup a/mysql-test/r/sp-vars.result b/mysql-test/r/sp-vars.result
> --- a/mysql-test/r/sp-vars.result	2007-05-18 09:29:22 -03:00
> +++ b/mysql-test/r/sp-vars.result	2008-04-07 14:29:38 -03:00
> @@ -109,26 +109,6 @@ v7	v8	v9	v10	v11	v12	v13	v14	v15	v16
>  10	10	10	0	0	10	10	10	10	0
>  v17	v18	v19	v20
>  12.00	12.12	12.00	12.12
> -Warnings:
> -Warning	1264	Out of range value for column 'v1' at row 1
> -Warning	1264	Out of range value for column 'v1u' at row 1
> -Warning	1264	Out of range value for column 'v2' at row 1
> -Warning	1264	Out of range value for column 'v2u' at row 1
> -Warning	1264	Out of range value for column 'v3' at row 1
> -Warning	1264	Out of range value for column 'v3u' at row 1
> -Warning	1264	Out of range value for column 'v4' at row 1
> -Warning	1264	Out of range value for column 'v4u' at row 1
> -Warning	1264	Out of range value for column 'v5' at row 1
> -Warning	1264	Out of range value for column 'v5u' at row 1
> -Warning	1264	Out of range value for column 'v6' at row 1
> -Warning	1264	Out of range value for column 'v6u' at row 1
> -Warning	1366	Incorrect integer value: 'String 10 ' for column 'v10' at row 1
> -Warning	1366	Incorrect integer value: 'String10' for column 'v11' at row 1
> -Warning	1265	Data truncated for column 'v12' at row 1
> -Warning	1265	Data truncated for column 'v13' at row 1
> -Warning	1366	Incorrect integer value: 'Hello, world' for column 'v16' at row 1
> -Note	1265	Data truncated for column 'v18' at row 1
> -Note	1265	Data truncated for column 'v20' at row 1

Let's double check with PeterG: is DECLARE considered a separate
statement? Should condition area be cleared in the begining of
DECLARE? Should a warning inside DECLARE be caught by a handler
that is declared a few lines earlier?

In this test case the condition area is cleared in the end by
SELECT, but we could construct a stored procedure that has only
DECLARE statements in it.

Additionally, the generated warnings were meaningful - they were
testing that assignments in DECLARE produce warnings. I think
instead of simply updating the test results we should rewrite the
test case to not rely on the current approach to warning handling
-- perhaps by putting each DECLARE into an own procedure.

>  #
> +# Bug#23032: Handlers declared in a SP do not handle warnings generated in sub-SP
> +#
> +
> +--disable_warnings
> +DROP PROCEDURE IF EXISTS p1;
> +DROP PROCEDURE IF EXISTS p2;
> +--enable_warnings
> +
> +delimiter |;
> +
> +CREATE PROCEDURE p1()
> +  BEGIN
> +    SELECT CAST('10 ' as unsigned integer);
> +    SELECT 1;
> +    CALL p2();
> +  END|
> +
> +CREATE PROCEDURE p2()
> +  BEGIN
> +    SELECT CAST('10 ' as unsigned integer);
> +  END|
> +
> +delimiter ;|
> +
> +CALL p1();
> +
> +DROP PROCEDURE p1;
> +DROP PROCEDURE p2;
> +
> +--disable_warnings
> +DROP PROCEDURE IF EXISTS p1;
> +DROP PROCEDURE IF EXISTS p2;
> +DROP PROCEDURE IF EXISTS p3;
> +--enable_warnings
> +
> +delimiter |;
> +
> +CREATE PROCEDURE p1()
> +  BEGIN
> +    DECLARE CONTINUE HANDLER FOR SQLWARNING SELECT 'warning';
> +    CALL p2();
> +    CALL p3();
> +  END|
> +
> +CREATE PROCEDURE p2()
> +  BEGIN
> +    SELECT CAST('10 ' as unsigned integer);
> +  END|
> +
> +CREATE PROCEDURE p3()
> +  BEGIN
> +    SELECT CAST('10 ' as unsigned integer);
> +    SELECT 1;
> +  END|
> +
> +delimiter ;|
> +
> +CALL p1();
> +
> +DROP PROCEDURE p1;
> +DROP PROCEDURE p2;
> +DROP PROCEDURE p3;

Could you please extend the test coverage?

 - have a test case that shows that a warning thrown by the last
   statement of sub-sub-sp is handled by the continue handler of
   the top level SP
 - issue "show warnings" and "show errors" to demonstrate how
 these statements work with the new way to clear the
 diagnostics/condition area.
 - cover every different subclass of sp_instr with a test,
   demonstrating how it clears the warnings/errors of the previous
   instruction
 - cover every allowed sqlcom_ with a test?

 - perhaps more to come
 
> +/*
> +  Check if an exception has occurred and a handler has been found
> +
> +  @param thd thread handle
> +  @param ctx runtime context of the stored routine
> +  @param ip location of the found handler
> +  @param instr stored procedure instruction
> +  @param execute_arena per-instruction arena
> +  @param backup_arena per-instruction arena
> +
> +  @return TRUE if a handler has been found, FALSE otherwise.
> +*/
> +
> +static bool
> +exception(THD *thd, sp_rcontext *ctx, uint *ip, sp_instr *instr,
> +          Query_arena *execute_arena, Query_arena *backup_arena)
> +{

I believe the standard has a fairly narrow definition for
"exception" condition.
In other words, here we check not only for errors, but also for
warnings.

Could you please use a more descriptive name? Perhaps
find_handler_and_clear_da?

> +  uint hf;
> +
> +  ctx->clear_handler();

Why do you need to call clear_handler() here?
Please add a comment.

> +  if (! ctx->find_handler(thd))
> +    return FALSE;
> +
> +  switch (ctx->found_handler(ip, &hf)) {
> +  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(instr->get_cont_dest());
> +    // Fall through

Please use /* */ comments.

> +  default:
> +    ctx->clear_handler();
> +    ctx->enter_handler(*ip);
> +    thd->clear_error();
> +    mysql_reset_errors(thd, 1);
> +    thd->is_fatal_error= 0;
> +    thd->killed= THD::NOT_KILLED;
> +    thd->mysys_var->abort= 0;
> +    return TRUE;
> +  }
> +  return FALSE;
> +}
> +
> +
>  
> +    /*
> +      Clear the Diagnostics Area and the Condition Area in preparation for
> +      the next command, use a reference to the last instruction because each
> +      instruction needs to clean up it's own mess. Although this is happening
> +      at the end of the execution, it's the same as cleaning the areas at the
> +      beginning of the execution of the next statement.
> +    */
> +    i->reset(thd);

I don't see how that will work with GET DIAGNOSTICS statement,
which doesn't clear the diagnostics/condition area, but only
queries it. MySQL's current idea of GET DIAGNOSTICS is SHOW
WARNIGNS -- I think it's exactly the type of an instruction that
shouldn't clear the warnings.

And here we meet the shortage of the approach you took with
sp_instr::reset(): you choose whether to reset or not based on the
instruction being executed, which is right. But for
sp_instr_stmt depending on SQLCOM we may choose to not clear the
area either. 

Perhaps sp_instr_stmt::reset() should do nothing -- and rely on
the code of the SQLCOM_ at hand to clear the area or leave it
intact.

What do you think?


> -    
> -    /* 
> -      Will write this SP statement into binlog separately 
> +
> +    /*
> +      Will write this SP statement into binlog separately
>        (TODO: consider changing the condition to "not inside event union")
>      */
>      if (thd->prelocked_mode == NON_PRELOCKED)
>        thd->user_var_events_alloc= thd->mem_root;
> -    
> +
>      err_status= i->execute(thd, &ip);
>  
>      if (i->free_list)
>        cleanup_items(i->free_list);
> -    
> -    /* 
> +
> +    /*
>        If we've set thd->user_var_events_alloc to mem_root of this SP
>        statement, clean all the events allocated in it.
>      */
> @@ -1229,7 +1283,7 @@ sp_head::execute(THD *thd)
>  
>      /* we should cleanup free_list and memroot, used by instruction */
>      thd->cleanup_after_query();
> -    free_root(&execute_mem_root, MYF(0));    
> +    free_root(&execute_mem_root, MYF(0));

Please push all such changes in formatting separately, if you can.

> @@ -2885,7 +2934,7 @@ sp_instr_set::exec_core(THD *thd, uint *
>  {
>    int res= thd->spcont->set_variable(thd, m_offset, &m_value);
>  
> -  if (res && thd->spcont->found_handler_here())
> +  if (res && thd->spcont->find_handler(thd))

Why do you need to look for handler here? Shouldn't you be able to
look for a handler at one place in the code now, when the area is
not cleared till the beginning of the next statement?

> @@ -3440,9 +3489,7 @@ sp_instr_copen::execute(THD *thd, uint *
>      */
>      if (!res)
>      {
> -      uint dummy1, dummy2;
> -
> -      if (thd->spcont->found_handler(&dummy1, &dummy2))
> +      if (thd->spcont->find_handler(thd))
>          res= -1;
>      }

Same here.

-- 
Konstantin Osipov                    Moscow, Russia
MySQL Server Runtime Team Lead       Database Group, Sun Microsystems
Thread
bk commit into 6.0 tree (davi:1.2622) BUG#23032Davi Arnaut7 Apr
  • Re: bk commit into 6.0 tree (davi:1.2622) BUG#23032Konstantin Osipov8 Apr
    • Re: bk commit into 6.0 tree (davi:1.2622) BUG#23032Davi Arnaut8 Apr