Hi Serg, On Wed, Mar 30, 2016 at 7:12 AM, Sergei Golubchik <serg@mariadb.org> wrote:

Hi, Nirbhay!

Here's a review of MDEV-5535. Sorry, it took a while - this was a big patch.

The approach is fine, my comments are mostly about details.

Thanks for the review and suggestions. I have tried to address them all, and while doing so, the original patch has changed quite a bit. Please see my comments inline.

On Mar 07, Nirbhay Choubey wrote:

...

diff --git a/mysql-test/r/temp_table.result b/mysql-test/r/temp_table.result index ee0b3ab..94b02a6 100644 --- a/mysql-test/r/temp_table.result +++ b/mysql-test/r/temp_table.result @@ -308,3 +308,185 @@ show status like 'com_drop%table'; Variable_name Value Com_drop_table 2 Com_drop_temporary_table 1 +# +# MDEV-5535: Cannot reopen temporary table +# +DROP DATABASE IF EXISTS temp_db; +CREATE DATABASE temp_db; +USE temp_db; +# +# SHOW TABLES do not list temporary tables. +# +CREATE TEMPORARY TABLE temp_t1(i INT) ENGINE=INNODB; +INSERT INTO temp_t1 VALUES(1); +SELECT * FROM temp_t1; +i +1 +SHOW TABLES; +Tables_in_temp_db +DROP TABLE temp_t1; +# +# Create and drop a temporary table. +# +CREATE TEMPORARY TABLE temp_t1(i INT) ENGINE=INNODB; +INSERT INTO temp_t1 VALUES(1); +SELECT * FROM temp_t1; +i +1 +DROP TABLE temp_t1; +SELECT * FROM temp_t1; +ERROR 42S02: Table 'temp_db.temp_t1' doesn't exist +# +# Create a tempporary table and base table with same name and DROP TABLE. +# +CREATE TABLE t1(c1 VARCHAR(20)) ENGINE=INNODB; +INSERT INTO t1 VALUES("BASE TABLE"); +CREATE TEMPORARY TABLE t1(c1 VARCHAR(20)) ENGINE=INNODB; +INSERT INTO t1 VALUES("TEMPORARY TABLE"); +SELECT * FROM t1; +c1 +TEMPORARY TABLE +DROP TABLE t1; +SELECT * FROM t1; +c1 +BASE TABLE +DROP TABLE t1; +SELECT * FROM t1; +ERROR 42S02: Table 'temp_db.t1' doesn't exist +# +# Create a tempporary table and base table with same name and DROP TEMPORARY +# TABLE. +# +CREATE TABLE t1(c1 VARCHAR(20)) ENGINE=INNODB; +INSERT INTO t1 VALUES("BASE TABLE"); +CREATE TEMPORARY TABLE t1(c1 VARCHAR(20)) ENGINE=INNODB; +INSERT INTO t1 VALUES("TEMPORARY TABLE"); +SELECT * FROM t1; +c1 +TEMPORARY TABLE +DROP TEMPORARY TABLE t1; +SELECT * FROM t1; +c1 +BASE TABLE +DROP TEMPORARY TABLE t1; +ERROR 42S02: Unknown table 'temp_db.t1' +SELECT * FROM t1; +c1 +BASE TABLE +DROP TABLE t1; +# +# Create a temporary table and drop its parent database. +# +USE temp_db; +CREATE TEMPORARY TABLE temp_t1(i INT) ENGINE=INNODB; +INSERT INTO temp_t1 VALUES (1); +DROP DATABASE temp_db; +CREATE DATABASE temp_db; +USE temp_db; +DROP TEMPORARY TABLE temp_t1; +# +# Similar to above, but this time with a base table with same name. +# +USE temp_db; +CREATE TABLE t1(i INT)ENGINE=INNODB; +CREATE TEMPORARY TABLE t1(i INT) ENGINE=INNODB; +INSERT INTO t1 VALUES (1); +DROP DATABASE temp_db; +CREATE DATABASE temp_db; +USE temp_db; +DROP TEMPORARY TABLE t1; +DROP TABLE t1; +ERROR 42S02: Unknown table 'temp_db.t1' +# +# Create a temporary table within a function. +# +USE temp_db; +CREATE FUNCTION f1() RETURNS INT +BEGIN +DROP TEMPORARY TABLE IF EXISTS temp_t1; +CREATE TEMPORARY TABLE temp_t1(i INT) ENGINE=INNODB; +INSERT INTO `temp_t1` VALUES(1); +RETURN (SELECT COUNT(*) FROM temp_t1); +END| +SELECT f1(); +f1() +1 +SELECT * FROM temp_t1; +i +1 +DROP TABLE temp_t1; +CREATE TEMPORARY TABLE `temp_t1`(i INT) ENGINE=INNODB; +SELECT f1(); +f1() +1 +SELECT * FROM temp_t1; +i +1 +DROP FUNCTION f1; +# +# Create and drop a temporary table within a function. +# +CREATE FUNCTION f2() RETURNS INT +BEGIN +DROP TEMPORARY TABLE IF EXISTS temp_t1; +CREATE TEMPORARY TABLE temp_t1(i INT) ENGINE=INNODB; +INSERT INTO temp_t1 VALUES(1); +DROP TABLE temp_t1; +RETURN 0; +END| +ERROR HY000: Explicit or implicit commit is not allowed in stored function or trigger. +# +# Create a temporary table within a function and select it from another +# function. +# +CREATE FUNCTION f2() RETURNS INT +BEGIN +DROP TEMPORARY TABLE IF EXISTS temp_t1; +CREATE TEMPORARY TABLE temp_t1 (i INT) ENGINE=INNODB; +INSERT INTO temp_t1 VALUES (1); +RETURN f2_1(); +END| +CREATE FUNCTION f2_1() RETURNS INT +RETURN (SELECT COUNT(*) FROM temp_t1)| +DROP TEMPORARY TABLE temp_t1; +SELECT f2(); +f2() +1 +DROP TEMPORARY TABLE temp_t1; +DROP FUNCTION f2; +# +# Create temporary table like base table. +# +CREATE TABLE t1(i INT) ENGINE=INNODB; +INSERT INTO t1 VALUES(1); +CREATE TEMPORARY TABLE temp_t1 LIKE t1; +SELECT * FROM temp_t1; +i +CREATE TEMPORARY TABLE t1 LIKE t1; +ERROR 42000: Not unique table/alias: 't1' +DROP TABLE temp_t1, t1; +# +# Create temporary table as base table. +# +CREATE TABLE t1(i INT) ENGINE=INNODB; +INSERT INTO t1 VALUES(1); +CREATE TEMPORARY TABLE temp_t1 AS SELECT * FROM t1; +SELECT * FROM temp_t1; +i +1 +DROP TABLE temp_t1, t1; +# +# Reopen temporary table +# +CREATE TEMPORARY TABLE temp_t1(i int)ENGINE=INNODB; +INSERT INTO temp_t1 VALUES(1), (2); +SELECT * FROM temp_t1 a, temp_t1 b; +i i +1 1 +1 2 +2 1 +2 2 +DROP TABLE temp_t1; +# Cleanup +DROP DATABASE temp_db; +# MDEV-5535: End of tests

What's that? You have lots of tests for temporary tables - not that I mind the number, but they are mostly unrelated to MDEV-5535. Only the one last test is about the new feature. I'd expect it to be the other way around - many tests for the new functionality and few, if at all - for the unmodified behavior.

These generic tests were added to cover more possible use cases as the patch modified a good fraction of existing code. I have now moved these unrelated tests to a separate commit and added some more tests specific to MDEV-5535 in reopen_temp_table.test.

...

diff --git a/mysql-test/r/temp_table_frm.result b/mysql-test/r/temp_table_frm.result index 19c6638..58e9a0a 100644 --- a/mysql-test/r/temp_table_frm.result +++ b/mysql-test/r/temp_table_frm.result @@ -16,6 +16,6 @@ variable_name session_status.variable_value - t1.variable_value OPENED_FILES 0 OPENED_PLUGIN_LIBRARIES 0 OPENED_TABLE_DEFINITIONS 2 -OPENED_TABLES 1 +OPENED_TABLES 2

why did it change?

UPDATE: now, I see - you close and open temporary tables all the time. See my comment about it below.

Ok.

...

OPENED_VIEWS 0 drop table t1; diff --git a/mysql-test/suite/handler/handler.inc b/mysql-test/suite/handler/handler.inc index c71dc53..ca67b62 100644 --- a/mysql-test/suite/handler/handler.inc +++ b/mysql-test/suite/handler/handler.inc @@ -489,7 +489,7 @@ handler t1 open as a1; handler a1 read a=(1); handler a1 read a next; handler a1 read a next; ---error ER_CANT_REOPEN_TABLE +#--error ER_CANT_REOPEN_TABLE

remove it, don't comment. And, please, fix the comment before this test.

Removed. What comment are you referring to?

...

select a,b from t1; handler a1 read a prev; handler a1 read a prev; diff --git a/mysql-test/suite/multi_source/status_vars.result b/mysql-test/suite/multi_source/status_vars.result index 12917f9..50d500e 100644 --- a/mysql-test/suite/multi_source/status_vars.result +++ b/mysql-test/suite/multi_source/status_vars.result @@ -76,22 +76,34 @@ start slave; include/wait_for_slave_to_start.inc set binlog_format = statement; create temporary table tmp1 (i int) engine=MyISAM; +show status like 'Slave_open_temp_table_definitions'; +Variable_name Value +Slave_open_temp_table_definitions 1 show status like 'Slave_open_temp_tables'; Variable_name Value -Slave_open_temp_tables 1 +Slave_open_temp_tables 0

why did it change?

This is because temporary tables were closed at the end of the statement. Also, you must have noted the new 'Slave_open_temp_table_definitions' status variable in this patch. But now I have modified the patch based on your suggestion and these changes have been reverted.

...

set default_master_connection = 'master1'; +show status like 'Slave_open_temp_table_definitions'; +Variable_name Value +Slave_open_temp_table_definitions 1 show status like 'Slave_open_temp_tables'; Variable_name Value -Slave_open_temp_tables 1 +Slave_open_temp_tables 0 set binlog_format = statement; create temporary table tmp1 (i int) engine=MyISAM; +show status like 'Slave_open_temp_table_definitions'; +Variable_name Value +Slave_open_temp_table_definitions 2 show status like 'Slave_open_temp_tables'; Variable_name Value -Slave_open_temp_tables 2 +Slave_open_temp_tables 0 set default_master_connection = ''; +show status like 'Slave_open_temp_table_definitions'; +Variable_name Value +Slave_open_temp_table_definitions 2 show status like 'Slave_open_temp_tables'; Variable_name Value -Slave_open_temp_tables 2 +Slave_open_temp_tables 0 include/reset_master_slave.inc include/reset_master_slave.inc include/reset_master_slave.inc diff --git a/mysql-test/suite/multi_source/status_vars.test b/mysql-test/suite/multi_source/status_vars.test index d1cfda7..f9d2e85 100644 --- a/mysql-test/suite/multi_source/status_vars.test +++ b/mysql-test/suite/multi_source/status_vars.test @@ -107,9 +107,11 @@ create temporary table tmp1 (i int) engine=MyISAM;

--connection slave --sync_with_master 0,'master1' +show status like 'Slave_open_temp_table_definitions'; show status like 'Slave_open_temp_tables';

You can do show status like 'Slave_open_temp_table%'; instead. here and everywhere.

These changes have been reverted. Slave_open_temp_table_definitions is no longer part of the patch. It can later be added in a separate task.

...

set default_master_connection = 'master1'; +show status like 'Slave_open_temp_table_definitions'; show status like 'Slave_open_temp_tables';

--connect (master2,127.0.0.1,root,,,$SERVER_MYPORT_2) diff --git a/sql/rpl_rli.cc b/sql/rpl_rli.cc index 7bdd9c1..426989c 100644 --- a/sql/rpl_rli.cc +++ b/sql/rpl_rli.cc @@ -1060,24 +1061,14 @@ void

Relay_log_info::inc_group_relay_log_pos(ulonglong log_pos,

...

void Relay_log_info::close_temporary_tables() { - TABLE *table,*next; DBUG_ENTER("Relay_log_info::close_temporary_tables");

- for (table=save_temporary_tables ; table ; table=next) + if (sql_driver_thd) { - next=table->next; - - /* Reset in_use as the table may have been created by another thd */ - table->in_use=0; - /* - Don't ask for disk deletion. For now, anyway they will be deleted

when

...

- slave restarts, but it is a better intetntion to not delete them. - */ - DBUG_PRINT("info", ("table: 0x%lx", (long) table)); - close_temporary(table, 1, 0); + sql_driver_thd->temporary_tables.close_tables(true);

Hmm, really? see that comment above in the old code:

/* Reset in_use as the table may have been created by another thd */

and now you assume that all temptables belong to sql_driver_thd? but sql_driver_thd comment says

THD for the main sql thread, the one that starts threads to process slave requests. If there is only one thread, then this THD is also used for SQL processing.

and indeed in parallel replication there can be many sql threads, all with their own temporary tables.

You are right and IIUC temp tables are associated to each master. I also realized that sql_driver_thd may not be available when this method gets invoked. It has been redone where it now first closes/free all TABLEs followed by freeing all TABLE_SHAREs.

...

} - save_temporary_tables= 0; - slave_open_temp_tables= 0; + save_temp_table_shares= 0; + DBUG_VOID_RETURN; }

@@ -1753,6 +1744,10 @@ void rpl_group_info::cleanup_context(THD *thd, bool error) } m_table_map.clear_tables(); slave_close_thread_tables(thd); + + /* Cleanup temporary tables. */ + thd->temporary_tables.cleanup(); +

why this was not needed before?

Its because I was closing all the open tables here. But now its not the case and this has been reverted.

if (error)

...

{ thd->mdl_context.release_transactional_locks(); diff --git a/sql/sp_head.cc b/sql/sp_head.cc index cf0b8e8..45346cd 100644 --- a/sql/sp_head.cc +++ b/sql/sp_head.cc @@ -2929,6 +2929,7 @@ void sp_head::add_mark_lead(uint ip, List<sp_instr> *leads) thd->get_stmt_da()->set_overwrite_status(false); } thd_proc_info(thd, "closing tables"); + /* main.temp_table: check this */

what does that mean?

A leftover - was a note for myself, removed.

...

close_thread_tables(thd); thd_proc_info(thd, 0);

@@ -2970,8 +2971,7 @@ void sp_head::add_mark_lead(uint ip,

List<sp_instr> *leads)

...

open_tables stage. */ if (!res || !thd->is_error() || - (thd->get_stmt_da()->sql_errno() != ER_CANT_REOPEN_TABLE &&

can ER_CANT_REOPEN_TABLE still happen somewhere?

Yes, in the post-review commit, the following scenario results in this error : CREATE TEMPORARY TABLE t4 (i INT); INSERT INTO t4 VALUES(1), (2); DELIMITER |; CREATE FUNCTION f4() RETURNS INT BEGIN DROP TEMPORARY TABLE t4; RETURN 1; END| DELIMITER ;| --error ER_CANT_REOPEN_TABLE SELECT f4() FROM t4; I have undone the above change.

...

- thd->get_stmt_da()->sql_errno() != ER_NO_SUCH_TABLE && + (thd->get_stmt_da()->sql_errno() != ER_NO_SUCH_TABLE && thd->get_stmt_da()->sql_errno() != ER_NO_SUCH_TABLE_IN_ENGINE && thd->get_stmt_da()->sql_errno() != ER_UPDATE_TABLE_USED)) thd->stmt_arena->state= Query_arena::STMT_EXECUTED; diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 6656a84..e73ada2 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -4230,7 +4234,8 @@ void THD::restore_backup_open_tables_state(Open_tables_backup *backup) Before we will throw away current open tables state we want to be sure that it was properly cleaned up. */ - DBUG_ASSERT(open_tables == 0 && temporary_tables == 0 && + DBUG_ASSERT(open_tables == 0 && + temporary_tables.is_empty() &&

temporary_tables.is_empty() is not the same as temporary_tables == 0, because it takes into account thd->rgi_slave

Right. Fixed.

...

derived_tables == 0 && lock == 0 && locked_tables_mode == LTM_NONE && diff --git a/sql/sql_class.h b/sql/sql_class.h index ae240ae..02b5739 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -799,6 +800,13 @@ enum killed_type volatile int64 local_memory_used; /* Memory allocated for global usage */ volatile int64 global_memory_used; + + /* Open temporary tables. */ + ulong open_temporary_tables;

this one doesn't seem to be used anywhere

Yes, and the additions below too. They have been removed.

...

+ /* Number of temporary table definitions opened by THD. */ + ulong opened_temporary_table_definitions; + /* Number of temporary tables opened by THD. */ + ulong opened_temporary_tables; } STATUS_VAR;

/* @@ -3506,13 +3525,13 @@ class THD :public Statement, */ DBUG_PRINT("debug", ("temporary_tables: %s, in_sub_stmt: %s, system_thread: %s", - YESNO(temporary_tables), YESNO(in_sub_stmt), + YESNO(temporary_tables.is_empty()), YESNO(in_sub_stmt), show_system_thread(system_thread))); if (in_sub_stmt == 0) { if (wsrep_binlog_format() == BINLOG_FORMAT_ROW) set_current_stmt_binlog_format_row(); - else if (temporary_tables == NULL) + else if (temporary_tables.is_empty())

temporary_tables.is_empty() is not the same as temporary_tables == NULL

Fixed.

...

set_current_stmt_binlog_format_stmt(); } DBUG_VOID_RETURN; diff --git a/sql/sql_handler.cc b/sql/sql_handler.cc index e8ade81..52b14e1 100644 --- a/sql/sql_handler.cc +++ b/sql/sql_handler.cc @@ -180,7 +180,7 @@ static void mysql_ha_close_table(SQL_HANDLER

*handler)

...

table->file->ha_index_or_rnd_end(); table->query_id= thd->query_id; table->open_by_handler= 0; - mark_tmp_table_for_reuse(table); + //mark_tmp_table_for_reuse(table);

why?

The original patch closed the tables instead of reusing them. But now tables are being reused.

...

} my_free(handler->lock); handler->init(); diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index aa2cae5..c594ef1 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -4051,16 +4051,16 @@ static TABLE *create_table_from_items(THD *thd, } else { - if (open_temporary_table(thd, create_table)) - { - /* - This shouldn't happen as creation of temporary table should make - it preparable for open. Anyway we can't drop temporary table if - we are unable to find it. - */ - DBUG_ASSERT(0); - } - DBUG_ASSERT(create_table->table == create_info->table); + /* + TODO: Explain why we do not need to call THD::wait_for_prior_commit() + here? + */

So?

Done.

...

+ TABLE *tab= create_info->table; + tab->query_id= thd->query_id; + thd->thread_specific_used= true; + create_table->updatable= true; + create_table->table= tab; + tab->init(thd, create_table);

hmm, you don't create a table here. how comes?

The table got created above in mysql_create_table_no_lock(). These changes were about initializing the opened TABLE structure. But most of it has been undone. I am not using the pointer stored in create_info->table.

...

} } else diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 52dcc7e..64227d4 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -23,7 +23,7 @@ // set_handler_table_locks, // lock_global_read_lock, // make_global_read_lock_block_commit -#include "sql_base.h" // find_temporary_table +#include "sql_base.h" // Temporary_tables::find_table

really? it's in temporary_tables.h, not in sql_base.h anymore

sql_base.h is still needed for some other functions. Fixed the comment.

...

#include "sql_cache.h" // QUERY_CACHE_FLAGS_SIZE, query_cache_* #include "sql_show.h" // mysqld_list_*, mysqld_show_*, // calc_sum_of_all_status @@ -5730,6 +5731,8 @@ static bool do_execute_sp(THD *thd, sp_head *sp)

/* Free tables */ close_thread_tables(thd); + /* Do not close HANDLER tables. */ + thd->temporary_tables.close_tables(false);

why this wasn't here before?

This was added to close the tables at the end of the statement. But that's no longer the case, so reverted.

...

#ifdef WITH_WSREP thd->wsrep_consistency_check= NO_CONSISTENCY_CHECK; #endif /* WITH_WSREP */ diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 40032c3..f568432 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -4608,13 +4602,14 @@ handler *mysql_create_frm_image(THD *thd, which was created. @param[out] key_count Number of keys in table which was created.

- If one creates a temporary table, this is automatically opened - Note that this function assumes that caller already have taken exclusive metadata lock on table being created or used some other way to ensure that concurrent operations won't intervene. mysql_create_table() is a wrapper that can be used for this.

+ In case of temporary tables, the TABLE_SHARE is added to thd's + temporary_tables_share list. +

hmm, so you've decided not to open the table automatically...

In the post-review commit, I am also opening the table. (Temporary_tables.create_and_open_table()). Also updated the above comment.

...

@retval 0 OK @retval 1 error @retval -1 table existed but IF EXISTS was used @@ -4820,17 +4813,12 @@ int create_table_impl(THD *thd, create_info->table= 0; if (!frm_only && create_info->tmp_table()) { - /* - Open a table (skipping table cache) and add it into - THD::temporary_tables list. - */ - - TABLE *table= open_table_uncached(thd, create_info->db_type, frm, path, - db, table_name, true, true); + TABLE *table= + thd->temporary_tables.create_and_use_table(create_info->db_type, frm, + path, db, table_name, true);

if (!table) { - (void) rm_temporary_table(create_info->db_type, path);

why not? because create_and_use_table() is atomic?

I think that was a mistake. It has been fixed in the post-post review patch.

...

goto err; }

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;

I'd rather not add two pointers to TABLE_SHARE just for the sake of temporary tables. Use something else, please. May be List<> will work for you? Or, like

struct TMP_TABLE_SHARE: TABLE_SHARE { TMP_TABLE_SHARE *next, *prev; }

I like the latter approach. The struct also has the list of open TABLEs.

...

/** Category of this table. */ TABLE_CATEGORY table_category; diff --git a/sql/table_cache.cc b/sql/table_cache.cc index 2dd368a..b307841 100644 --- a/sql/table_cache.cc +++ b/sql/table_cache.cc @@ -589,6 +589,7 @@ void tdc_unlock_share(TDC_element *element) key Table cache key key_length Length of key flags operation: what to open table or view + out_table TABLE for the requested table

please, put unrelated comment fixes in a separate commit (it's very easy to do with git citool)

Done.

...

IMPLEMENTATION Get a table definition from the table definition cache. diff --git a/sql/table_cache.h b/sql/table_cache.h index 2c5b0fc..ac36269 100644 --- a/sql/table_cache.h +++ b/sql/table_cache.h @@ -1,3 +1,5 @@ +#ifndef TABLE_CACHE_H_INCLUDED +#define TABLE_CACHE_H_INCLUDED

and this one too. commit all unrelated changes separately

Done.

...

/* Copyright (c) 2000, 2012, Oracle and/or its affiliates. Copyright (c) 2010, 2011 Monty Program Ab Copyright (C) 2013 Sergey Vojtovich and MariaDB Foundation diff --git a/storage/myisam/mi_close.c b/storage/myisam/mi_close.c index f0a82bc..23e01ef 100644 --- a/storage/myisam/mi_close.c +++ b/storage/myisam/mi_close.c @@ -67,7 +67,7 @@ int mi_close(register MI_INFO *info) if (share->kfile >= 0 && flush_key_blocks(share->key_cache, share->kfile, &share->dirty_part_map, - ((share->temporary || share->deleting) ? + ((share->deleting) ?

Why? in all your myisam/aria changes (this one and others) you've enabled updating of on-disk data for temporary tables. like, flush keycache blocks, update file header, etc. Why should it be done persistently on disk? Temporary tables are not persistent, if mariadb crashes nobody will care what old temporary table files will contain. It should be enough to have all up-to-date information in memory.

I had to make these changes to enable closing & reopening of temporary tables possible. But that's no longer the case now. So, reverted all the changes in aria, myisam engine.

...

FLUSH_IGNORE_CHANGED : FLUSH_RELEASE))) error=my_errno; diff --git a/sql/temporary_tables.h b/sql/temporary_tables.h new file mode 100644 index 0000000..c345bea --- /dev/null +++ b/sql/temporary_tables.h @@ -0,0 +1,112 @@ +#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 + +class Temporary_tables { +public: + Temporary_tables() : m_thd(0), m_table_shares(0), m_opened_tables(0) {} + bool init(THD *thd); + bool is_empty(); + bool reset(); + bool cleanup(); + TABLE_SHARE *create_table(handlerton *hton, LEX_CUSTRING *frm, + const char *path, const char *db, + const char *table_name); + TABLE_SHARE *find_table(const TABLE_LIST *tl); + TABLE_SHARE *find_table_reduced_key_length(const char *key, uint key_length); + TABLE_SHARE *find_table(const char *db, const char *table_name); + TABLE *find_open_table(const char *db, const char *table_name); + TABLE *create_and_use_table(handlerton *hton, LEX_CUSTRING *frm, + const char *path, const char *db, + const char *table_name, bool open_in_engine); + TABLE *open_table(TABLE_SHARE *share, const char *alias, bool open_in_engine); + bool open_table(TABLE_LIST *tl); + bool open_tables(TABLE_LIST *tl); + bool close_tables(bool all); + bool rename_table(TABLE *table, const char *db, const char *table_name); + bool drop_table(TABLE *table, bool *is_trans, bool delete_in_engine); + void mark_tables_as_free_for_reuse(); + +private: + uint create_table_def_key(char *key, + const char *db, + const char *table_name); + TABLE_SHARE *find_table(const char *key, uint key_length); + TABLE *find_open_table(const char *key, uint key_length); + TABLE *open_table(const char *db, const char *table_name); + bool close_table(TABLE *table); + bool rm_temporary_table(handlerton *hton, const char *path); + bool wait_for_prior_commit(); + bool write_query_log_events(); + void lock_tables(); + void unlock_tables(); + + /* + Return true if the table was created explicitly. + */ + bool is_user_table(TABLE_SHARE *share) + { + const char *name= share->table_name.str; + return strncmp(name, tmp_file_prefix, tmp_file_prefix_length); + }

Is that right? I can do CREATE TEMPORARY TABLE `#sql-foo` (a int).

the correct test needs to look at, for example, table->s->path or some flag, may be...

Why wouldn't it use share->tmp_table?

Yes, I too think a check for TRANSACTIONAL_TMP_TABLE and NON_TRANSACTIONAL_TMP_TABLE should do.

...

+ + /* 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; + } + } + + uint tmpkeyval(TABLE_SHARE *share) + { + return uint4korr(share->table_cache_key.str + + share->table_cache_key.length - 4); + } + +private: + THD *m_thd;

Hmm, is that necessary? Temporary_tables class is instantiated in only one place - inside the THD. So Temporary_tables always belongs to its thd, and storing a point to the parent THD in the Temporary_tables you basically add another void* to already big THD class, while thid pointer stores no useful information at all.

A hackish way to fix is is to replace m_thd with

(((char*)this) - offsetof(THD, temporary_tables))

I'm not suggesting this hack, it is just to prove that m_thd is redundant. I hope you'll find a nicer and safer solution :)

How about current_thd in place of m_thd?

By the way, any increase in the THD size will most probably cause a test failure on labrador, you'll need to increase the DEFAULT_THREAD_STACK to fix it. Please check that - even if you remove m_thd there is still a m_table_shares pointer that increases THD size.

I have pushed the patch in a bb-* branch. I will monitor the tests and update it accordingly.

...

+ TABLE_SHARE *m_table_shares; + TABLE *m_opened_tables; +}; + +#endif /* TEMPORARY_TABLES_H */ diff --git a/sql/temporary_tables.cc b/sql/temporary_tables.cc new file mode 100644 index 0000000..5d8bce6 --- /dev/null +++ b/sql/temporary_tables.cc @@ -0,0 +1,1265 @@ +/* + 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, + get_table_def_key, + 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 */ + + +/* + 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 TABLE_SHAREs. + + @return false Temporary tables exist + true No temporary table exist +*/ +bool Temporary_tables::is_empty() +{ + bool result; + + if (!m_thd) + { + return true; + } + + rpl_group_info *rgi_slave= m_thd->rgi_slave; + + if (rgi_slave) + { + result= (rgi_slave->rli->save_temp_table_shares == NULL) ? true : false; + } + else + { + result= (m_table_shares == NULL) ? true : false; + } + + 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; + m_opened_tables= 0; + DBUG_RETURN(false); +} + + +/* + Cleanup the session's temporary tables by closing all open temporary tables + as well as freeing the respective TABLE_SHAREs. + It also writes "DROP TEMPORARY TABLE .." query log events to the binary log. + + Currently, it always returns false (success). + + @return false Success + true Error +*/ +bool Temporary_tables::cleanup() +{ + DBUG_ENTER("Temporary_tables::cleanup"); + + TABLE_SHARE *share; + TABLE_SHARE *next; + lock_tables();

old code used to have here:

if (!thd->temporary_tables) DBUG_RETURN(FALSE); DBUG_ASSERT(!thd->rgi_slave);

you don't do either, instead you do lock_tables() (which is only needed if thd->rgi_slave != NULL). How comes?

Its because, cleanup() was called in rpl_group_info::cleanup_context(). That's no longer the case. This has been renamed to close_tables() (to align with the old close_temporary_tables() and has the assertion and checks in place.

...

+ + /* + Ensure we don't have open HANDLERs for tables we are about to close. + This is necessary when close_temporary_tables() is called as part

s/close_temporary_tables/Temporary_tables::cleanup/ in the comment

Done. s/close_temporary_tables/Temporary_tables::close_tables/

...

+ of execution of BINLOG statement (e.g. for format description event). + */ + mysql_ha_rm_temporary_tables(m_thd); + + // Close all open temporary tables. + close_tables(true); + + // Write DROP TEMPORARY TABLE query log events to binary log. + if (!m_thd->rgi_slave)

another if (!m_thd->rgi_slave) while the old code had DBUG_ASSERT instead

Fixed.

...

+ { + write_query_log_events(); + } + + // Free all TABLE_SHARES. + share= m_table_shares; + + while (share) { + next= share->next; + rm_temporary_table(share->db_type(), share->path.str); + + /* Delete the share from table share list */ + unlink<TABLE_SHARE>(&m_table_shares, share); + + free_table_share(share); + my_free(share); + + /* Decrement Slave_open_temp_table_definitions status variable count. */ + if (m_thd->rgi_slave) + { + thread_safe_decrement32(&slave_open_temp_table_definitions); + }

why did you put write_query_log_events() in a separate function?

I felt the original function was quite big, so moved the binary log related stuff to a separate function.

now you need to iterate the list of tables twice.

Right. Fixed.

...

+ + share= next; + } + + reset(); + + unlock_tables(); + + DBUG_RETURN(false); +} + + +/* + 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 +*/ +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"); + + TABLE_SHARE *share= NULL; + char key_cache[MAX_DBKEY_LENGTH], *saved_key_cache, *tmp_path; + uint key_length; + int res; + + lock_tables(); + + if (wait_for_prior_commit()) + { + goto end; /* Failure */ + } + + /* Create the table definition key for the temporary table. */ + key_length= create_table_def_key(key_cache, db, table_name); + + if (!(share= (TABLE_SHARE *) my_malloc(sizeof(TABLE_SHARE) + strlen(path) + + 1 + key_length, MYF(MY_WME)))) + { + goto end; /* 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->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); + share= NULL; + goto end; + } + + share->m_psi= PSI_CALL_get_table_share(true, share); + + /* Add share to the head of table share list. */ + link<TABLE_SHARE>(&m_table_shares, share); + + /* Increment Slave_open_temp_table_definitions status variable count. */ + if (m_thd->rgi_slave) + { + thread_safe_increment32(&slave_open_temp_table_definitions); + } + +end: + unlock_tables(); + + DBUG_RETURN(share); +} + + +/* + Lookup the 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 +*/ +TABLE_SHARE *Temporary_tables::find_table(const char *db, + const char *table_name) +{ + DBUG_ENTER("Temporary_tables::find_table"); + + TABLE_SHARE *share; + char key[MAX_DBKEY_LENGTH]; + uint key_length; + + key_length= create_table_def_key(key, db, table_name); + share= find_table(key, key_length); + + DBUG_RETURN(share); +} + + +/* + Lookup TABLE_SHARE using the specified TABLE_LIST element. Return NULL is none + found. + + @return Success A pointer to table share object + Failure NULL +*/ +TABLE_SHARE *Temporary_tables::find_table(const TABLE_LIST *tl) +{ + DBUG_ENTER("Temporary_tables::find_table"); + + TABLE_SHARE *share; + const char *tmp_key; + char key[MAX_DBKEY_LENGTH]; + uint key_length; + + key_length= get_table_def_key(tl, &tmp_key); + memcpy(key, tmp_key, key_length); + 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; + + share= find_table(key, key_length); + + DBUG_RETURN(share); +} + + +/* + Lookup TABLE_SHARE using the specified table definition key. Return NULL is + none found. + + @return Success A pointer to table share object + Failure NULL +*/ +TABLE_SHARE *Temporary_tables::find_table(const char *key, + uint key_length) +{ + DBUG_ENTER("Temporary_tables::find_table"); + + TABLE_SHARE *share; + TABLE_SHARE *result= NULL; + + 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; + } + } + + unlock_tables(); + + DBUG_RETURN(result); +} + + +/* + Lookup TABLE_SHARE based on the specified key. This key, however, 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_SHARE *Temporary_tables::find_table_reduced_key_length(const char *key, + uint key_length) +{ + DBUG_ENTER("Temporary_tables::find_table_reduced_key_length"); + + TABLE_SHARE *share; + TABLE_SHARE *result= NULL; + + lock_tables(); + + for (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))) + { + result= share; + break; + } + } + + unlock_tables(); + + DBUG_RETURN(result); +} + + +/* + Lookup the list of opened temporary tables using the specified + db/table_name. Return NULL is none found. + + @return Success A pointer to table object + Failure NULL +*/ +TABLE *Temporary_tables::find_open_table(const char *db, + const char *table_name) +{ + DBUG_ENTER("Temporary_tables::find_open_table"); + + TABLE *table; + char key[MAX_DBKEY_LENGTH]; + uint key_length; + + if (wait_for_prior_commit()) + { + DBUG_RETURN(NULL); /* Failure */ + } + + key_length= create_table_def_key(key, db, table_name); + + table= find_open_table(key, key_length); + + DBUG_RETURN(table); +} + + +/* + Lookup the list of opened temporary tables using the specified + key. Return NULL is none found. + + @return Success A pointer to table object + Failure NULL +*/ +TABLE *Temporary_tables::find_open_table(const char *key, + uint key_length) +{ + DBUG_ENTER("Temporary_tables::find_open_table"); + + TABLE *table, *result= NULL; + + for (table= m_opened_tables; table; table= table->next) + { + if (table->s->table_cache_key.length == key_length && + !(memcmp(table->s->table_cache_key.str, key, key_length))) + { + result= table; + break; + } + } + + DBUG_RETURN(result); +} + + +/* + 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 + + + @return Success A pointer to table object + Failure NULL +*/ +TABLE *Temporary_tables::create_and_use_table(handlerton *hton,

better create_and_open()

Renamed.

...

+ LEX_CUSTRING *frm, + const char *path, + const char *db, + const char *table_name, + bool open_in_engine) +{ + DBUG_ENTER("Temporary_tables::create_and_use_table"); + + TABLE_SHARE *share; + TABLE *table; + + if (wait_for_prior_commit()) + { + DBUG_RETURN(NULL); /* Failure */ + } + + if (!(share= create_table(hton, frm, path, db, table_name))) + { + DBUG_RETURN(NULL); + } + + if ((table= open_table(share, table_name, open_in_engine))) + { + DBUG_RETURN(table); + } + + DBUG_RETURN(NULL); +} + + +/* + 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(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); /* Failure */ + } + + 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; + + lock_tables(); + + /* Add table to the head of table list. */ + link<TABLE>(&m_opened_tables, table); + + /* Increment Slave_open_temp_table_definitions status variable count. */ + if (m_thd->rgi_slave) + { + thread_safe_increment32(&slave_open_temp_tables); + } + + unlock_tables(); + + DBUG_PRINT("tmptable", ("Opened table: '%s'.'%s' 0x%lx", table->s->db.str, + table->s->table_name.str, (long) table)); + DBUG_RETURN(table); +} + + +/* + Lookup the table share list and open a table based on db/table_name. + Return NULL if none found. + + @param db [in] Schema name + @param table_name [in] Table name + + @return Success A pointer to table object + Failure 0 +*/ +TABLE *Temporary_tables::open_table(const char *db, + const char *table_name) +{ + DBUG_ENTER("Temporary_tables::open_table"); + + TABLE *result= 0; + TABLE_SHARE *share; + + if ((share= find_table(db, table_name))) + { + result= open_table(share, table_name, true); + }

why this open_table(db, table_name) is doing so much less than open_table(table_list) below?

hmm.. looks like a leftover. Its a private method so, I could have added it to do an internal lookup based in db & table_name. But I do not see it being used anywhere. Removed.

...

+ + DBUG_RETURN(result); +} + + +/* + Lookup the table share list and open a table based on the specified + TABLE_LIST element. Return false if the table was opened successfully. + + @param tl [in] TABLE_LIST + + @return false Success + true Failure +*/ +bool Temporary_tables::open_table(TABLE_LIST *tl) +{ + DBUG_ENTER("Temporary_tables::open_table"); + + TABLE *table= NULL; + TABLE_SHARE *share; + + /* + 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 (wait_for_prior_commit()) + { + DBUG_RETURN(true); /* Failure */ + }

why do you have it here? The old open_temporary_table() only had it below, you have it twice.

This is not call only once before the call to lock_tables(). BTW, in the post-review patch, I have added a check for rli->save_temp_table_shares in Temporary_tables::wait_for_prior_commit() as a pre-condition to invoke THD::wait_for_prior_commit(). Also, as a general rule, I am calling wait_for_prior_commits() before lock_tables(). This is done because since the control has reached here, the slave thread may be wanting to access temp table, so it must wait for prior commits. Calls to lock_tables() have been placed at points that tries too access the temp table and share list (mostly public methods). There is also a flag m_locked to prevent double locking.

...

+ + lock_tables(); + + if (tl->open_type == OT_BASE_ONLY || m_table_shares == NULL) + { + DBUG_PRINT("info", ("skip_temporary is set or no temporary tables")); + unlock_tables(); + DBUG_RETURN(false); + } + + unlock_tables(); + + if ((share= find_table(tl)) && + (table= open_table(share, tl->get_table_name(), true)))

Why would you open temporary tables all the time? old code didn't do that. temporary tables were automatically opened when created and automatically dropped when closed. I understand that you *might* need to open a second instance of a temporary table (but perhaps this can be avoided too), but why would close them again? just keep them open for reuse. In that case your open_table will just pick one unused TABLE from the m_opened_tables list

Done, the code now opens & reuses the tables and closes them only on DROP or end of session.

...

+ { + if (wait_for_prior_commit()) + { + DBUG_RETURN(true); /* Failure */ + } + +#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_RETURN(false); + } + + if (!table && + 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); +} + + +/* + 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. + + @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; + + if (wait_for_prior_commit()) + { + DBUG_RETURN(NULL); /* Failure */ + }

why do you wait_for_prior_commit here? old open_temporary_tables didn't do that, because every individual open_table() below waits for prior commit too.

Right, that was misplaced.

...

+ + 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))) + { + DBUG_RETURN(true); + } + } + + 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())); + + /* Delete the table from table list */ + unlink<TABLE>(&m_opened_tables, table); + + free_io_cache(table); + 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); +} + +/* + Close all the opened table. When 'all' is set to false, tables opened by + handlers and ones with query_id different than that of m_thd will not be + be closed. Currently, false (success) is always returned. + + @param all [in] Whether to close all tables? + + @return false Success + true Failure +*/ +bool Temporary_tables::close_tables(bool all) +{ + TABLE *table; + TABLE *next; + + table= m_opened_tables; + + while(table) { + next= table->next; + + if (all || ((table->query_id == m_thd->query_id) && + !(table->open_by_handler))) + { + mysql_lock_remove(m_thd, m_thd->lock, table); + close_table(table); + } + + table= next; + } + 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::write_query_log_events() +{ + DBUG_ENTER("Temporary_tables::write_query_log_events"); + DBUG_ASSERT(!m_thd->rgi_slave); + + TABLE_SHARE *share; + TABLE_SHARE *next; + TABLE_SHARE *prev_share; + // Assume thd->variables.option_bits has OPTION_QUOTE_SHOW_CREATE. + bool was_quote_show= true; + bool error= 0; + 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]; + + /* + Return in case there are no temporary tables or binary logging is + disabled. + */ + if (!(m_table_shares && mysql_bin_log.is_open())) + { + DBUG_RETURN(false); + } + + 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. + */

why does it have to be sorted? (I know that the old code also sorted the list, but I still cannot understand why)

This is done to allow the proper logging of temporary tables with same names but different thread IDs. https://bugs.mysql.com/bug.php?id=17263

...

+ + for (prev_share= m_table_shares, share= prev_share->next; + share; + prev_share= share, share= share->next) + { + TABLE_SHARE *prev_sorted; /* Same as for prev_share */ + 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 though it is slight overkill. + */

it's not an overkill, it's really needed

Indeed. Fixed the comment.

...

+ 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, + strlen(share->table_name.str));

you don't need strlen(share->table_name.str) because you can write share->table_name.length instead :) (may be this applies to other strlen's too)

Fixed.

...

+ s_query.append(','); + next= share->next; + } + + 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 close_temporary_tables failed. (Actually, the SQL + thread only calls close_temporary_tables while applying old

s/close_temporary_tables/Temporary_tables::cleanup/ in the comment

Done.

...

+ 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; + } + } + + if (!was_quote_show) + { + /* + Restore option. + */ + m_thd->variables.option_bits&= ~OPTION_QUOTE_SHOW_CREATE; + } + + 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; + TABLE_SHARE *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 thd->temporary_tables. + If the table is found: + - If the table is being used by some outer statement, i.e. + ref_count > 1, we only close the given table and return. + - 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. Even though part of the work done by this function + is redundant when the table is internal, as long as we + link both internal and user temporary tables into the same + temporary tables list, it's impossible to tell here whether + we're dealing with an internal or a user temporary table.

why is it impossible? there is is_user_table() function. I know that it's broken, but it should be fixed - it's used elsewhere.

I don't see why this is impossible. I have modified is_user_table() to use share->tmp_table instead.

...

+ + @param thd [in] Thread handler + @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.

fix the comment to match the function prototype, please. don't forget to explain why do you need delete_in_engine parameter (e.g. delete_in_engine is false in ALTER TABLE)

Ok.

...

+ + @retval 0 the table was found and dropped successfully. + @retval -1 the table is in use by a outer query

retval is wrong

Fixed.

...

+*/ + + +/* + @return false Table was either dropped or closed in + case multiple open tables were found + referring the table share. + true Error +*/ +bool Temporary_tables::drop_table(TABLE *table, + bool *is_trans, + bool delete_in_engine)

rename 'delete_in_engine' please, because it also includes removing the frm file, not only "in engine". I cannot think of a good name, sorry :(

Renamed to "delete_table" as was used by old close_temporary_table().

...

+{ + DBUG_ENTER("Temporary_tables::drop_table"); + + TABLE_SHARE *share; + handlerton *hton; + uint ref_count= 0; + bool result; + + DBUG_ASSERT(table); + DBUG_PRINT("tmptable", ("Dropping table: '%s'.'%s'", + table->s->db.str, table->s->table_name.str)); +

old code had here

/* Table might be in use by some outer statement. */ if (table->query_id && table->query_id != thd->query_id) { my_error(ER_CANT_REOPEN_TABLE, MYF(0), table->alias.c_ptr()); DBUG_RETURN(-1); }

the intention is not to allow a temporary table being dropped from a trigger or a stored function, if the table is used by the outer statement.

How do you solve this now? Apparently you do, somehow, because there is a test case for it.

In the old code, I was not dropping the table which had other active TABLEs, but reasoning was incorrect. The post-review code throws ER_CANT_REOPEN_TABLE in such case. BTW, this is the only place left now that throws ER_CANT_REOPEN_TABLE.

...

+ lock_tables(); + + if (is_trans) + *is_trans= table->file->has_transactions(); + + share= table->s; + hton= share->db_type(); + + /* + Iterate over the list of open tables to find the number of tables + referencing this table share. + */ + for (TABLE *tab= m_opened_tables; tab; tab= tab->next) + { + if (tab->s == share) + { + ref_count ++;

Strictly speaking, you can store a list of TABLE's in the TABLE_SHARE - that is, make TABLE_SHARE have a pointer to the list of its own TABLE's. That'd be particularly easy, if you introduce TMP_TABLE_SHARE as I've mentioned above

But on the other hand, we don't expect many temporary tables opened at the same time, so this list is supposed to be short, right? Then this optimization doesn't matter practically.

I used the TMP_TABLE_SHARE approach. It maintains a TABLE_SHARE-wise list of open temporary TABLEs. Pretty neat!

...

+ } + } + + DBUG_ASSERT(ref_count > 0); + + /* + 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); + + if (close_table(table)) + { + result= true; + goto end; + } + + /* There are other tables referencing this table share. */ + if (ref_count > 1) + { + result= false;

so, it is not considered an error? why?

The post-review patch does consider this as error and returns with ER_CANT_REOPEN_TABLE, if there is any other "active" TABLE object in the list.

...

+ goto end; + } + + if (delete_in_engine) + { + rm_temporary_table(hton, share->path.str); + } + + /* Delete the share from table share list */ + unlink<TABLE_SHARE>(&m_table_shares, share); + + free_table_share(share); + my_free(share); + + /* Decrement Slave_open_temp_table_definitions status variable count. */ + if (m_thd->rgi_slave) + { + thread_safe_decrement32(&slave_open_temp_table_definitions); + } + + result= false; + +end: + unlock_tables(); + + DBUG_RETURN(result); +} + + +/* + 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); +} + + +/** + Delete a temporary table. + + @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::rm_temporary_table(handlerton *base, const char *path) +{ + bool error= false; + handler *file; + char frm_path[FN_REFLEN + 1]; + + DBUG_ENTER("Temporary_tables::rm_temporary_table"); + + 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); +} + + +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->is_parallel_exec && + m_thd->wait_for_prior_commit()) + { + DBUG_RETURN(true); + } + + DBUG_RETURN(false); +} + + +void Temporary_tables::mark_tables_as_free_for_reuse() { + TABLE *table; + TABLE *next; + + DBUG_ENTER("mark_temp_tables_as_free_for_reuse"); + + if (m_thd->query_id == 0) + { + /* Thread has not executed any statement and has not used any tmp tables */ + DBUG_VOID_RETURN; + } + + lock_tables(); + + if (!m_thd->temporary_tables.is_empty())

do you need that? it checks whether m_table_shares or rgi_slave->rli->save_temp_table_shares is NULL. But after lock_tables() m_table_shares is equal to rgi_slave->rli->save_temp_table_shares, so it's enough to check m_table_shares. And if m_table_shares is NULL then m_opened_tables is also NULL, so the whole if() condition is redundant it only duplicates this while() below.

Fixed.

...

+ { + + table= m_opened_tables; + + while(table) { + next= table->next; + + if ((table->query_id == m_thd->query_id) && ! table->open_by_handler) + { + mysql_lock_remove(m_thd, m_thd->lock, table); + close_table(table);

I don't get it. Old code had mark_tmp_table_for_reuse() here, you have mysql_lock_remove and close_table. But temporary tables aren't locked, are they? And constantly closing/reopening temporary tables is a waste of resources, you can keep them open until they're dropped.

Right. Fixed.

...

+ } + + table= next; + } + } + + unlock_tables();

Old code had here:

if (rgi_slave) { /* Temporary tables are shared with other by sql execution threads. As a safety messure, clear the pointer to the common area. */ thd->temporary_tables= 0; }

shouldn't you have put it into unlock_tables() ? (yes, it was "messure" in the old comment, not my typo :)

I agree, resetting m_opened_tables & m_table_shares in unlock_tables(). And also fixed the typo. :)

...

+ + DBUG_VOID_RETURN; +} + + +void Temporary_tables::lock_tables() +{ + 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; + } +} + + +void Temporary_tables::unlock_tables() +{ + 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); + } +} +

Thanks again! - Nirbhay

Regards, Sergei Chief Architect MariaDB and security@mariadb.org