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
>
