From: Dmitry Lenev
Date: May 14 2011 8:27am
Subject: Re: bzr commit into mysql-5.5 branch (jon.hauglid:3381)
 Bug#11853126
List-Archive: http://lists.mysql.com/commits/137371
Message-Id: <20110514082709.GA1928@bandersnatch>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: quoted-printable

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@m=
ysql.com-20110513050009-s26u7s8ccuua2y0n
>=20
>  3381 Jon Olav Hauglid	2011-05-13
>       Bug#11853126 RE-ENABLE CONCURRENT READS WHILE CREATING
>                    SECONDARY INDEX IN INNODB
>      =20
>       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 witho=
ut
                                        ^^^^^^
Typo:                                   former

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

...

> =3D=3D=3D 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=3D 0;
> =20
>    DBUG_ENTER("ha_partition::add_index");
> +  *add=3D new handler_add_index(table, key_info, num_of_keys);
>    /*
>      There has already been a check in fix_partition_func in mysql_alte=
r_table
>      before this call, which checks for unique/primary key violations o=
f the
>      partitioning function. So no need for extra check here.
>    */
>    for (file=3D m_file; *file; file++)
> -    if ((ret=3D  (*file)->add_index(table_arg, key_info, num_of_keys))=
)
> +  {
> +    handler_add_index *add_index;
> +    if ((ret=3D  (*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.

...

> =3D=3D=3D 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=3D save_key_info;
>          goto err_new_table_cleanup;
>        }
> +      pending_inplace_add_index=3D true;
>      }
>      /*end of if (index_add_count)*/
> =20
>      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_n=
ame,
> +                                                   MDL_EXCLUSIVE));
> +        table->file->final_add_index(add, true);

See my comment about handling error from this method above.

> +        pending_inplace_add_index=3D false;
> +      }
>        /* The prepare_drop_index() method takes an array of key numbers=
. */
>        key_numbers=3D (uint*) thd->alloc(sizeof(uint) * index_drop_coun=
t);
>        keyno_p=3D key_numbers;
> @@ -6591,8 +6612,21 @@ bool mysql_alter_table(THD *thd,char *ne
>      my_casedn_str(files_charset_info, old_name);
> =20
>    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=3D false;
> +    }
> +    // Mark this TABLE instance as stale to avoid out-of-sync index in=
formation.
> +    table->m_needs_reopen=3D true;
>      goto err_new_table_cleanup;
> -
> +  }
> +  if (pending_inplace_add_index)
> +  {
> +    table->file->final_add_index(add, true);
> +    pending_inplace_add_index=3D 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.

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

Are you MySQL certified?  http://www.mysql.com/certification