maria-developers team mailing list archive

[Nirbhay Choubey <nirbhay@xxxxxxxxxxx>]

Hi Svoj,

[Also adding Monty for his inputs.]

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

> Hi Nirbhay,
>
> last_version of MyISAM is used for debugging only and has no functional
> effect.
> last_version of Aria has some functional effect.
>
> Effect of HA_EXTRA_PREPARE_FOR_RENAME for MyISAM is flush key blocks and
> reset
> last_version.
> Effect of HA_EXTRA_PREPARE_FOR_RENAME for Aria is more complex.
> Other engines ignore HA_EXTRA_PREPARE_FOR_RENAME.
>
> I don't completely understand why "ALTER TABLE ... RENAME TO ..." has to
> copy
> data between tables.


ALTER TABLE implementation takes a shortcut for operations not affecting
.frm files,
which includes simple RENAMEs. But this is skipped for temp tables and thus
copying
always takes place for temp tables.

The following commit (from 2002) added this exception for temp tables:
https://github.com/MariaDB/server/commit/a7798dfd0a6472bf65fffc2ade543605e177ff9c

I have created a patch to also include temp tables to this shortcut :
https://gist.github.com/nirbhayc/442a0c269ce48b283543cac434aaf44e



> But it is probably subject of another bug (or even task).
>

Right. I will open a separate MDEV for this.


> copy_data_between_tables() seem to be the only function that does
> HA_EXTRA_PREPARE_FOR_RENAME for temporary table (intermediate in this
> case).
>
> I don't think the above HA_EXTRA_PREPARE_FOR_RENAME is reasonable for
> MyISAM.
> There's no point in resetting last_version and flushing key blocks, since
> we're
> going to reuse this itermediate table anyway.
>
> I don't think the above can be reasonable for any engine.
>

Makes sense.


>
> Said the above, I'd vote to remove this call. But please check with some
> Aria
> expert.


> Your fix is mostly alright, but I guess we shouldn't reset last_version in
> case
> of HA_EXTRA_PREPARE_FOR_DROP.
>

Monty ^^ ?


Best,
Nirbhay



>
> Regards,
> Sergey
>
> On Wed, Jun 15, 2016 at 08:57:06AM -0400, Nirbhay Choubey wrote:
> > revision-id: 511bd2ca3269bc7bf80e30cceeab534f7f3e5666
> (mariadb-10.2.0-83-g511bd2c)
> > parent(s): b2ae32aafdd2787ad456f38833f630182ded25e8
> > author: Nirbhay Choubey
> > committer: Nirbhay Choubey
> > timestamp: 2016-06-15 08:57:04 -0400
> > message:
> >
> > MDEV-10216: Assertion `strcmp(share->unique_file_name,filename) ||
> >
> > .. share->last_version' failed in myisam/mi_open.c:67: test_if_reopen
> >
> > During the RENAME operation since the renamed temporary table is also
> > opened and added to myisam_open_list/maria_open_list, resetting the
> > last_version at the end of operation (HA_EXTRA_PREPARE_FOR_RENAME)
> > will cause an assertion failure when a subsequent query tries to open
> > an additional temporary table instance and thus attempts to reuse it
> > from the open table list.
> >
> > Fixed by not resetting share->last_version for temporary tables so that
> > the share gets reused when needed.
> >
> > ---
> >  mysql-test/r/reopen_temp_table.result | 18 ++++++++++++++++++
> >  mysql-test/t/reopen_temp_table.test   | 16 ++++++++++++++++
> >  storage/maria/ma_extra.c              |  7 ++++++-
> >  storage/myisam/mi_extra.c             |  7 ++++++-
> >  4 files changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/mysql-test/r/reopen_temp_table.result
> b/mysql-test/r/reopen_temp_table.result
> > index 08affaa..e63a21e 100644
> > --- a/mysql-test/r/reopen_temp_table.result
> > +++ b/mysql-test/r/reopen_temp_table.result
> > @@ -164,5 +164,23 @@ SELECT COUNT(*)=4 FROM t6;
> >  COUNT(*)=4
> >  1
> >  DROP TABLE t5, t6;
> > +#
> > +# MDEV-10216: Assertion `strcmp(share->unique_file_name,filename) ||
> > +# share->last_version' failed in myisam/mi_open.c:67: test_if_reopen
> > +#
> > +CREATE TEMPORARY TABLE t7 (i INT) ENGINE=MYISAM;
> > +INSERT INTO t7 VALUES(1);
> > +ALTER TABLE t7 RENAME TO t;
> > +SELECT * FROM t a, t b;
> > +i    i
> > +1    1
> > +DROP TABLE t;
> > +CREATE TEMPORARY TABLE t7 (i INT) ENGINE=ARIA;
> > +INSERT INTO t7 VALUES(1);
> > +ALTER TABLE t7 RENAME TO t;
> > +SELECT * FROM t a, t b;
> > +i    i
> > +1    1
> > +DROP TABLE t;
> >  # Cleanup
> >  DROP DATABASE temp_db;
> > diff --git a/mysql-test/t/reopen_temp_table.test
> b/mysql-test/t/reopen_temp_table.test
> > index 98de983..83a5bbc 100644
> > --- a/mysql-test/t/reopen_temp_table.test
> > +++ b/mysql-test/t/reopen_temp_table.test
> > @@ -159,5 +159,21 @@ SELECT COUNT(*)=6 FROM t5;
> >  SELECT COUNT(*)=4 FROM t6;
> >  DROP TABLE t5, t6;
> >
> > +--echo #
> > +--echo # MDEV-10216: Assertion
> `strcmp(share->unique_file_name,filename) ||
> > +--echo # share->last_version' failed in myisam/mi_open.c:67:
> test_if_reopen
> > +--echo #
> > +CREATE TEMPORARY TABLE t7 (i INT) ENGINE=MYISAM;
> > +INSERT INTO t7 VALUES(1);
> > +ALTER TABLE t7 RENAME TO t;
> > +SELECT * FROM t a, t b;
> > +DROP TABLE t;
> > +
> > +CREATE TEMPORARY TABLE t7 (i INT) ENGINE=ARIA;
> > +INSERT INTO t7 VALUES(1);
> > +ALTER TABLE t7 RENAME TO t;
> > +SELECT * FROM t a, t b;
> > +DROP TABLE t;
> > +
> >  --echo # Cleanup
> >  DROP DATABASE temp_db;
> > diff --git a/storage/maria/ma_extra.c b/storage/maria/ma_extra.c
> > index fd21d28..5b33ad6 100644
> > --- a/storage/maria/ma_extra.c
> > +++ b/storage/maria/ma_extra.c
> > @@ -399,7 +399,12 @@ int maria_extra(MARIA_HA *info, enum
> ha_extra_function function,
> >        share->bitmap.changed_not_flushed= 0;
> >      }
> >      /* last_version must be protected by intern_lock; See
> collect_tables() */
> > -    share->last_version= 0L;                 /* Impossible version */
> > +    /*
> > +      Temporary table has already been added to maria_open_list, so
> > +      lets not reset the last_version.
> > +    */
> > +    if (!share->temporary)
> > +      share->last_version= 0L;                  /* Impossible version */
> >      mysql_mutex_unlock(&share->intern_lock);
> >      break;
> >    }
> > diff --git a/storage/myisam/mi_extra.c b/storage/myisam/mi_extra.c
> > index a47c198..bc5705a 100644
> > --- a/storage/myisam/mi_extra.c
> > +++ b/storage/myisam/mi_extra.c
> > @@ -265,7 +265,12 @@ int mi_extra(MI_INFO *info, enum ha_extra_function
> function, void *extra_arg)
> >      /* Fall trough */
> >    case HA_EXTRA_PREPARE_FOR_RENAME:
> >      mysql_mutex_lock(&THR_LOCK_myisam);
> > -    share->last_version= 0L;                 /* Impossible version */
> > +    /*
> > +      Temporary table has already been added to myisam_open_list, so
> > +      lets not reset the last_version.
> > +    */
> > +    if (!share->temporary)
> > +      share->last_version= 0L;                  /* Impossible version */
> >      mysql_mutex_lock(&share->intern_lock);
> >      /* Flush pages that we don't need anymore */
> >      if (flush_key_blocks(share->key_cache, share->kfile,
> > _______________________________________________
> > commits mailing list
> > commits@xxxxxxxxxxx
> > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
> _______________________________________________
> commits mailing list
> commits@xxxxxxxxxxx
> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits