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.
> >
> > 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().
--
Dmitry Lenev, Software Developer
Oracle Development SPB/MySQL, www.mysql.com
Are you MySQL certified? http://www.mysql.com/certification
