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