April 2016 - developers - lists.mariadb.org

Re: [Maria-developers] 1e883e9: MDEV-5535: Cannot reopen temporary table
by Sergei Golubchik 19 May '16

19 May '16

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 intention 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(a)mariadb.org

2 4

[Maria-developers] Please review MDEV-6353 my_ismbchar() and my_mbcharlen() refactoring
by Alexander Barkov 17 May '16

17 May '16

Hi Sergei, Please review a partial patch for MDEV-6353. It removes my_mbcharlen() in all remaining pieces of the code, except LOAD DATA and SELECT INTO OUTFILE. I'll do LOAD DATA and SELECT INTO OUTFILE in a separate patch. Thanks.

2 4

[Maria-developers] MDEV-9712 Performance degradation of nested NULLIF
by Alexander Barkov 05 May '16

05 May '16

Hello Sergei, Please have a look into a draft fix for MDEV-9712. The problem happens because in 10.1 execution time for various recursive stages (like walk, update_used_table and propagate_equal_fields) in NULLIF is O(recursion_level^2), because complexity is doubled on every recursion level when we copy args[0] to args[2]. This fix simplifies some stages to make them faster. It reduced execution time from 144 seconds to 12 seconds. The idea is just not to iterate through args[2] when it's just a copy of args[0] in these stages: - Item_func_nullif::walk - Item_func_nullif::update_used_tables I'm not fully sure that skipping args[2] is always correct though. For some transformers it may not be correct. <offtopic> Btw, walk() can be called from the partitioning code when arg_count is still 2 rather than 3. Looks suspicious. Perhaps there is a hidden bug behind that. </offtopic> The remaining heavy piece of the code is: Item* propagate_equal_fields(THD *thd, const Context &ctx, COND_EQUAL *cond) { Context cmpctx(ANY_SUBST, cmp.compare_type(), cmp.compare_collation()); args[0]->propagate_equal_fields_and_change_item_tree(thd, cmpctx, cond, &args[0]); args[1]->propagate_equal_fields_and_change_item_tree(thd, cmpctx, cond, &args[1]); args[2]->propagate_equal_fields_and_change_item_tree(thd, Context_identity(), cond, &args[2]); return this; } We cannot use the same approach here, because args[0] and args[2] are propagated with a different context (ANY_SUBST vs IDENTITY_SUBST). Not sure what to do with this. Perhaps, data types could have an extra attribute to tell that ANY_SUBST and IDENITTY_SUBST are equal. So, for example, for non-ZEROFILL integers, ANY_SUBST and IDENTITY_SUBST do the same thing and thus propagation could be done one time only. But this would be a partial solution, for a limited number of data types. Another option is to leave propagation as is. Another option is do nothing at all. If we rewrite the reported query as CASE, it will look heavy indeed. So it's Ok to expect heavy execution complexity. Thanks.

2 3

Re: [Maria-developers] 9058071: MDEV-9362: InnoDB tables using DATA_DIRECTORY created using MySQL 5.6 do not work with MariaDB 10.1
by Sergei Golubchik 02 May '16

02 May '16

Hi, Jan! I have various comments, see below. Nothing big, nothing that's really the approach you've taken. In fact, I like where it's going. But this is a huge and intrusive patch. I wonder, should we rather do it in 10.2? See below, you yourself write: On Feb 25, Jan Lindström wrote: > > Caution: Please take backups of your database before migrating 10.1.12. This is not what users normally do when upgrading from a GA version to the next minor GA version. Anyway, see comments about the patch below: > diff --git a/mysql-test/suite/encryption/r/innodb-bad-key-change.result b/mysql-test/suite/encryption/r/innodb-bad-key-change.result > index cf97918..39e08f4 100644 > --- a/mysql-test/suite/encryption/r/innodb-bad-key-change.result > +++ b/mysql-test/suite/encryption/r/innodb-bad-key-change.result > @@ -36,8 +36,7 @@ SELECT * FROM t1; > ERROR HY000: Got error 192 'Table encrypted but decryption failed. This could be because correct encryption management plugin is not loaded, used encryption key is not available or encryption method does not match.' from InnoDB > SHOW WARNINGS; > Level Code Message > -Warning 1812 Tablespace is missing for table 'test/t1' > -Warning 192 Table test/t1 is encrypted but encryption service or used key_id 2 is not available. Can't continue reading table. > +Warning 192 Table test/t1 is encrypted but encryption service or used key_id is not available. Can't continue reading table. why did that happen? > Error 1296 Got error 192 'Table encrypted but decryption failed. This could be because correct encryption management plugin is not loaded, used encryption key is not available or encryption method does not match.' from InnoDB > DROP TABLE t1; > # Start server with keys.txt > diff --git a/storage/innobase/include/dict0tableoptions.h b/storage/innobase/include/dict0tableoptions.h > new file mode 100644 > index 0000000..def0e39 > --- /dev/null > +++ b/storage/innobase/include/dict0tableoptions.h > @@ -0,0 +1,127 @@ > +/***************************************************************************** > + > +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 Street, Suite 500, Boston, MA 02110-1335 USA > + > +*****************************************************************************/ > + > +/**************************************************//** > +@file include/dict0tableoptions.h > +Definitions for the system table SYS_TABLE_OPTIONS > + > +Created 22/01/2006 Jan Lindström > +*******************************************************/ > + > +#ifndef dict0tableoptions_h > +#define dict0tableoptions_h > + > +#include "univ.i" > + > +/** Data structure to hold contents of SYS_TABLE_OPTIONS */ This is a move in the interesting direction. One of the ideas I was toying with for a few years is to remove .frm files for InnoDB tables. In MariaDB a storage engine can live without frm files if the engine supports table discovery. This is rather easy to implement, but InnoDB does not store the complete table definition, so the discovery will be lossy. Now with your SYS_TABLE_OPTIONS table we're getting closer to having the complete table definition saved inside InnoDB. And one step closer to frm-less InnoDB tables. > +struct dict_tableoptions_t{ > + table_id_t table_id; why do you need table_id in the option structure? > + /* true if table page compressed */ > + bool page_compressed; > + /* Used compression level if set */ > + ulonglong page_compression_level; > + /* dict0types.h: ATOMIC_WRITES_DEFAULT, _ON, or _OFF */ > + atomic_writes_t atomic_writes; > + /* fil0crypt.h: FIL_SPACE_ENCRYPTION_DEFAULT, _ON, or _OFF */ > + fil_encryption_t encryption; > + /* Used encryption key identifier if set */ > + ulonglong encryption_key_id; > + /* true if table options need to be stored */ why wouldn't they need to be? > + bool need_stored; > +}; > + > +/********************************************************************//** > +This function parses a SYS_TABLE_OPTIONS record, extracts necessary > +information from the record and returns it to the caller. > +@return error message or NULL if successfull */ > +UNIV_INTERN > +const char* > +dict_process_sys_tableoptions( > +/*==========================*/ > + mem_heap_t* heap, /*!< in/out: heap memory */ > + const rec_t* rec, /*!< in: current SYS_TABLE_OPTIONS rec */ > + dict_tableoptions_t* table_options); /*!< out: table options */ > + > +/********************************************************************//** > +Gets the table options from SYS_TABLE_OPTIONS based on table_id > +@return true if found, false if not found */ > +UNIV_INTERN > +bool > +dict_get_table_options( > +/*===================*/ > + table_id_t table_id, /*!< in: table id */ > + dict_tableoptions_t* options, /*!< out:table options */ > + bool fixed); /*!< in: can we fix the > + dictionary ? */ > + > +/********************************************************************//** > +Gets the table options from SYS_TABLE_OPTIONS > +@return true if found, false if not found */ > +UNIV_INTERN > +bool > +dict_get_table_options( > +/*===================*/ > + dict_table_t* table, /*!< in/out: table */ > + bool fixed); /*!< in: can we fix the > + dictionary ? */ > + > +/********************************************************************//** > +Update the record in SYS_TABLE_OPTIONS. > +@return DB_SUCCESS if OK, dberr_t if the update failed */ > +UNIV_INTERN > +dberr_t > +dict_update_tableoptions( > +/*=====================*/ > + const dict_table_t* table); /*!< in: table object */ > + > +/********************************************************************//** > +Insert record into SYS_TABLE_OPTIONS > +@return DB_SUCCESS if OK, dberr_t if the insert failed */ > +UNIV_INTERN > +dberr_t > +dict_insert_tableoptions( > +/*=====================*/ > + const dict_table_t* table, /*!< in: table object */ > + bool fixed); /*!< in: can we fix the > + dictionary ? */ > + > +/********************************************************************//** > +Update the table flags in SYS_TABLES. > +@return DB_SUCCESS if OK, dberr_t if the update failed */ > +UNIV_INTERN > +dberr_t > +dict_update_table_flags( > +/*=====================*/ > + const dict_table_t* table, /*!< in: table object */ > + bool fixed); /*!< in: can we fix the > + dictionary ? */ > + > +/********************************************************************//** > +Delete the record in SYS_TABLE_OPTIONS. > +@return DB_SUCCESS if OK, dberr_t if the update failed */ > +UNIV_INTERN > +dberr_t > +dict_delete_tableoptions( > +/*=====================*/ > + const dict_table_t* table, /*!< in: table object */ > + trx_t* trx, /*!< in: trx */ > + bool fixed); /*!< in: can we fix the > + dictionary ? */ > + > +#endif /* dict0tableoptions_h */ > + > diff --git a/storage/innobase/dict/dict0tableoptions.cc b/storage/innobase/dict/dict0tableoptions.cc > new file mode 100644 > index 0000000..527c5e3 > --- /dev/null > +++ b/storage/innobase/dict/dict0tableoptions.cc > @@ -0,0 +1,482 @@ > +/***************************************************************************** > + > +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 Street, Suite 500, Boston, MA 02110-1335 USA > + > +*****************************************************************************/ > + > +/**************************************************//** > +@file dict/dict0tableoptions.cc > +Function implementations for the system table SYS_TABLE_OPTIONS > + > +Created 22/01/2006 Jan Lindström > +*******************************************************/ > + > +#include "mysql_version.h" > +#include "btr0pcur.h" > +#include "btr0btr.h" > +#include "page0page.h" > +#include "mach0data.h" > +#include "dict0dict.h" > +#include "dict0boot.h" > +#include "dict0stats.h" > +#include "dict0mem.h" > +#include "rem0cmp.h" > +#include "srv0start.h" > +#include "srv0srv.h" > +#include "dict0crea.h" > +#include "dict0priv.h" > +#include "ha_prototypes.h" /* innobase_casedn_str() */ > +#include "fts0priv.h" > +#include "dict0tableoptions.h" > +#include "dict0load.h" > +#include "row0mysql.h" > + > +/********************************************************************//** > +This function parses a SYS_TABLE_OPTIONS record, extracts necessary > +information from the record and returns it to the caller. > +@return error message or NULL if successfull */ > +UNIV_INTERN > +const char* > +dict_process_sys_tableoptions( > +/*==========================*/ > + mem_heap_t* heap, /*!< in/out: heap memory */ > + const rec_t* rec, /*!< in: current SYS_TABLE_OPTIONS rec */ > + dict_tableoptions_t* table_options) /*!< out: table options */ > +{ > + const byte* field; > + ulint len=0; > + > + if (rec_get_deleted_flag(rec, 0)) { > + return("delete-marked record in SYS_TABLE_OPTIONS"); > + } > + > + if (rec_get_n_fields_old(rec) != DICT_NUM_FIELDS__SYS_TABLEOPTIONS) { > + return("wrong number of columns in SYS_TABLE_OPTIONS record"); > + } How does it work future-wise? How do you add new options to the table, say, in 10.2? > diff --git a/storage/innobase/api/api0api.cc b/storage/innobase/api/api0api.cc > index 739ea9f..4af32a5 100644 > --- a/storage/innobase/api/api0api.cc > +++ b/storage/innobase/api/api0api.cc > @@ -270,7 +271,7 @@ ib_open_table_by_name( > dict_table_t* table; > > table = dict_table_open_on_name(name, FALSE, FALSE, > - DICT_ERR_IGNORE_NONE); > + DICT_ERR_IGNORE_NONE, NULL); What does that mean that you pass NULL as the last argument? You won't know whether the table is encrypted or compressed - so you, basically won't be able to read the table? > > if (table != NULL && table->ibd_file_missing) { > table = NULL; > diff --git a/storage/innobase/buf/buf0buf.cc b/storage/innobase/buf/buf0buf.cc > index f4e7c0d..f1953af 100644 > --- a/storage/innobase/buf/buf0buf.cc > +++ b/storage/innobase/buf/buf0buf.cc > @@ -4677,7 +4678,7 @@ buf_page_io_complete( > "However key management plugin or used key_id %lu is not found or" > " used encryption algorithm or method does not match." > " Can't continue opening the table.", > - bpage->key_version); > + bpage->space, bpage->key_version); This seems to be an unrelated bugfix. And it seems to be already fixed in the latest 10.1 > > if (bpage->space > TRX_SYS_SPACE) { > if (corrupted) { > diff --git a/storage/innobase/dict/dict0dict.cc b/storage/innobase/dict/dict0dict.cc > index 2b72835..13f1ee6 100644 > --- a/storage/innobase/dict/dict0dict.cc > +++ b/storage/innobase/dict/dict0dict.cc > @@ -1187,6 +1189,16 @@ dict_table_open_on_name( > > /* If table is encrypted return table */ > if (ignore_err == DICT_ERR_IGNORE_NONE > + && !table->is_encrypted && options) { hmm, where else table->is_encrypted can be set? > + if ((options->encryption == FIL_SPACE_ENCRYPTION_ON || > + (options->encryption == FIL_SPACE_ENCRYPTION_DEFAULT && srv_encrypt_tables)) > + && !encryption_key_id_exists((unsigned int)options->encryption_key_id)) { > + table->is_encrypted = true; > + } > + } > + > + /* If table is encrypted return table */ > + if (ignore_err == DICT_ERR_IGNORE_NONE > && table->is_encrypted) { > /* Make life easy for drop table. */ > if (table->can_be_evicted) { > diff --git a/storage/innobase/dict/dict0load.cc b/storage/innobase/dict/dict0load.cc > index d8bd0a6..2b732f1 100644 > --- a/storage/innobase/dict/dict0load.cc > +++ b/storage/innobase/dict/dict0load.cc > @@ -56,7 +64,8 @@ static const char* SYSTEM_TABLE_NAME[] = { > "SYS_FOREIGN", > "SYS_FOREIGN_COLS", > "SYS_TABLESPACES", > - "SYS_DATAFILES" > + "SYS_DATAFILES", > + "SYS_TABLE_OPTIONS" May be "SYS_MARIADB_OPTIONS" ? Not really InnoDB sys table style, I agree, but guarantees no conflicts in the future. > }; > > /* If this flag is TRUE, then we will load the cluster index's (and tables') > @@ -2365,6 +2386,11 @@ dict_load_table( > btr_pcur_close(&pcur); > mtr_commit(&mtr); > > + if (table && options) { no need to do if (table), because table is guaranteed to be not NULL here > + ut_ad(table->table_options); > + memcpy(table->table_options, options, sizeof(dict_tableoptions_t)); > + } > + > if (table->space == 0) { > /* The system tablespace is always available. */ > } else if (table->flags2 & DICT_TF2_DISCARDED) { > diff --git a/storage/innobase/dict/dict0mem.cc b/storage/innobase/dict/dict0mem.cc > index 1724ac0..39d0d55 100644 > --- a/storage/innobase/dict/dict0mem.cc > +++ b/storage/innobase/dict/dict0mem.cc > @@ -85,7 +86,6 @@ dict_mem_table_create( > mem_heap_t* heap; > > ut_ad(name); > - ut_a(dict_tf_is_valid(flags)); why? > ut_a(!(flags2 & ~DICT_TF2_BIT_MASK)); > > heap = mem_heap_create(DICT_HEAP_SIZE); > diff --git a/storage/innobase/fil/fil0fil.cc b/storage/innobase/fil/fil0fil.cc > index 928c72c..4f75465 100644 > --- a/storage/innobase/fil/fil0fil.cc > +++ b/storage/innobase/fil/fil0fil.cc > @@ -716,27 +717,6 @@ fil_node_open_file( > ut_error; > } > > - if (UNIV_UNLIKELY(space->flags != flags)) { > - fprintf(stderr, > - "InnoDB: Error: table flags are 0x%lx" > - " in the data dictionary\n" > - "InnoDB: but the flags in file %s are 0x%lx!\n", > - space->flags, node->name, flags); do you still the check somewhere where SYS_TABLE_OPTIONS is compared with frm? > - > - ut_error; > - } > - > - if (UNIV_UNLIKELY(space->flags != flags)) { > - if (!dict_tf_verify_flags(space->flags, flags)) { > - fprintf(stderr, > - "InnoDB: Error: table flags are 0x%lx" > - " in the data dictionary\n" > - "InnoDB: but the flags in file %s are 0x%lx!\n", > - space->flags, node->name, flags); > - ut_error; > - } > - } > - > if (size_bytes >= FSP_EXTENT_SIZE * UNIV_PAGE_SIZE) { > /* Truncate the size to whole extent size. */ > size_bytes = ut_2pow_round(size_bytes, > @@ -1917,11 +1909,6 @@ fil_check_first_page( > flags = mach_read_from_4(FSP_HEADER_OFFSET + FSP_SPACE_FLAGS + page); > > if (UNIV_PAGE_SIZE != fsp_flags_get_page_size(flags)) { > - fprintf(stderr, > - "InnoDB: Error: Current page size %lu != " > - " page size on page %lu\n", > - UNIV_PAGE_SIZE, fsp_flags_get_page_size(flags)); > - why? > return("innodb-page-size mismatch"); > } > > @@ -3379,8 +3362,7 @@ fil_create_new_single_table_tablespace( > ulint size, /*!< in: the initial size of the > tablespace file in pages, > must be >= FIL_IBD_FILE_INITIAL_SIZE */ > - fil_encryption_t mode, /*!< in: encryption mode */ > - ulint key_id) /*!< in: encryption key_id */ > + const dict_table_t* table) /*!< in: table or NULL */ what does NULL mean here? > { > os_file_t file; > ibool ret; > @@ -3640,6 +3627,140 @@ fil_report_bad_tablespace( > (ulong) expected_id, (ulong) expected_flags); > } > > +/****************************************************************** > +Set flags for a tablespace */ > +static > +void > +fil_space_set_fsp_flags( > +/*=====================*/ > + ulint id, /*!< in: space id */ > + uint flags) /*!< in: fsp flags */ > +{ > + fil_space_t* space; > + > + ut_ad(fil_system); > + > + mutex_enter(&fil_system->mutex); > + > + space = fil_space_get_by_id(id); > + > + if (space != NULL) { > + space->flags = flags; > + } > + > + mutex_exit(&fil_system->mutex); > +} > + > +/****************************************************************** > +Update tablespace (fsp) flags on page 0 a bit more verbose comment would've been be nice. like, say that it's used to migrate from mariadb-flags-in-page-0 to sys_table_options approach. > +@return true if successfull, false if not */ > +static > +void > +fil_update_page0( > +/*=============*/ > + byte* page0, /*!< in: page 0 or NULL */ why do you need this page0 argument if it is *always* NULL? > + os_file_t data_file, /*!< in: data file */ > + ulint space, /*!< in: space id */ > + ulint flags, /*!< in: old fsp flags */ > + ulint fsp_flags)/*!< in: new fsp flags */ > +{ > + ulint psize = FSP_FLAGS_GET_PAGE_SSIZE_MARIADB(flags); > + ulint zip_size = FSP_FLAGS_GET_ZIP_SSIZE(flags); > + > + if (!psize) { > + psize = UNIV_PAGE_SIZE_ORIG; > + } > + > + fsp_flags = fsp_flags_set_page_size(fsp_flags, psize); > + > + if (flags != fsp_flags) { > + > + ib_logf(IB_LOG_LEVEL_INFO, > + "InnoDB: Adjusted space_id %lu tablespace flags from %lu to %lu.\n", > + space, flags, fsp_flags); > + > + mtr_t mtr; > + mtr_start(&mtr); > + > + if (!page0) { > + buf_block_t* block = buf_page_get_gen(space, > + zip_size, 0, > + RW_X_LATCH, > + NULL, > + BUF_GET, > + __FILE__, __LINE__, > + &mtr); > + page0 = buf_block_get_frame(block); > + } > + > + mlog_write_ulint(page0 + FSP_HEADER_OFFSET + FSP_SPACE_FLAGS, > + fsp_flags, MLOG_4BYTES, &mtr); > + /* Redo log this as bytewise update to page 0 > + followed by an MLOG_FILE_WRITE_FSP_FLAGS */ > + byte* log_ptr = mlog_open(&mtr, 11 + 8); > + if (log_ptr != NULL) { > + log_ptr = mlog_write_initial_log_record_fast( > + page0, > + MLOG_FILE_WRITE_FSP_FLAGS, > + log_ptr, &mtr); > + mach_write_to_4(log_ptr, fsp_flags); > + log_ptr += 4; > + mach_write_to_4(log_ptr, space); > + log_ptr += 4; > + mlog_close(&mtr, log_ptr); > + } > + > + mtr_commit(&mtr); > + lsn_t end_lsn = mtr.end_lsn; > + buf_flush_init_for_writing(page0, NULL, end_lsn); > + flags = mach_read_from_4(FSP_HEADER_OFFSET + FSP_SPACE_FLAGS + page0); > + ut_ad(flags == fsp_flags); > + > + /* Flush dirty page to the storage */ > + ulint n_pages = 0; > + ulint sum_pages = 0; > + bool success = false; > + do { > + success = buf_flush_list(ULINT_MAX, end_lsn, &n_pages); > + buf_flush_wait_batch_end(NULL, BUF_FLUSH_LIST); > + sum_pages += n_pages; > + } while (!success); > + > + fil_space_set_fsp_flags(space, fsp_flags); > + } > +} > + > +/****************************************************************** > +Parse a MLOG_FILE_WRITE_FSP_FLAGS log entry > +@return position on log buffer */ > +UNIV_INTERN > +byte* > +fil_parse_write_fsp_flags( > +/*======================*/ > + byte* ptr, /*!< in: Log entry start */ > + byte* end_ptr,/*!< in: Log entry end */ > + buf_block_t* block) /*!< in: buffer block */ > +{ > + /* check that redo log entry is complete */ > + uint entry_size = 4 + 4; // size of flags + space_id > + > + if (end_ptr - ptr < entry_size){ > + return NULL; > + } > + > + ulint flags = mach_read_from_4(ptr); > + ptr += 4; > + ulint space_id = mach_read_from_4(ptr); > + ptr += 4; > + > + ut_a(fsp_flags_is_valid(flags)); > + > + /* update fil_space memory cache with flags */ > + fil_space_set_fsp_flags(space_id, flags); > + > + return ptr; > +} > + > /********************************************************************//** > Tries to open a single-table tablespace and optionally checks that the > space id in it is correct. If this does not succeed, print an error message > diff --git a/storage/innobase/fts/fts0fts.cc b/storage/innobase/fts/fts0fts.cc > index cc4a4f9..f81fd31 100644 > --- a/storage/innobase/fts/fts0fts.cc > +++ b/storage/innobase/fts/fts0fts.cc > @@ -1981,7 +1982,13 @@ fts_create_one_index_table( > dict_mem_table_add_col(new_table, heap, "ilist", DATA_BLOB, > 4130048, 0); > > - error = row_create_table_for_mysql(new_table, trx, false, FIL_SPACE_ENCRYPTION_DEFAULT, FIL_DEFAULT_ENCRYPTION_KEY); > + /* Get default encryption key id if set */ > + if (new_table && new_table->table_options && new_table cannot be NULL here > + new_table->table_options->encryption_key_id == 0) { > + new_table->table_options->encryption_key_id = innobase_get_default_encryption_key_id(trx); do you want to do it here? may be better do it in row_create_table_for_mysql? > + } > + > + error = row_create_table_for_mysql(new_table, trx, false); > > if (error != DB_SUCCESS) { > trx->error_state = error; > diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc > index 527c5be..3d5aaee 100644 > --- a/storage/innobase/handler/ha_innodb.cc > +++ b/storage/innobase/handler/ha_innodb.cc > @@ -5459,6 +5460,43 @@ ha_innobase::innobase_initialize_autoinc() > } > > /*****************************************************************//** > +Set up the table options structure based on frm. */ > +UNIV_INTERN > +void > +ha_innobase::set_table_options( > +/*===========================*/ > + THD* thd, /*!< in: thd */ > + TABLE* table, /*!< in: table */ > + dict_tableoptions_t* options) /*!< in: table options */ > +{ > + ha_table_option_struct* moptions = table->s->option_struct; > + > + options->page_compressed = moptions->page_compressed; > + options->page_compression_level = moptions->page_compression_level; > + options->atomic_writes = (atomic_writes_t)moptions->atomic_writes; > + options->encryption = (fil_encryption_t)moptions->encryption; > + options->encryption_key_id = moptions->encryption_key_id; > + > + if (options->encryption_key_id == 0) { > + options->encryption_key_id = THDVAR(thd, default_encryption_key_id); > + } No, this is not how it works. The *server* sets the encryption_key_id based on the default_encryption_key_id. The engine does not need to bother. > + > + if (options->page_compression_level == 0) { > + options->page_compression_level = page_zip_level; > + } same here > + > + /* Table options should be stored if they are not same as > + defaults */ > + if (moptions->page_compressed || > + (options->atomic_writes == ATOMIC_WRITES_ON || > + options->atomic_writes == ATOMIC_WRITES_OFF) || > + (options->encryption == FIL_SPACE_ENCRYPTION_OFF || > + options->encryption == FIL_SPACE_ENCRYPTION_ON)) { > + options->need_stored = true; > + } > +} > + > +/*****************************************************************//** > Creates and opens a handle to a table which already exists in an InnoDB > database. > @return 1 if error, 0 if success */ > @@ -11603,6 +11647,10 @@ ha_innobase::check_table_options( > return "ENCRYPTION_KEY_ID"; > > } > + > + table_options->need_stored = true; > + > + table_options->need_stored = true; typo > } > > /* Check atomic writes requirements */ > @@ -20422,3 +20483,25 @@ ib_push_warning( > my_free(buf); > va_end(args); > } > + > +/********************************************************************//** > +Helper function to get default_encryption_key_id from THD > +(trx->mysql_thd). > +@return default_encryption_key_id from THD or > +FIL_DEFAULT_ENCRYPTION_KEY */ > +UNIV_INTERN > +ulint > +innobase_get_default_encryption_key_id( See comment above. You _probably_ don't need this method > +/*===================================*/ > + trx_t* trx) /*! in: trx */ > +{ > + ulint key_id = FIL_DEFAULT_ENCRYPTION_KEY; > + > + if (trx && trx->mysql_thd) { > + THD *thd = (THD *)trx->mysql_thd; > + > + key_id = THDVAR(thd, default_encryption_key_id); > + } > + > + return (key_id); > +} > diff --git a/storage/innobase/handler/i_s.cc b/storage/innobase/handler/i_s.cc > index ef69e7d..75baa9d 100644 > --- a/storage/innobase/handler/i_s.cc > +++ b/storage/innobase/handler/i_s.cc > @@ -9130,3 +9129,230 @@ UNIV_INTERN struct st_maria_plugin i_s_innodb_sys_semaphore_waits = > STRUCT_FLD(version_info, INNODB_VERSION_STR), > STRUCT_FLD(maturity, MariaDB_PLUGIN_MATURITY_BETA), > }; > + > +/** SYS_TABLE_OPTIONS ************************************************/ > +/* Fields of the dynamic table INFORMATION_SCHEMA.INNODB_SYS_TABLE_OPTIONS */ > +static ST_FIELD_INFO innodb_sys_tableoptions_fields_info[] = Good idea! > +{ > + // SYS_TABLE_OPTIONS_TABLE_ID 0 > + {STRUCT_FLD(field_name, "TABLE_ID"), > + STRUCT_FLD(field_length, MY_INT64_NUM_DECIMAL_DIGITS), > + STRUCT_FLD(field_type, MYSQL_TYPE_LONGLONG), > + STRUCT_FLD(value, 0), > + STRUCT_FLD(field_flags, MY_I_S_UNSIGNED), > + STRUCT_FLD(old_name, ""), > + STRUCT_FLD(open_method, SKIP_OPEN_TABLE)}, > + > + // SYS_TABLE_OPTIONS_PAGE_COMPRESSED 1 > + {STRUCT_FLD(field_name, "PAGE_COMPRESSED"), > + STRUCT_FLD(field_length, 7), > + STRUCT_FLD(field_type, MYSQL_TYPE_STRING), > + STRUCT_FLD(value, 0), > + STRUCT_FLD(field_flags, 0), > + STRUCT_FLD(old_name, ""), > + STRUCT_FLD(open_method, SKIP_OPEN_TABLE)}, > + > + // SYS_TABLE_OPTIONS_PAGE_COMPRESSION_LEVEL 2 > + {STRUCT_FLD(field_name, "PAGE_COMPRESSION_LEVEL"), > + STRUCT_FLD(field_length, MY_INT32_NUM_DECIMAL_DIGITS), > + STRUCT_FLD(field_type, MYSQL_TYPE_LONG), > + STRUCT_FLD(value, 0), > + STRUCT_FLD(field_flags, MY_I_S_UNSIGNED), > + STRUCT_FLD(old_name, ""), > + STRUCT_FLD(open_method, SKIP_OPEN_TABLE)}, > + > + // SYS_TABLE_OPTIONS_ATOMIC_WRITES 3 > + {STRUCT_FLD(field_name, "ATOMIC_WRITES"), > + STRUCT_FLD(field_length, 9), > + STRUCT_FLD(field_type, MYSQL_TYPE_STRING), > + STRUCT_FLD(value, 0), > + STRUCT_FLD(field_flags, 0), > + STRUCT_FLD(old_name, ""), > + STRUCT_FLD(open_method, SKIP_OPEN_TABLE)}, > + > + // SYS_TABLE_OPTIONS_ENCRYPTED 4 > + {STRUCT_FLD(field_name, "ENCRYPTED"), > + STRUCT_FLD(field_length, 9), > + STRUCT_FLD(field_type, MYSQL_TYPE_STRING), > + STRUCT_FLD(value, 0), > + STRUCT_FLD(field_flags, 0), > + STRUCT_FLD(old_name, ""), > + STRUCT_FLD(open_method, SKIP_OPEN_TABLE)}, > + > + // SYS_TABLE_OPTIONS_ENCRYPTION_KEY_ID 5 > + {STRUCT_FLD(field_name, "ENCRYPTION_KEY_ID"), > + STRUCT_FLD(field_length, MY_INT32_NUM_DECIMAL_DIGITS), > + STRUCT_FLD(field_type, MYSQL_TYPE_LONG), > + STRUCT_FLD(value, 0), > + STRUCT_FLD(field_flags, MY_I_S_UNSIGNED), > + STRUCT_FLD(old_name, ""), > + STRUCT_FLD(open_method, SKIP_OPEN_TABLE)}, > + > + END_OF_ST_FIELD_INFO > +}; > + > +/*******************************************************************//** > +Function to go through each record in SYS_TABLE_OPTIONS table, and fill the > +information_schema.innodb_sys_table_options table with related table information > +@return 0 on success */ > +static > +int > +i_s_dict_fill_sys_table_options( > +/*============================*/ > + THD* thd, /*!< in: thread */ > + TABLE_LIST* tables, /*!< in/out: tables to fill */ > + Item* ) /*!< in: condition (not used) */ > +{ > + btr_pcur_t pcur; > + const rec_t* rec; > + mem_heap_t* heap; > + mtr_t mtr; > + > + DBUG_ENTER("i_s_sys_table_options_fill_table"); > + RETURN_IF_INNODB_NOT_STARTED(tables->schema_table_name); > + > + /* deny access to user without PROCESS_ACL privilege */ > + if (check_global_access(thd, PROCESS_ACL, true)) { > + DBUG_RETURN(0); > + } > + > + heap = mem_heap_create(1000); > + mutex_enter(&(dict_sys->mutex)); > + mtr_start(&mtr); > + > + rec = dict_startscan_system(&pcur, &mtr, SYS_TABLE_OPTIONS); > + > + while (rec) { > + const char* err_msg; > + dict_tableoptions_t options; > + > + /* Create and populate a dict_tableoptions_t structure with > + information from SYS_TABLE_OPTIONS row */ > + memset(&options, 0, sizeof(dict_tableoptions_t)); > + > + err_msg = dict_process_sys_tableoptions( > + heap, rec, &options); > + > + mtr_commit(&mtr); > + mutex_exit(&dict_sys->mutex); > + > + if (!err_msg) { > + Field** fields = tables->table->field; > + > + OK(fields[SYS_TABLE_OPTIONS_TABLE_ID]->store((longlong) options.table_id)); > + OK(fields[SYS_TABLE_OPTIONS_PAGE_COMPRESSION_LEVEL]->store((ulint)options.page_compression_level)); > + OK(fields[SYS_TABLE_OPTIONS_ENCRYPTION_KEY_ID]->store((ulint)options.encryption_key_id)); > + > + if (options.page_compressed) { > + OK(field_store_string(fields[SYS_TABLE_OPTIONS_PAGE_COMPRESSED], "YES")); > + } else { > + OK(field_store_string(fields[SYS_TABLE_OPTIONS_PAGE_COMPRESSED], "NO")); > + } > + > + if (options.encryption == FIL_SPACE_ENCRYPTION_DEFAULT) { > + OK(field_store_string(fields[SYS_TABLE_OPTIONS_ENCRYPTED], "DEFAULT")); > + } else if (options.encryption == FIL_SPACE_ENCRYPTION_ON) { > + OK(field_store_string(fields[SYS_TABLE_OPTIONS_ENCRYPTED], "ON")); > + } else { > + OK(field_store_string(fields[SYS_TABLE_OPTIONS_ENCRYPTED], "OFF")); > + } > + > + if (options.atomic_writes == ATOMIC_WRITES_DEFAULT) { > + OK(field_store_string(fields[SYS_TABLE_OPTIONS_ATOMIC_WRITES], "DEFAULT")); > + } else if (options.atomic_writes == ATOMIC_WRITES_ON) { > + OK(field_store_string(fields[SYS_TABLE_OPTIONS_ATOMIC_WRITES], "ON")); > + } else { > + OK(field_store_string(fields[SYS_TABLE_OPTIONS_ATOMIC_WRITES], "OFF")); > + } > + > + OK(schema_table_store_record(thd, tables->table)); > + > + } else { > + push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, > + ER_CANT_FIND_SYSTEM_REC, "%s", > + err_msg); > + } > + > + mem_heap_empty(heap); > + > + /* Get the next record */ > + mutex_enter(&dict_sys->mutex); > + mtr_start(&mtr); > + rec = dict_getnext_system(&pcur, &mtr); > + } > + > + mtr_commit(&mtr); > + mutex_exit(&dict_sys->mutex); > + mem_heap_free(heap); > + > + DBUG_RETURN(0); > +} > +/*******************************************************************//** > +Bind the dynamic table INFORMATION_SCHEMA.INNODB_SYS_TABLE_OPTIONS > +@return 0 on success */ > +static > +int > +innodb_sys_table_options_init( > +/*============================*/ > + void* p) /*!< in/out: table schema object */ > +{ > + ST_SCHEMA_TABLE* schema; > + > + DBUG_ENTER("innodb_sys_table_options_init"); > + > + schema = (ST_SCHEMA_TABLE*) p; > + > + schema->fields_info = innodb_sys_tableoptions_fields_info; > + schema->fill_table = i_s_dict_fill_sys_table_options; > + > + DBUG_RETURN(0); > +} > + > +UNIV_INTERN struct st_maria_plugin i_s_innodb_sys_table_options = > +{ > + /* the plugin type (a MYSQL_XXX_PLUGIN value) */ > + /* int */ > + STRUCT_FLD(type, MYSQL_INFORMATION_SCHEMA_PLUGIN), > + > + /* pointer to type-specific plugin descriptor */ > + /* void* */ > + STRUCT_FLD(info, &i_s_info), > + > + /* plugin name */ > + /* const char* */ > + STRUCT_FLD(name, "INNODB_SYS_TABLE_OPTIONS"), > + > + /* plugin author (for SHOW PLUGINS) */ > + /* const char* */ > + STRUCT_FLD(author, maria_plugin_author), > + > + /* general descriptive text (for SHOW PLUGINS) */ > + /* const char* */ > + STRUCT_FLD(descr, "InnoDB SYS_TABLE_OPTIONS"), > + > + /* the plugin license (PLUGIN_LICENSE_XXX) */ > + /* int */ > + STRUCT_FLD(license, PLUGIN_LICENSE_GPL), > + > + /* the function to invoke when plugin is loaded */ > + /* int (*)(void*); */ > + STRUCT_FLD(init, innodb_sys_table_options_init), > + > + /* the function to invoke when plugin is unloaded */ > + /* int (*)(void*); */ > + STRUCT_FLD(deinit, i_s_common_deinit), > + > + /* plugin version (for SHOW PLUGINS) */ > + /* unsigned int */ > + STRUCT_FLD(version, INNODB_VERSION_SHORT), > + > + /* struct st_mysql_show_var* */ > + STRUCT_FLD(status_vars, NULL), > + > + /* struct st_mysql_sys_var** */ > + STRUCT_FLD(system_vars, NULL), > + > + /* Maria extension */ > + STRUCT_FLD(version_info, INNODB_VERSION_STR), > + STRUCT_FLD(maturity, MariaDB_PLUGIN_MATURITY_BETA), > +}; > diff --git a/storage/innobase/include/dict0dict.h b/storage/innobase/include/dict0dict.h > index b15d364..fa65884 100644 > --- a/storage/innobase/include/dict0dict.h > +++ b/storage/innobase/include/dict0dict.h > @@ -944,15 +948,8 @@ dict_tf_set( > ulint* flags, /*!< in/out: table */ > rec_format_t format, /*!< in: file format */ > ulint zip_ssize, /*!< in: zip shift size */ > - bool remote_path, /*!< in: table uses DATA DIRECTORY > + bool remote_path); /*!< in: table uses DATA DIRECTORY > */ > - bool page_compressed,/*!< in: table uses page compressed > - pages */ > - ulint page_compression_level, /*!< in: table page compression > - level */ > - ulint atomic_writes) /*!< in: table atomic > - writes option value*/ > - __attribute__((nonnull)); Why do you remove nonnull attributes? They help gcc to optimize the code > /********************************************************************//** > Convert a 32 bit integer table flags to the 32 bit integer that is > written into the tablespace header at the offset FSP_SPACE_FLAGS and is > diff --git a/storage/innobase/include/dict0dict.ic b/storage/innobase/include/dict0dict.ic > index a3a3446..3580081 100644 > --- a/storage/innobase/include/dict0dict.ic > +++ b/storage/innobase/include/dict0dict.ic > @@ -685,11 +629,7 @@ dict_sys_tables_type_validate( > ulint zip_ssize = DICT_TF_GET_ZIP_SSIZE(type); > ulint atomic_blobs = DICT_TF_HAS_ATOMIC_BLOBS(type); > ulint unused = DICT_TF_GET_UNUSED(type); > - ulint page_compression = DICT_TF_GET_PAGE_COMPRESSION(type); > - ulint page_compression_level = DICT_TF_GET_PAGE_COMPRESSION_LEVEL(type); By the way, what about page compression in MySQL InnoDB? where is that stored? will you support those tables? > - ulint atomic_writes = DICT_TF_GET_ATOMIC_WRITES(type); > - > - ut_a(atomic_writes <= ATOMIC_WRITES_OFF); > + ulint unused_mariadb = DICT_TF_GET_UNUSED_MARIADB(type); > > /* The low order bit of SYS_TABLES.TYPE is always set to 1. > If the format is UNIV_FORMAT_B or higher, this field is the same > diff --git a/storage/innobase/include/fil0fil.h b/storage/innobase/include/fil0fil.h > index 7fe0cc8..bc7f317 100644 > --- a/storage/innobase/include/fil0fil.h > +++ b/storage/innobase/include/fil0fil.h > @@ -39,7 +39,7 @@ Created 10/25/1995 Heikki Tuuri > #include "ibuf0types.h" > #include "log0log.h" > #endif /* !UNIV_HOTBACKUP */ > - > +#include "my_crypt.h" why? > #include <list> > > // Forward declaration > diff --git a/storage/innobase/srv/srv0start.cc b/storage/innobase/srv/srv0start.cc > index d7b37b5..29ce93d 100644 > --- a/storage/innobase/srv/srv0start.cc > +++ b/storage/innobase/srv/srv0start.cc > @@ -2537,7 +2543,16 @@ innobase_start_or_create_for_mysql(void) > > sys_datafiles_created = true; > > - /* This function assumes that SYS_DATAFILES exists */ > + err = dict_create_or_check_sys_table_options(); > + > + if (err != DB_SUCCESS) { > + return(err); > + } > + > + sys_table_options_created = true; > + > + /* This function assumes that SYS_DATAFILES > + and SYS_TABLE_OPTIONS exists */ "exist" > dict_check_tablespaces_and_store_max_id(dict_check); > } Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 3

Re: [Maria-developers] f9f290b: MDEV-9851: CREATE USER w/o IDENTIFIED BY clause causes crash when using cracklib plugin
by Sergei Golubchik 29 Apr '16

29 Apr '16

Hi, Nirbhay! On Mar 31, Nirbhay Choubey wrote: > revision-id: f9f290b6828eeb57cba611d006d2a9301dc52244 (mariadb-10.1.13-3-gf9f290b) > parent(s): f4d5fe277599da4549c97c660f324c88cf9a2542 > author: Nirbhay Choubey > committer: Nirbhay Choubey > timestamp: 2016-03-31 18:03:44 -0400 > message: > > MDEV-9851: CREATE USER w/o IDENTIFIED BY clause causes crash when using cracklib plugin > > Add a check for NULL password. > > diff --git a/plugin/cracklib_password_check/cracklib_password_check.c b/plugin/cracklib_password_check/cracklib_password_check.c > index c593173..c192cdf 100644 > --- a/plugin/cracklib_password_check/cracklib_password_check.c > +++ b/plugin/cracklib_password_check/cracklib_password_check.c > @@ -33,7 +33,8 @@ static int crackme(MYSQL_LEX_STRING *username, MYSQL_LEX_STRING *password) > if ((host= strchr(user, '@'))) > *host++= 0; > > - if ((res= FascistCheckUser(password->str, dictionary, user, host))) > + if ((password->str == NULL) || // No password > + (res= FascistCheckUser(password->str, dictionary, user, host))) > { > my_printf_error(ER_NOT_VALID_PASSWORD, "cracklib: %s", > MYF(ME_JUST_WARNING), res); You forgot to fix the simple_password_check plugin. And if all plugins need to do the same check - it's a strong indication that this should've been done in the server. So, please, fix this in sql_acl.cc instead. Like this: - struct validation_data data= { &user->user, &user->pwtext }; + struct validation_data data= { &user->user, user->pwtext.str ? &user->pwtext : &empy_lex_str }; Ok to push with this fix and your test case. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1

Re: [Maria-developers] 95e1c46: MDEV-9898 SET ROLE NONE can crash mysqld.
by Sergei Golubchik 29 Apr '16

29 Apr '16

Hi, Alexey! On Apr 28, Alexey Botchkov wrote: > revision-id: 95e1c46c1709e7244332a025a527cacceab7da60 (mariadb-10.1.13-14-g95e1c46) > parent(s): 646c4cea58afbb369021a3d7b5ccbbf6aed708d4 > committer: Alexey Botchkov > timestamp: 2016-04-28 13:36:36 +0400 > message: > > MDEV-9898 SET ROLE NONE can crash mysqld. > The check_user_can_set_role() used find_user_exact() to get the > permissions for the SET ROLE NONE command. > Which returned NULL too often, for instance when user > authenticated as 'user'@'%'. > Now we use find_user_or_anon() instead. No, this is wrong. SET ROLE NONE should use the same rule as SET ROLE name. And the latter uses acl_user->wild_eq(user, host, ip). Thus SET ROLE NONE should find the user with ACL_USER::wild_eq, that is, it should use find_user_wild(). Ok to push with find_user_wild(). Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MariaDB packages for Alpine Linux
by Guillaume Lefranc 29 Apr '16

29 Apr '16

Hello, there is an outstanding request in docker-library/mariadb for Alpine support: https://github.com/docker-library/mariadb/issues/53 What I know is that we currently don't build packages for Alpine. Alpine is going to be the platform of choice for Docker containers in the future (you can read about the various advantages of Alpine here: https://www.brianchristner.io/docker-is-moving-to-alpine-linux/). Federico Razzoli has managed to make MariaDB 10.1 work in Alpine with some caveats (no tokudb, no oqgraph, no galera). You can look at his work here https://hub.docker.com/r/santec/mariadb-alpine/. I admit I don't know about portability of Galera lib on Alpine - should probably talk to Codership about that. Is there any way we can make this happen?

1 0

[Maria-developers] Why reserved keywords: UTC_TIME, UTC_DATE, UTC_DATETIME
by Alexander Barkov 29 Apr '16

29 Apr '16

Hello, I noticed that UTC_TIME, UTC_DATE, UTC_DATETIME are reserved keywords: MariaDB [test]> SELECT * FROM utc_time; ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'utc_time' at line 1 I think they should not. - These keywords are not mentioned in the SQL standard - They are not involved in any complex grammar that would prevent them from being non-reserved keywords Looks like a bug... Any objections to make them non-reserved keywords in 10.2?

6 8

Re: [Maria-developers] GSoC 2016: Unique indexes for blobs
by Sergei Golubchik 27 Apr '16

27 Apr '16

Hi, Shubham! On Apr 24, Shubham Barai wrote: > Hi Sergei, > > I have created github repository with a new branch for my prototype. Great, what's the url? btw, please, use "Reply to all", so that the whole list could see your progress. Also google recommends you to blog regularly about what you're doing: http://write.flossmanuals.net/gsocstudentguide/time-management-for-students/ but if you don't like that, you can send weekly progress report emails instead. Whatever suits you best. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 6

[Maria-developers] GSoC Student Introduction
by Charles Muurmu 27 Apr '16

27 Apr '16

Hi everyone, I am Charles Muurmu also known as laserlight on IRC and I was selected for GSoC 2016 to work on migrating MariaDB Cassandra storage engine from the Thrift API to Datastax driver. I will be mentored by Spetrunia. My goals for this community bonding period will be get myself ready by accomplishing the following tasks: Fork and clone the server code. Try to build the code. Setup a blog on which I will post weekly reports on progress. Study the Datastax C++ driver API and documentation. Study the ha_cassandra.xx code and storage engines architecture. I believe these will prepare me for my first task when coding begins which will be to integrate the Datastax C++ driver into the MariaDB Server build system. I equally look forward to more instructions from my Mentor on how we will work on this task and lastly, I would love to congratulate the other students selected with MariaDB. Cheers, laserlight.

1 0