maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxxx>]

Hi, Michael!

On Sep 23, Michael Widenius wrote:
> revno: 3192
> revision-id: monty@xxxxxxxxxxxx-20110922221338-wxnyyd5grsn689dy
> parent: sergii@xxxxxxxxx-20110922090400-jkxe2vagjakr4lbi
> committer: Michael Widenius <monty@xxxxxxxxxxxx>
> 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