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

[He Zhenxing]

Andrei Elkin wrote: 
> Zhen Xing, hello.
> 
> > 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)
> 
> Just to make it sure. My question merely emphasized that 
> 
>   thd->clear_error()
>   Query_log_event(THD::NOT_KILLED)
> 
> pattern corresponds to the success branch only.
> 
> 
> >
> >> >
> >> > 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.
> 
> While we have a certain plan it should not be really so dramatic.
> Be sure I would be happy to help you in any aspect.
> 
> Now, let's approach some details.
> 
> As I see it the current bug's immediate problem is possible due to
> violation of principles we have discussed.
> 
> sql_db.cc contains:
> 
>     if (mysql_bin_log.is_open())
>     {
>       Query_log_event qinfo(thd, query, query_length, 0, 
> 			    /* suppress_use */ TRUE, THD::NOT_KILLED);
>       /*
>         Write should use the database being created as the "current
>         database" and not the threads current database, which is the
>         default.
>       */
>       qinfo.db     = db;
>       qinfo.db_len = (uint) strlen(db);
> 
>       thd->clear_error();
>       /* These DDL methods and logging protected with LOCK_mysql_create_db */
>       mysql_bin_log.write(&qinfo);
>     }
> 
> where ` thd->clear_error() ' is *not* on the correct place.

Agree

> Should it be affront of Query_log_event qinfo() you would not attempt to write the
> hunk for
> Query_log_event::Query_log_event() :-)
> 
> So again, fixing this issue means to me to verify at least all the success 
> branches to make sure they follow  clear_error-first-event-instance-NOT-KILLED-next
> pattern.
> It would be nice to follow-up with the (partial) failure branch (like mentioned in
> our thread).
> However, I personally would be okay to consider fixing that separately. 
> The major difficulty over there that I see is which error to report with the event in
> the case of multiple errors.
> It is reasonable to choose the first one, but that can really force some significant
> refactoring.
> 

I think you're right,  and I think if net.last_error is set to one of
the killed errors, then the statement must have been interrupted, and we
should not ignore the killed error, so I think we should have an
assertion like this in Query_log_event():

  DBUG_ASSERT(killed_status != THD::NOT_KILLED ||
              (error_code != ER_SERVER_SHUTDOWN &&
               error_code != ER_QUERY_INTERRUPTED ))

I'll check more on this issue, and will supply a patch later.

Thank you for the great findings!

> 
> cheers,
> 
> Andrei
> 
> 
> 
> 
>