maria-developers team mailing list archive

[Nirbhay Choubey <nirbhay@xxxxxxxxxxx>]

Hi Svoj,

On Thu, Jun 23, 2016 at 7:35 AM, Sergey Vojtovich <svoj@xxxxxxxxxxx> wrote:

> 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.
>
>
I add a new function before this stub but still no change. Pretty weird. :)


> >
> >    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.
>

hmm.. While I do see locks getting initialized for transactional temporary
tables (get_lock_data()),
I still do not understand why would it be necessary. I also looked into the
past related commits but
could not find one that explains the rationale.

Anyway, I have added some more tests to check the behavior and will also
ask Monty for a 2nd
review.


> > @@ -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?
>

This should be skipped for temporary tables and its done in
mysql_alter_table().
I have now moved the logic specific to temporary tables to a separate
function to
make it more clearer (simple_tmp_rename_or_index_change()).


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

Right. It has been fixed now. Also fixed another issue MDEV-10320 found
while investigating this. I have also added some replication tests to verify
the code changes.


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

We don't really need this in case of shortcut

>
>
> > @@ -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?
>

Right. Updated.

Here's the newer version of the fix:
http://lists.askmonty.org/pipermail/commits/2016-July/009523.html

Best,
Nirbhay


> Regards,
> Sergey
> _______________________________________________
> commits mailing list
> commits@xxxxxxxxxxx
> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits