List:Commits« Previous MessageNext Message »
From:Ingo Strüwing Date:February 10 2009 9:12am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (v.narayanan:2750)
Bug#40675
View as plain text  
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
-- 
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Dr. Roland Bömer
Vorsitzender des Aufsichtsrates: Martin Häring   HRB München 161028
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