maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Roberto,

comments inline.

On Thu, Sep 12, 2013 at 12:15:13PM -0300, Roberto Spadim wrote:
> Hi Guys!
> Some mariadb's user comments :)
> 
> 
> 2013/9/12 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.
> >
> That was the patch that i sent on MDEV, just a raw idea, but worked, maybe
> a copy and paste hehe =)
Yes, I used some of your patches. Thanks for sharing them.

> 
> 
> 
> > 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)
> >
> there was a warning in gcc about unsigned and signed when i put the patch
> in jira, but i didn't removed
> maybe "if ((longlong) (tmp->query_id)==id)" could remove the warning, but i
> don't know if it's ok
In your patch `id` is ulong and `query_id` is int64. In my patch `id` is longlong,
so it is not affected.

> 
> 
> 
> > > > +    {
> > > > +      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
> >
> I like the QUERY_ID with underscore to know that we are talking about the
> query_id, not the thread_id
> example:
> KILL CONNECTION QUERY_ID 99999
> KILL QUERY QUERY_ID 99999
I'll leave it up to you to convince Serg to use your syntax. :)

My preference is to kill connection by thread_id and query by query_id,
because I normally either want to stop a particular query, or stop all
activity in particular connection. But it is incompatible change.

> 
> 
> 
> > >
> > > 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
> >
> For me, it's ok to kill connection or query using the query id or the
> thread id, both are nice solutions
> the query_id can change while we read the processlist and send the kill
> command, while the thread_id many times don't
> i want kill using the query id as parameter not the thread id, if the query
> isn't what i want, mariadb will send an error or a warning, that's what i
> expect
> 
> i'm new to source code of mariadb, it's difficult for me to change bison
> files, i tested but was very confusing to me now (maybe in future i could
> change it easier)
> i think a nice nice syntax could be:
> KILL [CONNECTION | CONNECTION QUERY ID | QUERY | QUERY ID ] exp
> just an idea I think that QUERY_ID is easier to implement in bison than
> QUERY ID, but i'm a begginner :P
> 
> 
> 
> > >    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.
> >
> wow! a lot of test changes!
> 
> 
> i didn't found a kill test with many ids, for example:
> KILL QUERY ID 1,2,3,4,5
> in my patch it didn't worked :P and i don't know how to allow it
> i think the "KILL 1,2,3,4,5" is allowed for KILL command based on threads,
> arent? could be nice the same syntax for kill query_id :) since it's the
> "same" KILL command, but using different "WHERE" parts
> 
> 
> another question :) maybe in futures MDEV could be nice something like:
> KILL QUERY_ID IN (1,2,3,4,5) or
> KILL QUERY_ID IN (SELECT QUERY_ID FROM information_schema.PROCESSLIST WHERE
> ..... )
> KILL THREAD_ID IN (1,2,3,4,5) or
> KILL THREAD_ID IN (SELECT ID FROM information_schema.PROCESSLIST WHERE
> ..... )
> KILL CONNECTION QUERY_ID IN (1,2,3,4,5) or
> KILL CONNECTION QUERY_ID IN (SELECT QUERY_ID FROM
> information_schema.PROCESSLIST WHERE ..... )
> KILL CONNECTION THREAD_ID IN (1,2,3,4,5) or
> KILL CONNECTION THREAD_ID IN (SELECT ID FROM information_schema.PROCESSLIST
> WHERE ..... )
> 
> it's another mdev, but what you think about it?
Heh, what about DELETE FROM INFORMATION_SCHEMA.PROCESSLIST WHERE ...? But I guess
that'll be quite complex.

Anyway we didn't yet implement multi-process kill. Feel free to create another
MDEV.

Regards,
Sergey