maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #09641
Re: 1e883e9: MDEV-5535: Cannot reopen temporary table
Hi Serg,
On Wed, Mar 30, 2016 at 7:12 AM, Sergei Golubchik <serg@xxxxxxxxxxx> 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@xxxxxxxxxxx
>
Follow ups
References
