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

[Andrei Elkin]

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

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