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

[He Zhenxing]

Andrei Elkin wrote: 
> >> >> 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.
> 
> Not in the success branch, right? Or like what?
> 

right, there is error, but the statement has to be logged (for example,
have already modified data, and cannot rollback)

> >
> > THD::NOT_KILLED only means ignore killed errors, if there is other
> > errors, it should binlog the error as is.
> >
> 
> To repeat we have two cases, the success branch that, I am suggesting, should always
> have the pattern like 
> 
> >>   if (!error)
> >>       thd->clear_error() 
> >>       Query_log_event qinfo(... THD::NOT_KILLED);
> >> 
> 
>    if (!error) thd->clear_error()
> 
> and the other one like
> 
> if (result) my_error(); Query_log_event().
> 
> For the first one *all* errors are supposed to be cleared. So here is one todo -
> to examine the code on that matter and fix what does not follow the rule.
> 
> For the 2nd case the first happened error is embedded into the event.
> I am saying that if the first error is one of the killed errors the event should get
> it.
> 
> Therefore supplying THD::NOT_KILLED in a case like the following
> 
> >>    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);
> 
> there --------------^
> 
> >>    mysql_bin_log.write(&qinfo);
> >>    }
> 
> can be incorrect if result == true is gained due to killing.


Yes, your suggestion would be ideal, but I think that would require
reconstruct many code, which I think is not a trivial work.


> 
> 
> cheers,
> 
> Andrei
> 
> 
> >> 
> >>    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 :-)