Re: bzr commit into mysql-5.0-bugteam branch (zhenxing.he:2814) Bug#45520

[He Zhenxing]

Andrei Elkin wrote:
> Zhen Xing, hello.
> 
> 
> >> >> or in mysql_grant_table()
> >> >> 
> >> >>   if (!result) /* success */
> >> >>   {
> >> >>     if (mysql_bin_log.is_open())
> >> >>     {
> >> >>       thd->clear_error();
> >> >>       Query_log_event qinfo(thd, thd->query,
> thd->query_length,
> >> >>                             0, FALSE, THD::NOT_KILLED);
> >> >>       mysql_bin_log.write(&qinfo);
> >> >>     }
> >> >>   }
> >> >> 
> >> >> etc.
> >> >> 
> >> >> Your ER_QUERY_INTERRUPTED/ER_SERVER_SHUTDOWN errors would be
> cleared.
> >> >> 
> >> >
> >> > There is a possibility that the
> ER_QUERY_INTERRUPTED/ER_SERVER_SHUTDOWN
> >> > error is set after calling thd->clear_error() and before
> constructing
> >> > the event. So the check has to be done within Query_log_event
> >> > constructor.
> 
> I might have said it better, sorry.
> Above constructor invocation supplies THD::NOT_KILLED to protect from such a case.
> In human words, if it's a success then any error that could be caught in time
> interval
> [ result := true, event-instantiation ] should be disregarded.
> 

Nice catch! You're right, if thd->clear_error() is called right before
calling Query_log_event constructor, then it is sure that the error code
will not be one of the killed errors.

But there are cases where thd->clear_error() was not called before
calling Query_log_event constructor with THD::NOT_KILLED, so I think the
code (to make sure that error_code is not one of the killed errors when
specified THD::NOT_KILLED) is still needed.

> >> 
> >> But is not that exactly what I am trying to say?
> >> If errors got cleared in thd->clear_error() at the end of a query
> execution, the following
> >> possible errors are irrelevant. They should not be associated with the query
> event at least.
> >> And as far as I can see neither to be send back to the client which is
> logical: the main operation has completed and the client should not be bothered.
> >> 
> >> If you would like we could speak phone tomorrow on that matter.
> >> 
> >> All in all, I am still confident in the idea
> >> 
> >
> > Hmm, I am confused.
> >
> > Here is my understanding of your opinion. Since thd->clear_error() is
> > called after execution of the statement and before calling
> > Query_log_event(), so that possible killed errors should be cleared, and
> > so there is no need to do the following in Query_log_event():
> >
> > +
> > +  /* thd_arg->main_da.sql_errno() might be ER_SERVER_SHUTDOWN or
> > +     ER_QUERY_INTERRUPTED, So here we need to make sure that
> > +     error_code is not set to these errors when specified NOT_KILLED
> > +     by the caller
> > +  */
> > +  if ((killed_status_arg == THD::NOT_KILLED) &&
> > +      (error_code == ER_SERVER_SHUTDOWN || error_code ==
> ER_QUERY_INTERRUPTED))
> > +    error_code= 0;
> >
> > Is this what you mean?
> 
> That's correct but only when combined with the clarification about NOT_KILLED.
> 
> >
> > If it is, then my answer is that there is a possibility that the killed
> > error could be set after we called thd->clear_errors() and before
> > calling the constructor Query_log_event() like this:
> >
> >   execute statement
> >   call thd->clear_errors(), this will clear all errors
> >   some one issued kill for this thread, and thus thd->killed will be one
> > of the killed errors (again)
> >   call Query_log_event(), and then the error code will be killed errors,
> > which is incorrect.
> 
> The constructor executes
> 
>  error_code=
>     (killed_status_arg == THD::NOT_KILLED) ? thd_arg->net.last_errno :
>     ((thd_arg->system_thread & SYSTEM_THREAD_DELAYED_INSERT) ? 0 :
>      thd->killed_errno());
> 
> to find `thd_arg->net.last_errno == 0' because of thd->clear_error().
> And it won't set the event's error to thd->killed_errno() because of 
> `killed_status_arg == THD::NOT_KILLED'.
> 
> >
> > So we have to add the code above to make sure this scenario does not
> > happen.
> >
> > Please don't hesitate to object if I understand it wrong.
> >
> 
> Thanks, sure I would do :-)
> 
> cheers,
> 
> Andrei