List:Commits« Previous MessageNext Message »
From:Magne Mæhre Date:October 21 2010 10:42am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3309)
Bug#49177
View as plain text  
On 08/10/10 17:25, Sergey Vojtovich wrote:
> Hi Magne,
> 
> thanks for taking care of this bug. I think it is really important
> to have this problem fixed, but please expect multiple commit/review
> cycles.
> 
> This e-mail is just a quick-look at this patch, identifying obvious
> problems.

Hi Svoj & Kostja.
Thanks for the good review comments.    Here are my comments..


> Btw, I'm just curious if you did try different approaches (e.g. other
> than hash) to fix this problem?

Not really.  Since, in most cases, the path part of the name is
identical (and might be long), we could envision using the
reversed string name in a search tree for lookup...   There is
probably also a size where it is cheaper to linearly scan the
hash rather than use key lookup..   I have not spent time on testing
these options.

>> === 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-09-30 09:29:51 +0000
>> @@ -642,7 +642,9 @@ ha_myisam::ha_myisam(handlerton *hton, T
>>                    HA_CAN_INSERT_DELAYED | HA_CAN_BIT_FIELD | HA_CAN_RTREEKEYS |
>>                    HA_HAS_RECORDS | HA_STATS_RECORDS_IS_EXACT),
>>     can_enable_indexes(1)
>> -{}
>> +{
>> +  mi_init_table_cache(table_cache_size);
>> +}
> Why? I think it is enough to initialize it in myisam_init(). Again, imho
> it is not cache. May be myisam_open_table_hash?

Agreed,  I'm changing the name

>> === 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-09-30 09:29:51 +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_cache->records; ++idx)
>>    {
>> -    MI_INFO *info=(MI_INFO*) pos->data;
>> +    MI_INFO *info=(MI_INFO*) my_hash_element(myisam_open_cache, idx);
>>      MYISAM_SHARE *share=info->s;
>>      if (!strcmp(share->unique_file_name,filename))
>>      {
>>
> No action needed yet. We could probably optimize this loop the same way
> as we optimized test_if_reopent().


Agreed.  This code is inside "#ifdef EXTRA_DEBUG", so I don't see much
point in changing it for performance reasons :)


>> +/**
>> + * Get the value used as hash key (helper function for the 
>> + * table cache hash).  Function is used as a callback 
>> + * from the hash table
>> + *
>> + */
> Coding style:
> When writing multi-line comments please put the '/*' and '*/' on their own
> lines, put the '*/' below the '/*', put a line break and a two-space indent
> after the '/*', do not use additional asterisks on the left of the comment.

Agreed.. fixing..


>> +static uchar *mi_table_cache_key(const uchar *record, size_t *length,
>> +                                 my_bool not_used __attribute__((unused)))
>> +{
>> +  MI_INFO *info= (MI_INFO *) record;
>> +  *length= strlen(info->s->unique_file_name);
>> +  return (uchar*) info->s->unique_file_name;
>> +}
> I'd avoid strlen() here. If not switching to LEX_STRING, can we at least
> have MI_INFO::unique_file_name_length?

Agreed.   Replacing with  info->s->unique_name_length


>> +/**
>> + * Retrieve the shared struct if the table is already
>> + * open (i.e  in the cache)
>> + *
>> + * @param 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)
>> +  /* Initialize table cache on first call */
>> +  if (!myisam_open_cache)
>> +    mi_init_table_cache(0);
> Ehm. A problem here: if we fail to initialize hash, myisam cannot operate.

You're right, of course..  I'm also moving the hash init out of
test_if_reopen.

> ...skip...
>> @@ -629,7 +683,8 @@ MI_INFO *mi_open(const char *name, int m
>>    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);
>> +
>> +  my_hash_insert(myisam_open_cache, (uchar *) m_info);
> If we fail to insert new hash entry, we must fail.

Yup... Adding test


>> === 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-09-30 09:29:51 +0000
>> @@ -26,54 +26,55 @@
..
> Concerns regarding mi_panic() are in e-mail from Kostja.

I'll address them in a reply to that email.


>> === 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-09-30 09:29:51 +0000
>> @@ -22,7 +22,7 @@
>>  #include "myisamdef.h"
>>  #endif
>>  
>> -LIST	*myisam_open_list=0;
>> +
> You remove myisam_open_list, but keep MI_INFO::open_list. Why?

Good catch..  Cleaned up (removed)


>>  uchar	myisam_file_magic[ SIZE_T_MAX, myisam_mmap_used= 0;
>>  
>> +/* 
>> +   MyISAM table cache.  After initialization,
>> +   myisam_open_cache will point to myisam_open_hash
>> +*/
>> +HASH    *myisam_open_cache=0; 
>> +HASH    myisam_open_hash;
> Ehm. I understand why they're two, but I'd prefer one variable here.

Yup...  removed one, and renamed the other.

New commit is coming up real-soon-now...


Thanks for a very constructive review!

--Magne
Thread
bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3309) Bug#49177Magne Mahre30 Sep
  • Re: bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3309)Bug#49177Konstantin Osipov7 Oct
    • Re: bzr commit into mysql-next-mr-bugfixing branch(magne.mahre:3309) Bug#49177Sergey Vojtovich7 Oct
    • Re: bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3309)Bug#49177Magne Mæhre21 Oct
  • Re: bzr commit into mysql-next-mr-bugfixing branch(magne.mahre:3309) Bug#49177Sergey Vojtovich8 Oct
    • Re: bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3309)Bug#49177Magne Mæhre21 Oct