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.
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.
> 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.
> 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.
> 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?
> 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.
>
> 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.
> }
> - 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?
> 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?
> 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?
> - 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
> 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
> + /* 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
> 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?
> }
> 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?
> + 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?
> }
> }
> 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
> #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?
> #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...
> @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?
> 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;
}
>
> /** 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)
>
> 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
> /* 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.
> 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?
> +
> + /* 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 :)
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.
> + 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?
> +
> + /*
> + 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
> + 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
> + {
> + 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?
now you need to iterate the list of tables twice.
> +
> + 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()
> + 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?
> +
> + 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.
> +
> + 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
> + {
> + 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.
> +
> + 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)
> +
> + 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
> + 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)
> + 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
> + 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.
> +
> + @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)
> +
> + @retval 0 the table was found and dropped successfully.
> + @retval -1 the table is in use by a outer query
retval is wrong
> +*/
> +
> +
> +/*
> + @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 :(
> +{
> + 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.
> + 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.
> + }
> + }
> +
> + 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?
> + 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.
> + {
> +
> + 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.
> + }
> +
> + 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 :)
> +
> + 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);
> + }
> +}
> +
Regards,
Sergei
Chief Architect MariaDB
and security@mariadb.org