From: Libing Song
Date: September 20 2010 7:56am
Subject: Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3489)
 Bug#56226
List-Archive: http://lists.mysql.com/commits/118560
Message-Id: <1284969409.1687.14.camel@anders-laptop>
MIME-Version: 1.0
Content-Type: text/plain; CHARSET=US-ASCII
Content-Transfer-Encoding: 7BIT

Hi Dmitry,
Thanks for your review.
I agree all your comments.
The new patch was committed, please review it.
http://lists.mysql.com/commits/118559

On Mon, 2010-09-20 at 11:40 +0400, Dmitry Lenev wrote:
> Hello Li-Bing!
> 
> * Libing Song <Li-Bing.Song@stripped> [10/09/20 11:25]:
> > On Mon, 2010-09-20 at 10:53 +0400, Dmitry Lenev wrote:
> > > Hello Li-Bing!
> > > 
> > > Here are my comment about your patch:
> > > 
> > > * Li-Bing.Song@stripped <Li-Bing.Song@stripped> [10/09/16 13:40]:
> > > > #At file:///home/anders/work/bzrwork1/wt2/mysql-5.1-bugteam/ based on revid:mattias.jonsson@stripped
> > > > 
> > > >  3489 Li-Bing.Song@stripped	2010-09-16
> > > >       Bug#56226  Table map set to 0 after altering MyISAM table
> > > >       
> > > >       A MyISAM table's table_map_id will always be reset to 0 after altered
> > > >       only its metadata. 0 is a valid value of table_map_id. But the problem will
> > > >       cause that more than one tables have the same table_map_id 0. If more than
> > > >       one tables which's table_map_id are 0 are updated in one statement, the update
> > > >       of these different tables will be write into only one rows event. It will
> > > >       cause slave server to crash. 
> > > >       
> > > >       After this patch, table_map_id always be refreshed as a correct value after
> > > >       altered only its metadata.
> > > 
> > > ...
> > > 
> > > > === modified file 'sql/sql_base.cc'
> > > > --- a/sql/sql_base.cc	2010-07-20 18:07:36 +0000
> > > > +++ b/sql/sql_base.cc	2010-09-16 09:35:41 +0000
> > > > @@ -662,6 +662,7 @@ void close_handle_and_leave_table_as_loc
> > > >      share->set_table_cache_key(key_buff, old_share->table_cache_key.str,
> > > >                                 old_share->table_cache_key.length);
> > > >      share->tmp_table= INTERNAL_TMP_TABLE;       // for intern_close_table()
> > > > +    share->table_map_id= ~0UL;
> > > >    }
> > > >  
> > > >    /*
> > > > @@ -3082,7 +3083,8 @@ bool reopen_table(TABLE *table)
> > > >    tmp.maybe_null=	table->maybe_null;
> > > >    tmp.status=		table->status;
> > > >  
> > > > -  tmp.s->table_map_id=  table->s->table_map_id;
> > > > +  if (table->s->table_map_id != ~0UL)
> > > > +    tmp.s->table_map_id=  table->s->table_map_id;
> > > >  
> > > 
> > > I don't think that there is a situation when it makes sense to
> > > inherit table_map_id from old version of share here.
> > > Especially since, AFAIU, shares for TABLE objects for which 
> > > reopen_table() is called are always dummy and not a real shares.
> > I agree with you. I just have a little bit worry that if it will effect
> > other code. As reopen_table is also called in other place.
> 
> Yes reopen_table() is also called in other places. But what I have
> said above is true for them as well.
OK.
> 
> > > 
> > > So I think it makes sense simply to remove this line and keep 
> > > close_handle_and_leave_table_as_lock() as is.
> > It doesn't make sense to initialize table_map_id as 0. It is not
> > reasonable. If table_map_id has been initialized as ~0UL, we would have
> > found this bug no soon after the code was committed.
> 
> Well. Maybe.
> 
> Dummy share created in close_handle_and_leave_table_as_lock() is
> not intended to be used for anything except being placeholder till
> reopen_table() happens or statement ends. This is emphasized by
> "share->tmp_table= INTERNAL_TMP_TABLE" assignment. I.e. it is not
> a real share and most of its fields do not make any sense.
> 
> It is an omission that we didn't take into account the fact that
> reopen_table() copies table_map_id from old share when we have
> introduced close_handle_and_leave_table_as_lock().
Got it. It's a temporary share.
> 

-- 
Your Sincerely,
Libing Song
==================================
MySQL Replication Team
Software Engineer


Email : Li-Bing.Song@stripped
Skype : libing.song
MSN   : slb_database@stripped
Phone : +86 010-6505-4020 ext. 319
Mobile: +86 138-1144-2038
==================================