Re: CREATE INDEX is not committed?

[Marko Mäkelä]

Hi Guilhem, Ramil,

> Thanks for the debugging, now it's clear.
> Indeed, something was changed recently (June 1st), the ha_commit was
> moved up to earlier than before (it was to fix BUG#28652). This was a
> bugfix in 5.0 and when merged into 5.1 it probably made a new bug as
> the commit now happens before add_index(), and thus InnoDB does not
> receive any commit order for its open table on which it has just added
> an index. Can be just the usual automerge problem.
> 
> Ramil, please consider my suggestion: we could move the commit a
> little down, to after add_index() and drop_index(), in other words, to
> just before we unlock tables:
> 
> - move ha_commit stuff here -
>   if (table->s->tmp_table != NO_TMP_TABLE)
>   {
>     /* We changed a temporary table */
>     if (error)
>       goto err1;
> etc
> 
> I would even do the commit independently of if "new_table" is NULL or
> not: if new_table!=NULL, it still makes sense to commit the changes
> made to this temporary table. I think the pre-bug-fix code committed
> in all cases. What do you think?

It should also commit at err1.  If ha_innobase::add_index() reports
a duplicate key, MySQL will not commit the transaction.  Instead,
InnoDB will crash during DROP TABLE, here in type_bit_innodb.test:

create table t1 (a bit) engine=innodb;
insert into t1 values (b'0'), (b'1'), (b'000'), (b'100'), (b'001');
select hex(a) from t1;
--error ER_DUP_ENTRY_WITH_KEY_NAME
alter table t1 add unique (a);
drop table t1;

The crash would occur at drop table, line 45 of type_bit_innodb.test.

I have now put back the call to ha_innobase::add_index(), so that we
won't be bitten by this again.  The plan is to distribute an InnoDB
plugin for MySQL 5.1 that includes features that were too late to be
included in official MySQL/InnoDB 5.1 builds.

Another thing: mysql_alter_table() is trying to drop a temporary table
it did not create:

err1:
  if (new_table)
  {
    /* close_temporary_table() frees the new_table pointer. */
    close_temporary_table(thd, new_table, 1, 1);
  }
  else
    VOID(quick_rm_table(new_db_type, new_db, tmp_name, FN_IS_TMP));

I believe that the else branch should be removed altogether, because
new_table==NULL should mean that handler::add_index() was called and
no temporary table was created.

Best regards,

	Marko