Hi Monty
Beside what we discussed on IRC today,
please find here other comments.
Regards,
Marc.
monty@stripped wrote:
> Below is the list of changes that have just been committed into a local
> 5.1 repository of monty. When monty does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2007-08-02 10:50:00+03:00, monty@stripped +4 -0
> Don't save & restore time fields from thd when it's not needed.
> Added back setting of 'some_tables_deleted' to not cause deadlocks in
> mysql_lock_table()
>
> BitKeeper/etc/ignore@stripped, 2007-08-02 10:33:31+03:00, monty@stripped +1 -0
> added tests/bug25714
>
> sql/lock.cc@stripped, 2007-08-02 10:49:56+03:00, monty@stripped +4 -0
> Added comment
>
> sql/log.cc@stripped, 2007-08-02 10:49:56+03:00, monty@stripped +10 -38
> Don't save & restore time fields from thd when it's not needed.
> Fix that we properly detect if open table failed
>
> sql/sql_base.cc@stripped, 2007-08-02 10:49:56+03:00, monty@stripped +6 -0
> Added back setting of 'some_tables_deleted' to not cause deadlocks in
> mysql_lock_table()
>
> diff -Nrup a/BitKeeper/etc/ignore b/BitKeeper/etc/ignore
> --- a/BitKeeper/etc/ignore 2007-07-25 01:55:09 +03:00
> +++ b/BitKeeper/etc/ignore 2007-08-02 10:33:31 +03:00
> @@ -2996,3 +2996,4 @@ win/vs8cache.txt
> zlib/*.ds?
> zlib/*.vcproj
> support-files/mysqld_multi.server
> +tests/bug25714
> diff -Nrup a/sql/lock.cc b/sql/lock.cc
> --- a/sql/lock.cc 2007-08-02 07:50:52 +03:00
> +++ b/sql/lock.cc 2007-08-02 10:49:56 +03:00
> @@ -290,6 +290,10 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd,
> }
> else if (!thd->some_tables_deleted || (flags & MYSQL_LOCK_IGNORE_FLUSH))
> {
> + /*
> + Thread was killed or lock aborted. Let upper level close all
> + used tables and retry or give error.
> + */
> thd->locked=0;
> break;
> }
> diff -Nrup a/sql/log.cc b/sql/log.cc
> --- a/sql/log.cc 2007-08-02 07:50:52 +03:00
> +++ b/sql/log.cc 2007-08-02 10:49:56 +03:00
> @@ -323,20 +323,11 @@ bool Log_to_csv_event_handler::
> Field_timestamp *field0;
> ulonglong save_thd_options;
> bool save_query_start_used;
> - time_t save_start_time;
> - time_t save_time_after_lock;
> - time_t save_user_time;
> - bool save_time_zone_used;
>
> + save_query_start_used= thd->query_start_used; // Because of
> field->set_time()
> save_thd_options= thd->options;
> thd->options&= ~OPTION_BIN_LOG;
>
> - save_query_start_used= thd->query_start_used;
> - save_start_time= thd->start_time;
> - save_time_after_lock= thd->time_after_lock;
> - save_user_time= thd->user_time;
> - save_time_zone_used= thd->time_zone_used;
> -
> bzero(& table_list, sizeof(TABLE_LIST));
> table_list.alias= table_list.table_name= GENERAL_LOG_NAME.str;
> table_list.table_name_length= GENERAL_LOG_NAME.length;
> @@ -346,12 +337,13 @@ bool Log_to_csv_event_handler::
> table_list.db= MYSQL_SCHEMA_NAME.str;
> table_list.db_length= MYSQL_SCHEMA_NAME.length;
>
> - table= open_performance_schema_table(thd, & table_list,
> - & open_tables_backup);
> + if (!(table= open_performance_schema_table(thd, & table_list,
> + & open_tables_backup)))
> + goto err;
> +
> need_close= TRUE;
>
If we don't call close_performance_schema_tables when the open failed,
that's another valid choice too, but in this case :
- open_performance_schema_tables should cleanup after itself on failures,
and restore the open_tables_state (thd::reset_n_backup_open_tables_state())
- the test "if (thd->lock)" located in close_performance_schema_table()
could be replaced by an assert
Independently of this, just a suggestion :
in open_performance_schema_table,
set table_list->skip_temporary to avoid some code in open_table()
>
> - if (!table ||
> - table->file->extra(HA_EXTRA_MARK_AS_LOG_TABLE) ||
> + if (table->file->extra(HA_EXTRA_MARK_AS_LOG_TABLE) ||
> table->file->ha_rnd_init(0))
> goto err;
>
> @@ -434,12 +426,7 @@ err:
> close_performance_schema_table(thd, & open_tables_backup);
>
> thd->options= save_thd_options;
> -
> thd->query_start_used= save_query_start_used;
> - thd->start_time= save_start_time;
> - thd->time_after_lock= save_time_after_lock;
> - thd->user_time= save_user_time;
> - thd->time_zone_used= save_time_zone_used;
> return result;
> }
>
> @@ -485,11 +472,6 @@ bool Log_to_csv_event_handler::
> bool need_close= FALSE;
> bool need_rnd_end= FALSE;
> Open_tables_state open_tables_backup;
> - bool save_query_start_used;
> - time_t save_start_time;
> - time_t save_time_after_lock;
> - time_t save_user_time;
> - bool save_time_zone_used;
> CHARSET_INFO *client_cs= thd->variables.character_set_client;
> DBUG_ENTER("Log_to_csv_event_handler::log_slow");
>
> @@ -502,18 +484,13 @@ bool Log_to_csv_event_handler::
> table_list.db= MYSQL_SCHEMA_NAME.str;
> table_list.db_length= MYSQL_SCHEMA_NAME.length;
>
> - save_query_start_used= thd->query_start_used;
> - save_start_time= thd->start_time;
> - save_time_after_lock= thd->time_after_lock;
> - save_user_time= thd->user_time;
> - save_time_zone_used= thd->time_zone_used;
> + if (!(table= open_performance_schema_table(thd, & table_list,
> + & open_tables_backup)))
> + goto err;
>
> - table= open_performance_schema_table(thd, & table_list,
> - & open_tables_backup);
> need_close= TRUE;
>
> - if (!table ||
> - table->file->extra(HA_EXTRA_MARK_AS_LOG_TABLE) ||
> + if (table->file->extra(HA_EXTRA_MARK_AS_LOG_TABLE) ||
> table->file->ha_rnd_init(0))
> goto err;
>
> @@ -635,11 +612,6 @@ err:
> if (need_close)
> close_performance_schema_table(thd, & open_tables_backup);
>
> - thd->query_start_used= save_query_start_used;
> - thd->start_time= save_start_time;
> - thd->time_after_lock= save_time_after_lock;
> - thd->user_time= save_user_time;
> - thd->time_zone_used= save_time_zone_used;
> DBUG_RETURN(result);
> }
>
> diff -Nrup a/sql/sql_base.cc b/sql/sql_base.cc
> --- a/sql/sql_base.cc 2007-08-01 11:30:11 +03:00
> +++ b/sql/sql_base.cc 2007-08-02 10:49:56 +03:00
> @@ -7198,6 +7198,12 @@ bool remove_table_from_cache(THD *thd, c
> else if (in_use != thd)
> {
> DBUG_PRINT("info", ("Table was in use by other thread"));
> + /*
> + Mark that table is going to be deleted from cache. This will
> + force threads that are in mysql_lock_tables() (but not yet
> + in thr_multi_lock()) to abort it's locks, close all tables and retry
> + */
> + in_use->some_tables_deleted= 1;
> if (table->is_name_opened())
> {
> DBUG_PRINT("info", ("Found another active instance of the table"));
>
>
