Re: CREATE INDEX is not committed?

[Guilhem Bichot]

Hello Marko, Ramil,

On Mon, Jun 18, 2007 at 03:38:05PM +0300, Marko Mäkelä wrote:

> Sorry, I was looking at an older MySQL tree that created the primary key in
> the non-smart way.  Also the newer tree reaches the lines you are asking for,
> but ha_commit_trans() does not invoke innobase_commit, probably because
> thd->transaction is empty:
> 
> $5 = {savepoints = 0x0, all = {nht = 0, no_2pc = false, ht = {
>       0x0 <repeats 15 times>}}, stmt = {nht = 0, no_2pc = false, ht = {
>       0x0 <repeats 15 times>}}, on = true, xid = {formatID = 0, 
>     gtrid_length = 0, bqual_length = 0, data = '\0' <repeats 127 times>}, 
>   xa_state = XA_NOTR, xid_state = {xid = {formatID = -1, gtrid_length = 20, 
>       bqual_length = 0, 
>       data = "MySQLXid\001\000\000\000\003", '\0' <repeats 114 times>}, 
>     xa_state = XA_NOTR, in_thd = true}, m_pending_rows_event = 0x0, 
>   changed_tables = 0x0, mem_root = {free = 0x88b74a8, used = 0x0, 
>     pre_alloc = 0x88b74a8, min_malloc = 32, block_size = 8136, block_num = 4, 
>     first_block_usage = 0, 
>     error_handler = 0x81b4034 <sql_alloc_error_handler>}}
> 
> Note that thd->transaction.all.nht == 0 and thd->transaction.stmt.nht == 0.
> 
> I suppose that thd->transaction is cleared earlier in
> mysql_alter_table(), when the transaction is committed before invoking
> handler::add_index():
> 
>   else
>   {
>     VOID(pthread_mutex_lock(&LOCK_open));
>     wait_while_table_is_used(thd, table, HA_EXTRA_FORCE_REOPEN);
>     table->file->ha_external_lock(thd, F_WRLCK);
>     alter_table_manage_keys(table, table->file->indexes_are_disabled(),
>                             alter_info->keys_onoff);
>     table->file->ha_external_lock(thd, F_UNLCK);
>     VOID(pthread_mutex_unlock(&LOCK_open));
>     error= ha_commit_stmt(thd);
>     if (ha_commit(thd))
>       error= 1;
>   }
> 
> Before the call to ha_commit_stmt(), we have thd->transaction.stmt like
> this:
> 
> {nht = 1, no_2pc = false, ht = { 0x87953a8, 0x0 <repeats 14 times>}}
> 
> That is the InnoDB handlerton.  After the call, the handlerton array is
> empty.
> 
> This can be fixed by re-registering the handler at
> ha_innobase::add_index(), but I would prefer it if mysql_alter_table()
> called ha_innobase::start_stmt() or delayed the commit until the very
> end.

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?

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /    Mr. Guilhem Bichot <guilhem@stripped>
 / /|_/ / // /\ \/ /_/ / /__   MySQL AB, Lead Software Engineer
/_/  /_/\_, /___/\___\_\___/   Bordeaux, France
       <___/   www.mysql.com