Re: [Maria-developers] [Commits] 511bd2c: MDEV-10216: Assertion `strcmp(share->unique_file_name, filename) ||
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@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
Hi Svoj, [Also adding Monty for his inputs.] On Thu, Jun 16, 2016 at 7:45 AM, Sergey Vojtovich <svoj@mariadb.org> 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/a7798dfd0a6472bf65fffc2ade543605e17... 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@mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits _______________________________________________ commits mailing list commits@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
participants (2)
-
Nirbhay Choubey -
Sergey Vojtovich