maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

>>>>> "Sergei" == Sergei Golubchik <serg@xxxxxxxxxxxx> writes:

Sergei> Hi, Michael!
Sergei> 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.

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