Re: CREATE INDEX is not committed?

[Marko Mäkelä]

Hi again,

On Mon, Jun 18, 2007 at 12:06:33PM +0300, Marko Mäkelä wrote:
> 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

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.

Best regards,

	Marko