Hi Guys!

Some mariadb's user comments :)

2013/9/12 Sergey Vojtovich <svoj@mariadb.org>

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

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

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

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

Thanks,
Sergey

_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp

thanks guys! you are the best!

Roberto Spadim