Re: CREATE INDEX is not committed?

[Marko Mäkelä]

Hi Guilhem,

On Wed, Jun 13, 2007 at 04:35:35PM +0200, Guilhem Bichot wrote:
> Hello,
> 
> On Wed, Jun 13, 2007 at 03:56:34PM +0300, Marko Mäkelä wrote:
> > On Wed, Jun 13, 2007 at 02:27:56PM +0200, Guilhem Bichot wrote:
> > > On Wed, Jun 13, 2007 at 12:05:32PM +0300, Marko Mäkelä wrote:
> > > > I wonder if the following is correct behavior.  This is from
> > > > innodb.test:
> > > > 
> > > > create table t1 (i int, j int ) ENGINE=innodb;
> > > > insert into t1 values (1,2);
> > > > select * from t1 where i=1 and j=2;
> > > > create index ax1 on t1 (i,j);
> > > > select * from t1 where i=1 and j=2;
> > > > 
> > > > If I set a breakpoint on innobase_commit, it gets invoked for the
> > > > INSERT, but not for the CREATE TABLE or CREATE INDEX statements.
> > > > 
> > > > I have the impression that this behavior has been changed in the past
> > > > few weeks.
> > > > 
> > > > The documentation of CREATE INDEX doesn't mention transactions.  I
> > > > suppose that InnoDB is not the only transactional engine where data
> > > > dictionary operations are not truly transactional.  Is it OK for the
> > > > engine to commit the transaction when it feels like that, or should
> > > > MySQL instead invoke commit?
> > > 
> > > Strange, I see innobase_commit called by this CREATE INDEX both in 5.1
> > > and 5.0. There is a ha_commit_stmt() in copy_data_between_tables().
> > > Which version and which source tree are you on?
> > 
> > You're right, innobase_commit is indeed called for CREATE INDEX in 5.1,
> > more specifically, in this revision:
> > 
> > ChangeSet@stripped, 2007-06-07 16:37:15+02:00, joerg@trift2. +8 -0
> >   Merge trift2.:/MySQL/M50/push-5.0
> >   into  trift2.:/MySQL/M51/push-5.1
> >   MERGE: 1.1810.2945.41
> > 
> > However, I should have said that I have been debugging fast index creation
> > that we are planning to release as a 5.1 plugin.  There, innobase_commit is
> > called *before* ha_innobase::add_index() but not after it.  If I remove
> > the call to innobase_commit_low() at the end of that function, many tests
> > will fail.
> 
> Strange. I'm looking at sql_table.cc in the latest 5.1 bk tree,
> mysql_alter_table(), and a few lines after it calls
> table->file->add_index(table, key_info, index_add_count)) there is:
> 
>     DBUG_PRINT("info", ("Committing before unlocking table"));
>     if (ha_commit_stmt(thd) || ha_commit(thd))
>       goto err1;
>     committed= 1;
> 
> Could you please check that:
> - you indeed come to the line above (ha_commit etc)
> - it then calls innobase_commit() ? (it might be that InnoDB is not
> registered as a participant of the "transaction")

I can confirm this:

#0  innobase_commit (hton=0x87856d0, thd=0x8865f28, all=false)
    at handler/ha_innodb.cc:1838
#1  0x08316ce6 in ha_commit_one_phase (thd=0x8865f28, all=false)
    at handler.cc:779
#2  0x083171a4 in ha_commit_trans (thd=0x8865f28, all=false) at handler.cc:749
#3  0x0832a4e9 in mysql_alter_table (thd=0x8865f28, new_db=0x88edfc0 "test", 
    new_name=0x88eddf0 "t1", lex_create_info=0xb5162464, 
    table_list=0x88ede18, fields=@0xb5162528, keys=@0x8867238, order_num=0, 
    order=0x0, ignore=false, alter_info=0xb51624e4, do_send_ok=true)
    at sql_table.cc:6473

This looks like a bug in our code.  I will have to investigate why the
transaction is not properly committed neither here nor in
ha_innobase::external_lock().

> > I wonder what the correct behavior is.  I would expect MySQL to commit
> > before and after handler::add_index().
> 
> After: I agree. Before: why?

If mysqld is killed during data dictionary operations, I suspect that
the rollback after InnoDB crash recovery will not delete or un-rename
the .frm file.  In this case (fast ALTER TABLE), MySQL could believe
that the table was altered.  Based on the output of SHOW CREATE TABLE,
the user would assume that all preceding transactions were committed.

	Marko