Konstantin Osipov wrote:
[..]
>
> 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
Done.
>> +/*
>> + 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?
Done. find_handler_and_setup()
>> + uint hf;
>> +
>> + ctx->clear_handler();
>
> Why do you need to call clear_handler() here?
> Please add a comment.
Clear any previous usage of find_handler.
>> + 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.
Done.
>> + 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.
The idea was that those kind of statements would be different kind of sp
instructions.
> 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?
I don't see a problem, maybe we could use that array of flags to signal
which statements should clear the DA.
[..]
>
>> @@ -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?
It seems the current code sets the variable to NULL if a handler is
found. There is also a similar one in sp_instr_set_case_expr::exec_core
>> @@ -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.
Similar issue, the comment above this expression explains the reasoning:
/*
Work around the fact that errors in selects are not returned
properly (but instead converted into a warning), so if a
condition handler caught, we have lost the result code.
*/
I wonder if in both places it makes sense to search for a handler. For
the first ones, I don't see a problem in always setting the variable to
NULL in case of errors. In the second case, we should rather check if a
error was thrown... what do you think?
Regards,
--
Davi Arnaut, Software Engineer
MySQL Inc, www.mysql.com
