* Magne Mahre <magne.mahre@stripped> [10/09/30 14:26]:
> === modified file 'storage/myisam/mi_panic.c'
> --- a/storage/myisam/mi_panic.c 2009-12-05 01:26:15 +0000
> +++ b/storage/myisam/mi_panic.c 2010-09-30 09:29:51 +0000
> @@ -26,54 +26,55 @@
> int mi_panic(enum ha_panic_function flag)
> {
> int error=0;
> - LIST *list_element,*next_open;
> MI_INFO *info;
> + uint idx;
> DBUG_ENTER("mi_panic");
>
> mysql_mutex_lock(&THR_LOCK_myisam);
> - for (list_element=myisam_open_list ; list_element ; list_element=next_open)
> + if (myisam_open_cache)
> {
> - next_open=list_element->next; /* Save if close */
> - info=(MI_INFO*) list_element->data;
> - switch (flag) {
> - case HA_PANIC_CLOSE:
> - mysql_mutex_unlock(&THR_LOCK_myisam); /* Not exactly right... */
> - if (mi_close(info))
> - error=my_errno;
> - mysql_mutex_lock(&THR_LOCK_myisam);
> - break;
> - case HA_PANIC_WRITE: /* Do this to free databases */
> + for (idx= 0; idx < myisam_open_cache->records; ++idx)
> + {
> + info=(MI_INFO*) my_hash_element(myisam_open_cache, idx);
> + switch (flag) {
> + case HA_PANIC_CLOSE:
> + mysql_mutex_unlock(&THR_LOCK_myisam); /* Not exactly right... */
> + if (mi_close(info))
> + error=my_errno;
I don't understand how it works at all.
mi_close() will erase the table entry from the hash. This
invalidates the iterator. I think for () loop needs to be changed
to
while (some_table_deleted)
{
for (idx= 0; idx < myisam_open_cache_records; ++idx)
{
continue;
}
some_table_deleted= false;
}
or alike...
Besides, in a fully concurrent environment, what is the guarantee
that mi_close() is not called twice on the same info? (The old
code seems to also have been prone to this bug).
I mean specifically when mi_panic is called simultaneously from
two threads:
thread1: take THR_LOCK_myisam, get the first record, release THR_LOCK_myisam
thread2: take THR_LOCK_myisam, get the first record, release THR_LOCK_myisam
thread1: mi_close first record
thread2: mi_close first record.
I think we need something like "panic_in_progress" to prevent
this situation, or add a comment that mi_panic should never be
called concurrently with itself.
--
