maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

thanks for your comments, answers inline.

On Thu, Sep 12, 2013 at 04:27:57PM +0200, Sergei Golubchik wrote:
> 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.
For no good reason, I'll fix 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
> ?
Same here. I was a bit scared by amount of affected test cases, so decided
to submit "early" patch.

Thanks,
Sergey