Hi! For those that don't know what this is about: - This is a fix for the case where you do a DROP TABLE of a MyISAM table with key delayed MyISAM writes all the changed key pages for the file to disk before closing and then deleting the table. This patch is a first attempt to fix that we don't write the key pages in case of drop.
"Oleksandr" == Oleksandr Byelkin <Oleksandr> writes:
Oleksandr> Hi! Oleksandr> I made different patch from you suggested maybe I am wrong but IMHO it Oleksandr> is better because: Oleksandr> 1) use already existing keycache calls Oleksandr> 2) do not require additional finding table MI_INFO by name and locking Oleksandr> around it Oleksandr> The main idea was that if we are going to drop table it can be passed Oleksandr> via existing table descriptors to the place where we call flush and it Oleksandr> does not matter if other threads trying to do something with the table Oleksandr> we will drop it in any case. Oleksandr> === modified file 'sql/handler.h' Oleksandr> --- sql/handler.h 2009-06-29 21:03:30 +0000 Oleksandr> +++ sql/handler.h 2009-08-09 19:52:01 +0000 Oleksandr> @@ -1342,6 +1342,7 @@ public: Oleksandr> virtual void column_bitmaps_signal(); Oleksandr> uint get_index(void) const { return active_index; } Oleksandr> virtual int close(void)=0; Oleksandr> + virtual void prepare_for_delete() {} Oleksandr> /** Oleksandr> @retval 0 Bulk update used by handler Why using prepare_for_delete(), instead of adding a more general call ? Oleksandr> === modified file 'sql/lock.cc' Oleksandr> --- sql/lock.cc 2009-04-25 10:05:32 +0000 Oleksandr> +++ sql/lock.cc 2009-08-09 22:08:41 +0000 Oleksandr> @@ -1049,10 +1049,12 @@ int lock_table_name(THD *thd, TABLE_LIST Oleksandr> DBUG_RETURN(-1); Oleksandr> table_list->table=table; Oleksandr> + table->s->deleting= table_list->deleting; Oleksandr> /* Return 1 if table is in use */ Oleksandr> DBUG_RETURN(test(remove_table_from_cache(thd, db, table_list->table_name, Oleksandr> - check_in_use ? RTFC_NO_FLAG : RTFC_WAIT_OTHER_THREAD_FLAG))); Oleksandr> + check_in_use ? RTFC_NO_FLAG : RTFC_WAIT_OTHER_THREAD_FLAG, Oleksandr> + table_list->deleting))); Oleksandr> } Oleksandr> === modified file 'sql/mysql_priv.h' Oleksandr> --- sql/mysql_priv.h 2009-04-25 10:05:32 +0000 Oleksandr> +++ sql/mysql_priv.h 2009-08-09 21:51:48 +0000 Oleksandr> @@ -1609,7 +1609,7 @@ uint prep_alter_part_table(THD *thd, TAB Oleksandr> #define RTFC_WAIT_OTHER_THREAD_FLAG 0x0002 Oleksandr> #define RTFC_CHECK_KILLED_FLAG 0x0004 Oleksandr> bool remove_table_from_cache(THD *thd, const char *db, const char *table, Oleksandr> - uint flags); Oleksandr> + uint flags, my_bool deleting); Oleksandr> #define NORMAL_PART_NAME 0 Oleksandr> #define TEMP_PART_NAME 1 Oleksandr> === modified file 'sql/sql_base.cc' Oleksandr> --- sql/sql_base.cc 2009-05-19 09:28:05 +0000 Oleksandr> +++ sql/sql_base.cc 2009-08-09 21:54:54 +0000 Oleksandr> @@ -927,7 +927,7 @@ bool close_cached_tables(THD *thd, TABLE Oleksandr> for (TABLE_LIST *table= tables; table; table= table->next_local) Oleksandr> { Oleksandr> if (remove_table_from_cache(thd, table->db, table->table_name, Oleksandr> - RTFC_OWNED_BY_THD_FLAG)) Oleksandr> + RTFC_OWNED_BY_THD_FLAG, table->deleting)) Oleksandr> found=1; Oleksandr> } Oleksandr> if (!found) Oleksandr> @@ -8395,7 +8395,7 @@ void flush_tables() Oleksandr> */ Oleksandr> bool remove_table_from_cache(THD *thd, const char *db, const char *table_name, Oleksandr> - uint flags) Oleksandr> + uint flags, my_bool deleting) Oleksandr> { Oleksandr> char key[MAX_DBKEY_LENGTH]; Oleksandr> uint key_length; Oleksandr> @@ -8482,7 +8482,10 @@ bool remove_table_from_cache(THD *thd, c Oleksandr> } Oleksandr> } Oleksandr> while (unused_tables && !unused_tables->s->version) Oleksandr> + { Oleksandr> + unused_tables->s->deleting= deleting; Oleksandr> VOID(hash_delete(&open_cache,(uchar*) unused_tables)); Oleksandr> + } Oleksandr> DBUG_PRINT("info", ("Removing table from table_def_cache")); Oleksandr> /* Remove table from table definition cache if it's not in use */ Oleksandr> @@ -8676,7 +8679,8 @@ int abort_and_upgrade_lock(ALTER_PARTITI Oleksandr> /* If MERGE child, forward lock handling to parent. */ Oleksandr> mysql_lock_abort(lpt->thd, lpt->table->parent ? lpt->table->parent : Oleksandr> lpt->table, TRUE); Oleksandr> - VOID(remove_table_from_cache(lpt->thd, lpt->db, lpt->table_name, flags)); Oleksandr> + VOID(remove_table_from_cache(lpt->thd, lpt->db, lpt->table_name, flags, Oleksandr> + FALSE)); Oleksandr> VOID(pthread_mutex_unlock(&LOCK_open)); Oleksandr> DBUG_RETURN(0); Oleksandr> } Oleksandr> @@ -8701,7 +8705,7 @@ void close_open_tables_and_downgrade(ALT Oleksandr> { Oleksandr> VOID(pthread_mutex_lock(&LOCK_open)); Oleksandr> remove_table_from_cache(lpt->thd, lpt->db, lpt->table_name, Oleksandr> - RTFC_WAIT_OTHER_THREAD_FLAG); Oleksandr> + RTFC_WAIT_OTHER_THREAD_FLAG, FALSE); Oleksandr> VOID(pthread_mutex_unlock(&LOCK_open)); Oleksandr> /* If MERGE child, forward lock handling to parent. */ Oleksandr> mysql_lock_downgrade_write(lpt->thd, lpt->table->parent ? lpt->table->parent : Oleksandr> === modified file 'sql/sql_table.cc' Oleksandr> --- sql/sql_table.cc 2009-06-18 12:39:21 +0000 Oleksandr> +++ sql/sql_table.cc 2009-08-09 21:48:04 +0000 Oleksandr> @@ -1599,6 +1599,8 @@ int mysql_rm_table_part2(THD *thd, TABLE Oleksandr> if ((share= get_cached_table_share(table->db, table->table_name))) Oleksandr> table->db_type= share->db_type(); Oleksandr> + table->deleting= TRUE; Oleksandr> + Oleksandr> /* Disable drop of enabled log tables */ Oleksandr> if (share && (share->table_category == TABLE_CATEGORY_PERFORMANCE) && Oleksandr> check_if_log_table(table->db_length, table->db, Oleksandr> @@ -1676,7 +1678,7 @@ int mysql_rm_table_part2(THD *thd, TABLE Oleksandr> abort_locked_tables(thd, db, table->table_name); Oleksandr> remove_table_from_cache(thd, db, table->table_name, Oleksandr> RTFC_WAIT_OTHER_THREAD_FLAG | Oleksandr> - RTFC_CHECK_KILLED_FLAG); Oleksandr> + RTFC_CHECK_KILLED_FLAG, TRUE); Oleksandr> /* Oleksandr> If the table was used in lock tables, remember it so that Oleksandr> unlock_table_names can free it Oleksandr> @@ -3862,7 +3864,7 @@ void wait_while_table_is_used(THD *thd,T Oleksandr> /* Wait until all there are no other threads that has this table open */ Oleksandr> remove_table_from_cache(thd, table->s->db.str, Oleksandr> table->s->table_name.str, Oleksandr> - RTFC_WAIT_OTHER_THREAD_FLAG); Oleksandr> + RTFC_WAIT_OTHER_THREAD_FLAG, FALSE); Oleksandr> /* extra() call must come only after all instances above are closed */ Oleksandr> VOID(table->file->extra(function)); Oleksandr> DBUG_VOID_RETURN; Oleksandr> @@ -4366,7 +4368,7 @@ static bool mysql_admin_table(THD* thd, Oleksandr> remove_table_from_cache(thd, table->table->s->db.str, Oleksandr> table->table->s->table_name.str, Oleksandr> RTFC_WAIT_OTHER_THREAD_FLAG | Oleksandr> - RTFC_CHECK_KILLED_FLAG); Oleksandr> + RTFC_CHECK_KILLED_FLAG, FALSE); Oleksandr> thd->exit_cond(old_message); Oleksandr> DBUG_EXECUTE_IF("wait_in_mysql_admin_table", wait_for_kill_signal(thd);); Oleksandr> if (thd->killed) Oleksandr> @@ -4624,7 +4626,8 @@ send_result_message: Oleksandr> { Oleksandr> pthread_mutex_lock(&LOCK_open); Oleksandr> remove_table_from_cache(thd, table->table->s->db.str, Oleksandr> - table->table->s->table_name.str, RTFC_NO_FLAG); Oleksandr> + table->table->s->table_name.str, Oleksandr> + RTFC_NO_FLAG, FALSE); Oleksandr> pthread_mutex_unlock(&LOCK_open); Oleksandr> } Oleksandr> /* May be something modified consequently we have to invalidate cache */ Oleksandr> === modified file 'sql/table.cc' Oleksandr> --- sql/table.cc 2009-06-29 21:03:30 +0000 Oleksandr> +++ sql/table.cc 2009-08-09 20:46:07 +0000 Oleksandr> @@ -1960,7 +1960,12 @@ int closefrm(register TABLE *table, bool Oleksandr> DBUG_PRINT("enter", ("table: 0x%lx", (long) table)); Oleksandr> if (table->db_stat) Oleksandr> - error=table->file->close(); Oleksandr> + { Oleksandr> + if (table->s->deleting) Oleksandr> + table->file->prepare_for_delete(); Oleksandr> + error= table->file->close(); Oleksandr> + } Oleksandr> + As we have a handler here, we not instead do ? table->file->extra(HA_EXTRA_PREPARE_FOR_DROP); There is no reason to add an extra prepare_for_delete() here. Oleksandr> --- storage/myisam/ha_myisam.cc 2009-06-29 21:03:30 +0000 Oleksandr> +++ storage/myisam/ha_myisam.cc 2009-08-09 20:42:15 +0000 Oleksandr> @@ -26,7 +26,9 @@ Oleksandr> #include <myisampack.h> Oleksandr> #include "ha_myisam.h" Oleksandr> #include <stdarg.h> Oleksandr> +C_MODE_START Oleksandr> #include "myisamdef.h" Oleksandr> +C_MODE_END Oleksandr> #include "rt_index.h" With my suggested change, no reason to do any changes in ha_myisam.cc or ha_myisam.h <cut> Oleksandr> +++ storage/myisam/mi_close.c 2009-08-09 22:01:32 +0000 Oleksandr> @@ -65,8 +65,9 @@ int mi_close(register MI_INFO *info) Oleksandr> { Oleksandr> if (share->kfile >= 0 && Oleksandr> flush_key_blocks(share->key_cache, share->kfile, Oleksandr> - share->temporary ? FLUSH_IGNORE_CHANGED : Oleksandr> - FLUSH_RELEASE)) Oleksandr> + (share->temporary || share->deleting) ? Oleksandr> + FLUSH_IGNORE_CHANGED : Oleksandr> + FLUSH_RELEASE)) Oleksandr> error=my_errno; Oleksandr> if (share->kfile >= 0) Oleksandr> { No reason for the above change. 1) In my suggestion, no reason to do this. 2) If we implement it your way, we could reuse 'share->temporary' for this cse. Oleksandr> === modified file 'storage/myisam/mi_locking.c' Oleksandr> --- storage/myisam/mi_locking.c 2009-04-01 09:34:52 +0000 Oleksandr> +++ storage/myisam/mi_locking.c 2009-08-09 20:42:00 +0000 Oleksandr> @@ -68,7 +68,10 @@ int mi_lock_database(MI_INFO *info, int Oleksandr> --share->tot_locks; Oleksandr> if (info->lock_type == F_WRLCK && !share->w_locks && Oleksandr> !share->delay_key_write && flush_key_blocks(share->key_cache, Oleksandr> - share->kfile,FLUSH_KEEP)) Oleksandr> + share->kfile, Oleksandr> + (share->deleting ? Oleksandr> + FLUSH_IGNORE_CHANGED : Oleksandr> + FLUSH_KEEP))) No reason to do the above. Reasons: - In case of delay_key_write, they above code will not be executed. - If delay_key_write is not set, things was flushed at previous statement. Did I miss some case? Oleksandr> --- storage/myisam/myisamdef.h 2009-04-25 09:04:38 +0000 Oleksandr> +++ storage/myisam/myisamdef.h 2009-08-09 20:41:25 +0000 Oleksandr> @@ -218,6 +218,7 @@ typedef struct st_mi_isam_share Oleksandr> my_bool changed, /* If changed since lock */ Oleksandr> global_changed, /* If changed since open */ Oleksandr> not_flushed, temporary, delay_key_write, concurrent_insert; Oleksandr> + my_bool deleting; /* we are going to delete this table */ Not needed. --------------- Other fixes: Please fix mi_extra.c as we dicussed (move the #ifdef so that things are flushed) do also the folloing fix to ma_extra.c: if (share->kfile.file >= 0) _ma_decrement_open_count(info); -> if (share->kfile.file >= 0 && do_flush) _ma_decrement_open_count(info); The idea is that we should not decrement the open_count in case of drop. This will ensure that if we die between flushing the key cache and close, the index will be rechecked. ------------- Regards, Monty