List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:May 14 2011 8:27am
Subject:Re: bzr commit into mysql-5.5 branch (jon.hauglid:3381)
Bug#11853126
View as plain text  
Hello Jon Olav!

Thank you for working on this!

Here are my comments about your patch:

* Jon Olav Hauglid <jon.hauglid@stripped> [11/05/13 13:20]:
> #At file:///export/home/x/mysql-5.5-bug11853126/ based on
> revid:build@stripped
> 
>  3381 Jon Olav Hauglid	2011-05-13
>       Bug#11853126 RE-ENABLE CONCURRENT READS WHILE CREATING
>                    SECONDARY INDEX IN INNODB
>       
>       The patches for Bug#11751388 and Bug#11784056 enabled concurrent
>       reads while creating secondary indexes in InnoDB. However, they
>       introduced a regression. This regression occured if ALTER TABLE
>       failed after the index had been added, for example during the
>       lock upgrade needed to update .FRM. If this happened, InnoDB
>       and the server got out of sync with regards to which indexes
>       actually existed. Therefore the patch for Bug#11815600 again
>       disabled concurrent reads.

...

>       In order to implement this change, the patch changes the storage
>       API for in-place index creation. handler::add_index() is split
>       into two functions, handler_add_index() and
>       handler::final_add_index(). The fromer for creating indexes without
                                        ^^^^^^
Typo:                                   former

>       making the visible and the latter for commiting (i.e. making
>       visible) new indexes or reverting the changes.
>       
>       Large parts of this patch was written by Marko Mäkelä.
                                  ^^^
Typo:                             were ?
     
>       Test case added to innodb_mysql_lock.test.
> 

...

> === modified file 'sql/ha_partition.cc'
> --- a/sql/ha_partition.cc	2011-05-10 13:24:34 +0000
> +++ b/sql/ha_partition.cc	2011-05-13 09:16:26 +0000
> @@ -6654,20 +6654,26 @@ bool ha_partition::check_if_incompatible
>  /**
>    Support of fast or online add/drop index
>  */
> -int ha_partition::add_index(TABLE *table_arg, KEY *key_info, uint num_of_keys)
> +int ha_partition::add_index(TABLE *table_arg, KEY *key_info, uint num_of_keys,
> +                            handler_add_index **add)
>  {
>    handler **file;
>    int ret= 0;
>  
>    DBUG_ENTER("ha_partition::add_index");
> +  *add= new handler_add_index(table, key_info, num_of_keys);
>    /*
>      There has already been a check in fix_partition_func in mysql_alter_table
>      before this call, which checks for unique/primary key violations of the
>      partitioning function. So no need for extra check here.
>    */
>    for (file= m_file; *file; file++)
> -    if ((ret=  (*file)->add_index(table_arg, key_info, num_of_keys)))
> +  {
> +    handler_add_index *add_index;
> +    if ((ret=  (*file)->add_index(table_arg, key_info, num_of_keys,
> &add_index)))
>        goto err;
> +    (*file)->final_add_index(add_index, true);
> +  }
>    DBUG_RETURN(ret);

Above and in other places (i.e. sql_table.cc) you assume that
final_add_index() can't fail... But the fact that this method
has a return value and its implementation in InnoDB suggests
otherwise. Maybe we should try to handle case when this method
returns an error somehow? E.g. in this case by returning an error
from ha_partition::add_index() and going to "err" label, to
drop indexes for those partitions for which they have been created
successfully.

...

> === modified file 'sql/sql_table.cc'
> --- a/sql/sql_table.cc	2011-05-06 08:27:04 +0000
> +++ b/sql/sql_table.cc	2011-05-13 09:16:26 +0000

...

> @@ -6475,11 +6481,23 @@ bool mysql_alter_table(THD *thd,char *ne
>          table->key_info= save_key_info;
>          goto err_new_table_cleanup;
>        }
> +      pending_inplace_add_index= true;
>      }
>      /*end of if (index_add_count)*/
>  
>      if (index_drop_count)
>      {
> +      /* Currently we must finalize add index if we also drop indexes */
> +      if (pending_inplace_add_index)
> +      {
> +        /* Committing index changes needs exclusive metadata lock. */
> +        DBUG_ASSERT(thd->mdl_context.is_lock_owner(MDL_key::TABLE,
> +                                                   table_list->db,
> +                                                   table_list->table_name,
> +                                                   MDL_EXCLUSIVE));
> +        table->file->final_add_index(add, true);

See my comment about handling error from this method above.

> +        pending_inplace_add_index= false;
> +      }
>        /* The prepare_drop_index() method takes an array of key numbers. */
>        key_numbers= (uint*) thd->alloc(sizeof(uint) * index_drop_count);
>        keyno_p= key_numbers;
> @@ -6591,8 +6612,21 @@ bool mysql_alter_table(THD *thd,char *ne
>      my_casedn_str(files_charset_info, old_name);
>  
>    if (wait_while_table_is_used(thd, table, HA_EXTRA_PREPARE_FOR_RENAME))
> +  {
> +    if (pending_inplace_add_index)
> +    {
> +      table->file->final_add_index(add, false);
> +      pending_inplace_add_index= false;
> +    }
> +    // Mark this TABLE instance as stale to avoid out-of-sync index information.
> +    table->m_needs_reopen= true;
>      goto err_new_table_cleanup;
> -
> +  }
> +  if (pending_inplace_add_index)
> +  {
> +    table->file->final_add_index(add, true);
> +    pending_inplace_add_index= false;
> +  }

Same comment here.

...

Otherwise, I am OK with your patch and think that it can be pushed
after addressing/discussing the above issues and getting changes to
InnoDB code reviewed by an expert from InnoDB team.

-- 
Dmitry Lenev, Software Developer
Oracle Development SPB/MySQL, www.mysql.com

Are you MySQL certified?  http://www.mysql.com/certification
Thread
bzr commit into mysql-5.5 branch (jon.hauglid:3381) Bug#11853126Jon Olav Hauglid13 May
  • Re: bzr commit into mysql-5.5 branch (jon.hauglid:3381)Bug#11853126Dmitry Lenev14 May