Hi Magne,
taking into account "WL#4305 - storage-engine private data area
per physical table", do you think we still should proceed with
this fix?
Mattias: is there any ETA for WL#4305?
Regards,
Sergey
On Thu, Oct 21, 2010 at 10:52:02AM +0000, Magne Mahre wrote:
> #At file:///export/home/tmp/x/mysql-next-mr-bugfixing-49177/ based on
> revid:anitha.gopi@stripped
>
> 3328 Magne Mahre 2010-10-21
> Bug#49177 MyISAM share list scalability problems
>
> (Note: MyISAM issue only)
>
> As the number of open tables is increased, table lookup
> (testing if a table is already open) and (in particular)
> the case when a table is not open, became increasingly more
> expensive.
>
> The problem was caused by the open table lookup mechanism,
> which was based on traversing a linked list comparing the
> file names.
>
> As the list was replaced by a hash table, the lookup
> time dropped significantly when used on systems with
> a large number of open tables.
>
> The performance on smaller installations remains in the same
> ballpark. Instead of growing exponentially, the lookup time
> now grows more or less linearly, at least in the ballpark
> with less than 100 000 open tables.
> @ storage/myisam/myisamchk.c
> Need to initialize MyISAM open table hash
> @ storage/myisam/myisampack.c
> Need to initialize MyISAM open table hash
>
> modified:
> storage/myisam/ha_myisam.cc
> storage/myisam/mi_close.c
> storage/myisam/mi_dbug.c
> storage/myisam/mi_keycache.c
> storage/myisam/mi_open.c
> storage/myisam/mi_panic.c
> storage/myisam/mi_static.c
> storage/myisam/myisamchk.c
> storage/myisam/myisamdef.h
> storage/myisam/myisampack.c
> === modified file 'storage/myisam/ha_myisam.cc'
> --- a/storage/myisam/ha_myisam.cc 2010-07-13 17:29:44 +0000
> +++ b/storage/myisam/ha_myisam.cc 2010-10-21 10:51:58 +0000
> @@ -2099,6 +2099,10 @@ static int myisam_init(void *p)
> myisam_hton->create= myisam_create_handler;
> myisam_hton->panic= myisam_panic;
> myisam_hton->flags= HTON_CAN_RECREATE | HTON_SUPPORT_LOG_TABLES;
> +
> + if (mi_init_open_table_hash(table_cache_size))
> + return 1;
> +
> return 0;
> }
>
>
> === modified file 'storage/myisam/mi_close.c'
> --- a/storage/myisam/mi_close.c 2010-07-08 21:20:08 +0000
> +++ b/storage/myisam/mi_close.c 2010-10-21 10:51:58 +0000
> @@ -54,7 +54,9 @@ int mi_close(register MI_INFO *info)
> info->opt_flag&= ~(READ_CACHE_USED | WRITE_CACHE_USED);
> }
> flag= !--share->reopen;
> - myisam_open_list=list_delete(myisam_open_list,&info->open_list);
> +
> + my_hash_delete(&myisam_open_table_hash, (uchar *) info);
> +
> mysql_mutex_unlock(&share->intern_lock);
>
> my_free(mi_get_rec_buff_ptr(info, info->rec_buff));
>
> === modified file 'storage/myisam/mi_dbug.c'
> --- a/storage/myisam/mi_dbug.c 2010-06-07 12:05:34 +0000
> +++ b/storage/myisam/mi_dbug.c 2010-10-21 10:51:58 +0000
> @@ -181,14 +181,14 @@ void _mi_print_key(FILE *stream, registe
> my_bool check_table_is_closed(const char *name, const char *where)
> {
> char filename[FN_REFLEN];
> - LIST *pos;
> + uint idx;
> DBUG_ENTER("check_table_is_closed");
>
> (void) fn_format(filename,name,"",MI_NAME_IEXT,4+16+32);
> mysql_mutex_lock(&THR_LOCK_myisam);
> - for (pos=myisam_open_list ; pos ; pos=pos->next)
> + for (idx= 0; idx < myisam_open_table_hash.records; ++idx)
> {
> - MI_INFO *info=(MI_INFO*) pos->data;
> + MI_INFO *info=(MI_INFO*) my_hash_element(&myisam_open_table_hash, idx);
> MYISAM_SHARE *share=info->s;
> if (!strcmp(share->unique_file_name,filename))
> {
>
> === modified file 'storage/myisam/mi_keycache.c'
> --- a/storage/myisam/mi_keycache.c 2009-12-05 01:26:15 +0000
> +++ b/storage/myisam/mi_keycache.c 2010-10-21 10:51:58 +0000
> @@ -137,16 +137,16 @@ int mi_assign_to_key_cache(MI_INFO *info
> void mi_change_key_cache(KEY_CACHE *old_key_cache,
> KEY_CACHE *new_key_cache)
> {
> - LIST *pos;
> + uint idx;
> DBUG_ENTER("mi_change_key_cache");
>
> /*
> Lock list to ensure that no one can close the table while we manipulate it
> */
> mysql_mutex_lock(&THR_LOCK_myisam);
> - for (pos=myisam_open_list ; pos ; pos=pos->next)
> + for (idx= 0; idx < myisam_open_table_hash.records; ++idx)
> {
> - MI_INFO *info= (MI_INFO*) pos->data;
> + MI_INFO *info=(MI_INFO*) my_hash_element(&myisam_open_table_hash, idx);
> MYISAM_SHARE *share= info->s;
> if (share->key_cache == old_key_cache)
> mi_assign_to_key_cache(info, (ulonglong) ~0, new_key_cache);
>
> === modified file 'storage/myisam/mi_open.c'
> --- a/storage/myisam/mi_open.c 2010-07-23 20:59:42 +0000
> +++ b/storage/myisam/mi_open.c 2010-10-21 10:51:58 +0000
> @@ -36,24 +36,76 @@ if (pos > end_pos) \
> goto err; \
> }
>
> +/*
> + Get the value used as hash key (helper function for the
> + open table hash). Function is used as a callback
> + from the hash table
> +*/
> +static uchar *mi_open_table_hash_key(const uchar *record, size_t *length,
> + my_bool not_used __attribute__((unused)))
> +{
> + MI_INFO *info= (MI_INFO *) record;
> + *length= info->s->unique_name_length;
> + return (uchar*) info->s->unique_file_name;
> +}
>
> -/******************************************************************************
> -** Return the shared struct if the table is already open.
> -** In MySQL the server will handle version issues.
> -******************************************************************************/
> +/**
> + Initialize open table hash
> +
> + Function is normally called from myisam_init
> + with the system variable table_cache_size used
> + as hash_size.
> +
> + @param[in] hash_size Initial has size (elements)
> + @return inidicates success or failure of initialization
> + @retval 0 success
> + @retval 1 failure
>
> +*/
> +int mi_init_open_table_hash(ulong hash_size)
> +{
> + if (hash_size == 0)
> + hash_size= 32; /* default hash size */
> +
> + if (my_hash_init(&myisam_open_table_hash, &my_charset_filename,
> + hash_size, 0, 0, mi_open_table_hash_key, 0, 0))
> + return 1; /* error */
> +
> + return 0;
> +}
> +
> +
> +/**
> + Retrieve the shared struct if the table is already
> + open (i.e in the open table hash)
> +
> + @param[in] filename table file name
> + @return shared struct, 0 if not in the cache
> +*/
> MI_INFO *test_if_reopen(char *filename)
> {
> - LIST *pos;
> + HASH_SEARCH_STATE current_record;
> + int len= strlen(filename);
>
> - for (pos=myisam_open_list ; pos ; pos=pos->next)
> + MI_INFO *info= (MI_INFO*) my_hash_first(&myisam_open_table_hash,
> + (uchar *) filename,
> + len, ¤t_record);
> + /*
> + There might be more than one instance of a table share for
> + a given table in the hash table. We're interested in the one with
> + last_version set, so we iterate until we find it
> + */
> + while (info)
> {
> - MI_INFO *info=(MI_INFO*) pos->data;
> MYISAM_SHARE *share=info->s;
> - if (!strcmp(share->unique_file_name,filename) &&
> share->last_version)
> - return info;
> + if (share->last_version)
> + break;
> +
> + info= (MI_INFO*) my_hash_next(&myisam_open_table_hash,
> + (uchar *) filename,
> + len, ¤t_record);
> }
> - return 0;
> + return info;
> }
>
>
> @@ -628,8 +680,9 @@ MI_INFO *mi_open(const char *name, int m
> #ifdef THREAD
> thr_lock_data_init(&share->lock,&m_info->lock,(void*) m_info);
> #endif
> - m_info->open_list.data=(void*) m_info;
> - myisam_open_list=list_add(myisam_open_list,&m_info->open_list);
> +
> + if (my_hash_insert(&myisam_open_table_hash, (uchar *) m_info))
> + goto err;
>
> mysql_mutex_unlock(&THR_LOCK_myisam);
>
>
> === 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-10-21 10:51:58 +0000
> @@ -26,90 +26,102 @@
> 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 (my_hash_inited(&myisam_open_table_hash))
> {
> - 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 */
> -#ifdef CANT_OPEN_FILES_TWICE
> - if (info->s->options & HA_OPTION_READ_ONLY_DATA)
> - break;
> -#endif
> - if (flush_key_blocks(info->s->key_cache, info->s->kfile,
> FLUSH_RELEASE))
> - error=my_errno;
> - if (info->opt_flag & WRITE_CACHE_USED)
> - if (flush_io_cache(&info->rec_cache))
> - error=my_errno;
> - if (info->opt_flag & READ_CACHE_USED)
> + if (flag == HA_PANIC_CLOSE)
> + {
> + while (myisam_open_table_hash.records)
> {
> - if (flush_io_cache(&info->rec_cache))
> - error=my_errno;
> - reinit_io_cache(&info->rec_cache,READ_CACHE,0,
> - (pbool) (info->lock_type != F_UNLCK),1);
> + /*
> + As long as there are records in the hash, fetch the
> + first, and close it.
> + */
> + info=(MI_INFO*) my_hash_element(&myisam_open_table_hash, 0);
> + mysql_mutex_unlock(&THR_LOCK_myisam); /* Not exactly right... */
> + if (mi_close(info))
> + error= my_errno;
> + mysql_mutex_lock(&THR_LOCK_myisam);
> }
> - if (info->lock_type != F_UNLCK && ! info->was_locked)
> + (void) mi_log(0); /* Close log if neaded */
> + ft_free_stopwords();
> + }
> + else
> + {
> + for (idx= 0; idx < myisam_open_table_hash.records; ++idx)
> {
> - info->was_locked=info->lock_type;
> - if (mi_lock_database(info,F_UNLCK))
> - error=my_errno;
> - }
> + info=(MI_INFO*) my_hash_element(&myisam_open_table_hash, idx);
> + switch (flag) {
> + case HA_PANIC_CLOSE: break; /* To eliminate warning. Handled above */
> + case HA_PANIC_WRITE: /* Do this to free databases */
> #ifdef CANT_OPEN_FILES_TWICE
> - if (info->s->kfile >= 0 &&
> mysql_file_close(info->s->kfile, MYF(0)))
> - error = my_errno;
> - if (info->dfile >= 0 && mysql_file_close(info->dfile,
> MYF(0)))
> - error = my_errno;
> - info->s->kfile=info->dfile= -1; /* Files aren't open anymore */
> - break;
> + if (info->s->options & HA_OPTION_READ_ONLY_DATA)
> + break;
> #endif
> - case HA_PANIC_READ: /* Restore to before WRITE */
> + if (flush_key_blocks(info->s->key_cache, info->s->kfile,
> FLUSH_RELEASE))
> + error=my_errno;
> + if (info->opt_flag & WRITE_CACHE_USED)
> + if (flush_io_cache(&info->rec_cache))
> + error=my_errno;
> + if (info->opt_flag & READ_CACHE_USED)
> + {
> + if (flush_io_cache(&info->rec_cache))
> + error=my_errno;
> + reinit_io_cache(&info->rec_cache,READ_CACHE,0,
> + (pbool) (info->lock_type != F_UNLCK),1);
> + }
> + if (info->lock_type != F_UNLCK && ! info->was_locked)
> + {
> + info->was_locked=info->lock_type;
> + if (mi_lock_database(info,F_UNLCK))
> + error=my_errno;
> + }
> #ifdef CANT_OPEN_FILES_TWICE
> - { /* Open closed files */
> - char name_buff[FN_REFLEN];
> - if (info->s->kfile < 0)
> - if ((info->s->kfile= mysql_file_open(mi_key_file_kfile,
> - fn_format(name_buff,
> - info->filename, "",
> - N_NAME_IEXT, 4),
> - info->mode, MYF(MY_WME))) < 0)
> - error = my_errno;
> - if (info->dfile < 0)
> - {
> - if ((info->dfile= mysql_file_open(mi_key_file_dfile,
> - fn_format(name_buff,
> - info->filename, "",
> - N_NAME_DEXT, 4),
> - info->mode, MYF(MY_WME))) < 0)
> - error = my_errno;
> - info->rec_cache.file=info->dfile;
> - }
> - }
> + if (info->s->kfile >= 0 &&
> mysql_file_close(info->s->kfile, MYF(0)))
> + error = my_errno;
> + if (info->dfile >= 0 && mysql_file_close(info->dfile,
> MYF(0)))
> + error = my_errno;
> + info->s->kfile=info->dfile= -1; /* Files aren't open anymore */
> + break;
> #endif
> - if (info->was_locked)
> - {
> - if (mi_lock_database(info, info->was_locked))
> - error=my_errno;
> - info->was_locked=0;
> + case HA_PANIC_READ: /* Restore to before WRITE */
> +#ifdef CANT_OPEN_FILES_TWICE
> + { /* Open closed files */
> + char name_buff[FN_REFLEN];
> + if (info->s->kfile < 0)
> + if ((info->s->kfile= mysql_file_open(mi_key_file_kfile,
> + fn_format(name_buff,
> + info->filename, "",
> + N_NAME_IEXT, 4),
> + info->mode, MYF(MY_WME))) <
> 0)
> + error = my_errno;
> + if (info->dfile < 0)
> + {
> + if ((info->dfile= mysql_file_open(mi_key_file_dfile,
> + fn_format(name_buff,
> + info->filename, "",
> + N_NAME_DEXT, 4),
> + info->mode, MYF(MY_WME))) <
> 0)
> + error = my_errno;
> + info->rec_cache.file=info->dfile;
> + }
> + }
> +#endif
> + if (info->was_locked)
> + {
> + if (mi_lock_database(info, info->was_locked))
> + error=my_errno;
> + info->was_locked=0;
> + }
> + break;
> + }
> }
> - break;
> }
> }
> - if (flag == HA_PANIC_CLOSE)
> - {
> - (void) mi_log(0); /* Close log if neaded */
> - ft_free_stopwords();
> - }
> mysql_mutex_unlock(&THR_LOCK_myisam);
> if (!error)
> DBUG_RETURN(0);
>
> === modified file 'storage/myisam/mi_static.c'
> --- a/storage/myisam/mi_static.c 2010-08-05 12:34:19 +0000
> +++ b/storage/myisam/mi_static.c 2010-10-21 10:51:58 +0000
> @@ -22,7 +22,7 @@
> #include "myisamdef.h"
> #endif
>
> -LIST *myisam_open_list=0;
> +
> uchar myisam_file_magic[]=
> { (uchar) 254, (uchar) 254,'\007', '\001', };
> uchar myisam_pack_file_magic[]=
> @@ -40,6 +40,9 @@ ulong myisam_concurrent_insert= 0;
> ulonglong myisam_max_temp_length= MAX_FILE_SIZE;
> ulong myisam_data_pointer_size=4;
> ulonglong myisam_mmap_size= SIZE_T_MAX, myisam_mmap_used= 0;
> +HASH myisam_open_table_hash;
> +
> +
>
> static int always_valid(const char *filename __attribute__((unused)))
> {
>
> === modified file 'storage/myisam/myisamchk.c'
> --- a/storage/myisam/myisamchk.c 2010-07-23 20:16:29 +0000
> +++ b/storage/myisam/myisamchk.c 2010-10-21 10:51:58 +0000
> @@ -88,6 +88,13 @@ int main(int argc, char **argv)
> get_options(&argc,(char***) &argv);
> myisam_quick_table_bits=decode_bits;
> error=0;
> +
> + if (mi_init_open_table_hash(0))
> + {
> + fprintf(stderr, "Can't initialize MyISAM storage engine\n");
> + exit(-1);
> + }
> +
> while (--argc >= 0)
> {
> int new_error=myisamchk(&check_param, *(argv++));
>
> === modified file 'storage/myisam/myisamdef.h'
> --- a/storage/myisam/myisamdef.h 2010-07-26 11:34:07 +0000
> +++ b/storage/myisam/myisamdef.h 2010-10-21 10:51:58 +0000
> @@ -25,6 +25,7 @@
> #include <my_no_pthread.h>
> #endif
> #include <mysql/psi/mysql_file.h>
> +#include "hash.h"
>
> /* undef map from my_nosys; We need test-if-disk full */
> #if defined(my_write)
> @@ -287,7 +288,6 @@ struct st_myisam_info {
> uint data_changed; /* Somebody has changed data */
> uint save_update; /* When using KEY_READ */
> int save_lastinx;
> - LIST open_list;
> IO_CACHE rec_cache; /* When cacheing records */
> uint preload_buff_size; /* When preloading indexes */
> myf lock_wait; /* is 0 or MY_DONT_WAIT */
> @@ -477,7 +477,7 @@ extern mysql_mutex_t THR_LOCK_myisam;
>
> /* Some extern variables */
>
> -extern LIST *myisam_open_list;
> +extern HASH myisam_open_table_hash;
> extern uchar myisam_file_magic[], myisam_pack_file_magic[];
> extern uint myisam_read_vec[], myisam_readnext_vec[];
> extern uint myisam_quick_table_bits;
> @@ -759,6 +759,7 @@ my_bool mi_check_status(void* param);
> void mi_disable_non_unique_index(MI_INFO *info, ha_rows rows);
>
> extern MI_INFO *test_if_reopen(char *filename);
> +extern int mi_init_open_table_hash(ulong size);
> my_bool check_table_is_closed(const char *name, const char *where);
> int mi_open_datafile(MI_INFO *info, MYISAM_SHARE *share, const char *orn_name,
> File file_to_dup);
>
> === modified file 'storage/myisam/myisampack.c'
> --- a/storage/myisam/myisampack.c 2010-07-23 20:17:55 +0000
> +++ b/storage/myisam/myisampack.c 2010-10-21 10:51:58 +0000
> @@ -210,6 +210,13 @@ int main(int argc, char **argv)
> if (load_defaults("my",load_default_groups,&argc,&argv))
> exit(1);
>
> + if (mi_init_open_table_hash(0))
> + {
> + fputs("Can't initialize MyISAM storage engine", stderr);
> + exit(1);
> + }
> +
> +
> default_argv= argv;
> get_options(&argc,&argv);
>
>
>
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe: http://lists.mysql.com/commits?unsub=1
--
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
