Re: [Maria-developers] 56a2835: MDEV-5535: Cannot reopen temporary table
Hi, Nirbhay! Just a couple of questions/comments, see below. And please pay attention to Kristian's email about locking and parallel replication. I don't think I can confidently review it in a patch, so you may want to spend more time testing or analyzing that code. Ok to push, when you're ready! Thanks! On May 26, Nirbhay Choubey wrote:
revision-id: 56a2835872c4ac7296ec0ae2ff618822855b0fc0 (mariadb-10.1.8-82-g56a2835) parent(s): 28c289626f318631d707f85b057a90af99018b06 author: Nirbhay Choubey committer: Nirbhay Choubey timestamp: 2016-05-26 20:42:47 -0400 message:
MDEV-5535: Cannot reopen temporary table
mysqld maintains a list of TABLE objects for all temporary tables in THD, where each table is represented by a TABLE object. A query referencing a temporary table for more than once, however, failed with ER_CANT_REOPEN_TABLE error.
"because a TABLE_SHARE was allocate together with the TABLE, so temporary tables always had one TABLE per TABLE_SHARE"
This patch lift this restriction by preserving the TABLE_SHARE object of each created temporary table, so that multiple instances of TABLE objects can be created.
better "by separating TABLE and TABLE_SHARE objects and storing TABLE_SHARE's in the list in a THD, and TABLE's in the list in the corresponding TABLE_SHARE"
diff --git a/sql/rpl_rli.cc b/sql/rpl_rli.cc index 7bdd9c1..77f458b 100644 --- a/sql/rpl_rli.cc +++ b/sql/rpl_rli.cc @@ -1060,24 +1061,43 @@ void Relay_log_info::inc_group_relay_log_pos(ulonglong log_pos,
void Relay_log_info::close_temporary_tables() { - TABLE *table,*next; DBUG_ENTER("Relay_log_info::close_temporary_tables");
- for (table=save_temporary_tables ; table ; table=next) + TMP_TABLE_SHARE *share; + TABLE *table; + + while ((share= save_temporary_tables.pop_front())) { - next=table->next; + /* + Iterate over the list of tables for this TABLE_SHARE and close them. + */ + while ((table= share->all_tmp_tables.pop_front())) + { + DBUG_PRINT("tmptable", ("closing table: '%s'.'%s'", + table->s->db.str, table->s->table_name.str)); + + /* Reset in_use as the table may have been created by another thd */ + table->in_use= 0; + free_io_cache(table); + /* + Lets not free TABLE_SHARE here as there could be multiple TABLEs opened + for the same table (TABLE_SHARE). + */ + closefrm(table, false); + my_free(table); + }
- /* Reset in_use as the table may have been created by another thd */ - table->in_use=0; /* Don't ask for disk deletion. For now, anyway they will be deleted when slave restarts, but it is a better intention to not delete them. */ - DBUG_PRINT("info", ("table: 0x%lx", (long) table)); - close_temporary(table, 1, 0); + + free_table_share(share); + my_free(share); } - save_temporary_tables= 0; - slave_open_temp_tables= 0; + + save_temporary_tables.empty();
Is it needed? you've popped all shares from the list, it's empty. I'd rather add an assert instead
+ DBUG_VOID_RETURN; }
diff --git a/sql/sql_class.h b/sql/sql_class.h index ae240ae..a70c4a9 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -1247,6 +1247,61 @@ enum enum_locked_tables_mode LTM_PRELOCKED_UNDER_LOCK_TABLES };
+/** + The following structure is an extension to TABLE_SHARE and is + exclusively for temporary tables. + + @note: + Although, TDC_element has data members (like next, prev & + all_tables) to store the list of TABLE_SHARE & TABLE objects + related to a particular TABLE_SHARE, they cannot be moved to + TABLE_SHARE in order to be reused for temporary tables. This + is because, as concurrent threads iterating through hash of + TDC_element's may need access to all_tables, but if all_tables + is made part of TABLE_SHARE, then TDC_element->share->all_tables + is not always guaranteed to be valid, as TDC_element can live + longer than TABLE_SHARE. +*/ +struct TMP_TABLE_SHARE : public TABLE_SHARE +{ +private: + /* + Link to all temporary table shares. Declared as private to + avoid direct manipulation with those objects. One should + use methods of I_P_List template instead. + */ + TMP_TABLE_SHARE *tmp_next; + TMP_TABLE_SHARE **tmp_prev; + + friend struct All_tmp_table_shares; + +public: + /* + Doubly-linked (back-linked) lists of used and unused TABLE objects + for this share.
and unused? you don't free them but keep in the list?
+ */ + All_share_tables_list all_tmp_tables; +}; + +/** + Helper class which specifies which members of TMP_TABLE_SHARE are + used for participation in the list of temporary tables. +*/ + +struct All_tmp_table_shares +{ + static inline TMP_TABLE_SHARE **next_ptr(TMP_TABLE_SHARE *l) + { + return &l->tmp_next; + } + static inline TMP_TABLE_SHARE ***prev_ptr(TMP_TABLE_SHARE *l) + { + return &l->tmp_prev; + } +}; + +/* Also used in rpl_rli.h. */ +typedef I_P_List <TMP_TABLE_SHARE, All_tmp_table_shares> All_tmp_tables_list;
/** Class that holds information about tables which were opened and locked diff --git a/sql/temporary_tables.cc b/sql/temporary_tables.cc new file mode 100644 index 0000000..1514b72 --- /dev/null +++ b/sql/temporary_tables.cc ... +TABLE *THD::find_temporary_table(const char *db, + const char *table_name) +{ + DBUG_ENTER("THD::find_temporary_table"); + + TABLE *table; + char key[MAX_DBKEY_LENGTH]; + uint key_length; + bool locked; + + if (!(has_temporary_tables() || (rgi_slave && has_slave_temporary_tables())))
hmm, really? you had here is_empty(true) before and your is_empty() was working like this: if (flag_is_true) { do rgi_slave } else { do thd } your old is_empty was not checking thd if the argument was true, your new code does, as if your is_empty() was do thd; if (flag_is_true) { do rgi_slave}
+ { + DBUG_RETURN(NULL); + } + + key_length= create_tmp_table_def_key(key, db, table_name);
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Serg! On Fri, Jun 3, 2016 at 4:10 PM, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nirbhay!
Just a couple of questions/comments, see below.
And please pay attention to Kristian's email about locking and parallel replication. I don't think I can confidently review it in a patch, so you may want to spend more time testing or analyzing that code.
Yes, I did add some related rpl test cases.
Ok to push, when you're ready! Thanks!
On May 26, Nirbhay Choubey wrote:
revision-id: 56a2835872c4ac7296ec0ae2ff618822855b0fc0 (mariadb-10.1.8-82-g56a2835) parent(s): 28c289626f318631d707f85b057a90af99018b06 author: Nirbhay Choubey committer: Nirbhay Choubey timestamp: 2016-05-26 20:42:47 -0400 message:
MDEV-5535: Cannot reopen temporary table
mysqld maintains a list of TABLE objects for all temporary tables in THD, where each table is represented by a TABLE object. A query referencing a temporary table for more than once, however, failed with ER_CANT_REOPEN_TABLE error.
"because a TABLE_SHARE was allocate together with the TABLE, so temporary tables always had one TABLE per TABLE_SHARE"
This patch lift this restriction by preserving the TABLE_SHARE object of each created temporary table, so that multiple instances of TABLE objects can be created.
better "by separating TABLE and TABLE_SHARE objects and storing TABLE_SHARE's in the list in a THD, and TABLE's in the list in the corresponding TABLE_SHARE"
That's better indeed. I will update that when I merge the consolidated patches to the main branch.
diff --git a/sql/rpl_rli.cc b/sql/rpl_rli.cc index 7bdd9c1..77f458b 100644 --- a/sql/rpl_rli.cc +++ b/sql/rpl_rli.cc @@ -1060,24 +1061,43 @@ void Relay_log_info::inc_group_relay_log_pos(ulonglong log_pos,
void Relay_log_info::close_temporary_tables() { - TABLE *table,*next; DBUG_ENTER("Relay_log_info::close_temporary_tables");
- for (table=save_temporary_tables ; table ; table=next) + TMP_TABLE_SHARE *share; + TABLE *table; + + while ((share= save_temporary_tables.pop_front())) { - next=table->next; + /* + Iterate over the list of tables for this TABLE_SHARE and close them. + */ + while ((table= share->all_tmp_tables.pop_front())) + { + DBUG_PRINT("tmptable", ("closing table: '%s'.'%s'", + table->s->db.str, table->s->table_name.str)); + + /* Reset in_use as the table may have been created by another thd */ + table->in_use= 0; + free_io_cache(table); + /* + Lets not free TABLE_SHARE here as there could be multiple TABLEs opened + for the same table (TABLE_SHARE). + */ + closefrm(table, false); + my_free(table); + }
- /* Reset in_use as the table may have been created by another thd */ - table->in_use=0; /* Don't ask for disk deletion. For now, anyway they will be deleted when slave restarts, but it is a better intention to not delete them. */ - DBUG_PRINT("info", ("table: 0x%lx", (long) table)); - close_temporary(table, 1, 0); + + free_table_share(share); + my_free(share); } - save_temporary_tables= 0; - slave_open_temp_tables= 0; + + save_temporary_tables.empty();
Is it needed? you've popped all shares from the list, it's empty. I'd rather add an assert instead.
Right. I realized this later. Fixed.
+ DBUG_VOID_RETURN; }
diff --git a/sql/sql_class.h b/sql/sql_class.h index ae240ae..a70c4a9 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -1247,6 +1247,61 @@ enum enum_locked_tables_mode LTM_PRELOCKED_UNDER_LOCK_TABLES };
+/** + The following structure is an extension to TABLE_SHARE and is + exclusively for temporary tables. + + @note: + Although, TDC_element has data members (like next, prev & + all_tables) to store the list of TABLE_SHARE & TABLE objects + related to a particular TABLE_SHARE, they cannot be moved to + TABLE_SHARE in order to be reused for temporary tables. This + is because, as concurrent threads iterating through hash of + TDC_element's may need access to all_tables, but if all_tables + is made part of TABLE_SHARE, then TDC_element->share->all_tables + is not always guaranteed to be valid, as TDC_element can live + longer than TABLE_SHARE. +*/ +struct TMP_TABLE_SHARE : public TABLE_SHARE +{ +private: + /* + Link to all temporary table shares. Declared as private to + avoid direct manipulation with those objects. One should + use methods of I_P_List template instead. + */ + TMP_TABLE_SHARE *tmp_next; + TMP_TABLE_SHARE **tmp_prev; + + friend struct All_tmp_table_shares; + +public: + /* + Doubly-linked (back-linked) lists of used and unused TABLE objects + for this share.
and unused? you don't free them but keep in the list?
Yes. We did agree to not free/close but reuse them.
+ */ + All_share_tables_list all_tmp_tables; +}; + +/** + Helper class which specifies which members of TMP_TABLE_SHARE are + used for participation in the list of temporary tables. +*/ + +struct All_tmp_table_shares +{ + static inline TMP_TABLE_SHARE **next_ptr(TMP_TABLE_SHARE *l) + { + return &l->tmp_next; + } + static inline TMP_TABLE_SHARE ***prev_ptr(TMP_TABLE_SHARE *l) + { + return &l->tmp_prev; + } +}; + +/* Also used in rpl_rli.h. */ +typedef I_P_List <TMP_TABLE_SHARE, All_tmp_table_shares> All_tmp_tables_list;
/** Class that holds information about tables which were opened and locked diff --git a/sql/temporary_tables.cc b/sql/temporary_tables.cc new file mode 100644 index 0000000..1514b72 --- /dev/null +++ b/sql/temporary_tables.cc ... +TABLE *THD::find_temporary_table(const char *db, + const char *table_name) +{ + DBUG_ENTER("THD::find_temporary_table"); + + TABLE *table; + char key[MAX_DBKEY_LENGTH]; + uint key_length; + bool locked; + + if (!(has_temporary_tables() || (rgi_slave && has_slave_temporary_tables())))
hmm, really? you had here is_empty(true) before and your is_empty() was working like this:
if (flag_is_true) { do rgi_slave } else { do thd }
your old is_empty was not checking thd if the argument was true, your new code does, as if your is_empty() was
do thd; if (flag_is_true) { do rgi_slave}
+ { + DBUG_RETURN(NULL); + } + + key_length= create_tmp_table_def_key(key, db, table_name);
Yea, there was a problem here. It should have checked for !rgi_slave for thd before proceeding check slave temporary tables. Its now : (!rgi_slave && has_temporary_tables()) || (rgi_slave && unlikely(has_slave_temporary_tables()))) Best, Nirbhay
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
participants (2)
-
Nirbhay Choubey -
Sergei Golubchik