List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:April 8 2008 5:25pm
Subject:Re: bk commit into 6.0 tree (davi:1.2622) BUG#23032
View as plain text  
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
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