Hi!
On Oct 04, Sergey Vojtovich wrote:
> ChangeSet@stripped, 2007-10-04 14:56:25+05:00, svoj@stripped +15 -0
> WL#3951 - MyISAM: Additional Error Logs for Data Corruption
>
> When table currption is detected, in addition to current error message
> provide following information:
> - list of threads (and queries) accessing a table;
> - thread_id of a thread that detected corruption;
> - source file name and line number where this corruption was detected;
> - optional extra information (string).
First (unrelated to the patch) - when you'll be pushing this, please
push also a fix for the post-commit trigger to have WL#xxx in the
subject (but in a separate changeset, of course). I don't know why it
doesn't do it currently :(
> diff -Nrup a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc
> --- a/storage/myisam/ha_myisam.cc 2007-09-21 15:42:33 +05:00
> +++ b/storage/myisam/ha_myisam.cc 2007-10-04 14:56:23 +05:00
> @@ -476,6 +476,55 @@ void mi_check_print_warning(MI_CHECK *pa
> va_end(args);
> }
>
> +
> +/**
> + Report list of threads (and queries) accessing a table, thread_id of a
> + thread that detected corruption, source file name and line number where
> + this corruption was detected, optional extra information (string).
> +
> + This function is intended to be used when table corruption is detected.
> +
> + @param[in] file MI_INFO object.
> + @param[in] message Optional error message.
> + @param[in] sfile Name of source file.
> + @param[in] sline Line number in source file.
> +
> + @return void
> +*/
> +
> +void _mi_report_crashed(MI_INFO *file, const char *message,
> + const char *sfile, uint sline)
> +{
> + THD *cur_thd;
> + LIST *element;
> + pthread_mutex_lock(&file->s->intern_lock);
> + if ((cur_thd= (THD*) file->in_use.data))
> + sql_print_error("Got an error from thread_id=%lu, %s:%d",
> cur_thd->thread_id,
> + sfile, sline);
> + else
> + sql_print_error("Got an error from unknown thread, %s:%d", sfile, sline);
> + if (message)
> + sql_print_error("%s", message);
> + for (element= file->s->in_use; element; element= list_rest(element))
> + {
> + THD *thd= (THD*) element->data;
> + if (thd)
> + {
> + if (thd->command == COM_DELAYED_INSERT)
> + sql_print_error("thread_id=%lu, query=Delayed insert thread",
> + thd->thread_id);
> + else if (thd->query)
> + sql_print_error("thread_id=%lu, query=%.*s", thd->thread_id,
> + thd->query_length, thd->query);
Try to access thd only via accessor functions, not directly. In this
case you could use thd_security_context()
> + else
> + sql_print_error("thread_id=%lu, query=no query", thd->thread_id);
> + }
> + else
> + sql_print_error("thread_id=unknown, query=no query");
> + }
> + pthread_mutex_unlock(&file->s->intern_lock);
> +}
> +
> }
>
> @@ -1680,9 +1729,22 @@ int ha_myisam::delete_table(const char *
>
> int ha_myisam::external_lock(THD *thd, int lock_type)
> {
> - return mi_lock_database(file, !table->s->tmp_table ?
> - lock_type : ((lock_type == F_UNLCK) ?
> - F_UNLCK : F_EXTRA_LCK));
> + int res= mi_lock_database(file, !table->s->tmp_table ?
> + lock_type : ((lock_type == F_UNLCK) ?
> + F_UNLCK : F_EXTRA_LCK));
> + if (!res)
> + {
> + pthread_mutex_lock(&file->s->intern_lock);
> + if (lock_type == F_UNLCK)
> + file->s->in_use= list_delete(file->s->in_use,
> &file->in_use);
> + else
> + {
> + file->in_use.data= thd;
> + file->s->in_use= list_add(file->s->in_use, &file->in_use);
> + }
> + pthread_mutex_unlock(&file->s->intern_lock);
That's two file->s->intern_lock locks per external_lock(). I think it's
unnecessary, try to do everything under one lock (moving list
manipulations into mi_lock_database()).
> + }
> + return res;
> }
>
> THR_LOCK_DATA **ha_myisam::store_lock(THD *thd,
> diff -Nrup a/storage/myisammrg/ha_myisammrg.cc b/storage/myisammrg/ha_myisammrg.cc
> --- a/storage/myisammrg/ha_myisammrg.cc 2007-08-13 18:11:17 +05:00
> +++ b/storage/myisammrg/ha_myisammrg.cc 2007-10-04 14:56:23 +05:00
> @@ -411,7 +411,34 @@ int ha_myisammrg::extra_opt(enum ha_extr
>
> int ha_myisammrg::external_lock(THD *thd, int lock_type)
> {
> - return myrg_lock_database(file,lock_type);
> + int res= myrg_lock_database(file,lock_type);
> + if (!res)
> + {
> + THD *thd= current_thd;
> + MYRG_TABLE *tmp;
> + if (lock_type == F_UNLCK)
> + {
> + for (tmp= file->open_tables; tmp != file->end_table; tmp++)
> + {
> + pthread_mutex_lock(&tmp->table->s->intern_lock);
> + tmp->table->s->in_use= list_delete(tmp->table->s->in_use,
> + &tmp->table->in_use);
> + pthread_mutex_unlock(&tmp->table->s->intern_lock);
> + }
> + }
> + else
> + {
> + for (tmp= file->open_tables; tmp != file->end_table; tmp++)
> + {
> + pthread_mutex_lock(&tmp->table->s->intern_lock);
> + tmp->table->in_use.data= thd;
> + tmp->table->s->in_use= list_add(tmp->table->s->in_use,
> + &tmp->table->in_use);
> + pthread_mutex_unlock(&tmp->table->s->intern_lock);
> + }
> + }
> + }
> + return res;
> }
Same here, about moving list manipulations in mi_lock_database()
(this should be easy to do, I assume, after you change
ha_myisam::external_lock)
Regards / Mit vielen Grüssen,
Sergei
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Sergei Golubchik <serg@stripped>
/ /|_/ / // /\ \/ /_/ / /__ Principal Software Developer
/_/ /_/\_, /___/\___\_\___/ MySQL GmbH, Dachauer Str. 37, D-80335 München
<___/ Geschäftsführer: Kaj Arnö - HRB
München 162140
