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

[He Zhenxing]

Andrei Elkin wrote:
> Zhen Xing, hello.
> 
> Sorry for delaying with an answer.
> 
> > Andrei Elkin wrote:
> >> Zhen Xing,
> >> 
> >> >> killed_status as NOT KILLED provided to Query_log_event
> constructor
> >> >> in many cases (for instance in mysql_truncate) in combined with
> prior (the optimal way) or posterior 
> >> >> 
> >> >>    thd->clear_error();
> >> >> 
> >> >> So you must see the point. The NOT-KILLED-status caller's intention
> is to neither send any error back to the client neither affect the event instance with
> that error.
> >> 
> >> I did not say an important preposition - the query execution has not fail,
> like
> >> in mysqld_truncate():
> >> 
> >>     if (!error)
> >>     {
> >>       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);
> >>       }
> >>       send_ok(thd);		// This should return record count
> >>     }
> >> 
> >> or in sp.cc's db_create_routine():
> >> 
> >>     if (table->file->write_row(table->record[0]))
> >>       ret= SP_WRITE_ROW_FAILED;
> >>     else if (mysql_bin_log.is_open())
> >>     {
> >>       thd->clear_error();
> >> 
> >>       String log_query;
> >>       log_query.set_charset(system_charset_info);
> >> 
> >>       if (!create_string(thd, &log_query,
> >>                          sp->m_type,
> >>                          (sp->m_explicit_name ? sp->m_db.str : NULL),
> 
> >>                          (sp->m_explicit_name ? sp->m_db.length : 0),
> 
> >>                          sp->m_name.str, sp->m_name.length,
> >>                          sp->m_params.str, sp->m_params.length,
> >>                          retstr.c_ptr(), retstr.length(),
> >>                          sp->m_body.str, sp->m_body.length,
> >>                          sp->m_chistics,
> &(thd->lex->definer->user),
> >>                          &(thd->lex->definer->host)))
> >>       {
> >>         ret= SP_INTERNAL_ERROR;
> >>         goto done;
> >>       }
> >> 
> >>       /* Such a statement can always go directly to binlog, no trans cache
> */
> >>       Query_log_event qinfo(thd, log_query.c_ptr(), log_query.length(), 0,
> >>                             FALSE, THD::NOT_KILLED);
> >>       mysql_bin_log.write(&qinfo);
> >>     }
> >> 
> >> 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.
> 
> 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?

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.

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.


> >> >> Having 
> >> >>    thd->clear_error() 
> >> >> be called in front of
> >> >>    Query_log_event qinfo(... THD::NOT_KILLED);
> >> >> would be a better way of fixing this issue.
> >> >> 
> >> >> I suggest to analyze pieces of code where NOT-KILLED-status is not
> prepared with
> >> >> thd->clear_error() and consider to fixing that.
> >> >> 
> 
> cheers,
> 
> Andrei
> 
> 
> >> >
> >> 
> >> > No, this is not correct, NO_KILLED means only ignore the KILLED errors
> >> > (ER_QUERY_INTERRUPTED/ER_SERVER_SHUTDOWN), but other errors should not
> >> > be ignored. thd->clear_error() would clear all errors.
> >> 
> >> 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);
> >> 
> >> 
> >> I identified a different use case though.
> >> 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.
> >> 
> >
> > The best way is to passing the error code (instead of killed_status) to
> > the Query_log_event constructor, which is the work of bug#41948, but
> > this work will not be backported to 5.0.
> >
> >> 
> >> cheers,
> >> 
> >> Andrei
> >> 
> >> 
> >> -- 
> >> MySQL Code Commits Mailing List
> >> For list archives: http://lists.mysql.com/commits
> >> To unsubscribe:    http://lists.mysql.com/commits?unsub=1
>