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

[He Zhenxing]

Andrei Elkin wrote:
> Zhen Xing, good morning.
> 
> > 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.
> 
> We would do much it better if treated all clear_error()-missed event instantiating
> places
> instead as offered the other mail.
> 
>     I suggest to analyze pieces of code where NOT-KILLED-status is not prepared with
>     thd->clear_error() and consider to fixing that.
> 
> The treatment should is simply to follow the pattern 
> 
> thd->clear_error()
> Query_log_event(THD::NOT_KILLED)
> 

I'm afraid this does not work, there are cases that we have to reserved
the error code.

THD::NOT_KILLED only means ignore killed errors, if there is other
errors, it should binlog the error as is.

> in the success branch. To quote my other earlier mail:
> 
>   The mentioned above cases comprise one big (the biggest) class of instances
>   where clear_error() method is correct. The class includes most of DDL:s and logics
> is
>   essentially
> 
>   if (!error)
>       thd->clear_error() 
>       Query_log_event qinfo(... THD::NOT_KILLED);
> 
> Still there is a case, quoting it further
> 
>    Like one in mysql_create_user():
> 
>    if (result)
>    my_error(ER_CANNOT_USER, MYF(0), "CREATE USER", wrong_users.c_ptr_safe());
> 
>    if (some_users_created && mysql_bin_log.is_open())
>    {
>    Query_log_event qinfo(thd, thd->query, thd->query_length,
>    0, FALSE, THD::NOT_KILLED);
>    mysql_bin_log.write(&qinfo);
>    }
> 
>    logs as your are saying with an error. But I wonder what one is better to
>    store in the event if errors would happen in this order: ER_QUERY_INTERRUPTED,
> ER_2?
>    Your change masquerades ER_QUERY_INTERRUPTED whereas it very probably induced ER_2
> and therefore
>    has the full right to be reported.
>    The same applies to the rest of mysql_*_user().
>    In this class of instances, I think, we need to revise need of THD::NOT_KILLED in
> the arg-list
>    rather than to enforce masking out the killed flag.
> 
> To sum up, in branches where thd->clear_error() is reasonably not invocated we
> should not mask out the killed status.
> 
> cheers,
> 
> Andrei
> 
> 
> >
> >> >> 
> >> >> 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 :-)