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

[Andrei Elkin]

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.



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


cheers,

Andrei