maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi.

Good work.
I only have one small comment about the code, see below.
And one about the syntax.

> === modified file 'sql/sql_parse.cc'
> --- sql/sql_parse.cc	2013-08-14 08:48:50 +0000
> +++ sql/sql_parse.cc	2013-09-11 12:29:32 +0000
> @@ -7139,24 +7139,60 @@ THD *find_thread_by_id(ulong id)
>  
>  
>  /**
> -  kill on thread.
> +  Find a thread by query id and return it, locking it LOCK_thd_data
> +
> +  @param id  Identifier of the query we're looking for
> +
> +  @return NULL    - not found
> +          pointer - thread found, and its LOCK_thd_data is locked.
> +*/
> +
> +static THD *find_thread_by_query_id(longlong id)

why didn't you reuse find_thread_by_id()?
this function is almost identical to it.

> +{
> +  THD *tmp;
> +  mysql_mutex_lock(&LOCK_thread_count); // For unlink from list
> +  I_List_iterator<THD> it(threads);
> +  while ((tmp=it++))
> +  {
> +    if (tmp->get_command() == COM_DAEMON)
> +      continue;
> +    if (tmp->query_id == id)
> +    {
> +      mysql_mutex_lock(&tmp->LOCK_thd_data);    // Lock from delete
> +      break;
> +    }
> +  }
> +  mysql_mutex_unlock(&LOCK_thread_count);
> +  return tmp;
> +}
> +
> +
> === modified file 'sql/sql_yacc.yy'
> --- sql/sql_yacc.yy	2013-08-13 11:35:36 +0000
> +++ sql/sql_yacc.yy	2013-09-11 12:29:32 +0000
> @@ -12890,6 +12891,11 @@ kill_expr:
>              Lex->users_list.push_back($2);
>              Lex->kill_type= KILL_TYPE_USER;
>            }
> +        | ID_SYM expr
> +          {
> +            Lex->value_list.push_front($2);
> +            Lex->kill_type= KILL_TYPE_QUERY;
> +          }

So, you implemented KILL [ CONNECTION | QUERY ] [ ID ] expr
It allows, in particular

   KILL CONNECTION ID 10

which looks strange, why would that mean that 10 is a query id?

I originally suggested KILL [ CONNECTION | QUERY [ ID ] ] expr
to allow only

   KILL CONNECTION expr
   KILL QUERY expr
   KILL QUERY ID expr

because in this case it's quite clear "QUERY ID" and because
I thought it's a bit strange to kill a connection by query_id.

If you want to allow that (as I saw in the comments, Roberto didn't
thought it's strange to kill a connection by query id), may be you'd
better use

   KILL [ CONNECTION | QUERY ] [ QUERY_ID ] expr
?

Regards,
Sergei