List:Commits« Previous MessageNext Message »
From:V Narayanan Date:February 10 2009 8:41pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (v.narayanan:2750)
Bug#40675
View as plain text  
Ingo Strüwing wrote:

Thank you for the detailed comments Ingo! I have fixed them all and 
submitted a fresh
patch.

Narayanan
> Hi VN,
>
> great work! Approved. But please extend some comments and remove the
> purecov comments around myrg_open(). There may be a memory leak on
> error. Please see details below.
>
> V Narayanan, 06.02.2009 13:45:
>
>   
>> #At file:///home/narayanan/Work/mysql/W-M/mysql-5.1-bugteam/
>>     
>
>
> Just for your information: seemingly you don't have the current version
> of the mysql plugin. It continues this line like "based on revid:...".
> Not a problem to me, I just wanted to note it.
>
>   
>>  2750 V Narayanan	2009-02-06
>>       Bug #40675  MySQL 5.1 crash with index merge algorithm and Merge tables
>>       
>>       A Query in the MyISAM merge table was crashing if the index merge
> algorithm
>>       was being used
>>       
>>       Index Merge optimization requires the reading of multiple indexes at the
>>       same time. Reading multiple indexes at once with current SE API means
>>       that we need to have handler instance for each to-be-read index. This is
>>       done by creating clones of the handlers instances. The clone internally
>>       does a open of the handler.
>>       
>>       The open for a MERGE engine is handled in the following phases
>>       
>>       1) open parent table
>>       2) generate list of underlying
>>          table
>>       3) attach underlying tables
>>       
>>       But the current implementation does only the first phase (i.e.)
>>       open parent table.
>>       
>>       The current patch fixes this at the MERGE engine level, by handling the
>>       clone operation within the MERGE engine rather than in the storage engine
>>       API.
>>     
>
>
> Please continue the comment like: "It opens and attaches the MyISAM
> tables on the MyISAM storage engine interface directly within the MERGE
> engine. The new MyISAM table instances, as well as the MERGE clone
> itself, are not visible in the table cache. This is not a problem
> because all locking is handled by the original MERGE table from which
> this is cloned of."
>
> ...
>   
>> --- a/storage/myisammrg/ha_myisammrg.cc	2008-04-25 21:45:58 +0000
>> +++ b/storage/myisammrg/ha_myisammrg.cc	2009-02-06 12:45:37 +0000
>>     
> ...
>   
>> @@ -422,6 +429,59 @@ int ha_myisammrg::open(const char *name,
>>     
> ...
>   
>> +handler *ha_myisammrg::clone(MEM_ROOT *mem_root)
>> +{
>> +  MYRG_TABLE    *u_table,*newu_table;
>> +  ha_myisammrg *new_handler= 
>> +    (ha_myisammrg*) get_new_handler(table->s, mem_root,
> table->s->db_type());
>>     
> ...
>   
>> +  if (!(new_handler->ref= (uchar*) alloc_root(mem_root,
> ALIGN_SIZE(ref_length)*2)))
>> +    return NULL;
>>     
>
>
> Don't you need to delete new_handler before returning NULL?
>
>   
>> +
>> +  /*
>> +    Open the MySQL tables that are under the MERGE table
>> +    parent
>>     
>
>
> Please change the comment like: "Open and attaches the MyISAM tables,
> that are under the MERGE table parent, on the MyISAM storage engine
> interface directly within the MERGE engine. The new MyISAM table
> instances, as well as the MERGE clone itself, are not visible in the
> table cache. This is not a problem because all locking is handled by the
> original MERGE table from which this is cloned of."
>
>   
>> +  */
>> +  if (!(new_handler->file= myrg_open(table->s->normalized_path.str,
> table->db_stat, HA_OPEN_IGNORE_IF_LOCKED)))
>>     
>
>
> Please keep source lines <= 80 columns.
>
> Good that I resisted the suggestion, to remove myrg_open() altogether
> after it became unused from the server. :)
>
> Since we do now again use myrg_open() in the server, please remove the
> /* purecov: begin deadcode */ /* not used in MySQL server */ comments
> before and after myrg_open() in myrg_open.c.
>
>   
>> +  {
>> +    return NULL;
>>     
>
>
> Don't you need to delete new_handler before returning NULL?
>
> ...
>   
>> +  if (!new_handler->ha_open(table, table->s->normalized_path.str,
> table->db_stat,
>> +                            HA_OPEN_IGNORE_IF_LOCKED))
>> +    return new_handler;
>> +  return NULL;
>>     
>
>
> Don't you need to close and delete new_handler before returning NULL?
>
> I suggest coverage testing to check the exceptional branches.
>
> ...
>
> Regards
> Ingo
>   

Thread
bzr commit into mysql-5.1-bugteam branch (v.narayanan:2750) Bug#40675V Narayanan6 Feb
  • Re: bzr commit into mysql-5.1-bugteam branch (v.narayanan:2750)Bug#40675Ingo Strüwing10 Feb
    • Re: bzr commit into mysql-5.1-bugteam branch (v.narayanan:2750)Bug#40675V Narayanan10 Feb