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

[He Zhenxing]

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.

> 
> 
> >> 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.
> >> 
> >
> 
> > 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