Re: [Maria-developers] [Commits] c285dbe: MDEV-5535: Cannot reopen temporary table
Hi, Nirbhay! I like the approach, there were only few comments, but still there were some :) See below. On May 16, Nirbhay Choubey wrote:
revision-id: c285dbece2881e1d864bb758d77edc899d568c04 (mariadb-10.1.8-82-gc285dbe) parent(s): 222ca736f737e888115c732c8ecad84faf0a2529 author: Nirbhay Choubey committer: Nirbhay Choubey timestamp: 2016-05-16 23:59:10 -0400 message:
MDEV-5535: Cannot reopen temporary table
Would be nice to have a somewhat more verbose commit comment here :)
diff --git a/mysql-test/t/reopen_temp_table-master.test b/mysql-test/t/reopen_temp_table-master.test new file mode 100644 index 0000000..5ac2ca8 --- /dev/null +++ b/mysql-test/t/reopen_temp_table-master.test @@ -0,0 +1 @@ +--tmpdir=$MYSQLTEST_VARDIR//tmp
Eh... I suppose you mean reopen_temp_table-master.opt And, in fact, you can simply use reopen_temp_table.opt Btw, why two slashes? And why do you need to set tmpdir at all? I suspect you don't - because your tests apparently succeed without it :)
diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index aa2cae5..f729733 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -4051,7 +4051,12 @@ static TABLE *create_table_from_items(THD *thd, } else { - if (open_temporary_table(thd, create_table)) + /* + The pointer to the newly created temporary table has been stored in + table->create_info. + */ + create_table->table= create_info->table;
Where was this happening before you've added an explicit assignment? (note the assert below - it worked, so somewhere this must've been assigned)
+ if (!create_table->table) { /* This shouldn't happen as creation of temporary table should make @@ -4060,7 +4065,6 @@ static TABLE *create_table_from_items(THD *thd, */ DBUG_ASSERT(0); } - DBUG_ASSERT(create_table->table == create_info->table);
why?
} } else @@ -4308,6 +4326,27 @@ bool select_create::send_eof() DBUG_RETURN(true); }
+ if (table->s->tmp_table) + { + /* + Now is good time to add the new table to THD temporary tables list. + But, before that we need to check if same table got created by the sub- + statement. + */ + if (thd->temporary_tables.find_table_share(table->s->table_cache_key.str, + table->s->table_cache_key.length)) + { + my_error(ER_TABLE_EXISTS_ERROR, MYF(0), table->alias.c_ptr());
ER_TABLE_EXISTS_ERROR, eh? I'm not sure it's the best way to solve this. It's an error that neither CREATE ... IF NOT EXISTS or CREATE OR REPLACE can fix. But I don't have a good solution for this. If a concurrent thread would've tried to create a table meanwhile (assuming, non-temporary), it would wait on the metadata lock, that protects table creation. so, logically, trying to create a table from inside create table or trying to drop a used table from stored function (your ER_CANT_REOPEN_TABLE) should fail with ER_LOCK_DEADLOCK or ER_LOCK_ABORTED - because it's, basically, trying to get a conflicting metadata lock within the same thread, clearly a case of the deadlock. but somehow I believe that returning ER_LOCK_DEADLOCK on a purely myisam test case with temporary tables - that will be confusing as hell :( so, ok, let's keep your ER_TABLE_EXISTS_ERROR. But, if possible, please commit this code (save_tmp_table_share and ER_CANT_REOPEN_TABLE) in a separate commit, after the main MDEV-5535 commit. To have it clearly distinct in the history, in case we'll want to change this behaviour later.
+ abort_result_set(); + DBUG_RETURN(true); + } + else + { + DBUG_ASSERT(save_tmp_table_share); + thd->temporary_tables.relink_table_share(save_tmp_table_share); + } + } + /* Do an implicit commit at end of statement for non-temporary tables. This can fail, but we should unlock the table diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 40032c3..93fba14 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -2281,23 +2281,17 @@ int mysql_rm_table_no_locks(THD *thd, TABLE_LIST *tables, bool if_exists, */ DBUG_ASSERT(!(thd->locked_tables_mode && table->open_type != OT_BASE_ONLY && - find_temporary_table(thd, table) && + thd->temporary_tables.find_table(table) && table->mdl_request.ticket != NULL));
- /* - drop_temporary_table may return one of the following error codes: - . 0 - a temporary table was successfully dropped. - . 1 - a temporary table was not found. - . -1 - a temporary table is used by an outer statement. - */ if (table->open_type == OT_BASE_ONLY || !is_temporary_table(table)) error= 1; else { table_creation_was_logged= table->table->s->table_creation_was_logged; - if ((error= drop_temporary_table(thd, table->table, &is_trans)) == -1) + if (thd->temporary_tables.drop_table(table->table, &is_trans, true)) { - DBUG_ASSERT(thd->in_sub_stmt); + error= 1; goto err;
Why are changes in this hunk? (comment, assert, -1, etc)
} table->table= 0; diff --git a/sql/table.h b/sql/table.h index ab39603..8b7c665 100644 --- a/sql/table.h +++ b/sql/table.h @@ -600,6 +601,7 @@ struct TABLE_STATISTICS_CB struct TABLE_SHARE { TABLE_SHARE() {} /* Remove gcc warning */ + TABLE_SHARE *next, *prev;
Eh? Did you forget to remove this?
/** Category of this table. */ TABLE_CATEGORY table_category; diff --git a/sql/temporary_tables.cc b/sql/temporary_tables.cc new file mode 100644 index 0000000..ed5b6b8 --- /dev/null +++ b/sql/temporary_tables.cc @@ -0,0 +1,1457 @@ +/* + Copyright (c) 2016 MariaDB Corporation + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +*/ + +#include "sql_acl.h" /* TMP_TABLE_ACLS */ +#include "sql_base.h" /* free_io_cache, + tdc_create_key */ +#include "lock.h" /* mysql_lock_remove */ +#include "log_event.h" /* Query_log_event */ +#include "sql_show.h" /* append_identifier */ +#include "sql_handler.h" /* mysql_ha_rm_temporary_tables */ +#include "temporary_tables.h" /* Temporary_tables */ +#include "rpl_rli.h" /* rpl_group_info */ + +#define IS_USER_TABLE(A) ((A->tmp_table == TRANSACTIONAL_TMP_TABLE) || \ + (A->tmp_table == NON_TRANSACTIONAL_TMP_TABLE)) + + +/* + Initialize the Temporary_tables object. Currently it always returns + false (success). + + @param thd [IN] Thread handle + + @return false Success + true Error +*/ +bool Temporary_tables::init(THD *thd) +{ + DBUG_ENTER("Temporary_tables::init"); + this->m_thd= thd; + DBUG_RETURN(false); +} + + +/* + Check whether temporary tables exist. The decision is made based on the + existence of TMP_TABLE_SHAREs. + + @param check_slave [IN] Also check the slave temporary tables. + + @return false Temporary tables exist + true No temporary table exist +*/ +bool Temporary_tables::is_empty(bool check_slave)
not a very good idea. You always use it as is_empty(true) or is_empty(false). that is, the caller always knows whether to check slave or not. but then you use a common function and it needs to do a completely unnecessary if() inside. Better to have two functions, like is_empty() and is_empty_slave(). Or at least make this function inline, then the compiler will have a chance to optimize it.
+{ + DBUG_ENTER("Temporary_tables::is_empty"); + + bool result; + + if (!m_thd) + { + DBUG_RETURN(true); + } + + rpl_group_info *rgi_slave= m_thd->rgi_slave; + + if (check_slave && rgi_slave) + { + result= (rgi_slave->rli->save_temp_table_shares == NULL) ? true : false; + } + else + { + result= (m_table_shares == NULL) ? true : false; + } + + DBUG_RETURN(result); +} + + + /* + Reset the Temporary_tables object. Currently, it always returns + false (success). + + @return false Success + true Error +*/ +bool Temporary_tables::reset() +{ + DBUG_ENTER("Temporary_tables::reset"); + m_table_shares= 0; + DBUG_RETURN(false); +} + + +/* + Create a temporary table, open it and return the TABLE handle. + + @param hton [IN] Handlerton + @param frm [IN] Binary frm image + @param path [IN] File path (without extension) + @param db [IN] Schema name + @param table_name [IN] Table name + @param open_in_engine [IN] Whether open table in SE + @param created [OUT] Whether table was created?
can it ever happen for *create==true but a return value is NULL? or the other way around? how?
+ + + @return Success A pointer to table object + Failure NULL +*/ +TABLE *Temporary_tables::create_and_open_table(handlerton *hton, + LEX_CUSTRING *frm, + const char *path, + const char *db, + const char *table_name, + bool open_in_engine, + bool *created) +{ + DBUG_ENTER("Temporary_tables::create_and_open_table"); + + TMP_TABLE_SHARE *share; + TABLE *table= NULL; + bool locked; + + *created= false; + + if (wait_for_prior_commit()) + { + DBUG_RETURN(NULL); + } + + locked= lock_tables();
old code didn't seem to have it here. how comes?
+ + if ((share= create_table(hton, frm, path, db, table_name))) + { + *created= true; + table= open_table(share, table_name, open_in_engine); + } + + if (locked) + { + DBUG_ASSERT(m_locked); + unlock_tables(); + } + + DBUG_RETURN(table); +} + +/* + Check whether an open table with db/table name is in use. + + @param db [IN] Database name + @param table_name [IN] Table name + + @return Success Pointer to first used table instance. + Failure NULL +*/ +TABLE *Temporary_tables::find_table(const char *db, + const char *table_name) +{ + DBUG_ENTER("Temporary_tables::find_table"); + + TABLE *table; + char key[MAX_DBKEY_LENGTH]; + uint key_length; + bool locked; + + if (is_empty(true)) + { + DBUG_RETURN(NULL); + } + + key_length= create_table_def_key(key, db, table_name); + + if (wait_for_prior_commit()) + { + DBUG_RETURN(NULL); + } + + locked= lock_tables(); + table = find_table(key, key_length, TABLE_IN_USE); + if (locked) + { + DBUG_ASSERT(m_locked); + unlock_tables(); + } + + DBUG_RETURN(table); +} + + +/* + Check whether an open table specified in TABLE_LIST is in use. + + @return tl [IN] TABLE_LIST + + @return Success Pointer to first used table instance. + Failure NULL +*/ +TABLE *Temporary_tables::find_table(const TABLE_LIST *tl) +{ + DBUG_ENTER("Temporary_tables::find_table"); + TABLE *table= find_table(tl->get_db_name(), tl->get_table_name()); + DBUG_RETURN(table); +} + + +/* + Check whether an open table with the specified key is in use. + The key, in this case, is not the usual key used for temporary tables. + It does not contain server_id & pseudo_thread_id. This function is + essentially used use to check whether there is any temporary table + which _shadows_ a base table. + (see: Query_cache::send_result_to_client()) + + @return Success A pointer to table share object + Failure NULL +*/ +TABLE *Temporary_tables::find_table_reduced_key_length(const char *key, + uint key_length) +{ + DBUG_ENTER("Temporary_tables::find_table_reduced_key_length"); + + TABLE *result= NULL; + bool locked; + + locked= lock_tables(); + + for (TMP_TABLE_SHARE *share= m_table_shares; share; share= share->next) + { + if ((share->table_cache_key.length - TMP_TABLE_KEY_EXTRA) == key_length + && !memcmp(share->table_cache_key.str, key, key_length)) + { + /* + A matching TMP_TABLE_SHARE is found. We now need to find a TABLE + instance in use. + */ + for (TABLE *table= share->table; table; table= table->next) + { + if (table->query_id != 0) + { + result= table; + break; + } + } + } + } + + if (locked) + { + DBUG_ASSERT(m_locked); + unlock_tables(); + } + + DBUG_RETURN(result); +} + + +/* + Lookup the TMP_TABLE_SHARE using the given db/table_name.The server_id and + pseudo_thread_id used to generate table definition key is taken from m_thd + (see create_table_def_key()). Return NULL is none found. + + @return Success A pointer to table share object + Failure NULL +*/ +TMP_TABLE_SHARE *Temporary_tables::find_table_share(const char *db, + const char *table_name) +{ + DBUG_ENTER("Temporary_tables::find_table_share"); + + TMP_TABLE_SHARE *share; + char key[MAX_DBKEY_LENGTH]; + uint key_length; + + key_length= create_table_def_key(key, db, table_name); + share= find_table_share(key, key_length); + + DBUG_RETURN(share); +} + + +/* + Lookup TMP_TABLE_SHARE using the specified TABLE_LIST element. + Return NULL is none found. + + @return Success A pointer to table share object + Failure NULL +*/ +TMP_TABLE_SHARE *Temporary_tables::find_table_share(const TABLE_LIST *tl) +{ + DBUG_ENTER("Temporary_tables::find_table_share"); + TMP_TABLE_SHARE *share= find_table_share(tl->get_db_name(), + tl->get_table_name()); + DBUG_RETURN(share); +} + + +/* + Lookup TMP_TABLE_SHARE using the specified table definition key. + Return NULL is none found. + + @return Success A pointer to table share object + Failure NULL +*/ +TMP_TABLE_SHARE *Temporary_tables::find_table_share(const char *key, + uint key_length) +{ + DBUG_ENTER("Temporary_tables::find_table_share"); + + TMP_TABLE_SHARE *share; + TMP_TABLE_SHARE *result= NULL; + bool locked; + + if (wait_for_prior_commit()) + { + DBUG_RETURN(NULL); + } + + locked= lock_tables(); + + for (share= m_table_shares; share; share= share->next) + { + if (share->table_cache_key.length == key_length && + !(memcmp(share->table_cache_key.str, key, key_length))) + { + result= share; + break; + } + } + + if (locked) + { + DBUG_ASSERT(m_locked); + unlock_tables(); + } + + DBUG_RETURN(result); +} + + +/* + Find a temporary table specified by TABLE_LIST instance in the open table + list and prepare its TABLE instance for use. If + + This function tries to resolve this table in the list of temporary tables + of this thread. Temporary tables are thread-local and "shadow" base + tables with the same name. + + @note In most cases one should use Temporary_tables::open_tables() instead + of this call. + + @note One should finalize process of opening temporary table for table + list element by calling open_and_process_table(). This function + is responsible for table version checking and handling of merge + tables. + + @note We used to check global_read_lock before opening temporary tables. + However, that limitation was artificial and is removed now. + + @param tl [IN] TABLE_LIST + + @return Error status. + @retval false On success. If a temporary table exists for the given + key, tl->table is set. + @retval TRUE On error. my_error() has been called.
letter case is a bit weird, 'false', but 'TRUE' :)
+*/ +bool Temporary_tables::open_table(TABLE_LIST *tl) +{ + DBUG_ENTER("Temporary_tables::open_table"); + + TMP_TABLE_SHARE *share; + TABLE *table= NULL; + bool locked; + + /* + Code in open_table() assumes that TABLE_LIST::table can be non-zero only + for pre-opened temporary tables. + */ + DBUG_ASSERT(tl->table == NULL); + + /* + This function should not be called for cases when derived or I_S + tables can be met since table list elements for such tables can + have invalid db or table name. + Instead Temporary_tables::open_tables() should be used. + */ + DBUG_ASSERT(!tl->derived && !tl->schema_table); + + if (tl->open_type == OT_BASE_ONLY || is_empty(true)) + { + DBUG_PRINT("info", ("skip_temporary is set or no temporary tables")); + DBUG_RETURN(false); + } + + if (wait_for_prior_commit())
this wasn't in the original code
+ { + DBUG_RETURN(true); + } + + locked= lock_tables();
neither was this
+ + /* + First check if there is a reusable open table available in the + open table list. + */ + if (find_and_use_table(tl, &table)) + { + if (locked) + { + DBUG_ASSERT(m_locked); + unlock_tables(); + } + DBUG_RETURN(true); /* Error */ + } + + /* + No reusable table was found. We will have to open a new instance. + */ + if (!table && (share= find_table_share(tl))) + { + table= open_table(share, tl->get_table_name(), true); + } + + if (locked) + { + DBUG_ASSERT(m_locked); + unlock_tables(); + } + + if (!table) + { + if (tl->open_type == OT_TEMPORARY_ONLY && + tl->open_strategy == TABLE_LIST::OPEN_NORMAL) + { + my_error(ER_NO_SUCH_TABLE, MYF(0), tl->db, tl->table_name); + DBUG_RETURN(true); + } + DBUG_RETURN(false); + } + +#ifdef WITH_PARTITION_STORAGE_ENGINE + if (tl->partition_names) + { + /* Partitioned temporary tables is not supported. */ + DBUG_ASSERT(!table->part_info); + my_error(ER_PARTITION_CLAUSE_ON_NONPARTITIONED, MYF(0)); + DBUG_RETURN(true); + } +#endif + + table->query_id= m_thd->query_id; + m_thd->thread_specific_used= true; + + /* It is neither a derived table nor non-updatable view. */ + tl->updatable= true; + tl->table= table; + + table->init(m_thd, tl); + + DBUG_PRINT("info", ("Using temporary table")); + DBUG_RETURN(false); +} + + +/* + Pre-open temporary tables corresponding to table list elements. + + @note One should finalize process of opening temporary tables + by calling open_tables(). This function is responsible + for table version checking and handling of merge tables.
what kind of version checking can there be for temporary tables?
+ + @param tl [IN] TABLE_LIST + + @return false On success. If a temporary table exists + for the given element, tl->table is set. + true On error. my_error() has been called. +*/ +bool Temporary_tables::open_tables(TABLE_LIST *tl) +{ + DBUG_ENTER("Temporary_tables::open_tables"); + + TABLE_LIST *first_not_own= m_thd->lex->first_not_own_table(); + + for (TABLE_LIST *table= tl; table && table != first_not_own; + table= table->next_global) + { + if (table->derived || table->schema_table) + { + /* + Derived and I_S tables will be handled by a later call to open_tables(). + */ + continue; + } + + if ((m_thd->temporary_tables.open_table(table)))
eh? couldn't you simply do if (open_table(table)) ? why do you need this "m_thd->temporary_tables." ?
+ { + DBUG_RETURN(true); + } + } + + DBUG_RETURN(false); +} + + +/* + Close all temporary tables created by 'CREATE TEMPORARY TABLE' for thread + creates one DROP TEMPORARY TABLE binlog event for each pseudo-thread. + + Temporary tables created in a sql slave is closed by + Relay_log_info::close_temporary_tables(). + + @return false Success + true Failure +*/ +bool Temporary_tables::close_tables() +{ + DBUG_ENTER("Temporary_tables::close_tables"); + + TMP_TABLE_SHARE *share; + TMP_TABLE_SHARE *share_next; + TABLE *table, *table_next; + bool error= false; + + if (!m_table_shares) + { + DBUG_RETURN(false); + } + DBUG_ASSERT(!m_thd->rgi_slave); + + /* + Ensure we don't have open HANDLERs for tables we are about to close. + This is necessary when Temporary_tables::close_tables() is called as + part of execution of BINLOG statement (e.g. for format description event). + */ + mysql_ha_rm_temporary_tables(m_thd); + + /* Close all open temporary tables. */ + for (TMP_TABLE_SHARE *share= m_table_shares; share; share= share->next) + { + /* Traverse the table list. */ + table= share->table; + while (table) + { + table_next= table->next; + free_table(table); + table= table_next; + } + } + + // Write DROP TEMPORARY TABLE query log events to binary log. + if (mysql_bin_log.is_open()) + { + error= log_events_and_free_shares(); + } + else + { + share= m_table_shares; + while (share) + { + share_next= share->next; + free_table_share(share, true); + share= share_next; + } + } + reset(); + + DBUG_RETURN(error); +} + + +/* + Rename a temporary table. + + @param table [IN] Table handle + @param db [IN] New schema name + @param table_name [IN] New table name + + @return false Success + true Error +*/ +bool Temporary_tables::rename_table(TABLE *table, + const char *db, + const char *table_name) +{ + DBUG_ENTER("Temporary_tables::rename_table"); + + char *key; + uint key_length; + + TMP_TABLE_SHARE *share= static_cast<TMP_TABLE_SHARE *>(table->s); + + if (!(key= (char *) alloc_root(&share->mem_root, MAX_DBKEY_LENGTH))) + { + DBUG_RETURN(true); + } + + /* + Temporary tables are renamed by simply changing their table definition key. + */ + key_length= create_table_def_key(key, db, table_name); + share->set_table_cache_key(key, key_length); + + DBUG_RETURN(false); +} + + +/* + Drop a temporary table. + + Try to locate the table in the list of open temporary tables. + If the table is found: + - If the table is locked with LOCK TABLES or by prelocking, + unlock it and remove it from the list of locked tables + (THD::lock). Currently only transactional temporary tables + are locked. + - Close the temporary table, remove its .FRM. + - Remove the table share from the list of temporary table shares. + + This function is used to drop user temporary tables, as well as + internal tables created in CREATE TEMPORARY TABLE ... SELECT + or ALTER TABLE. + + @param table [IN] Temporary table to be deleted + @param is_trans [OUT] Is set to the type of the table: + transactional (e.g. innodb) as true or + non-transactional (e.g. myisam) as false. + @paral delete_table [IN] Whether to delete the table files. + + @return false Table was dropped + true Error +*/ +bool Temporary_tables::drop_table(TABLE *table, + bool *is_trans, + bool delete_table) +{ + DBUG_ENTER("Temporary_tables::drop_table"); + + TMP_TABLE_SHARE *share; + TABLE *tab; + TABLE *tab_next; + bool result, locked; + + DBUG_ASSERT(table); + DBUG_ASSERT(table->query_id); + DBUG_PRINT("tmptable", ("Dropping table: '%s'.'%s'", + table->s->db.str, table->s->table_name.str)); + + if (wait_for_prior_commit()) + { + DBUG_RETURN(true); + }
old code didn't seem to have that. Why do you?
+ + locked= lock_tables(); + + share= static_cast<TMP_TABLE_SHARE *>(table->s);
suggestion: have a function: static inline TMP_TABLE_SHARE *tmp_table_share(TABLE *t) { DBUG_ASSERT(t->s->tmp_table); return static_cast<TMP_TABLE_SHARE *>(table->s); }
+ + /* Table might be in use by some outer statement. */ + for (tab= share->table; tab; tab= tab->next) + { + if (tab != table && tab->query_id != 0) + { + /* Found a table instance in use. This table cannot be be dropped. */ + my_error(ER_CANT_REOPEN_TABLE, MYF(0), table->alias.c_ptr()); + result= true; + goto end; + } + } + + if (is_trans) + *is_trans= table->file->has_transactions(); + + /* + Iterate over the list of open tables and close all tables referencing the + same table share.
the comment sounds a bit odd now, when all open tables are in the list inside the share.
+ */ + tab= share->table; + while (tab) + { + tab_next= tab->next; + if ((result= free_table(tab))) goto end; + tab= tab_next; + } + + result= free_table_share(share, delete_table); + +end: + if (locked) + { + DBUG_ASSERT(m_locked); + unlock_tables(); + } + + DBUG_RETURN(result); +} + + +/** + Delete the temporary table files. + + @param base [IN] Handlerton for table to be deleted. + @param path [IN] Path to the table to be deleted (i.e. path + to its .frm without an extension). + + @return false Success + true Error +*/ +bool Temporary_tables::remove_table(handlerton *base, const char *path) +{ + DBUG_ENTER("Temporary_tables::remove_table"); + + bool error= false; + handler *file; + char frm_path[FN_REFLEN + 1]; + + strxnmov(frm_path, sizeof(frm_path) - 1, path, reg_ext, NullS); + if (mysql_file_delete(key_file_frm, frm_path, MYF(0))) + { + error= true; + } + file= get_new_handler((TABLE_SHARE*) 0, current_thd->mem_root, base); + if (file && file->ha_delete_table(path)) + { + error= true; + sql_print_warning("Could not remove temporary table: '%s', error: %d", + path, my_errno); + } + + delete file; + DBUG_RETURN(error); +} + + +/* + Mark all temporary tables which were used by the current statement or + sub-statement as free for reuse, but only if the query_id can be cleared. + + @remark For temp tables associated with a open SQL HANDLER the query_id + is not reset until the HANDLER is closed. +*/ +void Temporary_tables::mark_tables_as_free_for_reuse() +{ + DBUG_ENTER("Temporary_tables::mark_tables_as_free_for_reuse"); + + bool locked; + + if (m_thd->query_id == 0) + { + /* + Thread has not executed any statement and has not used any + temporary tables. + */ + DBUG_VOID_RETURN; + } + + locked= lock_tables(); + + for (TMP_TABLE_SHARE *share= m_table_shares; share; share= share->next) + { + for (TABLE *table= share->table; table; table= table->next) + { + if ((table->query_id == m_thd->query_id) && !table->open_by_handler) + { + mark_table_as_free_for_reuse(table); + } + } + } + + if (locked) + { + DBUG_ASSERT(m_locked); + unlock_tables(); + } + + DBUG_VOID_RETURN; +} + + +/* + Reset a single temporary table. Effectively this "closes" one temporary + table in a session. + + @param table Temporary table +*/ +void Temporary_tables::mark_table_as_free_for_reuse(TABLE *table) +{ + DBUG_ENTER("Temporary_tables::mark_table_as_free_for_reuse"); + + DBUG_ASSERT(table->s->tmp_table); + + table->query_id= 0; + table->file->ha_reset(); + + /* Detach temporary MERGE children from temporary parent. */ + DBUG_ASSERT(table->file); + table->file->extra(HA_EXTRA_DETACH_CHILDREN); + + /* + Reset temporary table lock type to it's default value (TL_WRITE). + + Statements such as INSERT INTO .. SELECT FROM tmp, CREATE TABLE + .. SELECT FROM tmp and UPDATE may under some circumstances modify + the lock type of the tables participating in the statement. This + isn't a problem for non-temporary tables since their lock type is + reset at every open, but the same does not occur for temporary + tables for historical reasons. + + Furthermore, the lock type of temporary tables is not really that + important because they can only be used by one query at a time. + Nonetheless, it's safer from a maintenance point of view to reset + the lock type of this singleton TABLE object as to not cause problems + when the table is reused. + + Even under LOCK TABLES mode its okay to reset the lock type as + LOCK TABLES is allowed (but ignored) for a temporary table. + */ + table->reginfo.lock_type= TL_WRITE; + DBUG_VOID_RETURN; +} + + +TMP_TABLE_SHARE *Temporary_tables::unlink_table_share(TABLE_SHARE *share) +{ + DBUG_ENTER("Temporary_tables::unlink_table"); + TMP_TABLE_SHARE *tmp_table_share; + + lock_tables(); + tmp_table_share= static_cast<TMP_TABLE_SHARE *>(share); + unlink<TMP_TABLE_SHARE>(&m_table_shares, tmp_table_share); + unlock_tables(); + + DBUG_RETURN(tmp_table_share); +} + + +void Temporary_tables::relink_table_share(TMP_TABLE_SHARE *share) +{ + DBUG_ENTER("Temporary_tables::relink_table"); + + lock_tables(); + link<TMP_TABLE_SHARE>(&m_table_shares, share); + unlock_tables(); + + DBUG_VOID_RETURN; +} + + +/* + Create a table definition key. + + @param key [OUT] Buffer for the key to be created (must + be of size MAX_DBKRY_LENGTH) + @param db [IN] Database name + @param table_name [IN] Table name + + @return Key length. + + @note + The table key is create from: + db + \0 + table_name + \0 + + Additionally, we add the following to make each temporary table unique on + the slave. + + 4 bytes of master thread id + 4 bytes of pseudo thread id +*/ + +uint Temporary_tables::create_table_def_key(char *key, const char *db, + const char *table_name) +{ + DBUG_ENTER("Temporary_tables::create_table_def_key"); + + uint key_length; + + key_length= tdc_create_key(key, db, table_name); + int4store(key + key_length, m_thd->variables.server_id); + int4store(key + key_length + 4, m_thd->variables.pseudo_thread_id); + key_length += TMP_TABLE_KEY_EXTRA; + + DBUG_RETURN(key_length); +} + + +/* + Create a temporary table. + + @param hton [IN] Handlerton + @param frm [IN] Binary frm image + @param path [IN] File path (without extension) + @param db [IN] Schema name + @param table_name [IN] Table name + + @return Success A pointer to table share object + Failure NULL +*/ +TMP_TABLE_SHARE *Temporary_tables::create_table(handlerton *hton, + LEX_CUSTRING *frm, + const char *path, + const char *db, + const char *table_name) +{ + DBUG_ENTER("Temporary_tables::create_table"); + + TMP_TABLE_SHARE *share; + char key_cache[MAX_DBKEY_LENGTH]; + char *saved_key_cache; + char *tmp_path; + uint key_length; + int res; + + if (wait_for_prior_commit())
you're doing it in almost every method. And when one method calls another, you're doing it twice. Or thrice. Isn't is too much? (I don't know)
+ { + DBUG_RETURN(NULL); + } + + /* Create the table definition key for the temporary table. */ + key_length= create_table_def_key(key_cache, db, table_name); + + if (!(share= (TMP_TABLE_SHARE *) my_malloc(sizeof(TMP_TABLE_SHARE) + + strlen(path) + + 1 + key_length, MYF(MY_WME)))) + { + DBUG_RETURN(NULL); /* Out of memory */ + } + + tmp_path= (char *)(share + 1); + saved_key_cache= strmov(tmp_path, path) + 1; + memcpy(saved_key_cache, key_cache, key_length); + + init_tmp_table_share(m_thd, share, saved_key_cache, key_length, + strend(saved_key_cache) + 1, tmp_path); + + share->table= 0; + share->db_plugin= ha_lock_engine(m_thd, hton); + + /* + Prefer using frm image over file. The image might not be available in + ALTER TABLE, when the discovering engine took over the ownership (see + TABLE::read_frm_image). + */ + res= (frm->str) + ? share->init_from_binary_frm_image(m_thd, false, frm->str, frm->length) + : open_table_def(m_thd, share, GTS_TABLE | GTS_USE_DISCOVERY); + + if (res) + { + /* + No need to lock share->mutex as this is not needed for temporary tables. + */ + ::free_table_share(share); + my_free(share); + DBUG_RETURN(NULL); + } + + share->m_psi= PSI_CALL_get_table_share(true, share); + + /* Add share to the head of the temporary table share list. */ + link<TMP_TABLE_SHARE>(&m_table_shares, share); + + DBUG_RETURN(share); +} + + +/* + Find a table with the specified key. + + @param key [IN] Key + @param key_length [IN] Key length + @param state [IN] Open table state to look for + + @return Success Pointer to the table instance. + Failure NULL +*/ +TABLE *Temporary_tables::find_table(const char *key, uint key_length, + Table_state state) +{ + DBUG_ENTER("Temporary_tables::find_table"); + + for (TMP_TABLE_SHARE *share= m_table_shares; share; share= share->next) + { + if (share->table_cache_key.length == key_length && + !(memcmp(share->table_cache_key.str, key, key_length))) + { + /* A matching TMP_TABLE_SHARE is found. */ + for (TABLE *table= share->table; table; table= table->next) + { + switch (state) + { + case TABLE_IN_USE: + if (table->query_id > 0) DBUG_RETURN(table); + break; + case TABLE_NOT_IN_USE: + if (table->query_id == 0) DBUG_RETURN(table); + break; + case TABLE_ANY: + DBUG_RETURN(table); + default: /* Invalid */ + DBUG_ASSERT(0); + DBUG_RETURN(NULL); + } + } + } + } + + DBUG_RETURN(NULL); +} + + +/* + Find a reusable table in the open table list using the specified TABLE_LIST. + + @param tl [IN] Table list + @param out_table [OUT] Pointer to the requested TABLE object + + @return Success false + Failure true +*/ +bool Temporary_tables::find_and_use_table(const TABLE_LIST *tl, + TABLE **out_table) +{ + DBUG_ENTER("Temporary_tables::find_and_use_table"); + + char key[MAX_DBKEY_LENGTH]; + uint key_length; + bool result; + + key_length= create_table_def_key(key, tl->get_db_name(), + tl->get_table_name()); + result= use_table(find_table(key, key_length, TABLE_NOT_IN_USE), out_table); + + DBUG_RETURN(result); +} + + +/* + Open a table from the specified TABLE_SHARE with the given alias. + + @param share [IN] Table share + @param alias [IN] Table alias + @param open_in_engine [IN] Whether open table in SE + + @return Success A pointer to table object + Failure NULL +*/ +TABLE *Temporary_tables::open_table(TMP_TABLE_SHARE *share, + const char *alias, + bool open_in_engine) +{ + DBUG_ENTER("Temporary_tables::open_table"); + + TABLE *table; + + if (wait_for_prior_commit()) + { + DBUG_RETURN(NULL); + } + + if (!(table= (TABLE *) my_malloc(sizeof(TABLE), MYF(MY_WME)))) + { + DBUG_RETURN(NULL); /* Out of memory */ + } + + if (open_table_from_share(m_thd, share, alias, + (open_in_engine) ? + (uint) (HA_OPEN_KEYFILE | HA_OPEN_RNDFILE | + HA_GET_INDEX) : 0, + (uint) (READ_KEYINFO | COMPUTE_TYPES | + EXTRA_RECORD), + ha_open_options, + table, + open_in_engine ? false : true)) + { + my_free(table); + DBUG_RETURN(NULL); + } + + table->reginfo.lock_type= TL_WRITE; /* Simulate locked */ + table->grant.privilege= TMP_TABLE_ACLS; + share->tmp_table= (table->file->has_transactions() ? + TRANSACTIONAL_TMP_TABLE : NON_TRANSACTIONAL_TMP_TABLE); + + table->pos_in_table_list= 0; + table->query_id= m_thd->query_id; + + /* Add table to the head of table list. */ + link<TABLE>(&share->table, table);
I don't know if that's correct. You use TABLE::next and TABLE::prev pointers to link all tables of a given TMP_TABLE_SHARE into a list. But these pointers are used to link all tables into a THD::open_tables list (and may be for other purposes too?). In fact, TABLE has dedicated pointers for this list that you want: /** Links for the list of all TABLE objects for this share. Declared as private to avoid direct manipulation with those objects. One should use methods of I_P_List template instead. */ TABLE *share_all_next, **share_all_prev;
+ + /* Increment Slave_open_temp_table_definitions status variable count. */ + if (m_thd->rgi_slave) + { + thread_safe_increment32(&slave_open_temp_tables); + } + + DBUG_PRINT("tmptable", ("Opened table: '%s'.'%s' 0x%lx", table->s->db.str, + table->s->table_name.str, (long) table)); + DBUG_RETURN(table); +} + +
function comment?
+bool Temporary_tables::use_table(TABLE *table, TABLE **out_table) +{ + DBUG_ENTER("Temporary_tables::use_table"); + + *out_table= table; + if (!table) + DBUG_RETURN(false); +
old code had a block here about "Temporary tables are not safe for parallel replication". it's in wait_for_prior_commit now, but you aren't calling it here. why?
+ /* + We need to set the THD as it may be different in case of + parallel replication + */ + if (table->in_use != m_thd) + { + table->in_use= m_thd; + } + + DBUG_RETURN(false); +} + + +/* + Close a temporary table. + + @param table [IN] Table handle + + @return false Success + true Error +*/ +bool Temporary_tables::close_table(TABLE *table) +{ + DBUG_ENTER("Temporary_tables::close_table"); + + DBUG_PRINT("tmptable", ("closing table: '%s'.'%s' 0x%lx alias: '%s'", + table->s->db.str, table->s->table_name.str, + (long) table, table->alias.c_ptr())); + + free_io_cache(table);
don't forget to remove free_io_cache() calls when rebasing your work on top of the latest 10.2 (they were removed from close_temporary() in 260dd476b05 commit)
+ closefrm(table, false); + my_free(table); + + /* Decrement Slave_open_temp_table_definitions status variable count. */ + if (m_thd->rgi_slave) + { + thread_safe_decrement32(&slave_open_temp_tables); + } + + DBUG_RETURN(false); +} + + +bool Temporary_tables::wait_for_prior_commit() +{ + DBUG_ENTER("Temporary_tables::wait_for_prior_commit"); + + /* + Temporary tables are not safe for parallel replication. They were + designed to be visible to one thread only, so have no table locking. + Thus there is no protection against two conflicting transactions + committing in parallel and things like that. + + So for now, anything that uses temporary tables will be serialised + with anything before it, when using parallel replication. + + TODO: We might be able to introduce a reference count or something + on temp tables, and have slave worker threads wait for it to reach + zero before being allowed to use the temp table. Might not be worth + it though, as statement-based replication using temporary tables is + in any case rather fragile. + */ + if (m_thd->rgi_slave && + m_thd->rgi_slave->rli->save_temp_table_shares && + m_thd->rgi_slave->is_parallel_exec && + m_thd->wait_for_prior_commit()) + DBUG_RETURN(true); + + DBUG_RETURN(false); +} + + +/* + Write query log events with "DROP TEMPORARY TABLES .." for each pseudo + thread to the binary log. + + @return false Success + true Error +*/ +bool Temporary_tables::log_events_and_free_shares() +{ + DBUG_ENTER("Temporary_tables::log_events_and_free_shares"); + + DBUG_ASSERT(!m_thd->rgi_slave); + + TMP_TABLE_SHARE *share; + TMP_TABLE_SHARE *next; + TMP_TABLE_SHARE *prev_share; + // Assume thd->variables.option_bits has OPTION_QUOTE_SHOW_CREATE. + bool was_quote_show= true; + bool error= false; + bool found_user_tables= false; + // Better add "IF EXISTS" in case a RESET MASTER has been done. + const char stub[]= "DROP /*!40005 TEMPORARY */ TABLE IF EXISTS "; + char buf[FN_REFLEN]; + + String s_query(buf, sizeof(buf), system_charset_info); + s_query.copy(stub, sizeof(stub) - 1, system_charset_info); + + /* + Insertion sort of temporary tables by pseudo_thread_id to build ordered + list of sublists of equal pseudo_thread_id. + */ + + for (prev_share= m_table_shares, share= prev_share->next; + share; + prev_share= share, share= share->next) + { + TMP_TABLE_SHARE *prev_sorted; /* Same as for prev_share */ + TMP_TABLE_SHARE *sorted; + + if (IS_USER_TABLE(share)) + { + if (!found_user_tables) + found_user_tables= true; + + for (prev_sorted= NULL, sorted= m_table_shares; + sorted != share; + prev_sorted= sorted, sorted= sorted->next) + { + if (!IS_USER_TABLE(sorted) || + tmpkeyval(sorted) > tmpkeyval(share)) + { + /* + Move into the sorted part of the list from the unsorted. + */ + prev_share->next= share->next; + share->next= sorted; + if (prev_sorted) + { + prev_sorted->next= share; + } + else + { + m_table_shares= share; + } + share= prev_share; + break; + } + } + } + } + + /* + We always quote db & table names. + */ + if (found_user_tables && + !(was_quote_show= MY_TEST(m_thd->variables.option_bits & + OPTION_QUOTE_SHOW_CREATE))) + { + m_thd->variables.option_bits |= OPTION_QUOTE_SHOW_CREATE; + } + + /* + Scan sorted temporary tables to generate sequence of DROP. + */ + for (share= m_table_shares; share; share= next) + { + if (IS_USER_TABLE(share)) + { + bool save_thread_specific_used= m_thd->thread_specific_used; + my_thread_id save_pseudo_thread_id= m_thd->variables.pseudo_thread_id; + char db_buf[FN_REFLEN]; + String db(db_buf, sizeof(db_buf), system_charset_info); + + /* + Set pseudo_thread_id to be that of the processed table. + */ + m_thd->variables.pseudo_thread_id= tmpkeyval(share); + + db.copy(share->db.str, share->db.length, system_charset_info); + /* + Reset s_query() if changed by previous loop. + */ + s_query.length(sizeof(stub) - 1); + + /* + Loop forward through all tables that belong to a common database + within the sublist of common pseudo_thread_id to create single + DROP query. + */ + for (; + share && + IS_USER_TABLE(share) && + tmpkeyval(share) == m_thd->variables.pseudo_thread_id && + share->db.length == db.length() && + memcmp(share->db.str, db.ptr(), db.length()) == 0; + share= next) + { + /* + We are going to add ` around the table names and possible more + due to special characters. + */ + append_identifier(m_thd, &s_query, share->table_name.str, + share->table_name.length); + s_query.append(','); + next= share->next; + remove_table(share->db_type(), share->path.str); + ::free_table_share(share); + my_free(share); + } + + m_thd->clear_error(); + CHARSET_INFO *cs_save= m_thd->variables.character_set_client; + m_thd->variables.character_set_client= system_charset_info; + m_thd->thread_specific_used= true; + + Query_log_event qinfo(m_thd, s_query.ptr(), + s_query.length() - 1 /* to remove trailing ',' */, + false, true, false, 0); + qinfo.db= db.ptr(); + qinfo.db_len= db.length(); + m_thd->variables.character_set_client= cs_save; + + m_thd->get_stmt_da()->set_overwrite_status(true); + if ((error= (mysql_bin_log.write(&qinfo) || error))) + { + /* + If we're here following THD::cleanup, thence the connection + has been closed already. So lets print a message to the + error log instead of pushing yet another error into the + stmt_da. + + Also, we keep the error flag so that we propagate the error + up in the stack. This way, if we're the SQL thread we notice + that Temporary_tables::close_tables failed. (Actually, the SQL + thread only calls Temporary_tables::close_tables while applying + old Start_log_event_v3 events.) + */ + sql_print_error("Failed to write the DROP statement for " + "temporary tables to binary log"); + } + + m_thd->get_stmt_da()->set_overwrite_status(false); + m_thd->variables.pseudo_thread_id= save_pseudo_thread_id; + m_thd->thread_specific_used= save_thread_specific_used; + } + else + { + next= share->next; + free_table_share(share, true); + } + } + + if (!was_quote_show) + { + /* + Restore option. + */ + m_thd->variables.option_bits&= ~OPTION_QUOTE_SHOW_CREATE; + } + + DBUG_RETURN(error); +} + +/* + Delete the files and free the specified table share. +*/ +bool Temporary_tables::free_table_share(TMP_TABLE_SHARE *share, + bool delete_table) +{ + DBUG_ENTER("Temporary_tables::free_table_share"); + + bool result= false; + + /* Delete the share from table share list */ + unlink<TMP_TABLE_SHARE>(&m_table_shares, share); + + if (delete_table) + { + result= remove_table(share->db_type(), share->path.str); + } + + ::free_table_share(share); + my_free(share); + + DBUG_RETURN(result); +} + + +/* + Free the specified table object. +*/ +bool Temporary_tables::free_table(TABLE *table) +{ + DBUG_ENTER("Temporary_tables::free_table"); + + bool result= false; + + /* Delete the table from table list */ + unlink<TABLE>(&static_cast<TMP_TABLE_SHARE *>(table->s)->table, table); + + /* + If LOCK TABLES list is not empty and contains this table, unlock the table + and remove the table from this list. + */ + mysql_lock_remove(m_thd, m_thd->lock, table); + + result= close_table(table); + + DBUG_RETURN(result); +} + + +bool Temporary_tables::lock_tables() +{ + /* Do not proceed if a lock has already been taken. */ + if (m_locked) + { + return false; + } + + rpl_group_info *rgi_slave= m_thd->rgi_slave; + if (rgi_slave) + { + mysql_mutex_lock(&rgi_slave->rli->data_lock); + m_table_shares= rgi_slave->rli->save_temp_table_shares; + m_locked= true; + } + return m_locked; +} + + +void Temporary_tables::unlock_tables() +{ + if (!m_locked) + { + return; + } + + rpl_group_info *rgi_slave= m_thd->rgi_slave; + if (rgi_slave) + { + rgi_slave->rli->save_temp_table_shares= m_table_shares; + mysql_mutex_unlock(&rgi_slave->rli->data_lock); + m_locked= false; + /* + Temporary tables are shared with other by sql execution threads. + As a safety measure, clear the pointer to the common area. + */ + reset(); + } + return; +} + diff --git a/sql/temporary_tables.h b/sql/temporary_tables.h new file mode 100644 index 0000000..5a5ed0d --- /dev/null +++ b/sql/temporary_tables.h @@ -0,0 +1,150 @@ +#ifndef TEMPORARY_TABLES_H +#define TEMPORARY_TABLES_H +/* + Copyright (c) 2016 MariaDB Corporation + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +*/ + +#define TMP_TABLE_KEY_EXTRA 8 + +struct TMP_TABLE_SHARE: public TABLE_SHARE +{ + /* List of TABLE instances created out of this TABLE_SHARE. */ + TABLE *table;
Hmm, if you will use TABLE::share_all_prev and TABLE::share_all_next, as I've written above, you should also use TDC_element that keeps the list of TABLE's per TABLE_SHARE. And also it has next/prev pointers to link all TABLE_SHARE's in a list... Perhaps you won't need TMP_TABLE_SHARE after all? Could you talk with Svoj about it, please? perhaps you can simply reuse existing TABLE_SHARE code for linking TABLE_SHARE's into a list and linking TABLE's into a per-TABLE_SHARE list? TDC_element is a bit of an overkill for temporary tables, we can either live with that or extract those bits that you need into a "mini-TDC_element" ? Dunno, best to discuss it with Svoj.
+ + /* Pointers to access TMP_TABLE_SHARE instances. */ + TMP_TABLE_SHARE *next; + TMP_TABLE_SHARE *prev; +}; + + +class Temporary_tables +{ +public: + Temporary_tables() : m_thd(0), m_table_shares(0), m_locked(false) + {} + bool init(THD *thd); + bool is_empty(bool check_slave); + bool reset(); + + TABLE *create_and_open_table(handlerton *hton, LEX_CUSTRING *frm, + const char *path, const char *db, + const char *table_name, bool open_in_engine, + bool *created); + + TABLE *find_table(const char *db, const char *table_name); + TABLE *find_table(const TABLE_LIST *tl); + TABLE *find_table_reduced_key_length(const char *key, uint key_length); + + TMP_TABLE_SHARE *find_table_share(const char *db, const char *table_name); + TMP_TABLE_SHARE *find_table_share(const TABLE_LIST *tl); + TMP_TABLE_SHARE *find_table_share(const char *key, uint key_length); + + bool open_table(TABLE_LIST *tl); + bool open_tables(TABLE_LIST *tl); + + bool close_tables(); + bool rename_table(TABLE *table, const char *db, const char *table_name); + bool drop_table(TABLE *table, bool *is_trans, bool delete_table); + bool remove_table(handlerton *hton, const char *path); + void mark_tables_as_free_for_reuse(); + void mark_table_as_free_for_reuse(TABLE *table); + + TMP_TABLE_SHARE *unlink_table_share(TABLE_SHARE *share); + void relink_table_share(TMP_TABLE_SHARE *share); + +private: + /* THD handler */ + THD *m_thd;
already discussed
+ + /* List of temporary table shares */ + TMP_TABLE_SHARE *m_table_shares; + + /* Whether a lock has been acquired. */ + bool m_locked; + + /* Opened table states. */ + enum Table_state { + TABLE_IN_USE, + TABLE_NOT_IN_USE, + TABLE_ANY + }; + + uint create_table_def_key(char *key, + const char *db, + const char *table_name); + + TMP_TABLE_SHARE *create_table(handlerton *hton, + LEX_CUSTRING *frm, + const char *path, + const char *db, + const char *table_name); + + TABLE *find_table(const char *key, uint key_length, Table_state state); + + bool find_and_use_table(const TABLE_LIST *tl, TABLE **out_table); + + TABLE *open_table(TMP_TABLE_SHARE *share, const char *alias, + bool open_in_engine); + + bool use_table(TABLE *table, TABLE **out_table); + bool close_table(TABLE *table); + bool wait_for_prior_commit(); + bool log_events_and_free_shares(); + + bool free_table_share(TMP_TABLE_SHARE *share, bool delete_table); + bool free_table(TABLE *table); + + bool lock_tables(); + void unlock_tables(); + + /* List operations */ + template <class T> + void link(T **list, T *element) + { + element->next= *list; + if (element->next) + element->next->prev= element; + *list= element; + (*list)->prev= 0; + } + + template <class T> + void unlink(T **list, T *element) + { + if (element->prev) + { + element->prev->next= element->next; + if (element->prev->next) + element->next->prev= element->prev; + } + else + { + DBUG_ASSERT(element == *list); + + *list= element->next; + if (*list) + element->next->prev= 0; + } + }
No need to reinvent the wheel, you can use I_P_List for that.
+ + uint tmpkeyval(TMP_TABLE_SHARE *share) + { + return uint4korr(share->table_cache_key.str + + share->table_cache_key.length - 4); + } +}; + +#endif /* TEMPORARY_TABLES_H */
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Serg, On Thu, May 19, 2016 at 12:07 PM, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nirbhay!
I like the approach, there were only few comments, but still there were some :)
See below.
On May 16, Nirbhay Choubey wrote:
revision-id: c285dbece2881e1d864bb758d77edc899d568c04 (mariadb-10.1.8-82-gc285dbe) parent(s): 222ca736f737e888115c732c8ecad84faf0a2529 author: Nirbhay Choubey committer: Nirbhay Choubey timestamp: 2016-05-16 23:59:10 -0400 message:
MDEV-5535: Cannot reopen temporary table
Would be nice to have a somewhat more verbose commit comment here :)
Sure, I will.
diff --git a/mysql-test/t/reopen_temp_table-master.test b/mysql-test/t/reopen_temp_table-master.test new file mode 100644 index 0000000..5ac2ca8 --- /dev/null +++ b/mysql-test/t/reopen_temp_table-master.test @@ -0,0 +1 @@ +--tmpdir=$MYSQLTEST_VARDIR//tmp
Eh... I suppose you mean reopen_temp_table-master.opt
That's the fun of copying existing stuff. :)
And, in fact, you can simply use reopen_temp_table.opt Btw, why two slashes? And why do you need to set tmpdir at all? I suspect you don't - because your tests apparently succeed without it :)
Right. In fact, I just realized that mtr allots a separate tmpdir for all server instances. So, use of this opt file is redundant. Removed.
diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index aa2cae5..f729733 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -4051,7 +4051,12 @@ static TABLE *create_table_from_items(THD *thd, } else { - if (open_temporary_table(thd, create_table)) + /* + The pointer to the newly created temporary table has been stored in + table->create_info. + */ + create_table->table= create_info->table;
Where was this happening before you've added an explicit assignment?
(note the assert below - it worked, so somewhere this must've been assigned)
In create_table_impl() after the table is opened: .. 4839 create_info->table= table; // Store pointer to table .. The new code does a THD::create_and_open_tmp_table() in create_table_impl(), and simply assigns the new TABLE to create_table->table. And thus, open_temporary_table() and assert for equality is no longer needed, IMO.
+ if (!create_table->table) { /* This shouldn't happen as creation of temporary table should make @@ -4060,7 +4065,6 @@ static TABLE *create_table_from_items(THD *thd, */ DBUG_ASSERT(0); } - DBUG_ASSERT(create_table->table == create_info->table);
why?
Explained in the above comment.
} } else @@ -4308,6 +4326,27 @@ bool select_create::send_eof() DBUG_RETURN(true); }
+ if (table->s->tmp_table) + { + /* + Now is good time to add the new table to THD temporary tables
+ But, before that we need to check if same table got created by
+ statement. + */ + if (thd->temporary_tables.find_table_share(table->s->table_cache_key.str, +
list. the sub- table->s->table_cache_key.length))
+ { + my_error(ER_TABLE_EXISTS_ERROR, MYF(0), table->alias.c_ptr());
ER_TABLE_EXISTS_ERROR, eh? I'm not sure it's the best way to solve this. It's an error that neither CREATE ... IF NOT EXISTS or CREATE OR REPLACE can fix.
But I don't have a good solution for this. If a concurrent thread would've tried to create a table meanwhile (assuming, non-temporary), it would wait on the metadata lock, that protects table creation.
so, logically, trying to create a table from inside create table or trying to drop a used table from stored function (your ER_CANT_REOPEN_TABLE) should fail with ER_LOCK_DEADLOCK or ER_LOCK_ABORTED - because it's, basically, trying to get a conflicting metadata lock within the same thread, clearly a case of the deadlock.
but somehow I believe that returning ER_LOCK_DEADLOCK on a purely myisam test case with temporary tables - that will be confusing as hell :(
so, ok, let's keep your ER_TABLE_EXISTS_ERROR. But, if possible, please commit this code (save_tmp_table_share and ER_CANT_REOPEN_TABLE) in a separate commit, after the main MDEV-5535 commit. To have it clearly distinct in the history, in case we'll want to change this behaviour later.
I have committed this (and related) hunk in a separate commit on top of main commit. But, I have left the ER_CANT_REOPEN_TABLE related code in THD::drop_remporary_table in the main commit, as it was part of the original code. I hope that's ok.
+ abort_result_set(); + DBUG_RETURN(true); + } + else + { + DBUG_ASSERT(save_tmp_table_share); + thd->temporary_tables.relink_table_share(save_tmp_table_share); + } + } + /* Do an implicit commit at end of statement for non-temporary tables. This can fail, but we should unlock the table diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 40032c3..93fba14 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -2281,23 +2281,17 @@ int mysql_rm_table_no_locks(THD *thd, TABLE_LIST *tables, bool if_exists, */ DBUG_ASSERT(!(thd->locked_tables_mode && table->open_type != OT_BASE_ONLY && - find_temporary_table(thd, table) && + thd->temporary_tables.find_table(table) && table->mdl_request.ticket != NULL));
- /* - drop_temporary_table may return one of the following error codes: - . 0 - a temporary table was successfully dropped. - . 1 - a temporary table was not found. - . -1 - a temporary table is used by an outer statement. - */ if (table->open_type == OT_BASE_ONLY || !is_temporary_table(table)) error= 1; else { table_creation_was_logged= table->table->s->table_creation_was_logged; - if ((error= drop_temporary_table(thd, table->table, &is_trans)) == -1) + if (thd->temporary_tables.drop_table(table->table, &is_trans, true)) { - DBUG_ASSERT(thd->in_sub_stmt); + error= 1; goto err;
Why are changes in this hunk? (comment, assert, -1, etc)
The old drop_temporary_table never returned 1, but only 0 and -1. So, the comment above was incorrect. I chose to change it to return bool, instead.
} table->table= 0; diff --git a/sql/table.h b/sql/table.h index ab39603..8b7c665 100644 --- a/sql/table.h +++ b/sql/table.h @@ -600,6 +601,7 @@ struct TABLE_STATISTICS_CB struct TABLE_SHARE { TABLE_SHARE() {} /* Remove gcc warning */ + TABLE_SHARE *next, *prev;
Eh? Did you forget to remove this?
Yea, removed now.
/** Category of this table. */ TABLE_CATEGORY table_category; diff --git a/sql/temporary_tables.cc b/sql/temporary_tables.cc new file mode 100644 index 0000000..ed5b6b8 --- /dev/null +++ b/sql/temporary_tables.cc @@ -0,0 +1,1457 @@ +/* + Copyright (c) 2016 MariaDB Corporation + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301
+*/ + +#include "sql_acl.h" /* TMP_TABLE_ACLS */ +#include "sql_base.h" /* free_io_cache, + tdc_create_key */ +#include "lock.h" /* mysql_lock_remove */ +#include "log_event.h" /* Query_log_event */ +#include "sql_show.h" /* append_identifier */ +#include "sql_handler.h" /* mysql_ha_rm_temporary_tables */ +#include "temporary_tables.h" /* Temporary_tables */ +#include "rpl_rli.h" /* rpl_group_info */ + +#define IS_USER_TABLE(A) ((A->tmp_table == TRANSACTIONAL_TMP_TABLE) || \ + (A->tmp_table == NON_TRANSACTIONAL_TMP_TABLE)) + + +/* + Initialize the Temporary_tables object. Currently it always returns + false (success). + + @param thd [IN] Thread handle + + @return false Success + true Error +*/ +bool Temporary_tables::init(THD *thd) +{ + DBUG_ENTER("Temporary_tables::init"); + this->m_thd= thd; + DBUG_RETURN(false); +} + + +/* + Check whether temporary tables exist. The decision is made based on
+ existence of TMP_TABLE_SHAREs. + + @param check_slave [IN] Also check the slave temporary
USA the tables.
+ + @return false Temporary tables exist + true No temporary table exist +*/ +bool Temporary_tables::is_empty(bool check_slave)
not a very good idea. You always use it as is_empty(true) or is_empty(false). that is, the caller always knows whether to check slave or not. but then you use a common function and it needs to do a completely unnecessary if() inside. Better to have two functions, like is_empty() and is_empty_slave(). Or at least make this function inline, then the compiler will have a chance to optimize it.
Agree. I have split the method into two.
+{ + DBUG_ENTER("Temporary_tables::is_empty"); + + bool result; + + if (!m_thd) + { + DBUG_RETURN(true); + } + + rpl_group_info *rgi_slave= m_thd->rgi_slave; + + if (check_slave && rgi_slave) + { + result= (rgi_slave->rli->save_temp_table_shares == NULL) ? true : false; + } + else + { + result= (m_table_shares == NULL) ? true : false; + } + + DBUG_RETURN(result); +} + + + /* + Reset the Temporary_tables object. Currently, it always returns + false (success). + + @return false Success + true Error +*/ +bool Temporary_tables::reset() +{ + DBUG_ENTER("Temporary_tables::reset"); + m_table_shares= 0; + DBUG_RETURN(false); +} + + +/* + Create a temporary table, open it and return the TABLE handle. + + @param hton [IN] Handlerton + @param frm [IN] Binary frm image + @param path [IN] File path (without extension) + @param db [IN] Schema name + @param table_name [IN] Table name + @param open_in_engine [IN] Whether open table in SE + @param created [OUT] Whether table was created?
can it ever happen for *create==true but a return value is NULL? or the other way around? how?
Yes, but not the other way around. For instance, if the table was created successfully, but the subsequent malloc failed while an attempt was made to allocate TABLE object later in open_temporary_table().
+ + + @return Success A pointer to table object + Failure NULL +*/ +TABLE *Temporary_tables::create_and_open_table(handlerton *hton, + LEX_CUSTRING *frm, + const char *path, + const char *db, + const char *table_name, + bool open_in_engine, + bool *created) +{ + DBUG_ENTER("Temporary_tables::create_and_open_table"); + + TMP_TABLE_SHARE *share; + TABLE *table= NULL; + bool locked; + + *created= false; + + if (wait_for_prior_commit()) + { + DBUG_RETURN(NULL); + } + + locked= lock_tables();
old code didn't seem to have it here. how comes?
Since, in previous version of the patch, all temporary TABLE objects were added to a single list, I had it locked untile the TABLE got added. But now that's not the case, so moved the lock/unlock to THD::create_temporary_table() like in the original code.
+ + if ((share= create_table(hton, frm, path, db, table_name))) + { + *created= true; + table= open_table(share, table_name, open_in_engine); + } + + if (locked) + { + DBUG_ASSERT(m_locked); + unlock_tables(); + } + + DBUG_RETURN(table); +} + +/* + Check whether an open table with db/table name is in use. + + @param db [IN] Database name + @param table_name [IN] Table name + + @return Success Pointer to first used table instance. + Failure NULL +*/ +TABLE *Temporary_tables::find_table(const char *db, + const char *table_name) +{ + DBUG_ENTER("Temporary_tables::find_table"); + + TABLE *table; + char key[MAX_DBKEY_LENGTH]; + uint key_length; + bool locked; + + if (is_empty(true)) + { + DBUG_RETURN(NULL); + } + + key_length= create_table_def_key(key, db, table_name); + + if (wait_for_prior_commit()) + { + DBUG_RETURN(NULL); + } + + locked= lock_tables(); + table = find_table(key, key_length, TABLE_IN_USE); + if (locked) + { + DBUG_ASSERT(m_locked); + unlock_tables(); + } + + DBUG_RETURN(table); +} + + +/* + Check whether an open table specified in TABLE_LIST is in use. + + @return tl [IN] TABLE_LIST + + @return Success Pointer to first used table instance. + Failure NULL +*/ +TABLE *Temporary_tables::find_table(const TABLE_LIST *tl) +{ + DBUG_ENTER("Temporary_tables::find_table"); + TABLE *table= find_table(tl->get_db_name(), tl->get_table_name()); + DBUG_RETURN(table); +} + + +/* + Check whether an open table with the specified key is in use. + The key, in this case, is not the usual key used for temporary tables. + It does not contain server_id & pseudo_thread_id. This function is + essentially used use to check whether there is any temporary table + which _shadows_ a base table. + (see: Query_cache::send_result_to_client()) + + @return Success A pointer to table share object + Failure NULL +*/ +TABLE *Temporary_tables::find_table_reduced_key_length(const char *key, + uint key_length) +{ + DBUG_ENTER("Temporary_tables::find_table_reduced_key_length"); + + TABLE *result= NULL; + bool locked; + + locked= lock_tables(); + + for (TMP_TABLE_SHARE *share= m_table_shares; share; share= share->next) + { + if ((share->table_cache_key.length - TMP_TABLE_KEY_EXTRA) == key_length + && !memcmp(share->table_cache_key.str, key, key_length)) + { + /* + A matching TMP_TABLE_SHARE is found. We now need to find a TABLE + instance in use. + */ + for (TABLE *table= share->table; table; table= table->next) + { + if (table->query_id != 0) + { + result= table; + break; + } + } + } + } + + if (locked) + { + DBUG_ASSERT(m_locked); + unlock_tables(); + } + + DBUG_RETURN(result); +} + + +/* + Lookup the TMP_TABLE_SHARE using the given db/table_name.The server_id and + pseudo_thread_id used to generate table definition key is taken from m_thd + (see create_table_def_key()). Return NULL is none found. + + @return Success A pointer to table share object + Failure NULL +*/ +TMP_TABLE_SHARE *Temporary_tables::find_table_share(const char *db, + const char *table_name) +{ + DBUG_ENTER("Temporary_tables::find_table_share"); + + TMP_TABLE_SHARE *share; + char key[MAX_DBKEY_LENGTH]; + uint key_length; + + key_length= create_table_def_key(key, db, table_name); + share= find_table_share(key, key_length); + + DBUG_RETURN(share); +} + + +/* + Lookup TMP_TABLE_SHARE using the specified TABLE_LIST element. + Return NULL is none found. + + @return Success A pointer to table share object + Failure NULL +*/ +TMP_TABLE_SHARE *Temporary_tables::find_table_share(const TABLE_LIST *tl) +{ + DBUG_ENTER("Temporary_tables::find_table_share"); + TMP_TABLE_SHARE *share= find_table_share(tl->get_db_name(), + tl->get_table_name()); + DBUG_RETURN(share); +} + + +/* + Lookup TMP_TABLE_SHARE using the specified table definition key. + Return NULL is none found. + + @return Success A pointer to table share object + Failure NULL +*/ +TMP_TABLE_SHARE *Temporary_tables::find_table_share(const char *key, + uint key_length) +{ + DBUG_ENTER("Temporary_tables::find_table_share"); + + TMP_TABLE_SHARE *share; + TMP_TABLE_SHARE *result= NULL; + bool locked; + + if (wait_for_prior_commit()) + { + DBUG_RETURN(NULL); + } + + locked= lock_tables(); + + for (share= m_table_shares; share; share= share->next) + { + if (share->table_cache_key.length == key_length && + !(memcmp(share->table_cache_key.str, key, key_length))) + { + result= share; + break; + } + } + + if (locked) + { + DBUG_ASSERT(m_locked); + unlock_tables(); + } + + DBUG_RETURN(result); +} + + +/* + Find a temporary table specified by TABLE_LIST instance in the open table + list and prepare its TABLE instance for use. If + + This function tries to resolve this table in the list of temporary tables + of this thread. Temporary tables are thread-local and "shadow" base + tables with the same name. + + @note In most cases one should use Temporary_tables::open_tables() instead + of this call. + + @note One should finalize process of opening temporary table for table + list element by calling open_and_process_table(). This function + is responsible for table version checking and handling of merge + tables. + + @note We used to check global_read_lock before opening temporary tables. + However, that limitation was artificial and is removed now. + + @param tl [IN] TABLE_LIST + + @return Error status. + @retval false On success. If a temporary table exists for the given + key, tl->table is set. + @retval TRUE On error. my_error() has been called.
letter case is a bit weird, 'false', but 'TRUE' :)
Fixed.
+*/ +bool Temporary_tables::open_table(TABLE_LIST *tl) +{ + DBUG_ENTER("Temporary_tables::open_table"); + + TMP_TABLE_SHARE *share; + TABLE *table= NULL; + bool locked; + + /* + Code in open_table() assumes that TABLE_LIST::table can be non-zero only + for pre-opened temporary tables. + */ + DBUG_ASSERT(tl->table == NULL); + + /* + This function should not be called for cases when derived or I_S + tables can be met since table list elements for such tables can + have invalid db or table name. + Instead Temporary_tables::open_tables() should be used. + */ + DBUG_ASSERT(!tl->derived && !tl->schema_table); + + if (tl->open_type == OT_BASE_ONLY || is_empty(true)) + { + DBUG_PRINT("info", ("skip_temporary is set or no temporary tables")); + DBUG_RETURN(false); + } + + if (wait_for_prior_commit())
this wasn't in the original code
IIRC, I added them to fix some hanging/failing parallel replication tests, but now since the approach of storing temporary TABLE_SHARE and TABLE objects has changed, I will rerun the tests to see how they behave.
+ { + DBUG_RETURN(true); + } + + locked= lock_tables();
neither was this
Removed.
+ + /* + First check if there is a reusable open table available in the + open table list. + */ + if (find_and_use_table(tl, &table)) + { + if (locked) + { + DBUG_ASSERT(m_locked); + unlock_tables(); + } + DBUG_RETURN(true); /* Error */ + } + + /* + No reusable table was found. We will have to open a new instance. + */ + if (!table && (share= find_table_share(tl))) + { + table= open_table(share, tl->get_table_name(), true); + } + + if (locked) + { + DBUG_ASSERT(m_locked); + unlock_tables(); + } + + if (!table) + { + if (tl->open_type == OT_TEMPORARY_ONLY && + tl->open_strategy == TABLE_LIST::OPEN_NORMAL) + { + my_error(ER_NO_SUCH_TABLE, MYF(0), tl->db, tl->table_name); + DBUG_RETURN(true); + } + DBUG_RETURN(false); + } + +#ifdef WITH_PARTITION_STORAGE_ENGINE + if (tl->partition_names) + { + /* Partitioned temporary tables is not supported. */ + DBUG_ASSERT(!table->part_info); + my_error(ER_PARTITION_CLAUSE_ON_NONPARTITIONED, MYF(0)); + DBUG_RETURN(true); + } +#endif + + table->query_id= m_thd->query_id; + m_thd->thread_specific_used= true; + + /* It is neither a derived table nor non-updatable view. */ + tl->updatable= true; + tl->table= table; + + table->init(m_thd, tl); + + DBUG_PRINT("info", ("Using temporary table")); + DBUG_RETURN(false); +} + + +/* + Pre-open temporary tables corresponding to table list elements. + + @note One should finalize process of opening temporary tables + by calling open_tables(). This function is responsible + for table version checking and handling of merge tables.
what kind of version checking can there be for temporary tables?
Perhaps its related to TDC? But I do not understand why will it be needed for temporary tables.
+
+ @param tl [IN] TABLE_LIST + + @return false On success. If a temporary table exists + for the given element, tl->table is set. + true On error. my_error() has been called. +*/ +bool Temporary_tables::open_tables(TABLE_LIST *tl) +{ + DBUG_ENTER("Temporary_tables::open_tables"); + + TABLE_LIST *first_not_own= m_thd->lex->first_not_own_table(); + + for (TABLE_LIST *table= tl; table && table != first_not_own; + table= table->next_global) + { + if (table->derived || table->schema_table) + { + /* + Derived and I_S tables will be handled by a later call to open_tables(). + */ + continue; + } + + if ((m_thd->temporary_tables.open_table(table)))
eh? couldn't you simply do
if (open_table(table))
? why do you need this "m_thd->temporary_tables." ?
Sorry, my bad. Fixed.
+ { + DBUG_RETURN(true); + } + } + + DBUG_RETURN(false); +} + + +/* + Close all temporary tables created by 'CREATE TEMPORARY TABLE' for thread + creates one DROP TEMPORARY TABLE binlog event for each pseudo-thread. + + Temporary tables created in a sql slave is closed by + Relay_log_info::close_temporary_tables(). + + @return false Success + true Failure +*/ +bool Temporary_tables::close_tables() +{ + DBUG_ENTER("Temporary_tables::close_tables"); + + TMP_TABLE_SHARE *share; + TMP_TABLE_SHARE *share_next; + TABLE *table, *table_next; + bool error= false; + + if (!m_table_shares) + { + DBUG_RETURN(false); + } + DBUG_ASSERT(!m_thd->rgi_slave); + + /* + Ensure we don't have open HANDLERs for tables we are about to close. + This is necessary when Temporary_tables::close_tables() is called as + part of execution of BINLOG statement (e.g. for format description event). + */ + mysql_ha_rm_temporary_tables(m_thd); + + /* Close all open temporary tables. */ + for (TMP_TABLE_SHARE *share= m_table_shares; share; share= share->next) + { + /* Traverse the table list. */ + table= share->table; + while (table) + { + table_next= table->next; + free_table(table); + table= table_next; + } + } + + // Write DROP TEMPORARY TABLE query log events to binary log. + if (mysql_bin_log.is_open()) + { + error= log_events_and_free_shares(); + } + else + { + share= m_table_shares; + while (share) + { + share_next= share->next; + free_table_share(share, true); + share= share_next; + } + } + reset(); + + DBUG_RETURN(error); +} + + +/* + Rename a temporary table. + + @param table [IN] Table handle + @param db [IN] New schema name + @param table_name [IN] New table name + + @return false Success + true Error +*/ +bool Temporary_tables::rename_table(TABLE *table, + const char *db, + const char *table_name) +{ + DBUG_ENTER("Temporary_tables::rename_table"); + + char *key; + uint key_length; + + TMP_TABLE_SHARE *share= static_cast<TMP_TABLE_SHARE *>(table->s); + + if (!(key= (char *) alloc_root(&share->mem_root, MAX_DBKEY_LENGTH))) + { + DBUG_RETURN(true); + } + + /* + Temporary tables are renamed by simply changing their table definition key. + */ + key_length= create_table_def_key(key, db, table_name); + share->set_table_cache_key(key, key_length); + + DBUG_RETURN(false); +} + + +/* + Drop a temporary table. + + Try to locate the table in the list of open temporary tables. + If the table is found: + - If the table is locked with LOCK TABLES or by prelocking, + unlock it and remove it from the list of locked tables + (THD::lock). Currently only transactional temporary tables + are locked. + - Close the temporary table, remove its .FRM. + - Remove the table share from the list of temporary table shares. + + This function is used to drop user temporary tables, as well as + internal tables created in CREATE TEMPORARY TABLE ... SELECT + or ALTER TABLE. + + @param table [IN] Temporary table to be deleted + @param is_trans [OUT] Is set to the type of the table: + transactional (e.g. innodb) as true or + non-transactional (e.g. myisam) as false. + @paral delete_table [IN] Whether to delete the table files. + + @return false Table was dropped + true Error +*/ +bool Temporary_tables::drop_table(TABLE *table, + bool *is_trans, + bool delete_table) +{ + DBUG_ENTER("Temporary_tables::drop_table"); + + TMP_TABLE_SHARE *share; + TABLE *tab; + TABLE *tab_next; + bool result, locked; + + DBUG_ASSERT(table); + DBUG_ASSERT(table->query_id); + DBUG_PRINT("tmptable", ("Dropping table: '%s'.'%s'", + table->s->db.str, table->s->table_name.str)); + + if (wait_for_prior_commit()) + { + DBUG_RETURN(true); + }
old code didn't seem to have that. Why do you?
Removed.
+ + locked= lock_tables(); + + share= static_cast<TMP_TABLE_SHARE *>(table->s);
suggestion: have a function:
static inline TMP_TABLE_SHARE *tmp_table_share(TABLE *t) { DBUG_ASSERT(t->s->tmp_table); return static_cast<TMP_TABLE_SHARE *>(table->s); }
That's a good idea. Done.
+ + /* Table might be in use by some outer statement. */ + for (tab= share->table; tab; tab= tab->next) + { + if (tab != table && tab->query_id != 0) + { + /* Found a table instance in use. This table cannot be be dropped. */ + my_error(ER_CANT_REOPEN_TABLE, MYF(0), table->alias.c_ptr()); + result= true; + goto end; + } + } + + if (is_trans) + *is_trans= table->file->has_transactions(); + + /* + Iterate over the list of open tables and close all tables referencing the + same table share.
the comment sounds a bit odd now, when all open tables are in the list inside the share.
Fixed.
+ */ + tab= share->table; + while (tab) + { + tab_next= tab->next; + if ((result= free_table(tab))) goto end; + tab= tab_next; + } + + result= free_table_share(share, delete_table); + +end: + if (locked) + { + DBUG_ASSERT(m_locked); + unlock_tables(); + } + + DBUG_RETURN(result); +} + + +/** + Delete the temporary table files. + + @param base [IN] Handlerton for table to be deleted. + @param path [IN] Path to the table to be deleted (i.e. path + to its .frm without an extension). + + @return false Success + true Error +*/ +bool Temporary_tables::remove_table(handlerton *base, const char *path) +{ + DBUG_ENTER("Temporary_tables::remove_table"); + + bool error= false; + handler *file; + char frm_path[FN_REFLEN + 1]; + + strxnmov(frm_path, sizeof(frm_path) - 1, path, reg_ext, NullS); + if (mysql_file_delete(key_file_frm, frm_path, MYF(0))) + { + error= true; + } + file= get_new_handler((TABLE_SHARE*) 0, current_thd->mem_root, base); + if (file && file->ha_delete_table(path)) + { + error= true; + sql_print_warning("Could not remove temporary table: '%s', error: %d", + path, my_errno); + } + + delete file; + DBUG_RETURN(error); +} + + +/* + Mark all temporary tables which were used by the current statement or + sub-statement as free for reuse, but only if the query_id can be cleared. + + @remark For temp tables associated with a open SQL HANDLER the query_id + is not reset until the HANDLER is closed. +*/ +void Temporary_tables::mark_tables_as_free_for_reuse() +{ + DBUG_ENTER("Temporary_tables::mark_tables_as_free_for_reuse"); + + bool locked; + + if (m_thd->query_id == 0) + { + /* + Thread has not executed any statement and has not used any + temporary tables. + */ + DBUG_VOID_RETURN; + } + + locked= lock_tables(); + + for (TMP_TABLE_SHARE *share= m_table_shares; share; share= share->next) + { + for (TABLE *table= share->table; table; table= table->next) + { + if ((table->query_id == m_thd->query_id) && !table->open_by_handler) + { + mark_table_as_free_for_reuse(table); + } + } + } + + if (locked) + { + DBUG_ASSERT(m_locked); + unlock_tables(); + } + + DBUG_VOID_RETURN; +} + + +/* + Reset a single temporary table. Effectively this "closes" one temporary + table in a session. + + @param table Temporary table +*/ +void Temporary_tables::mark_table_as_free_for_reuse(TABLE *table) +{ + DBUG_ENTER("Temporary_tables::mark_table_as_free_for_reuse"); + + DBUG_ASSERT(table->s->tmp_table); + + table->query_id= 0; + table->file->ha_reset(); + + /* Detach temporary MERGE children from temporary parent. */ + DBUG_ASSERT(table->file); + table->file->extra(HA_EXTRA_DETACH_CHILDREN); + + /* + Reset temporary table lock type to it's default value (TL_WRITE). + + Statements such as INSERT INTO .. SELECT FROM tmp, CREATE TABLE + .. SELECT FROM tmp and UPDATE may under some circumstances modify + the lock type of the tables participating in the statement. This + isn't a problem for non-temporary tables since their lock type is + reset at every open, but the same does not occur for temporary + tables for historical reasons. + + Furthermore, the lock type of temporary tables is not really that + important because they can only be used by one query at a time. + Nonetheless, it's safer from a maintenance point of view to reset + the lock type of this singleton TABLE object as to not cause problems + when the table is reused. + + Even under LOCK TABLES mode its okay to reset the lock type as + LOCK TABLES is allowed (but ignored) for a temporary table. + */ + table->reginfo.lock_type= TL_WRITE; + DBUG_VOID_RETURN; +} + + +TMP_TABLE_SHARE *Temporary_tables::unlink_table_share(TABLE_SHARE *share) +{ + DBUG_ENTER("Temporary_tables::unlink_table"); + TMP_TABLE_SHARE *tmp_table_share; + + lock_tables(); + tmp_table_share= static_cast<TMP_TABLE_SHARE *>(share); + unlink<TMP_TABLE_SHARE>(&m_table_shares, tmp_table_share); + unlock_tables(); + + DBUG_RETURN(tmp_table_share); +} + + +void Temporary_tables::relink_table_share(TMP_TABLE_SHARE *share) +{ + DBUG_ENTER("Temporary_tables::relink_table"); + + lock_tables(); + link<TMP_TABLE_SHARE>(&m_table_shares, share); + unlock_tables(); + + DBUG_VOID_RETURN; +} + + +/* + Create a table definition key. + + @param key [OUT] Buffer for the key to be created (must + be of size MAX_DBKRY_LENGTH) + @param db [IN] Database name + @param table_name [IN] Table name + + @return Key length. + + @note + The table key is create from: + db + \0 + table_name + \0 + + Additionally, we add the following to make each temporary table unique on + the slave. + + 4 bytes of master thread id + 4 bytes of pseudo thread id +*/ + +uint Temporary_tables::create_table_def_key(char *key, const char *db, + const char *table_name) +{ + DBUG_ENTER("Temporary_tables::create_table_def_key"); + + uint key_length; + + key_length= tdc_create_key(key, db, table_name); + int4store(key + key_length, m_thd->variables.server_id); + int4store(key + key_length + 4, m_thd->variables.pseudo_thread_id); + key_length += TMP_TABLE_KEY_EXTRA; + + DBUG_RETURN(key_length); +} + + +/* + Create a temporary table. + + @param hton [IN] Handlerton + @param frm [IN] Binary frm image + @param path [IN] File path (without extension) + @param db [IN] Schema name + @param table_name [IN] Table name + + @return Success A pointer to table share object + Failure NULL +*/ +TMP_TABLE_SHARE *Temporary_tables::create_table(handlerton *hton, + LEX_CUSTRING *frm, + const char *path, + const char *db, + const char *table_name) +{ + DBUG_ENTER("Temporary_tables::create_table"); + + TMP_TABLE_SHARE *share; + char key_cache[MAX_DBKEY_LENGTH]; + char *saved_key_cache; + char *tmp_path; + uint key_length; + int res; + + if (wait_for_prior_commit())
you're doing it in almost every method. And when one method calls another, you're doing it twice. Or thrice. Isn't is too much? (I don't know)
I added these additional waits to fix some failing replication tests. But, whit new TMP_TABLE_SHARE approach, this could have changed. I have now reverted them to the original state and will run rpl test to assure if we do not need additional waits.
+ { + DBUG_RETURN(NULL); + } + + /* Create the table definition key for the temporary table. */ + key_length= create_table_def_key(key_cache, db, table_name); + + if (!(share= (TMP_TABLE_SHARE *) my_malloc(sizeof(TMP_TABLE_SHARE) + + strlen(path) + + 1 + key_length, MYF(MY_WME)))) + { + DBUG_RETURN(NULL); /* Out of memory */ + } + + tmp_path= (char *)(share + 1); + saved_key_cache= strmov(tmp_path, path) + 1; + memcpy(saved_key_cache, key_cache, key_length); + + init_tmp_table_share(m_thd, share, saved_key_cache, key_length, + strend(saved_key_cache) + 1, tmp_path); + + share->table= 0; + share->db_plugin= ha_lock_engine(m_thd, hton); + + /* + Prefer using frm image over file. The image might not be available in + ALTER TABLE, when the discovering engine took over the ownership (see + TABLE::read_frm_image). + */ + res= (frm->str) + ? share->init_from_binary_frm_image(m_thd, false, frm->str, frm->length) + : open_table_def(m_thd, share, GTS_TABLE | GTS_USE_DISCOVERY); + + if (res) + { + /* + No need to lock share->mutex as this is not needed for temporary tables. + */ + ::free_table_share(share); + my_free(share); + DBUG_RETURN(NULL); + } + + share->m_psi= PSI_CALL_get_table_share(true, share); + + /* Add share to the head of the temporary table share list. */ + link<TMP_TABLE_SHARE>(&m_table_shares, share); + + DBUG_RETURN(share); +} + + +/* + Find a table with the specified key. + + @param key [IN] Key + @param key_length [IN] Key length + @param state [IN] Open table state to look for + + @return Success Pointer to the table instance. + Failure NULL +*/ +TABLE *Temporary_tables::find_table(const char *key, uint key_length, + Table_state state) +{ + DBUG_ENTER("Temporary_tables::find_table"); + + for (TMP_TABLE_SHARE *share= m_table_shares; share; share= share->next) + { + if (share->table_cache_key.length == key_length && + !(memcmp(share->table_cache_key.str, key, key_length))) + { + /* A matching TMP_TABLE_SHARE is found. */ + for (TABLE *table= share->table; table; table= table->next) + { + switch (state) + { + case TABLE_IN_USE: + if (table->query_id > 0) DBUG_RETURN(table); + break; + case TABLE_NOT_IN_USE: + if (table->query_id == 0) DBUG_RETURN(table); + break; + case TABLE_ANY: + DBUG_RETURN(table); + default: /* Invalid */ + DBUG_ASSERT(0); + DBUG_RETURN(NULL); + } + } + } + } + + DBUG_RETURN(NULL); +} + + +/* + Find a reusable table in the open table list using the specified TABLE_LIST. + + @param tl [IN] Table list + @param out_table [OUT] Pointer to the requested TABLE object + + @return Success false + Failure true +*/ +bool Temporary_tables::find_and_use_table(const TABLE_LIST *tl, + TABLE **out_table) +{ + DBUG_ENTER("Temporary_tables::find_and_use_table"); + + char key[MAX_DBKEY_LENGTH]; + uint key_length; + bool result; + + key_length= create_table_def_key(key, tl->get_db_name(), + tl->get_table_name()); + result= use_table(find_table(key, key_length, TABLE_NOT_IN_USE), out_table); + + DBUG_RETURN(result); +} + + +/* + Open a table from the specified TABLE_SHARE with the given alias. + + @param share [IN] Table share + @param alias [IN] Table alias + @param open_in_engine [IN] Whether open table in SE + + @return Success A pointer to table object + Failure NULL +*/ +TABLE *Temporary_tables::open_table(TMP_TABLE_SHARE *share, + const char *alias, + bool open_in_engine) +{ + DBUG_ENTER("Temporary_tables::open_table"); + + TABLE *table; + + if (wait_for_prior_commit()) + { + DBUG_RETURN(NULL); + } + + if (!(table= (TABLE *) my_malloc(sizeof(TABLE), MYF(MY_WME)))) + { + DBUG_RETURN(NULL); /* Out of memory */ + } + + if (open_table_from_share(m_thd, share, alias, + (open_in_engine) ? + (uint) (HA_OPEN_KEYFILE | HA_OPEN_RNDFILE | + HA_GET_INDEX) : 0, + (uint) (READ_KEYINFO | COMPUTE_TYPES | + EXTRA_RECORD), + ha_open_options, + table, + open_in_engine ? false : true)) + { + my_free(table); + DBUG_RETURN(NULL); + } + + table->reginfo.lock_type= TL_WRITE; /* Simulate locked */ + table->grant.privilege= TMP_TABLE_ACLS; + share->tmp_table= (table->file->has_transactions() ? + TRANSACTIONAL_TMP_TABLE : NON_TRANSACTIONAL_TMP_TABLE); + + table->pos_in_table_list= 0; + table->query_id= m_thd->query_id; + + /* Add table to the head of table list. */ + link<TABLE>(&share->table, table);
I don't know if that's correct. You use TABLE::next and TABLE::prev pointers to link all tables of a given TMP_TABLE_SHARE into a list. But these pointers are used to link all tables into a THD::open_tables list (and may be for other purposes too?). In fact, TABLE has dedicated pointers for this list that you want:
/** Links for the list of all TABLE objects for this share. Declared as private to avoid direct manipulation with those objects. One should use methods of I_P_List template instead. */ TABLE *share_all_next, **share_all_prev;
Right, There is now a new list, TMP_TABLE_SHARE::all_tmp_tables, that uses these pointers to maintain a list of TABLEs for a particular TMP_TABLE_SHARE.
+ + /* Increment Slave_open_temp_table_definitions status variable count. */ + if (m_thd->rgi_slave) + { + thread_safe_increment32(&slave_open_temp_tables); + } + + DBUG_PRINT("tmptable", ("Opened table: '%s'.'%s' 0x%lx", table->s->db.str, + table->s->table_name.str, (long) table)); + DBUG_RETURN(table); +} + +
function comment?
Added.
+bool Temporary_tables::use_table(TABLE *table, TABLE **out_table) +{ + DBUG_ENTER("Temporary_tables::use_table"); + + *out_table= table; + if (!table) + DBUG_RETURN(false); +
old code had a block here about "Temporary tables are not safe for parallel replication". it's in wait_for_prior_commit now, but you aren't calling it here. why?
I was calling it in open_table() before calling find_and_use_table(). Restored it now.
+ /*
+ We need to set the THD as it may be different in case of + parallel replication + */ + if (table->in_use != m_thd) + { + table->in_use= m_thd; + } + + DBUG_RETURN(false); +} + + +/* + Close a temporary table. + + @param table [IN] Table handle + + @return false Success + true Error +*/ +bool Temporary_tables::close_table(TABLE *table) +{ + DBUG_ENTER("Temporary_tables::close_table"); + + DBUG_PRINT("tmptable", ("closing table: '%s'.'%s' 0x%lx alias: '%s'", + table->s->db.str, table->s->table_name.str, + (long) table, table->alias.c_ptr())); + + free_io_cache(table);
don't forget to remove free_io_cache() calls when rebasing your work on top of the latest 10.2 (they were removed from close_temporary() in 260dd476b05 commit)
Okay
+ closefrm(table, false); + my_free(table); + + /* Decrement Slave_open_temp_table_definitions status variable count. */ + if (m_thd->rgi_slave) + { + thread_safe_decrement32(&slave_open_temp_tables); + } + + DBUG_RETURN(false); +} + + +bool Temporary_tables::wait_for_prior_commit() +{ + DBUG_ENTER("Temporary_tables::wait_for_prior_commit"); + + /* + Temporary tables are not safe for parallel replication. They were + designed to be visible to one thread only, so have no table locking. + Thus there is no protection against two conflicting transactions + committing in parallel and things like that. + + So for now, anything that uses temporary tables will be serialised + with anything before it, when using parallel replication. + + TODO: We might be able to introduce a reference count or something + on temp tables, and have slave worker threads wait for it to reach + zero before being allowed to use the temp table. Might not be worth + it though, as statement-based replication using temporary tables is + in any case rather fragile. + */ + if (m_thd->rgi_slave && + m_thd->rgi_slave->rli->save_temp_table_shares && + m_thd->rgi_slave->is_parallel_exec && + m_thd->wait_for_prior_commit()) + DBUG_RETURN(true); + + DBUG_RETURN(false); +} + + +/* + Write query log events with "DROP TEMPORARY TABLES .." for each pseudo + thread to the binary log. + + @return false Success + true Error +*/ +bool Temporary_tables::log_events_and_free_shares() +{ + DBUG_ENTER("Temporary_tables::log_events_and_free_shares"); + + DBUG_ASSERT(!m_thd->rgi_slave); + + TMP_TABLE_SHARE *share; + TMP_TABLE_SHARE *next; + TMP_TABLE_SHARE *prev_share; + // Assume thd->variables.option_bits has OPTION_QUOTE_SHOW_CREATE. + bool was_quote_show= true; + bool error= false; + bool found_user_tables= false; + // Better add "IF EXISTS" in case a RESET MASTER has been done. + const char stub[]= "DROP /*!40005 TEMPORARY */ TABLE IF EXISTS "; + char buf[FN_REFLEN]; + + String s_query(buf, sizeof(buf), system_charset_info); + s_query.copy(stub, sizeof(stub) - 1, system_charset_info); + + /* + Insertion sort of temporary tables by pseudo_thread_id to build ordered + list of sublists of equal pseudo_thread_id. + */ + + for (prev_share= m_table_shares, share= prev_share->next; + share; + prev_share= share, share= share->next) + { + TMP_TABLE_SHARE *prev_sorted; /* Same as for prev_share */ + TMP_TABLE_SHARE *sorted; + + if (IS_USER_TABLE(share)) + { + if (!found_user_tables) + found_user_tables= true; + + for (prev_sorted= NULL, sorted= m_table_shares; + sorted != share; + prev_sorted= sorted, sorted= sorted->next) + { + if (!IS_USER_TABLE(sorted) || + tmpkeyval(sorted) > tmpkeyval(share)) + { + /* + Move into the sorted part of the list from the unsorted. + */ + prev_share->next= share->next; + share->next= sorted; + if (prev_sorted) + { + prev_sorted->next= share; + } + else + { + m_table_shares= share; + } + share= prev_share; + break; + } + } + } + } + + /* + We always quote db & table names. + */ + if (found_user_tables && + !(was_quote_show= MY_TEST(m_thd->variables.option_bits & + OPTION_QUOTE_SHOW_CREATE))) + { + m_thd->variables.option_bits |= OPTION_QUOTE_SHOW_CREATE; + } + + /* + Scan sorted temporary tables to generate sequence of DROP. + */ + for (share= m_table_shares; share; share= next) + { + if (IS_USER_TABLE(share)) + { + bool save_thread_specific_used= m_thd->thread_specific_used; + my_thread_id save_pseudo_thread_id= m_thd->variables.pseudo_thread_id; + char db_buf[FN_REFLEN]; + String db(db_buf, sizeof(db_buf), system_charset_info); + + /* + Set pseudo_thread_id to be that of the processed table. + */ + m_thd->variables.pseudo_thread_id= tmpkeyval(share); + + db.copy(share->db.str, share->db.length, system_charset_info); + /* + Reset s_query() if changed by previous loop. + */ + s_query.length(sizeof(stub) - 1); + + /* + Loop forward through all tables that belong to a common database + within the sublist of common pseudo_thread_id to create single + DROP query. + */ + for (; + share && + IS_USER_TABLE(share) && + tmpkeyval(share) == m_thd->variables.pseudo_thread_id && + share->db.length == db.length() && + memcmp(share->db.str, db.ptr(), db.length()) == 0; + share= next) + { + /* + We are going to add ` around the table names and possible more + due to special characters. + */ + append_identifier(m_thd, &s_query, share->table_name.str, + share->table_name.length); + s_query.append(','); + next= share->next; + remove_table(share->db_type(), share->path.str); + ::free_table_share(share); + my_free(share); + } + + m_thd->clear_error(); + CHARSET_INFO *cs_save= m_thd->variables.character_set_client; + m_thd->variables.character_set_client= system_charset_info; + m_thd->thread_specific_used= true; + + Query_log_event qinfo(m_thd, s_query.ptr(), + s_query.length() - 1 /* to remove trailing ',' */, + false, true, false, 0); + qinfo.db= db.ptr(); + qinfo.db_len= db.length(); + m_thd->variables.character_set_client= cs_save; + + m_thd->get_stmt_da()->set_overwrite_status(true); + if ((error= (mysql_bin_log.write(&qinfo) || error))) + { + /* + If we're here following THD::cleanup, thence the connection + has been closed already. So lets print a message to the + error log instead of pushing yet another error into the + stmt_da. + + Also, we keep the error flag so that we propagate the error + up in the stack. This way, if we're the SQL thread we notice + that Temporary_tables::close_tables failed. (Actually, the SQL + thread only calls Temporary_tables::close_tables while applying + old Start_log_event_v3 events.) + */ + sql_print_error("Failed to write the DROP statement for " + "temporary tables to binary log"); + } + + m_thd->get_stmt_da()->set_overwrite_status(false); + m_thd->variables.pseudo_thread_id= save_pseudo_thread_id; + m_thd->thread_specific_used= save_thread_specific_used; + } + else + { + next= share->next; + free_table_share(share, true); + } + } + + if (!was_quote_show) + { + /* + Restore option. + */ + m_thd->variables.option_bits&= ~OPTION_QUOTE_SHOW_CREATE; + } + + DBUG_RETURN(error); +} + +/* + Delete the files and free the specified table share. +*/ +bool Temporary_tables::free_table_share(TMP_TABLE_SHARE *share, + bool delete_table) +{ + DBUG_ENTER("Temporary_tables::free_table_share"); + + bool result= false; + + /* Delete the share from table share list */ + unlink<TMP_TABLE_SHARE>(&m_table_shares, share); + + if (delete_table) + { + result= remove_table(share->db_type(), share->path.str); + } + + ::free_table_share(share); + my_free(share); + + DBUG_RETURN(result); +} + + +/* + Free the specified table object. +*/ +bool Temporary_tables::free_table(TABLE *table) +{ + DBUG_ENTER("Temporary_tables::free_table"); + + bool result= false; + + /* Delete the table from table list */ + unlink<TABLE>(&static_cast<TMP_TABLE_SHARE *>(table->s)->table, table); + + /* + If LOCK TABLES list is not empty and contains this table, unlock the table + and remove the table from this list. + */ + mysql_lock_remove(m_thd, m_thd->lock, table); + + result= close_table(table); + + DBUG_RETURN(result); +} + + +bool Temporary_tables::lock_tables() +{ + /* Do not proceed if a lock has already been taken. */ + if (m_locked) + { + return false; + } + + rpl_group_info *rgi_slave= m_thd->rgi_slave; + if (rgi_slave) + { + mysql_mutex_lock(&rgi_slave->rli->data_lock); + m_table_shares= rgi_slave->rli->save_temp_table_shares; + m_locked= true; + } + return m_locked; +} + + +void Temporary_tables::unlock_tables() +{ + if (!m_locked) + { + return; + } + + rpl_group_info *rgi_slave= m_thd->rgi_slave; + if (rgi_slave) + { + rgi_slave->rli->save_temp_table_shares= m_table_shares; + mysql_mutex_unlock(&rgi_slave->rli->data_lock); + m_locked= false; + /* + Temporary tables are shared with other by sql execution threads. + As a safety measure, clear the pointer to the common area. + */ + reset(); + } + return; +} + diff --git a/sql/temporary_tables.h b/sql/temporary_tables.h new file mode 100644 index 0000000..5a5ed0d --- /dev/null +++ b/sql/temporary_tables.h @@ -0,0 +1,150 @@ +#ifndef TEMPORARY_TABLES_H +#define TEMPORARY_TABLES_H +/* + Copyright (c) 2016 MariaDB Corporation + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +*/ + +#define TMP_TABLE_KEY_EXTRA 8 + +struct TMP_TABLE_SHARE: public TABLE_SHARE +{ + /* List of TABLE instances created out of this TABLE_SHARE. */ + TABLE *table;
Hmm, if you will use TABLE::share_all_prev and TABLE::share_all_next, as I've written above, you should also use TDC_element that keeps the list of TABLE's per TABLE_SHARE. And also it has next/prev pointers to link all TABLE_SHARE's in a list... Perhaps you won't need TMP_TABLE_SHARE after all? Could you talk with Svoj about it, please? perhaps you can simply reuse existing TABLE_SHARE code for linking TABLE_SHARE's into a list and linking TABLE's into a per-TABLE_SHARE list? TDC_element is a bit of an overkill for temporary tables, we can either live with that or extract those bits that you need into a "mini-TDC_element" ? Dunno, best to discuss it with Svoj.
I discussed it with Svoj. We originally thought that all_tables, next and prev members can be moved from TDC_element to TABLE_SHARE and can be reused to link temporary table shares and table instances. But, since share can be freed concurrently, accessing all_tables via share in TDC code can lead to segfault (TDC_element->share->all_tables). So, we decided not to move them and stick to the TMP_TABLE_SHARE approach with additional members for next/prev shares and all_temp_tables list using share_all_next & share_all_prev.
+ + /* Pointers to access TMP_TABLE_SHARE instances. */ + TMP_TABLE_SHARE *next; + TMP_TABLE_SHARE *prev; +}; + + +class Temporary_tables +{ +public: + Temporary_tables() : m_thd(0), m_table_shares(0), m_locked(false) + {} + bool init(THD *thd); + bool is_empty(bool check_slave); + bool reset(); + + TABLE *create_and_open_table(handlerton *hton, LEX_CUSTRING *frm, + const char *path, const char *db, + const char *table_name, bool open_in_engine, + bool *created); + + TABLE *find_table(const char *db, const char *table_name); + TABLE *find_table(const TABLE_LIST *tl); + TABLE *find_table_reduced_key_length(const char *key, uint key_length); + + TMP_TABLE_SHARE *find_table_share(const char *db, const char *table_name); + TMP_TABLE_SHARE *find_table_share(const TABLE_LIST *tl); + TMP_TABLE_SHARE *find_table_share(const char *key, uint key_length); + + bool open_table(TABLE_LIST *tl); + bool open_tables(TABLE_LIST *tl); + + bool close_tables(); + bool rename_table(TABLE *table, const char *db, const char *table_name); + bool drop_table(TABLE *table, bool *is_trans, bool delete_table); + bool remove_table(handlerton *hton, const char *path); + void mark_tables_as_free_for_reuse(); + void mark_table_as_free_for_reuse(TABLE *table); + + TMP_TABLE_SHARE *unlink_table_share(TABLE_SHARE *share); + void relink_table_share(TMP_TABLE_SHARE *share); + +private: + /* THD handler */ + THD *m_thd;
already discussed
+ + /* List of temporary table shares */ + TMP_TABLE_SHARE *m_table_shares; + + /* Whether a lock has been acquired. */ + bool m_locked; + + /* Opened table states. */ + enum Table_state { + TABLE_IN_USE, + TABLE_NOT_IN_USE, + TABLE_ANY + }; + + uint create_table_def_key(char *key, + const char *db, + const char *table_name); + + TMP_TABLE_SHARE *create_table(handlerton *hton, + LEX_CUSTRING *frm, + const char *path, + const char *db, + const char *table_name); + + TABLE *find_table(const char *key, uint key_length, Table_state state); + + bool find_and_use_table(const TABLE_LIST *tl, TABLE **out_table); + + TABLE *open_table(TMP_TABLE_SHARE *share, const char *alias, + bool open_in_engine); + + bool use_table(TABLE *table, TABLE **out_table); + bool close_table(TABLE *table); + bool wait_for_prior_commit(); + bool log_events_and_free_shares(); + + bool free_table_share(TMP_TABLE_SHARE *share, bool delete_table); + bool free_table(TABLE *table); + + bool lock_tables(); + void unlock_tables(); + + /* List operations */ + template <class T> + void link(T **list, T *element) + { + element->next= *list; + if (element->next) + element->next->prev= element; + *list= element; + (*list)->prev= 0; + } + + template <class T> + void unlink(T **list, T *element) + { + if (element->prev) + { + element->prev->next= element->next; + if (element->prev->next) + element->next->prev= element->prev; + } + else + { + DBUG_ASSERT(element == *list); + + *list= element->next; + if (*list) + element->next->prev= 0; + } + }
No need to reinvent the wheel, you can use I_P_List for that.
I agree, done.
+ + uint tmpkeyval(TMP_TABLE_SHARE *share) + { + return uint4korr(share->table_cache_key.str + + share->table_cache_key.length - 4); + } +}; + +#endif /* TEMPORARY_TABLES_H */
Best, Nirbhay
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Nirbhay! On May 26, Nirbhay Choubey wrote:
+/* + Create a temporary table, open it and return the TABLE handle. + + @param hton [IN] Handlerton + @param frm [IN] Binary frm image + @param path [IN] File path (without extension) + @param db [IN] Schema name + @param table_name [IN] Table name + @param open_in_engine [IN] Whether open table in SE + @param created [OUT] Whether table was created?
can it ever happen for *create==true but a return value is NULL? or the other way around? how?
Yes, but not the other way around. For instance, if the table was created successfully, but the subsequent malloc failed while an attempt was made to allocate TABLE object later in open_temporary_table().
But do you need to handle this practically impossible case specially? I'd say - if that happens, just drop the table (or even don't drop it, it'll be removed on disconnect anyway) and pretend the whole never happened (return NULL). I mean, you don't need a separate *created pointer.
+ if (wait_for_prior_commit())
you're doing it in almost every method. And when one method calls another, you're doing it twice. Or thrice. Isn't is too much? (I don't know)
I added these additional waits to fix some failing replication tests. But, whit new TMP_TABLE_SHARE approach, this could have changed. I have now reverted them to the original state and will run rpl test to assure if we do not need additional waits.
And?
+ free_io_cache(table);
don't forget to remove free_io_cache() calls when rebasing your work on top of the latest 10.2 (they were removed from close_temporary() in 260dd476b05 commit)
Okay
Unless you've rebased already, it makes sense to do it now - as you're apparently almost ready to push Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Sergei Golubchik <serg@mariadb.org> writes:
On May 26, Nirbhay Choubey wrote:
+ if (wait_for_prior_commit())
Please call this method tmptable_wait_for_prior_commit() or something like that. To avoid confusion with THD::wait_for_prior_commit().
you're doing it in almost every method. And when one method calls another, you're doing it twice. Or thrice. Isn't is too much? (I don't know)
I added these additional waits to fix some failing replication tests. But, whit new TMP_TABLE_SHARE approach, this could have changed. I have now reverted them to the original state and will run rpl test to assure if we do not need additional waits.
That won't mean much. Temporary table handling in statement-based replication is very nasty code, and we surely do not have full test coverage of possible issues. And bugs only turn up as rare race conditions that are hard to reproduce. So you will have to make sure you understand in detail exactly where wait_for_prior_commit() (and possibly other needed code) is needed to ensure statement-based parallel replication will work in all cases (or at least work as well as the non-parallel case). The issue here is that temporary tables have no locking facilities. And the same temporary table can be used by multiple different transactions, which in parallel replication can be run in different threads. There is code that keeps track of a shared list of temporary tables, and shuffles it around between threads as needed (this is also needed for non-parallel replication, since temporary tables might need to be kept across SQL thread destruction and re-creation). See Relay_log_info::save_temporary_tables (as you probably already saw). Maybe Monty knows something about this (ISTR that he wrote that code originally), otherwise careful study of the code is probably the only way. Since there is no locking of temporary tables (at least before your patch?), it would be possible for two worker threads to simultaneously try to use the same temptable, which fails, obviously. This is handled rather crudely in the current code by simply serialising all transactions using temporary tables with all prior transactions. This is what wait_for_prior_commit() does - it waits for all transactions to commit that come before in the replication stream (within that replication domain). Hope this helps, - Kristian.
Hi Kristian! On Fri, Jun 3, 2016 at 7:53 AM, Kristian Nielsen <knielsen@knielsen-hq.org> wrote:
Sergei Golubchik <serg@mariadb.org> writes:
On May 26, Nirbhay Choubey wrote:
+ if (wait_for_prior_commit())
Please call this method tmptable_wait_for_prior_commit() or something like that. To avoid confusion with THD::wait_for_prior_commit().
I no longer have this wrapper and am calling THD::wait_for_prior_commit() directly.
you're doing it in almost every method. And when one method calls another, you're doing it twice. Or thrice. Isn't is too much? (I don't know)
I added these additional waits to fix some failing replication tests. But, whit new TMP_TABLE_SHARE approach, this could have changed. I have now reverted them to the original state and will run rpl test to assure if we do not need additional waits.
That won't mean much. Temporary table handling in statement-based replication is very nasty code, and we surely do not have full test coverage of possible issues. And bugs only turn up as rare race conditions that are hard to reproduce.
I kind of felt that. :)
So you will have to make sure you understand in detail exactly where wait_for_prior_commit() (and possibly other needed code) is needed to ensure statement-based parallel replication will work in all cases (or at least work as well as the non-parallel case).
The issue here is that temporary tables have no locking facilities. And the same temporary table can be used by multiple different transactions, which in parallel replication can be run in different threads. There is code that keeps track of a shared list of temporary tables, and shuffles it around between threads as needed (this is also needed for non-parallel replication, since temporary tables might need to be kept across SQL thread destruction and re-creation). See Relay_log_info::save_temporary_tables (as you probably already saw). Maybe Monty knows something about this (ISTR that he wrote that code originally), otherwise careful study of the code is probably the only way.
Since there is no locking of temporary tables (at least before your patch?), it would be possible for two worker threads to simultaneously try to use the same temptable, which fails, obviously. This is handled rather crudely in the current code by simply serialising all transactions using temporary tables with all prior transactions. This is what wait_for_prior_commit() does - it waits for all transactions to commit that come before in the replication stream (within that replication domain).
Right, and as I see it, we wait at the _entry_ point when a thread tries to access/create a temporary table.
Hope this helps,
Thanks for sharing these details. Best, Nirbhay
- Kristian.
Hi Serg! On Fri, Jun 3, 2016 at 7:20 AM, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nirbhay!
On May 26, Nirbhay Choubey wrote:
+/* + Create a temporary table, open it and return the TABLE handle. + + @param hton [IN] Handlerton + @param frm [IN] Binary frm image + @param path [IN] File path (without extension) + @param db [IN] Schema name + @param table_name [IN] Table name + @param open_in_engine [IN] Whether open table in SE + @param created [OUT] Whether table was created?
can it ever happen for *create==true but a return value is NULL? or the other way around? how?
Yes, but not the other way around. For instance, if the table was created successfully, but the subsequent malloc failed while an attempt was made to allocate TABLE object later in open_temporary_table().
But do you need to handle this practically impossible case specially? I'd say - if that happens, just drop the table (or even don't drop it, it'll be removed on disconnect anyway) and pretend the whole never happened (return NULL). I mean, you don't need a separate *created pointer.
I have update the function to remove the share from the list and free it in case it fails to open a table instance.
+ if (wait_for_prior_commit())
you're doing it in almost every method. And when one method calls another, you're doing it twice. Or thrice. Isn't is too much? (I don't know)
I added these additional waits to fix some failing replication tests. But, whit new TMP_TABLE_SHARE approach, this could have changed. I have now reverted them to the original state and will run rpl test to assure if we do not need additional waits.
And?
I found some failing rpl tests. Fixed.
+ free_io_cache(table);
don't forget to remove free_io_cache() calls when rebasing your work on top of the latest 10.2 (they were removed from close_temporary() in 260dd476b05 commit)
Okay
Unless you've rebased already, it makes sense to do it now - as you're apparently almost ready to push
Done. Best, Nirbhay
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
participants (3)
-
Kristian Nielsen
-
Nirbhay Choubey
-
Sergei Golubchik