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