Hi Konstantin,
OK to push from me. Please see suggestions for improvement below.
konstantin@stripped wrote:
...
> ChangeSet@stripped, 2007-07-19 03:28:47+04:00, kostja@bodhi.(none) +1 -0
> A fix for Bug#29431 killing an insert delayed thread causes crash
> No test case, since the bug requires a stress case with 30 INSERT DELAYED
> threads and 1 killer thread to repeat. The patch is verified
> manually.
>
> The server that is running DELAYED inserts would deadlock itself
> or crash under high load if some of the delayed threads were KILLed
> in the meanwhile.
>
> The fix is to change internal lock acquisition order of delayed inserts
> subsystem and to ensure that
> Delayed_insert::table_list::db does not point to volatile memory in some
> cases.
> For details, please see a comment for sql_insert.cc.
>
> sql/sql_insert.cc@stripped, 2007-07-19 03:28:45+04:00, kostja@bodhi.(none) +5 -6
> A fix for Bug#29431 killing an insert delayed thread causes crash
...
> delayed_get_table would:
...
> - discover that the delayed thread was killed
> - try to unlock() the delayed insert instance in memory and in
> Delayed_insert::unlock()
What do you mean with "and in" here? Trying to unlock() means to call
Delayed_insert::unlock() right?
Ah. You probably mean:
- try to unlock() the delayed insert instance
- in Delayed_insert::unlock() we do:
* lock LOCK_delayed_insert
...
> * lock LOCK_delayed_insert
> * decrease locks_in_memory and discover it's 0
...
> As a side note, the whole concept of two locks - LOCK_delayed_insert
> and LOCK_delayed_create is redundant, and we only need one of them
> to protect the global delayed threads list.
> As is redundant the concept of locks_in_memory, since we already
> have another counter with similar semantics - tables_in_use, both
> of them are devoted to counting the number of producers for a given
> consumer (delayed insert thread), only at different stages of
> producer-consumer relationship.
> 'dead' and 'status' variables in Delayed_insert are redundant too,
> since there is already 'di->thd.killed' and di->stacked_inserts.
> This redundancy made it hard to locate the bug, however the fix itself
> luckily is simple.
Agree. But is a changeset the right place to add such note? Wouldn't a
TODO comment in the code be more appropriate? Or a worklog entry/feature
request?
I would like to add the confusing naming of variables. We should always
call a Delayed_insert object 'di', not 'tmp'.
...
> 3) The fix in find_handler is pure safety - we needn't assume that
> Delayed_insert object already has a table opened at the point it is
> present in the delayed insert list, although now it's always the case.
I agree with the belonging change, but disagree with "now it's always
the case". Moving the list append to the client doesn't change that a di
object is added to the list only when it was opened. The same was true
before due to the enclosing LOCK_delayed_create.
However, a terminating delayed thread closes the table before unlinking
di from the list. Your patch doesn't change this. So find_handler()
could see a closed table in both cases.
>
> diff -Nrup a/sql/sql_insert.cc b/sql/sql_insert.cc
> --- a/sql/sql_insert.cc 2007-07-08 23:02:59 +04:00
> +++ b/sql/sql_insert.cc 2007-07-19 03:28:45 +04:00
> @@ -1706,7 +1706,7 @@ Delayed_insert *find_handler(THD *thd, T
> while ((tmp=it++))
> {
> if (!strcmp(tmp->thd.db,table_list->db) &&
I suggest to write strcmp(table_list->db, tmp->table_list.db) here. This
would make it more symmetric with the next line. Having argument and tmp
on different sides in both compares and using tmp->thd in one line, but
tmp->table_list in the other, looks like an attempt to maximize
confusion of a reader.
> - !strcmp(table_list->table_name,tmp->table->s->table_name))
> + !strcmp(table_list->table_name, tmp->table_list.table_name))
...
> tmp->table_list= *table_list; // Needed to open table
I'd add here: /* Replace volatile strings with local copies. */
> tmp->table_list.alias= tmp->table_list.table_name= tmp->thd.query;
> + tmp->table_list.db= tmp->thd.db;
...
Regards
Ingo
--
Ingo Strüwing, Senior Software Developer
MySQL GmbH, Radlkoferstr. 2, D-81373 München
Geschäftsführer: Kaj Arnö - HRB München 162140
