maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #00632
Re: keycache flush problem
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
