Re: [Maria-developers] [Commits] d1de640: MDEV-10216: Assertion `strcmp(share->unique_file_name, filename) ||
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
Hi Svoj,
On Thu, Jun 23, 2016 at 7:35 AM, Sergey Vojtovich
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@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
participants (2)
-
Nirbhay Choubey -
Sergey Vojtovich