maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Nirbhay,

This solution looks better than MyISAM/Aria based one. Still a few doubts inline.
I still suggest someone should do second review.

On Tue, Jun 21, 2016 at 01:47:15PM -0400, Nirbhay Choubey wrote:
> revision-id: d1de6402c8734e9a1929c2384c0cfc07c2ec48c0 (mariadb-10.2.0-83-gd1de640)
> parent(s): b2ae32aafdd2787ad456f38833f630182ded25e8
> author: Nirbhay Choubey
> committer: Nirbhay Choubey
> timestamp: 2016-06-21 13:47:12 -0400
> message:
> 
> MDEV-10216: Assertion `strcmp(share->unique_file_name,filename) ||
...skip...

> diff --git a/sql/sql_table.cc b/sql/sql_table.cc
> index 0be329e..07d4b42 100644
> --- a/sql/sql_table.cc
> +++ b/sql/sql_table.cc
> @@ -8151,12 +8151,15 @@ static bool fk_prepare_copy_alter_table(THD *thd, TABLE *table,
Looks like git confused function names here and for most of other hunks of this
patch. I vaguely remember this bug was affecting really old git versions like
pre-2.x.

>  
>    if (keys_onoff != Alter_info::LEAVE_AS_IS)
>    {
> -    if (wait_while_table_is_used(thd, table, extra_func))
> -      DBUG_RETURN(true);
> +    if (!table->s->tmp_table)
> +    {
> +      if (wait_while_table_is_used(thd, table, extra_func))
> +        DBUG_RETURN(true);
>  
> -    // It's now safe to take the table level lock.
> -    if (lock_tables(thd, table_list, alter_ctx->tables_opened, 0))
> -      DBUG_RETURN(true);
> +      // It's now safe to take the table level lock.
> +      if (lock_tables(thd, table_list, alter_ctx->tables_opened, 0))
> +        DBUG_RETURN(true);
> +    }
>  
>      error= alter_table_manage_keys(table,
>                                     table->file->indexes_are_disabled(),
Are you sure lock_tables() is not necessary for temporary tables here? It would
be nice to make sure this code is covered by a test case.

> @@ -8166,43 +8169,57 @@ static bool fk_prepare_copy_alter_table(THD *thd, TABLE *table,
>    if (!error && alter_ctx->is_table_renamed())
>    {
>      THD_STAGE_INFO(thd, stage_rename);
> -    handlerton *old_db_type= table->s->db_type();
> -    /*
> -      Then do a 'simple' rename of the table. First we need to close all
> -      instances of 'source' table.
> -      Note that if wait_while_table_is_used() returns error here (i.e. if
> -      this thread was killed) then it must be that previous step of
> -      simple rename did nothing and therefore we can safely return
> -      without additional clean-up.
> -    */
> -    if (wait_while_table_is_used(thd, table, extra_func))
> -      DBUG_RETURN(true);
> -    close_all_tables_for_name(thd, table->s, HA_EXTRA_PREPARE_FOR_RENAME, NULL);
> -
>      LEX_STRING old_db_name= { alter_ctx->db, strlen(alter_ctx->db) };
>      LEX_STRING old_table_name=
>                 { alter_ctx->table_name, strlen(alter_ctx->table_name) };
>      LEX_STRING new_db_name= { alter_ctx->new_db, strlen(alter_ctx->new_db) };
>      LEX_STRING new_table_name=
>                 { alter_ctx->new_alias, strlen(alter_ctx->new_alias) };
> -    (void) rename_table_in_stat_tables(thd, &old_db_name, &old_table_name,
> -                                       &new_db_name, &new_table_name);
>  
> -    if (mysql_rename_table(old_db_type, alter_ctx->db, alter_ctx->table_name,
> -                           alter_ctx->new_db, alter_ctx->new_alias, 0))
> -      error= -1;
> -    else if (Table_triggers_list::change_table_name(thd,
> -                                                    alter_ctx->db,
> -                                                    alter_ctx->alias,
> -                                                    alter_ctx->table_name,
> -                                                    alter_ctx->new_db,
> -                                                    alter_ctx->new_alias))
> -    {
> -      (void) mysql_rename_table(old_db_type,
> -                                alter_ctx->new_db, alter_ctx->new_alias,
> -                                alter_ctx->db, alter_ctx->table_name,
> -                                NO_FK_CHECKS);
> -      error= -1;
> +    if (!table->s->tmp_table)
> +    {
> +      handlerton *old_db_type= table->s->db_type();
> +      /*
> +        Then do a 'simple' rename of the table. First we need to close all
> +        instances of 'source' table.
> +        Note that if wait_while_table_is_used() returns error here (i.e. if
> +        this thread was killed) then it must be that previous step of
> +        simple rename did nothing and therefore we can safely return
> +        without additional clean-up.
> +      */
> +      if (wait_while_table_is_used(thd, table, extra_func))
> +        DBUG_RETURN(true);
> +      close_all_tables_for_name(thd, table->s, HA_EXTRA_PREPARE_FOR_RENAME,
> +                                NULL);
> +
> +      (void) rename_table_in_stat_tables(thd, &old_db_name, &old_table_name,
> +                                         &new_db_name, &new_table_name);
> +
> +      if (mysql_rename_table(old_db_type, alter_ctx->db, alter_ctx->table_name,
> +                             alter_ctx->new_db, alter_ctx->new_alias, 0))
> +        error= -1;
> +      else if (Table_triggers_list::change_table_name(thd,
> +                                                      alter_ctx->db,
> +                                                      alter_ctx->alias,
> +                                                      alter_ctx->table_name,
> +                                                      alter_ctx->new_db,
> +                                                      alter_ctx->new_alias))
> +      {
> +        (void) mysql_rename_table(old_db_type,
> +                                  alter_ctx->new_db, alter_ctx->new_alias,
> +                                  alter_ctx->db, alter_ctx->table_name,
> +                                  NO_FK_CHECKS);
> +        error= -1;
> +      }
> +    }
> +    else
> +    {
> +      if ((thd->rename_temporary_table(table, alter_ctx->new_db,
> +                                       alter_ctx->new_alias)))
> +      {
> +        /* Allocation error, no need to rename it back to the original name. */
> +        error= -1;
> +      }
>      }
>    }
>  
A few lines below we adjust MDL locks, is it safe for temporary tables?

Previously we didn't binlog temporary table creation if binlog format was row.
Now we binlog it independently of binlog format.

Previously we did "new_table->s->table_creation_was_logged=
table->s->table_creation_was_logged;". Now we don't.


> @@ -9623,7 +9639,8 @@ bool mysql_trans_commit_alter_copy_data(THD *thd)
>    to->file->ha_release_auto_increment();
>    if (to->file->ha_external_lock(thd,F_UNLCK))
>      error=1;
> -  if (error < 0 && to->file->extra(HA_EXTRA_PREPARE_FOR_RENAME))
> +  if (error < 0 && !to->s->tmp_table &&
> +      to->file->extra(HA_EXTRA_PREPARE_FOR_RENAME))
>      error= 1;
>    thd_progress_end(thd);
>    DBUG_RETURN(error > 0 ? -1 : 0);
Isn't to->s->tmp_table always temporary? Shouldn't we check from->s->tmp_table
instead?

Regards,
Sergey