Hi!
"Sergei" == Sergei Golubchik <serg@askmonty.org> writes:
Sergei> Hi, Michael! Sergei> On Sep 23, Michael Widenius wrote:
revno: 3192 revision-id: monty@askmonty.org-20110922221338-wxnyyd5grsn689dy parent: sergii@pisem.net-20110922090400-jkxe2vagjakr4lbi committer: Michael Widenius <monty@askmonty.org> branch nick: maria-5.3 timestamp: Fri 2011-09-23 01:13:38 +0300 message: Added new options to KILL. New syntax is KILL [HARD|SOFT] [CONNECTION|QUERY] [ID | USER user_name] - If USER is given, all threads for that user is signaled - If SOFT is used then the KILL will not be sent to the handler.
Sergei> Hm. So, this is the definition? "not sent to the handler"? Sergei> I liked the older version of it "not interrupt an operation that leaves Sergei> the table corrupted or unusable". This definition is easier to Sergei> understand for the user, who does not necessarily know or care Sergei> how every particular statement is split between the server and the Sergei> engine (especially, as this is different for different engines). Sergei> So, the point is - no matter how this SOFT is internally implemented (by Sergei> not sending to the handler or whatever), what is the official definition Sergei> of this feature? What should the manual say? Currently is says "will not Sergei> send the signal to storage engines" which - see above - is suboptimal, Sergei> in my opinion. The knowledge base is updated according to this. <cut>
=== modified file 'include/mysql.h.pp' --- a/include/mysql.h.pp 2011-07-21 12:50:25 +0000 +++ b/include/mysql.h.pp 2011-09-22 22:13:38 +0000 @@ -67,8 +67,8 @@ enum mysql_enum_shutdown_level { SHUTDOWN_WAIT_UPDATES= (unsigned char)(1 << 3), SHUTDOWN_WAIT_ALL_BUFFERS= ((unsigned char)(1 << 3) << 1), SHUTDOWN_WAIT_CRITICAL_BUFFERS= ((unsigned char)(1 << 3) << 1) + 1, - KILL_QUERY= 254, - KILL_CONNECTION= 255 + SHUTDOWN_KILL_QUERY= 254, + SHUTDOWN_KILL_CONNECTION= 255
Sergei> Why wouldn't you completely remove KILL_QUERY and KILL_CONNECTION from Sergei> this enum ? Sergei> Strictly speaking they are not "shutdown levels", they make no sense as Sergei> arguments to mysql_shutdown(). And, as you've noticed, shutdown levels Sergei> are completely ignored at the moment anyway - so nobody should miss Sergei> these constants if you remove them. I didn't want to remove them until I am 100 % sure no applications uses them. Now if anyone uses them, they will probably file a bug report and they can go around the problem for the time being by defining USE_OLD_FUNCTIONS. If no one complains, I will remove this in 5.5
=== modified file 'mysql-test/t/kill.test' --- a/mysql-test/t/kill.test 2009-12-03 11:19:05 +0000 +++ b/mysql-test/t/kill.test 2011-09-22 22:13:38 +0000 @@ -337,5 +337,35 @@ SELECT 1;
###########################################################################
+--echo # +--echo # Test kill USER +--echo # + +grant ALL on test.* to test@localhost; +grant ALL on test.* to test2@localhost; +connect (con3, localhost, test,,); +connect (con4, localhost, test2,,); +connection default; +--enable_info +kill hard query user test2@nohost; +kill soft query user test@localhost; +kill hard query user test@localhost; +kill soft connection user test2; +kill hard connection user test@localhost;
Sergei> What about real tests? When you kill soft actually kills (and doesn't Sergei> kill) a statement. You can ask Philip to write them, I suppose. I have tested this by hand as part of working with spamexperts for solving their REPAIR problem. I will ask Philip if he has any ideas for how to test this.
=== modified file 'sql/sql_parse.cc' --- a/sql/sql_parse.cc 2011-09-10 15:01:27 +0000 +++ b/sql/sql_parse.cc 2011-09-22 22:13:38 +0000 @@ -7231,6 +7240,76 @@ uint kill_one_thread(THD *thd, ulong id, } Sergei> ... +static uint kill_threads_for_user(THD *thd, LEX_USER *user, + killed_state kill_signal, ha_rows *rows) +{ + THD *tmp; + List<THD> threads_to_kill; + DBUG_ENTER("kill_threads_for_user"); + + *rows= 0; + + if (thd->is_fatal_error) // If we run out of memory + DBUG_RETURN(ER_OUT_OF_RESOURCES); + + DBUG_PRINT("enter", ("user: %s signal: %u", user->user.str, + (uint) kill_signal)); + + VOID(pthread_mutex_lock(&LOCK_thread_count)); // For unlink from list + I_List_iterator<THD> it(threads); + while ((tmp=it++)) + { + if (tmp->command == COM_DAEMON) + continue; + /* + Check that hostname (if given) and user name matches. + + host.str[0] == '%' means that host name was not given. See sql_yacc.yy + */ + if (((user->host.str[0] == '%' && !user->host.str[1]) || + !strcmp(tmp->security_ctx->host, user->host.str)) && + !strcmp(tmp->security_ctx->user, user->user.str)) + { + if (!(thd->security_ctx->master_access & SUPER_ACL) && + !thd->security_ctx->user_matches(tmp->security_ctx))
Sergei> So, without SUPER a user can only kill his own connections with Sergei> KILL USER. Yes. This is what one can already do with kill thread_id. Sergei> A question. Sergei> Should "KILL USER my_name" also kill the current connection, or it Sergei> should only kill all other connections? If you do 'kill QUERY USER ...' then there should not be any issue. If you do 'kill connection USER CURRENT_USER()' then also your current connection will be killed so one better have reconnect working on the client. As one can kill ones own connection with 'kill thread_id' I didn't think much about if this is a problem or not.
+ { + VOID(pthread_mutex_unlock(&LOCK_thread_count)); + DBUG_RETURN(ER_KILL_DENIED_ERROR); + } + if (!threads_to_kill.push_back(tmp, tmp->mem_root)) + pthread_mutex_lock(&tmp->LOCK_thd_data); // Lock from delete + } + } + VOID(pthread_mutex_unlock(&LOCK_thread_count)); + if (!threads_to_kill.is_empty()) + { + List_iterator_fast<THD> it(threads_to_kill); + THD *ptr; + while ((ptr= it++)) + { + ptr->awake(kill_signal); + pthread_mutex_unlock(&ptr->LOCK_thd_data); + (*rows)++; + } + }
Sergei> It may be a bit problematic. You lock thd->LOCK_thd_data for many Sergei> threads. This defines a specific locking order. This is how normal kill works and you would get a similar problem if several kill's would be run in parallel. Sergei> Which sounds quite fragile - I see no guarantee that this order cannot Sergei> be violated by another thread. It can be even violated by some other Sergei> feature that we'll implement in the future - and we won't notice it, Sergei> even your deadlock detector won't catch it, because you create the Sergei> dependency only during KILL USER, and only for specific THD's. Sergei> So, if we'll ever have this locking order violated, it will show up as Sergei> very random lockups in the undefined future :( Sergei> I would suggest to rewrite this code (if possible) to avoid locking all thd-> LOCK_thd_data's at once. Sergei> Note that this code doesn't have to be fast. I looked at how other code uses thd->LOCK_thd_data and there is bascily two usages: - Lock the thread from deleting 'thd' - Update a couple of variables. In all cases like this it's only over a few instructions and there is never another mutex inside the update. One possible way to fix that is that if the original thread was already killed, we only update 'thd->killed' to the new killed state and continue without putting the thread into the threads_to_kill list. That would at least ensure that you can never get two kills to block each other. Another way that comes to mind is to do an 'awake' but without any timeouts or mutex in THD::awake. In this case we could call awake() in the upper level loop and signal all threads that they should be killed. The problem is then how to ge the cond_broadcast done to the threads that are waiting on a condition. I need to think about this for a while.
+ DBUG_RETURN(0); +} + @@ -7241,16 +7320,33 @@ uint kill_one_thread(THD *thd, ulong id, +void sql_kill_user(THD *thd, LEX_USER *user, killed_state state) +{ + uint error; + ha_rows rows; + if (!(error= kill_threads_for_user(thd, user, state, &rows)))
Sergei> I don't really understand why you've created a separate Sergei> kill_threads_for_user() instead of putting the code here. Sergei> But ok, as you like. I just made it identical as 'sql_kill', to keep things simple. Another way would have to move this code up to mysql_exceute_command and get rid of both functions. <cut>
=== modified file 'storage/archive/ha_archive.cc' --- a/storage/archive/ha_archive.cc 2011-07-05 19:46:53 +0000 +++ b/storage/archive/ha_archive.cc 2011-09-22 22:13:38 +0000 @@ -17,6 +17,7 @@ #pragma implementation // gcc: Class implementation #endif
+#define MYSQL_SERVER 1
Sergei> why? Because the storage engine needs to get access to all the MYSQL_SERVER flags and not only the client flags. The other engine we have also defines this flag (at least ha_myisam.cc, ha_maria.cc which I looked at). (There is some strange '#ifdef MYSQL_SERVER' code in mysql_priv.h that I am not clear why it's there). It may be that this is not critical to have it there anymore, but at least it doesn't do any harm and made the code identical to some other engines... Regards, Monty