From: Sergey Vojtovich Date: November 19 2010 12:45pm Subject: Re: bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3328) Bug#49177 List-Archive: http://lists.mysql.com/commits/124437 Message-Id: <20101119124522.GA10450@june> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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 > #endif > #include > +#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=svoj@stripped -- Sergey Vojtovich MySQL AB, Software Engineer Izhevsk, Russia, www.mysql.com