maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

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. But it is probably subject of another bug (or even task).

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.

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.

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