* 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
