Re: [Maria-developers] [Commits] Rev 3192: Added new options to KILL. New syntax is KILL [HARD|SOFT] [CONNECTION|QUERY] [ID | USER user_name] in lp:maria/5.3
Hi, Michael! 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.
Hm. So, this is the definition? "not sent to the handler"? I liked the older version of it "not interrupt an operation that leaves the table corrupted or unusable". This definition is easier to understand for the user, who does not necessarily know or care how every particular statement is split between the server and the engine (especially, as this is different for different engines). So, the point is - no matter how this SOFT is internally implemented (by not sending to the handler or whatever), what is the official definition of this feature? What should the manual say? Currently is says "will not send the signal to storage engines" which - see above - is suboptimal, in my opinion.
This can be used to not interrupt critical things in the handler like 'REPAIR'.
Internally added more kill signals. This gives us more information of why a query/connection was killed. - KILL_SERVER is used when server is going down. In this case the users gets ER_SHUTDOWN as the reason connection was killed. - Changed signals to number in correct order, which makes it easier to test how the signal should affect the code. - New error message ER_CONNECTION_KILLED if connection was killed by 'KILL CONNECTION'. Before we got error ER_SHUTDOWN.
Changed names of not used parameters KILL_QUERY & KILL_CONNCTION to mysql_kill() to not conflict with defines in the server
=== 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
Why wouldn't you completely remove KILL_QUERY and KILL_CONNECTION from this enum ? Strictly speaking they are not "shutdown levels", they make no sense as arguments to mysql_shutdown(). And, as you've noticed, shutdown levels are completely ignored at the moment anyway - so nobody should miss these constants if you remove them.
}; enum enum_cursor_type {
=== 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;
What about real tests? When you kill soft actually kills (and doesn't kill) a statement. You can ask Philip to write them, I suppose.
=== 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, } ... +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))
So, without SUPER a user can only kill his own connections with KILL USER. A question. Should "KILL USER my_name" also kill the current connection, or it should only kill all other connections?
+ { + 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)++; + } + }
It may be a bit problematic. You lock thd->LOCK_thd_data for many threads. This defines a specific locking order. Which sounds quite fragile - I see no guarantee that this order cannot be violated by another thread. It can be even violated by some other feature that we'll implement in the future - and we won't notice it, even your deadlock detector won't catch it, because you create the dependency only during KILL USER, and only for specific THD's. So, if we'll ever have this locking order violated, it will show up as very random lockups in the undefined future :( I would suggest to rewrite this code (if possible) to avoid locking all thd->LOCK_thd_data's at once. Note that this code doesn't have to be fast.
+ 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)))
I don't really understand why you've created a separate kill_threads_for_user() instead of putting the code here. But ok, as you like.
+ my_ok(thd, rows); + else + { + /* + This is probably ER_OUT_OF_RESOURCES, but in the future we may + want to write the name of the user we tried to kill + */ + my_error(error, MYF(0), user->host.str, user->user.str); + } +} + === 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
why?
#include "mysql_priv.h" #include <myisam.h>
Regards, Sergei
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
Hi, Michael! On Sep 26, Michael Widenius wrote:
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.
I didn't say it's a problem. And of course, one can kill his own connection. I asked a question about what could be a more useful behavior for "KILL USER myself" - to kill the originating connection or not. But I'm personally fine with either outcome.
=== 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).
No, this is very wrong. Storage engines should not define MYSQL_SERVER - they generally should only access only data and definitions that are visible without MYSQL_SERVER. Definitions protected by MYSQL_SERVER are internal server stuff, not designed for storage engine consumption. There are certain engines that do access internal server stuff anyway - MyISAM and Aria being two of those (and XtraDB, for example). But a "proper" engine should not do that, and many engines do not. For example, InnoDB developers worked hard to remove all dependencies on MySQL internals from their engine, and now InnoDB does not need MYSQL_SERVER (although, it might change again in the future :). But anyway, if an engine does not need MYSQL_SERVER - it should not define it. If an engine really needs it - it should be changed not to. Regards, Sergei
participants (2)
-
Michael Widenius -
Sergei Golubchik