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 <[email protected]> > > > 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 _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
