- developers - lists.mariadb.org

Re: [Maria-developers] [Commits] c285dbe: MDEV-5535: Cannot reopen temporary table
by Sergei Golubchik 08 Jun '16

08 Jun '16

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

3 5

[Maria-developers] MATCH returns -1
by Alexander Barkov 08 Jun '16

08 Jun '16

Hello Sergei, This script: DROP TABLE IF EXISTS t1; CREATE TABLE t1 (a VARCHAR(30), b DOUBLE); INSERT INTO t1 values('aaaa bbbb cccc dddd', MATCH (a) AGAINST('bbbb' IN BOOLEAN MODE)); SELECT * FROM t1; returns: +---------------------+------+ | a | b | +---------------------+------+ | aaaa bbbb cccc dddd | -1 | +---------------------+------+ What does this -1 mean? Is this a bug? Thanks!

3 5

[Maria-developers] GSoc weekly reports (Aggregate Functions)
by Varun Gupta 08 Jun '16

08 Jun '16

Hi, Here is the week 2 report WEEK 2 dire = mariadb/server task - adding 'aggregate' field to the mysql.proc table. sub tasks 1) aggregate field ---> mysql.proc table 2)create function -----> aggregate and non-aggregate 3)show create function ----> aggregate and non-aggregate 4)drop function -----> aggregate and non-aggregate 5)alter function -----> aggregate and non-aggregate For the user he has two options for AGGREGATE (TRUE,FALSE); details 1) Added the field 'aggregate' to the proc table . Order should be maintained for the fields added. 2) Also included the column in the enumberation of db storage. 3) Added a new is_aggregate characteristic to the lex structure, which would help in identifying if a function is aggregate or not. 4) So after parsing the CREATE FUNCTION query ,adding to the table YES if function is aggregate else adding NO to the proc table. 5) SHOW FUNCTION query to load the aggregate field. We have the is_aggregate field to load the value in it. 6) DROP FUNCTION, required no changes to be made as the primary key does not have the aggregate field 7) ALTER FUNCTION , required adding the syntax for the AGGREGATE characteristic , so rules were added to add this additional alter characteristic. Then we had to update the routine row in the proc table for all the field that are going to be altered . So we store the new altered fields to the proc table for the stated fields.

2 1

[Maria-developers] Why Max Length of virtual column string is so small
by Sachin Setia 06 Jun '16

06 Jun '16

Hello Everyone, Actually I was thinking why there is so small limit on virtual column string.Does it have any design implication or some other reason Regards sachin

2 1

Re: [Maria-developers] Aggregate Stored Functions
by Varun Gupta 06 Jun '16

06 Jun '16

Hi, I have added the test where I have put both ALTER,CREATE and SHOW FUNCTION queries. Should I make separate test for the queries or clubbing them in one is fine . On Sat, Jun 4, 2016 at 7:25 PM, Varun Gupta <varungupta1803(a)gmail.com> wrote: > Hi, > I have also written a test for the alter, show and create functions. I > have pushed it, also I am about to go through some tests already written > and try to make the tests similar to them. > > On Sat, Jun 4, 2016 at 5:34 PM, Varun Gupta <varungupta1803(a)gmail.com> > wrote: > >> Hi, >> I am done with ALTER FUNCTION, i have done this by adding new field. I >> have committed the code. Please review. >> Also I wanted to discuss more about the new filed which i added to the >> sp_chistics struct. >> Also I am writing tests for the alter queries. I would also need some >> more suggestions about what I should do next . >> >> >> On Sat, Jun 4, 2016 at 10:49 AM, Varun Gupta <varungupta1803(a)gmail.com> >> wrote: >> >>> I do but to update the function, the function prototype of >>> sp_update_routine() is >>> sp_update_routine(THD *thd, stored_procedure_type >>> type, sp_name *name,st_sp_chistics *chistics), so to pass if the field to >>> be updated is AGGREGATE I have to have some way to send the updated value >>> to the above function. >>> >>> On Sat, Jun 4, 2016 at 2:24 AM, Sanja <sanja.byelkin(a)gmail.com> wrote: >>> >>>> Don't you have it in sp_head? >>>> Am 03.06.2016 22:33 schrieb "Varun Gupta" <varungupta1803(a)gmail.com>: >>>> >>>>> Hi, >>>>> I need a bit of suggestion on how to pass that we are changing the >>>>> aggregate field, from the parser I need to send the field so in the >>>>> mysql_execute_function I can call the sp_update_routine. >>>>> >>>>> My ideas is >>>>> 1) addition of one field to lex structure for aggregate. >>>>> 2) instead of having is_aggregate in sp_head, we could put that field >>>>> in in sp_name. >>>>> >>>>> On Fri, Jun 3, 2016 at 11:41 PM, Varun Gupta <varungupta1803(a)gmail.com >>>>> > wrote: >>>>> >>>>>> Hi, >>>>>> I have added the syntax , patch committed, please review it :) >>>>>> >>>>>> On Fri, Jun 3, 2016 at 10:35 PM, Varun Gupta < >>>>>> varungupta1803(a)gmail.com> wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> After going through the code I come to the conclusion that a rule >>>>>>> for AGGREGATE field needs to be added to the ALTER FUNCTION >>>>>>> characteristics. So I am going forward with adding this syntax. >>>>>>> >>>>>>> On Fri, Jun 3, 2016 at 6:14 PM, Sanja <sanja.byelkin(a)gmail.com> >>>>>>> wrote: >>>>>>> >>>>>>>> Hi! >>>>>>>> >>>>>>>> ALTER TABLE usually has the same syntax as CREATE (at least I do >>>>>>>> not remember adding option like here) so I doubts that it is correct. >>>>>>>> >>>>>>>> On Fri, Jun 3, 2016 at 2:40 PM, Varun Gupta < >>>>>>>> varungupta1803(a)gmail.com> wrote: >>>>>>>> >>>>>>>>> Hi, >>>>>>>>> For the alter function func_name field value, >>>>>>>>> the syntax we have is >>>>>>>>> ALTER FUNCTION_SYM sp_name sp_a_chistics >>>>>>>>> >>>>>>>>> sp_a_chistics: >>>>>>>>> /* Empty */ {} >>>>>>>>> | sp_a_chistics sp_chistic {} >>>>>>>>> >>>>>>>>> sp_a_chistics: >>>>>>>>> | AGGREGSTE_SYM option >>>>>>>>> option: >>>>>>>>> | YES >>>>>>>>> | NO >>>>>>>>> >>>>>>>>> or option could be any string. >>>>>>>>> >>>>>>>>> >>>>>>>>> On Fri, Jun 3, 2016 at 1:22 PM, Varun Gupta < >>>>>>>>> varungupta1803(a)gmail.com> wrote: >>>>>>>>> >>>>>>>>>> Hi, >>>>>>>>>> The error is fixed , now show works for aggregate functions too :) >>>>>>>>>> >>>>>>>>>> On Fri, Jun 3, 2016 at 1:01 PM, Varun Gupta < >>>>>>>>>> varungupta1803(a)gmail.com> wrote: >>>>>>>>>> >>>>>>>>>>> For non aggregate function , show output is correct and when I >>>>>>>>>>> run the query SELECT * from mysql.proc table, aggregate field shows NO. >>>>>>>>>>> For aggregate function, when I run the query SELECT * from >>>>>>>>>>> mysql.proc table, aggregate field shows YES. >>>>>>>>>>> >>>>>>>>>>> On Fri, Jun 3, 2016 at 12:56 PM, Varun Gupta < >>>>>>>>>>> varungupta1803(a)gmail.com> wrote: >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Yes it does, but I am not adding anything to the buffer in that >>>>>>>>>>>> case. >>>>>>>>>>>> >>>>>>>>>>>> On Fri, Jun 3, 2016 at 12:53 PM, Sanja <sanja.byelkin(a)gmail.com >>>>>>>>>>>> > wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Could you be more specific? what test? >>>>>>>>>>>>> >>>>>>>>>>>>> Does your code change SHOW for non-aggregate function? >>>>>>>>>>>>> >>>>>>>>>>>>> On Fri, Jun 3, 2016 at 9:20 AM, Varun Gupta < >>>>>>>>>>>>> varungupta1803(a)gmail.com> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>> To solve the problem with the buffer ,can you tell me is >>>>>>>>>>>>>> there any tests that are run before I run my own tests . >>>>>>>>>>>>>> I meant if I change the value of >>>>>>>>>>>>>> the buf->append(STRING_WITH_LEN("FUNCTION ")); >>>>>>>>>>>>>> to buf->append(STRING_WITH_LEN("pUNCTION ")); , even then i get the same >>>>>>>>>>>>>> error. >>>>>>>>>>>>>> >>>>>>>>>>>>>> On Fri, Jun 3, 2016 at 1:32 AM, Varun Gupta < >>>>>>>>>>>>>> varungupta1803(a)gmail.com> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> Have you gone through the patch. Do you like that I have >>>>>>>>>>>>>>> added a field agg_res to the functions ? Or should I find some other to >>>>>>>>>>>>>>> figure it out . >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Thu, Jun 2, 2016 at 10:59 PM, Varun Gupta < >>>>>>>>>>>>>>> varungupta1803(a)gmail.com> wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> patch commited . >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On Thu, Jun 2, 2016 at 10:53 PM, Vicențiu Ciorbaru < >>>>>>>>>>>>>>>> cvicentiu(a)gmail.com> wrote: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Strange. Need to try this out myself. If you don't figure >>>>>>>>>>>>>>>>> it out I'll come back later tonight with some info. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Make sure the lengths are indeed correct. Maybe you are >>>>>>>>>>>>>>>>> getting a segfault there. Or maybe some code expects that string to have a >>>>>>>>>>>>>>>>> different length for some reason. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Vicentiu >>>>>>>>>>>>>>>>> On Thu, 2 Jun 2016 at 20:21, Varun Gupta < >>>>>>>>>>>>>>>>> varungupta1803(a)gmail.com> wrote: >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> If I just remove the buf->append statement it works . >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> On Thu, Jun 2, 2016 at 10:48 PM, Vicențiu Ciorbaru < >>>>>>>>>>>>>>>>>> cvicentiu(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Hi Varun, >>>>>>>>>>>>>>>>>>> Your problem is before this code I think. Does it work >>>>>>>>>>>>>>>>>>> without your changes in show_create_sp? >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Vicentiu >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> On Thu, 2 Jun 2016 at 20:16, Varun Gupta < >>>>>>>>>>>>>>>>>>> varungupta1803(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> In the file sp.cc, function show_create_sp, we have the >>>>>>>>>>>>>>>>>>>> buffer that holds the output . >>>>>>>>>>>>>>>>>>>> 1) First i have the buf->alloc , here I change the >>>>>>>>>>>>>>>>>>>> size to also include AGGREGATE. >>>>>>>>>>>>>>>>>>>> 2) Then i do buf->append(STRING_WITH_LEN("AGGREGATE >>>>>>>>>>>>>>>>>>>> ")), but this gives an error, >>>>>>>>>>>>>>>>>>>> ERROR : At line 78: query 'call mtr.check_testcase()' >>>>>>>>>>>>>>>>>>>> failed: 1457: Failed to load routine mtr.check_testcase. The table >>>>>>>>>>>>>>>>>>>> mysql.proc is missing, corrupt, or contains bad data (internal code -6) >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> On Thu, Jun 2, 2016 at 10:33 PM, Vicențiu Ciorbaru < >>>>>>>>>>>>>>>>>>>> cvicentiu(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> Hi varun, >>>>>>>>>>>>>>>>>>>>> Can you be more specific? Lots of things can go wrong. >>>>>>>>>>>>>>>>>>>>> Vicentiu >>>>>>>>>>>>>>>>>>>>> On Thu, 2 Jun 2016 at 20:02, Varun Gupta < >>>>>>>>>>>>>>>>>>>>> varungupta1803(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>>>>>>>>> for the show_create_routine , i am trying to add the >>>>>>>>>>>>>>>>>>>>>> AGGREGATE string to the buffer , but I am getting an error. For the query >>>>>>>>>>>>>>>>>>>>>> SHOW CREATE FUNCTION func_name , is there a script where I need to make >>>>>>>>>>>>>>>>>>>>>> changes ? >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> On Thu, Jun 2, 2016 at 4:02 PM, Varun Gupta < >>>>>>>>>>>>>>>>>>>>>> varungupta1803(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>>>>>>>>>> I have fixed the errors and now the aggregate field >>>>>>>>>>>>>>>>>>>>>>> is getting stored in the database. I have made a commit. >>>>>>>>>>>>>>>>>>>>>>> Now I am working on the show_create_sp to include >>>>>>>>>>>>>>>>>>>>>>> aggregate in the output. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> On Thu, Jun 2, 2016 at 12:48 PM, Sanja < >>>>>>>>>>>>>>>>>>>>>>> sanja.byelkin(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> /* DB storage of Stored PROCEDUREs and FUNCTIONs */ >>>>>>>>>>>>>>>>>>>>>>>> enum >>>>>>>>>>>>>>>>>>>>>>>> { >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> On Thu, Jun 2, 2016 at 9:16 AM, Varun Gupta < >>>>>>>>>>>>>>>>>>>>>>>> varungupta1803(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>>>>>>>>>>>> In the second point which place are you talking >>>>>>>>>>>>>>>>>>>>>>>>> about >>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> On Thu, Jun 2, 2016 at 12:33 PM, Sanja < >>>>>>>>>>>>>>>>>>>>>>>>> sanja.byelkin(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>> Above assert means that Aria engine is not >>>>>>>>>>>>>>>>>>>>>>>>>> initiated (it would be better to have whole stack trace). >>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>> 1) Primary key is not a field but index :) >>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>> 2) I do not see that you changed enum where field >>>>>>>>>>>>>>>>>>>>>>>>>> of the database numbered. >>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>> On Thu, Jun 2, 2016 at 6:59 AM, Varun Gupta < >>>>>>>>>>>>>>>>>>>>>>>>>> varungupta1803(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>>>>>>>>>>>>>> I have put the aggregate field just before the >>>>>>>>>>>>>>>>>>>>>>>>>>> primary key field . I hope this is fine . I have pushed the change on >>>>>>>>>>>>>>>>>>>>>>>>>>> github. >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> On Thu, Jun 2, 2016 at 12:59 AM, Varun Gupta < >>>>>>>>>>>>>>>>>>>>>>>>>>> varungupta1803(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>> This is the error that leads to sigabrt error >>>>>>>>>>>>>>>>>>>>>>>>>>>> mysqld: >>>>>>>>>>>>>>>>>>>>>>>>>>>> /home/batman/gsoc/MARIADB/server/storage/maria/ma_create.c:83: >>>>>>>>>>>>>>>>>>>>>>>>>>>> maria_create: Assertion `maria_inited' failed. >>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>> On Thu, Jun 2, 2016 at 12:06 AM, Sanja < >>>>>>>>>>>>>>>>>>>>>>>>>>>> sanja.byelkin(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>> mtr has --boot-ddd option or just find other >>>>>>>>>>>>>>>>>>>>>>>>>>>>> way to say us an error which prevents bootstrap SQL running >>>>>>>>>>>>>>>>>>>>>>>>>>>>> Am 01.06.2016 20:33 schrieb "Varun Gupta" < >>>>>>>>>>>>>>>>>>>>>>>>>>>>> varungupta1803(a)gmail.com>: >>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I have put the field at the end of the table >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> but still I am not able to install system databases. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Could not install system database from >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> /home/batman/gsoc/MARIADB/server/mysql-test/var/tmp/bootstrap.sql . I have >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> seen this file looks fine to me . >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Wed, Jun 1, 2016 at 11:04 PM, Vicențiu >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Ciorbaru <vicentiu(a)mariadb.org> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi Varun, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Adding extra fields to the proc table should >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> be done at the end of the table. We have code that assumes the table has >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> columns in that particular order. This is most likely why you are getting >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the failures. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Vicentiu >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Wed, 1 Jun 2016 at 20:05 Sanja < >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> sanja.byelkin(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1) Why you decided to put it in the middle? >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> for more incompatibility? >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 2) Do you know that space should be put >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> after come and not before? (it is about SQL you changed). >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 3) check that there is no really >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> mtr.check_testcase if it is absent then check how your changes broke >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> creation of procedures (SQL of creation the procedure you can find >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> in mysql-test/include/mtr_check.sql >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> If it is there then check why code of >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> finding procedure become broken. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Wed, Jun 1, 2016 at 6:13 PM, Varun Gupta >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> <varungupta1803(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hey, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I have made changes to the code and have >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> pushed the code on github, I am stuck with an error which I can't figure >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> out . The error is >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> At line 78: query 'call >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> mtr.check_testcase()' failed: 1305: PROCEDURE mtr.check_testcase does not >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> exist >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> not ok >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Wed, Jun 1, 2016 at 3:40 PM, Sanja < >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> sanja.byelkin(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yes, called from mysql_install_db >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Wed, Jun 1, 2016 at 11:59 AM, Varun >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Gupta <varungupta1803(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Sanja , in mysql_system_tables.sql is >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> where I found tables are created? Are u talking about this script. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Wed, Jun 1, 2016 at 3:17 PM, Sanja < >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> sanja.byelkin(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Probably you have to change code where >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the table is created (if I remember correctly it is bootstrap script), then >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> recreate database (mysql-test-run is doing it for you) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> then upgrade script should be fixed but >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> it can be done later. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Wed, Jun 1, 2016 at 11:44 AM, Varun >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Gupta <varungupta1803(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Well I am getting an error which says >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> that column expected 21 but are 20. Do I need to drop the already created >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> mysql.proc table? In my opinion I should. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Wed, Jun 1, 2016 at 1:23 PM, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Vicențiu Ciorbaru < >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> vicentiu(a)mariadb.org> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi Varun, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Looks good to me. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Vicentiu >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Wed, 1 Jun 2016 at 10:49 Varun >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Gupta <varungupta1803(a)gmail.com> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> As I was going through with adding a >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> field to the proc table. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This is what I think should be >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> done, please correct me if I am wrong :) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> First add a new field to the enum { >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> MYSQL_PROC_FIELD_IS_AGGREGATE } >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Then store the value in >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> table->field[MYSQL_PROC_FIELD_IS_AGGREGATE]->store(sp->is_aggregate >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ?1:2),TRUE); >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Wed, Jun 1, 2016 at 9:32 AM, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Varun Gupta < >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> varungupta1803(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi Vicentiu, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> As Sanja was telling that the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> mysql.proc table should have an additional field to store if a function is >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> AGGREGATE or not. Well I completely agree with it. So I can add that or by >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the FETCH GROUP NEXT ROW , we can tell a function is aggregate or not , in >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> such case we would not need any new field in the mysql.proc table >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Tue, May 31, 2016 at 9:46 PM, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Sanja <sanja.byelkin(a)gmail.com> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ok >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Tue, May 31, 2016 at 6:12 PM, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Varun Gupta < >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> varungupta1803(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> For the aggregate functions like >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> for sum, we go through these functions as in these function we have the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> aggregator which calls the add() function >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Tue, May 31, 2016 at 8:52 PM, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Sanja <sanja.byelkin(a)gmail.com> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi! >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Tue, May 31, 2016 at 5:10 PM, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Varun Gupta < >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> varungupta1803(a)gmail.com> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> For the aggregate results as we >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> have functions which we need to execute from sp_head::execute >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> a) For the first record , we >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> use init_sum_functions >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> b) For the other records we >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> would have update_sum_functions >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> We would have the entire >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> aggregation in these 2 functions. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Why? I do not understand why we >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> need this and what they will do. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Maybe, you do not understand how >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> it works now. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Item_field has reference to the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> table and as soon as table set to the correct record it will read correct >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> values >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> But it should not bother you! >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> You get expressions from the parameters and get values from them, the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> question is only that it should be done in correct moment when tables set >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> on the record you need (and actually you should not even know what is under >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the hood because it can be even different table if aggregation made via >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> temporary table. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> it is completely legal to ask >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> SUM(a-b) where a and b could be fields of different tables. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>> >> >

2 1

Re: [Maria-developers] InnoDB blob for primary key
by Sachin Setia 06 Jun '16

06 Jun '16

yes sir https://github.com/SachinSetiya/server regards sachin On Mon, Jun 6, 2016 at 12:39 PM, Sachin Setia <sachinsetia1001(a)gmail.com> wrote: > yes sir > https://github.com/SachinSetiya/server > regards > sachin > > On Mon, Jun 6, 2016 at 12:31 PM, Oleksandr Byelkin <sanja(a)montyprogram.com > > wrote: > >> Are there there in github? >> Am 06.06.2016 07:57 schrieb Sachin Setia <sachinsetia1001(a)gmail.com>: >> >> Hello Sir >> Weekly Report for second week of gsoc >> 1. Implemented unique in the case of null >> 2. Implemented unique(A,b,c....) >> >> Currently working on >> tests , field hiding >> >> Regards >> sachin >> >> On Tue, May 31, 2016 at 10:49 PM, Sergei Golubchik <serg(a)mariadb.org> >> wrote: >> >> Hi, Sachin! >> >> On May 31, Sachin Setia wrote: >> > Hello Sir >> > Weekly Report For Gsoc >> > 1. Implemented hash function in parser for one column >> > 2. Implemenetd hash function for unlimited columns in parser >> > 3. Implemented unique checking in case of hash collusion(it retrieves >> the >> > row and compare data) but this only woks for unique blob column >> > >> > Currently Working on >> > unique checking in case of unique(a,b,c) >> >> Thanks. This is ok. >> >> Next time, please, send it to maria-developers list, not to me only :) >> >> Do you have questions? Any code you want me to look at? >> >> Regards, >> Sergei >> Chief Architect MariaDB >> and security(a)mariadb.org >> >> >> >

1 0

[Maria-developers] core review #1
by Sergei Golubchik 05 Jun '16

05 Jun '16

Hi, Shubham! Here's a first review. Summary: looks good so far, main comments: 1. Coding style. Use the same coding style as everywhere else in the file you're editing. It's the same in sql/ and in myisam/ directories. But InnoDB uses different coding style. 2. Tests, tests, tests! Please start adding tests now, and commit them into your branch. They should be somewhere under mysql-test/ directory. The full documentation is here: https://mariadb.com/kb/en/mariadb/mysqltest/ But even without it you can create your tests by starting of from existing test files. More comments below: > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index dfce503..31f282b 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -3876,8 +3876,9 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info, > column->length= MAX_LEN_GEOM_POINT_FIELD; > if (!column->length) > { > - my_error(ER_BLOB_KEY_WITHOUT_LENGTH, MYF(0), column->field_name.str); > - DBUG_RETURN(TRUE); > + key_info->algorithm=HA_KEY_ALG_HASH; There's more to it. The task title is, indeed, "unique index for blobs", but the actual goal is to have unique indexes for anything that is too long for normal btree indexes. That's what MI_UNIQUE does. so, you should support unique indexes also for blobs where column->length is specified, but is too large. Or unique indexes for a combination of ten long varchar columns, for example. So, you also need to patch the code where it issues ER_TOO_LONG_KEY. Also we'd want to use MI_UNIQUE for keys where a user has explicitly specified USING HASH, but let's look at it later > + // my_error(ER_BLOB_KEY_WITHOUT_LENGTH, MYF(0), column->field_name.str); > + // DBUG_RETURN(TRUE); > } > } > #ifdef HAVE_SPATIAL > diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc > index 72bc4c0..9aba216 100644 > --- a/storage/myisam/ha_myisam.cc > +++ b/storage/myisam/ha_myisam.cc > @@ -216,44 +216,59 @@ static void mi_check_print_msg(HA_CHECK *param, const char* msg_type, > 0 OK > !0 error code > */ > - > int table2myisam(TABLE *table_arg, MI_KEYDEF **keydef_out, > - MI_COLUMNDEF **recinfo_out, uint *records_out) > + MI_COLUMNDEF **recinfo_out,MI_UNIQUEDEF **uniquedef_out ,uint *records_out) > { > - uint i, j, recpos, minpos, fieldpos, temp_length, length; > + uint i, j, recpos, minpos, fieldpos, temp_length, length, k, l, m; > enum ha_base_keytype type= HA_KEYTYPE_BINARY; > uchar *record; > KEY *pos; > MI_KEYDEF *keydef; > + MI_UNIQUEDEF *uniquedef; > MI_COLUMNDEF *recinfo, *recinfo_pos; > HA_KEYSEG *keyseg; > TABLE_SHARE *share= table_arg->s; > uint options= share->db_options_in_use; > DBUG_ENTER("table2myisam"); > + pos= table_arg->key_info; > + share->uniques=0; > + for (i= 0; i < share->keys; i++,pos++) > + { > + if(pos->algorithm==HA_KEY_ALG_HASH) > + { > + share->uniques++ ; > + } > + } > if (!(my_multi_malloc(MYF(MY_WME), > recinfo_out, (share->fields * 2 + 2) * sizeof(MI_COLUMNDEF), > - keydef_out, share->keys * sizeof(MI_KEYDEF), > + keydef_out, (share->keys - share->uniques) * sizeof(MI_KEYDEF), > + uniquedef_out, share->uniques * sizeof(MI_UNIQUEDEF), > &keyseg, > (share->key_parts + share->keys) * sizeof(HA_KEYSEG), > NullS))) > DBUG_RETURN(HA_ERR_OUT_OF_MEM); /* purecov: inspected */ > keydef= *keydef_out; > recinfo= *recinfo_out; > + uniquedef= *uniquedef_out; > pos= table_arg->key_info; > + k=0; > + m=0; better call your indexes 'k' and 'u', for keydefs and uniques. > for (i= 0; i < share->keys; i++, pos++) > { > - keydef[i].flag= ((uint16) pos->flags & (HA_NOSAME | HA_FULLTEXT | HA_SPATIAL)); > - keydef[i].key_alg= pos->algorithm == HA_KEY_ALG_UNDEF ? > + if(pos->algorithm!=HA_KEY_ALG_HASH) > + { > + keydef[k].flag= ((uint16) pos->flags & (HA_NOSAME | HA_FULLTEXT | HA_SPATIAL)); > + keydef[k].key_alg= pos->algorithm == HA_KEY_ALG_UNDEF ? > (pos->flags & HA_SPATIAL ? HA_KEY_ALG_RTREE : HA_KEY_ALG_BTREE) : > pos->algorithm; > - keydef[i].block_length= pos->block_size; > - keydef[i].seg= keyseg; > - keydef[i].keysegs= pos->user_defined_key_parts; > + keydef[k].block_length= pos->block_size; > + keydef[k].seg= keyseg; > + keydef[k].keysegs= pos->user_defined_key_parts; > for (j= 0; j < pos->user_defined_key_parts; j++) > { > Field *field= pos->key_part[j].field; > type= field->key_type(); > - keydef[i].seg[j].flag= pos->key_part[j].key_part_flag; > + keydef[k].seg[j].flag= pos->key_part[j].key_part_flag; > > if (options & HA_OPTION_PACK_KEYS || > (pos->flags & (HA_PACK_KEY | HA_BINARY_PACK_KEY | > @@ -266,52 +281,104 @@ int table2myisam(TABLE *table_arg, MI_KEYDEF **keydef_out, > { > /* No blobs here */ > if (j == 0) > - keydef[i].flag|= HA_PACK_KEY; > + keydef[k].flag|= HA_PACK_KEY; > if (!(field->flags & ZEROFILL_FLAG) && > (field->type() == MYSQL_TYPE_STRING || > field->type() == MYSQL_TYPE_VAR_STRING || > ((int) (pos->key_part[j].length - field->decimals())) >= 4)) > - keydef[i].seg[j].flag|= HA_SPACE_PACK; > + keydef[k].seg[j].flag|= HA_SPACE_PACK; > } > else if (j == 0 && (!(pos->flags & HA_NOSAME) || pos->key_length > 16)) > - keydef[i].flag|= HA_BINARY_PACK_KEY; > + keydef[k].flag|= HA_BINARY_PACK_KEY; > } > - keydef[i].seg[j].type= (int) type; > - keydef[i].seg[j].start= pos->key_part[j].offset; > - keydef[i].seg[j].length= pos->key_part[j].length; > - keydef[i].seg[j].bit_start= keydef[i].seg[j].bit_end= > - keydef[i].seg[j].bit_length= 0; > - keydef[i].seg[j].bit_pos= 0; > - keydef[i].seg[j].language= field->charset_for_protocol()->number; > + keydef[k].seg[j].type= (int) type; > + keydef[k].seg[j].start= pos->key_part[j].offset; > + keydef[k].seg[j].length= pos->key_part[j].length; > + keydef[k].seg[j].bit_start= keydef[k].seg[j].bit_end= > + keydef[k].seg[j].bit_length= 0; > + keydef[k].seg[j].bit_pos= 0; > + keydef[k].seg[j].language= field->charset_for_protocol()->number; > > if (field->null_ptr) > { > - keydef[i].seg[j].null_bit= field->null_bit; > - keydef[i].seg[j].null_pos= (uint) (field->null_ptr- > + keydef[k].seg[j].null_bit= field->null_bit; > + keydef[k].seg[j].null_pos= (uint) (field->null_ptr- > (uchar*) table_arg->record[0]); > } > else > { > - keydef[i].seg[j].null_bit= 0; > - keydef[i].seg[j].null_pos= 0; > + keydef[k].seg[j].null_bit= 0; > + keydef[k].seg[j].null_pos= 0; > } > if (field->type() == MYSQL_TYPE_BLOB || > field->type() == MYSQL_TYPE_GEOMETRY) > { > - keydef[i].seg[j].flag|= HA_BLOB_PART; > + keydef[k].seg[j].flag|= HA_BLOB_PART; > /* save number of bytes used to pack length */ > - keydef[i].seg[j].bit_start= (uint) (field->pack_length() - > + keydef[k].seg[j].bit_start= (uint) (field->pack_length() - > portable_sizeof_char_ptr); > } > else if (field->type() == MYSQL_TYPE_BIT) > { > - keydef[i].seg[j].bit_length= ((Field_bit *) field)->bit_len; > - keydef[i].seg[j].bit_start= ((Field_bit *) field)->bit_ofs; > - keydef[i].seg[j].bit_pos= (uint) (((Field_bit *) field)->bit_ptr - > + keydef[k].seg[j].bit_length= ((Field_bit *) field)->bit_len; > + keydef[k].seg[j].bit_start= ((Field_bit *) field)->bit_ofs; > + keydef[k].seg[j].bit_pos= (uint) (((Field_bit *) field)->bit_ptr - > (uchar*) table_arg->record[0]); > } > } > keyseg+= pos->user_defined_key_parts; > + k++; > + } > + else > + { > + uniquedef[m].sql_key_no=i; > + uniquedef[m].null_are_equal=1; > + uniquedef[m].seg=keyseg; please follow code formatting rules that you can see elsewhere in this file > + uniquedef[m].keysegs= pos->user_defined_key_parts; > + for (l= 0; l < pos->user_defined_key_parts; l++) > + { > + Field *field= pos->key_part[l].field; > + type= field->key_type(); > + uniquedef[m].seg[l].flag= pos->key_part[l].key_part_flag; > + uniquedef[m].seg[l].type= (int) type; > + uniquedef[m].seg[l].start= pos->key_part[l].offset; > + uniquedef[m].seg[l].length= pos->key_part[l].length; > + uniquedef[m].seg[l].bit_start= uniquedef[m].seg[l].bit_end= > + uniquedef[m].seg[l].bit_length= 0; > + uniquedef[m].seg[l].bit_pos= 0; > + uniquedef[m].seg[l].language= field->charset_for_protocol()->number; > + > + if (field->null_ptr) > + { > + uniquedef[m].seg[l].null_bit= field->null_bit; > + uniquedef[m].seg[l].null_pos= (uint) (field->null_ptr- > + (uchar*) table_arg->record[0]); > + } > + else > + { > + uniquedef[m].seg[l].null_bit= 0; > + uniquedef[m].seg[l].null_pos= 0; > + } > + if (field->type() == MYSQL_TYPE_BLOB || > + field->type() == MYSQL_TYPE_GEOMETRY) > + { > + uniquedef[m].seg[l].flag|= HA_BLOB_PART; > + /* save number of bytes used to pack length */ > + uniquedef[m].seg[l].bit_start= (uint) (field->pack_length() - > + portable_sizeof_char_ptr); > + } > + else if (field->type() == MYSQL_TYPE_BIT) > + { > + uniquedef[m].seg[l].bit_length= ((Field_bit *) field)->bit_len; > + uniquedef[m].seg[l].bit_start= ((Field_bit *) field)->bit_ofs; > + uniquedef[m].seg[l].bit_pos= (uint) (((Field_bit *) field)->bit_ptr - > + (uchar*) table_arg->record[0]); > + } this seems to be the same code as for keydefs, isn't it? better to extract it into a separate function. Like: setup_keyparts(pos, keydef[k].seg); setup_keyparts(pos, uniquedef[m].seg); > + > + } > + keyseg+= pos->user_defined_key_parts; > + m++; > + } > } > if (table_arg->found_next_number_field) > keydef[share->next_number_index].flag|= HA_AUTO_KEY; > @@ -453,6 +528,7 @@ int check_definition(MI_KEYDEF *t1_keyinfo, MI_COLUMNDEF *t1_recinfo, > MI_KEYDEF *t2_keyinfo, MI_COLUMNDEF *t2_recinfo, > uint t2_keys, uint t2_recs, bool strict, TABLE *table_arg) > { > + return 0; to be fixed, I suppose? > uint i, j; > DBUG_ENTER("check_definition"); > my_bool mysql_40_compat= table_arg && table_arg->s->frm_version < FRM_VER_TRUE_VARCHAR; > diff --git a/storage/myisam/mi_write.c b/storage/myisam/mi_write.c > index ff96ee8..4d2b62a 100644 > --- a/storage/myisam/mi_write.c > +++ b/storage/myisam/mi_write.c > @@ -188,6 +189,28 @@ int mi_write(MI_INFO *info, uchar *record) > mi_flush_bulk_insert(info, j); > } > info->errkey= (int) i; > + prev=-1; > + keydef_no=0; > + for (k=0 ; k < share->state.header.uniques ; k++) > + { > + MI_UNIQUEDEF *def= share->uniqueinfo + k; > + > + if(keydef_no < (int) i+1) > + { > + diff= ((int)def->sql_key_no) - prev -1; > + keydef_no += diff; > + } > + prev= (int) def->sql_key_no; > + if(keydef_no <= (int) i+1) > + { > + info->errkey += 1; > + } > + else > + break; this can be done a bit simpler, without diff, prev, or keydef_no: for (k=0; k < share->state.header.uniques; k++, i++) if (i < share->uniqueinfo[k].sql_key_no) break; info->errkey= (int) i; > + > + } > + > + > while ( i-- > 0) > { > if (mi_is_key_active(share->state.key_map, i)) Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1

[Maria-developers] JSON libraries
by Alexey Botchkov 03 Jun '16

03 Jun '16

Hi. So i spent quite some time trying to get in use an existing JSON-parsing libraries. But surprisingly enough none seemed to be good enough. Problems i met were like this: 1. Parsers are just slow. - because they copy data - particularly key names and values. - because they use memory allocation/deallocations - because they convert numbers from strings to some numeric formats. Not that i criticise them - it's all mostly because authors of these libraries were concentrating on the ease of use for the inexperienced user. 2. They have to implement the support for different character sets, and cannot use our code for charsets directly. That leads to further slowdown of execution and some inconsistencies with the existing behaviour. 3. Their interface is inconvenient. In order to implement the required JSON functions we'd like to start/stop parsing on various stages, skip parts of JSON, insert something to the found place etc. With the usual interface of the libraries doing all this looks really ugly and inefficient. So that MySQL doesn't even try to implement this - it only uses the library to convert the JSON to internal format and then only operates with that internal. 4. Libraries tend to have their own ways of reporting errors while we'd like use our error messages. Now MySQL just prints the library-generated messages, they cannot translate or somehow modyfy them. 5. We have to parse not only JSON, but also json-paths, as parts of various JSON functions. Libraries don't usually parse the json-path. Partly because these paths can be very different in various applications. Still parts of the path (strings to be specific) should be parsed just as JSON, and it'd be good to use the JSON parser for it, which can require ugly tricks. Like MySQL constructs JSON from that piece of json-path to be able to feed that to the JSON parser. Above all that, i must note that the JSON is pretty simple and compact as a data format. And i seriously belive handling it can be done with great efficiency. So i decided to invest in local JSON parser, taking JSON_CHECKER as a source of inspiration. That JSON_CHECKER was produced by JSON.org itself, is really fast and nice. But it's not a parser - it just checks if the text makes the correct JSON. Well it took some time, but now ready. Now opening it for review. Best regards. HF

2 2

Re: [Maria-developers] [Commits] 9b88b52: MDEV-8931: (server part of) session state tracking
by Sergei Golubchik 02 Jun '16

02 Jun '16

Hi, Sanja! Here's a combined review of all three patches: On May 30, Oleksandr Byelkin wrote: > diff --git a/include/mysql_com.h b/include/mysql_com.h > index c13999a..954c173 100644 > --- a/include/mysql_com.h > +++ b/include/mysql_com.h > @@ -520,6 +544,30 @@ enum enum_mysql_set_option > MYSQL_OPTION_MULTI_STATEMENTS_OFF > }; > > +/* > + Type of state change information that the server can include in the Ok > + packet. > + Note : 1) session_state_type shouldn't go past 255 (i.e. 1-byte boundary). > + 2) Modify the definition of SESSION_TRACK_END when a new member is > + added. these "notes" are a fragile way of adding restrictions. 1. add a compile_time_assert for SESSION_TRACK_END < 256 2. in fact, you have compile_time_assert's for SESSION_TRACK_SCHEMA and SESSION_TRACK_STATE_CHANGE, but they say < 251. And it looks like SESSION_TRACK_END should be < 251 (not 256) too. 3. perhaps it's better to move SESSION_TRACK_END into the enum, and give it a visually distinct name, like SESSION_TRACK_always_at_the_end then it'll always be the last, and you can compile_time_assert that it's less than 251. > +*/ > +enum enum_session_state_type > +{ > + SESSION_TRACK_SYSTEM_VARIABLES, /* Session system variables */ > + SESSION_TRACK_SCHEMA, /* Current schema */ > + SESSION_TRACK_STATE_CHANGE, /* track session state changes */ > + SESSION_TRACK_GTIDS, > + SESSION_TRACK_TRANSACTION_CHARACTERISTICS, /* Transaction chistics */ > + SESSION_TRACK_TRANSACTION_STATE /* Transaction state */ > +}; > + > +#define SESSION_TRACK_BEGIN SESSION_TRACK_SYSTEM_VARIABLES > + > +#define SESSION_TRACK_END SESSION_TRACK_TRANSACTION_STATE > + > +#define IS_SESSION_STATE_TYPE(T) \ > + (((int)(T) >= SESSION_TRACK_BEGIN) && ((T) <= SESSION_TRACK_END)) > + > #define net_new_transaction(net) ((net)->pkt_nr=0) > > #ifdef __cplusplus > diff --git a/libmysqld/lib_sql.cc b/libmysqld/lib_sql.cc > index bcb45ae..c7ebb79 100644 > --- a/libmysqld/lib_sql.cc > +++ b/libmysqld/lib_sql.cc > @@ -1171,7 +1171,8 @@ bool > net_send_ok(THD *thd, > uint server_status, uint statement_warn_count, > ulonglong affected_rows, ulonglong id, const char *message, > - bool unused __attribute__((unused))) > + bool unused1 __attribute__((unused)), > + bool unused2 __attribute__((unused))) as I wrote in the previous review: 1. you don't need __attribute__((unused)) in C++, you can omit the parameter name 2. we compile with -Wno-unused-parameter anyway > { > DBUG_ENTER("emb_net_send_ok"); > MYSQL_DATA *data; > diff --git a/mysql-test/r/mysqld--help.result b/mysql-test/r/mysqld--help.result > index cc43265..be6893c 100644 > --- a/mysql-test/r/mysqld--help.result > +++ b/mysql-test/r/mysqld--help.result > @@ -903,6 +903,22 @@ The following options may be given as the first argument: > files within specified directory > --server-id=# Uniquely identifies the server instance in the community > of replication partners > + --session-track-schema > + Track changes to the 'default schema'. why in quotes? > + (Defaults to on; use --skip-session-track-schema to disable.) > + --session-track-state-change > + Track changes to the 'session state'. ditto > + --session-track-system-variables=name > + Track changes in registered system variables. > + --session-track-transaction-info=name > + Track changes to the transaction attributes. OFF to > + disable; STATE to track just transaction state (Is there > + an active transaction? Does it have any data? etc.); > + CHARACTERISTICS to track transaction state and report all > + statements needed to start a transaction with the same > + characteristics (isolation level, read only/read write, > + snapshot - but not any work done / data modified within > + the transaction). > --show-slave-auth-info > Show user and password in SHOW SLAVE HOSTS on this > master. > @@ -1387,6 +1403,10 @@ safe-user-create FALSE > secure-auth TRUE > secure-file-priv (No default value) > server-id 0 > +session-track-schema TRUE > +session-track-state-change FALSE > +session-track-system-variables autocommit,character_set_client,character_set_connection,character_set_results,time_zone this is what, a default value? why any session tracking is enabled by default? > +session-track-transaction-info OFF > show-slave-auth-info FALSE > silent-startup FALSE > skip-grant-tables TRUE > diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result > index 6dca520..b739c67 100644 > --- a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result > +++ b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result > @@ -3313,6 +3313,34 @@ NUMERIC_BLOCK_SIZE 1 > ENUM_VALUE_LIST NULL > READ_ONLY NO > COMMAND_LINE_ARGUMENT REQUIRED > +VARIABLE_NAME SESSION_TRACK_SCHEMA these variables don't do anything in embedded. do we want to have them here at all? > +SESSION_VALUE ON > +GLOBAL_VALUE ON > +GLOBAL_VALUE_ORIGIN COMPILE-TIME > +DEFAULT_VALUE ON > +VARIABLE_SCOPE SESSION > +VARIABLE_TYPE BOOLEAN > +VARIABLE_COMMENT Track changes to the 'default schema'. > +NUMERIC_MIN_VALUE NULL > +NUMERIC_MAX_VALUE NULL > +NUMERIC_BLOCK_SIZE NULL > +ENUM_VALUE_LIST OFF,ON > +READ_ONLY NO > +COMMAND_LINE_ARGUMENT OPTIONAL > +VARIABLE_NAME SESSION_TRACK_STATE_CHANGE > +SESSION_VALUE OFF > +GLOBAL_VALUE OFF > +GLOBAL_VALUE_ORIGIN COMPILE-TIME > +DEFAULT_VALUE OFF > +VARIABLE_SCOPE SESSION > +VARIABLE_TYPE BOOLEAN > +VARIABLE_COMMENT Track changes to the 'session state'. > +NUMERIC_MIN_VALUE NULL > +NUMERIC_MAX_VALUE NULL > +NUMERIC_BLOCK_SIZE NULL > +ENUM_VALUE_LIST OFF,ON > +READ_ONLY NO > +COMMAND_LINE_ARGUMENT OPTIONAL > VARIABLE_NAME SKIP_EXTERNAL_LOCKING > SESSION_VALUE NULL > GLOBAL_VALUE ON > diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > index 1620579..89999a3 100644 > --- a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > +++ b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > @@ -3775,6 +3775,48 @@ NUMERIC_BLOCK_SIZE 1 > ENUM_VALUE_LIST NULL > READ_ONLY NO > COMMAND_LINE_ARGUMENT REQUIRED > +VARIABLE_NAME SESSION_TRACK_SCHEMA > +SESSION_VALUE ON > +GLOBAL_VALUE ON > +GLOBAL_VALUE_ORIGIN COMPILE-TIME > +DEFAULT_VALUE ON > +VARIABLE_SCOPE SESSION > +VARIABLE_TYPE BOOLEAN > +VARIABLE_COMMENT Track changes to the 'default schema'. > +NUMERIC_MIN_VALUE NULL > +NUMERIC_MAX_VALUE NULL > +NUMERIC_BLOCK_SIZE NULL > +ENUM_VALUE_LIST OFF,ON > +READ_ONLY NO > +COMMAND_LINE_ARGUMENT OPTIONAL > +VARIABLE_NAME SESSION_TRACK_STATE_CHANGE > +SESSION_VALUE OFF > +GLOBAL_VALUE OFF > +GLOBAL_VALUE_ORIGIN COMPILE-TIME > +DEFAULT_VALUE OFF > +VARIABLE_SCOPE SESSION > +VARIABLE_TYPE BOOLEAN > +VARIABLE_COMMENT Track changes to the 'session state'. > +NUMERIC_MIN_VALUE NULL > +NUMERIC_MAX_VALUE NULL > +NUMERIC_BLOCK_SIZE NULL > +ENUM_VALUE_LIST OFF,ON > +READ_ONLY NO > +COMMAND_LINE_ARGUMENT OPTIONAL > +VARIABLE_NAME SESSION_TRACK_SYSTEM_VARIABLES > +SESSION_VALUE autocommit,character_set_client,character_set_connection,character_set_results,time_zone > +GLOBAL_VALUE autocommit,character_set_client,character_set_connection,character_set_results,time_zone > +GLOBAL_VALUE_ORIGIN COMPILE-TIME > +DEFAULT_VALUE autocommit,character_set_client,character_set_connection,character_set_results,time_zone > +VARIABLE_SCOPE SESSION > +VARIABLE_TYPE VARCHAR > +VARIABLE_COMMENT Track changes in registered system variables. > +NUMERIC_MIN_VALUE NULL > +NUMERIC_MAX_VALUE NULL > +NUMERIC_BLOCK_SIZE NULL > +ENUM_VALUE_LIST NULL > +READ_ONLY NO > +COMMAND_LINE_ARGUMENT REQUIRED where's SESSION_TRACK_TRANSACTION_INFO? > VARIABLE_NAME SKIP_EXTERNAL_LOCKING > SESSION_VALUE NULL > GLOBAL_VALUE ON > diff --git a/mysql-test/suite/sys_vars/t/session_track_system_variables_basic.test b/mysql-test/suite/sys_vars/t/session_track_system_variables_basic.test > new file mode 100644 > index 0000000..bbb32bb > --- /dev/null > +++ b/mysql-test/suite/sys_vars/t/session_track_system_variables_basic.test > @@ -0,0 +1,133 @@ > +--source include/not_embedded.inc > + > +--echo # > +--echo # Variable name : session_track_system_variables > +--echo # Scope : Global & Session > +--echo # > + > +--echo # Global - default > +SELECT @@global.session_track_system_variables; > +--echo # Session - default > +SELECT @@session.session_track_system_variables; > +--echo > + > +--echo # via INFORMATION_SCHEMA.GLOBAL_VARIABLES > +--disable_warnings > +SELECT * FROM INFORMATION_SCHEMA.GLOBAL_VARIABLES WHERE VARIABLE_NAME LIKE 'session_track%' ORDER BY VARIABLE_NAME; > +--enable_warnings why do you disable warnings here? > + > +--echo # via INFORMATION_SCHEMA.SESSION_VARIABLES > +--disable_warnings > +SELECT * FROM INFORMATION_SCHEMA.SESSION_VARIABLES WHERE VARIABLE_NAME LIKE 'session_track%' ORDER BY VARIABLE_NAME; > +--enable_warnings > + > +# Save the global value to be used to restore the original value. > +SET @global_saved_tmp = @@global.session_track_system_variables; > +--echo > + > +--echo # Altering global variable's value > +SET @@global.session_track_system_variables='autocommit'; > +SELECT @@global.session_track_system_variables; > +SELECT @@session.session_track_system_variables; > +--echo > + > +--echo # Altering session variable's value > +SET @@session.session_track_system_variables='autocommit'; > +SELECT @@global.session_track_system_variables; > +SELECT @@session.session_track_system_variables; > +--echo > + > +--echo # Variables' values in a new session. > +connect (con1,"127.0.0.1",root,,test,$MASTER_MYPORT,); > + > +--echo # Global - expect "autocommit" > +SELECT @@global.session_track_system_variables; > +--echo > +--echo # Session - expect "autocommit" > +SELECT @@session.session_track_system_variables; > +--echo > + > +--echo # Switching to the default connection. > +connection default; > + > +SELECT @@global.session_track_system_variables; > +SELECT @@session.session_track_system_variables; > +--echo > + > +--echo # Test if DEFAULT is working as expected. > +SET @@global.session_track_system_variables = DEFAULT; > +SET @@session.session_track_system_variables = DEFAULT; > +--echo > + > +SELECT @@global.session_track_system_variables; > +SELECT @@session.session_track_system_variables; > +--echo > + > +--echo # Variables' values in a new session (con2). > +connect (con2,"127.0.0.1",root,,test,$MASTER_MYPORT,); > + > +SELECT @@global.session_track_system_variables; > +SELECT @@session.session_track_system_variables; > +--echo > + > +--echo # Altering session should not affect global. > +SET @@session.session_track_system_variables = 'sql_mode'; > +SELECT @@global.session_track_system_variables; > +SELECT @@session.session_track_system_variables; > +--echo > + > +--echo # Variables' values in a new session (con3). > +connect (con3,"127.0.0.1",root,,test,$MASTER_MYPORT,); > + > +--echo # Altering global should not affect session. > +SET @@global.session_track_system_variables = 'sql_mode'; > +SELECT @@global.session_track_system_variables; > +SELECT @@session.session_track_system_variables; > +--echo > + > +--echo # Switching to the default connection. > +connection default; > + > +--echo # Testing NULL > +SET @@global.session_track_system_variables = NULL; > +SET @@session.session_track_system_variables = NULL; > + > +--echo # Global - expect "" instead of NULL > +SELECT @@global.session_track_system_variables; > +--echo # Session - expect "" instead of NULL > +SELECT @@session.session_track_system_variables; > + > +--echo # testing with duplicate entries. > +# Lets first set it to some valid value. > +SET @@global.session_track_system_variables= "time_zone"; > +SET @@session.session_track_system_variables= "time_zone"; > +# Now set with duplicate entries (must pass) > +SET @@global.session_track_system_variables= "sql_mode,sql_mode"; > +SET @@session.session_track_system_variables= "sql_mode,sql_mode"; > +SELECT @@global.session_track_system_variables; > +SELECT @@session.session_track_system_variables; > +--echo > + > +--echo # testing ordering > +SET @@global.session_track_system_variables= "time_zone,sql_mode"; > +SET @@session.session_track_system_variables= "time_zone,sql_mode"; > +SELECT @@global.session_track_system_variables; > +SELECT @@session.session_track_system_variables; > +--echo > + > +--echo # special values > +SET @@global.session_track_system_variables= "*"; > +SET @@session.session_track_system_variables= "*"; > +SELECT @@global.session_track_system_variables; > +SELECT @@session.session_track_system_variables; > +SET @@global.session_track_system_variables= ""; > +SET @@session.session_track_system_variables= ""; > +SELECT @@global.session_track_system_variables; > +SELECT @@session.session_track_system_variables; > +--echo > + > + > +--echo # Restoring the original values. > +SET @@global.session_track_system_variables = @global_saved_tmp; > + > +--echo # End of tests. > diff --git a/sql/sql_string.h b/sql/sql_string.h > index 51a11c7..feab807 100644 > --- a/sql/sql_string.h > +++ b/sql/sql_string.h > @@ -559,6 +566,17 @@ class String > return Ptr+ old_length; /* Area to use */ > } > > + inline bool prep_alloc(uint32 arg_length, uint32 step_alloc) why couldn't you use String::reserve(arg_length, step_alloc) ? > + { > + uint32 new_length= arg_length + str_length; > + if (new_length > Alloced_length) > + { > + if (realloc(new_length + step_alloc)) > + return true; > + } > + return false; > + } > + > inline bool append(const char *s, uint32 arg_length, uint32 step_alloc) > { > uint32 new_length= arg_length + str_length; > @@ -623,6 +641,8 @@ class String > { > return !sortcmp(this, other, cs); > } > + void q_net_store_length(ulonglong length); > + void q_net_store_data(const uchar *from, size_t length); why didn't you define these two inline? > }; > > > diff --git a/sql/sql_string.cc b/sql/sql_string.cc > index 40339d5..2690310 100644 > --- a/sql/sql_string.cc > +++ b/sql/sql_string.cc > @@ -1157,3 +1158,16 @@ uint convert_to_printable(char *to, size_t to_len, > *t= '\0'; > return t - to; > } > + > +void String::q_net_store_length(ulonglong length) > +{ May be, add DBUG_ASSERT that the buffer is big enough? > + char *pos= (char *) net_store_length((uchar *)(Ptr + str_length), length); > + str_length= pos - Ptr; > +} > + > +void String::q_net_store_data(const uchar *from, size_t length) > +{ and here too > + q_net_store_length(length); > + bool res= append((const char *)from, length); should be q_append here > + DBUG_ASSERT(!res); > +} > diff --git a/sql/mysqld.h b/sql/mysqld.h > index e538cbd..4173752 100644 > --- a/sql/mysqld.h > +++ b/sql/mysqld.h > @@ -135,6 +135,7 @@ extern my_bool lower_case_file_system; > extern my_bool opt_enable_named_pipe, opt_sync_frm, opt_allow_suspicious_udfs; > extern my_bool opt_secure_auth; > extern const char *current_dbug_option; > +extern const char *current_session_track_system_variables; this 'current_session_track_system_variables' string is not mentioned anywhere else in your patch > extern char* opt_secure_file_priv; > extern char* opt_secure_backup_file_priv; > extern size_t opt_secure_backup_file_priv_len; > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > index 845d114..e3aba86 100644 > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -691,6 +691,14 @@ THD *next_global_thread(THD *thd) > } > > struct system_variables global_system_variables; > +/** > + Following is just for options parsing, used with a difference against > + global_system_variables. > + > + TODO: something should be done to get rid of following variables > +*/ > +const char *current_dbug_option=""; > + why did you move it? > struct system_variables max_system_variables; > struct system_status_var global_status_var; > > @@ -5313,6 +5320,17 @@ static int init_server_components() > } > plugins_are_initialized= TRUE; /* Don't separate from init function */ > > + { > + Session_tracker session_track_system_variables_check; > + if (session_track_system_variables_check. > + server_boot_verify(system_charset_info)) make server_boot_verify a class method and invoke it here without creating a useless instance of Session_tracker > + { > + sql_print_error("The variable session_track_system_variables has " > + "invalid values."); > + unireg_abort(1); > + } > + } > + > /* we do want to exit if there are any other unknown options */ > if (remaining_argc > 1) > { > diff --git a/sql/protocol.cc b/sql/protocol.cc > index 6469581..73a704a 100644 > --- a/sql/protocol.cc > +++ b/sql/protocol.cc > @@ -197,7 +198,8 @@ bool net_send_error(THD *thd, uint sql_errno, const char *err, > @param affected_rows Number of rows changed by statement > @param id Auto_increment id for first row (if used) > @param message Message to send to the client (Used by mysql_status) > - > + @param is_eof this called inted of old EOF packet "instead" > + > @return > @retval FALSE The message was successfully sent > @retval TRUE An error occurred and the messages wasn't sent properly > @@ -209,10 +211,18 @@ bool > net_send_ok(THD *thd, > uint server_status, uint statement_warn_count, > ulonglong affected_rows, ulonglong id, const char *message, > + bool is_eof, > bool skip_flush) > { > NET *net= &thd->net; > - uchar buff[MYSQL_ERRMSG_SIZE+10],*pos; > + StringBuffer<MYSQL_ERRMSG_SIZE + 10> store; > + > + /* > + To be used to manage the data storage in case session state change > + information is present. > + */ confusing comment, better remove it completely > + bool state_changed= false; > + > bool error= FALSE; > DBUG_ENTER("net_send_ok"); > > @@ -222,38 +232,81 @@ net_send_ok(THD *thd, > DBUG_RETURN(FALSE); > } > > - buff[0]=0; // No fields > - pos=net_store_length(buff+1,affected_rows); > - pos=net_store_length(pos, id); > + /* > + OK send instead of EOF still require 0xFE header, but OK packet content. > + */ > + if (is_eof) > + { > + DBUG_ASSERT(thd->client_capabilities & CLIENT_DEPRECATE_EOF); > + store.q_append((char)254); > + } > + else > + store.q_append('\0'); > + > + /* affected rows */ > + store.q_net_store_length(affected_rows); > + > + /* last insert id */ > + store.q_net_store_length(id); > + > if (thd->client_capabilities & CLIENT_PROTOCOL_41) > { > DBUG_PRINT("info", > ("affected_rows: %lu id: %lu status: %u warning_count: %u", > (ulong) affected_rows, > (ulong) id, > (uint) (server_status & 0xffff), > (uint) statement_warn_count)); > - int2store(pos, server_status); > - pos+=2; > + store.q_append2b(server_status); > > /* We can only return up to 65535 warnings in two bytes */ > uint tmp= MY_MIN(statement_warn_count, 65535); > - int2store(pos, tmp); > - pos+= 2; > + store.q_append2b(tmp); > } > else if (net->return_status) // For 4.0 protocol > { > - int2store(pos, server_status); > - pos+=2; > + store.q_append2b(server_status); > } > thd->get_stmt_da()->set_overwrite_status(true); > > - if (message && message[0]) > - pos= net_store_data(pos, (uchar*) message, strlen(message)); > - error= my_net_write(net, buff, (size_t) (pos-buff)); > - if (!error && !skip_flush) > + if ((thd->client_capabilities & CLIENT_SESSION_TRACK)) > + { > + if (server_status & SERVER_SESSION_STATE_CHANGED) > + state_changed= true; > + /* the info field */ > + if (state_changed || (message && message[0])) > + { > + store.q_net_store_data((uchar*) message, message ? strlen(message) : 0); use safe_strlen(message) instead > + } > + > + /* session state change information */ > + if (unlikely(state_changed)) > + { > + store.set_charset(thd->variables.collation_database); > + > + thd->session_tracker.store(thd, &store); > + } > + } > + else if (message && message[0]) > + { > + /* the info field, if there is a message to store */ > + DBUG_ASSERT(strlen(message) <= MYSQL_ERRMSG_SIZE); > + store.q_net_store_data((uchar*) message, strlen(message)); > + } > + I think, better to write it as state_changed= thd->client_capabilities & CLIENT_SESSION_TRACK && server_status & SERVER_SESSION_STATE_CHANGED; if (state_changed || (message && message[0])) { DBUG_ASSERT(safe_strlen(message) <= MYSQL_ERRMSG_SIZE); store.q_net_store_data((uchar*) safe_str(message), safe_strlen(message)); } if (unlikely(state_changed)) { store.set_charset(thd->variables.collation_database); thd->session_tracker.store(thd, &store); } that is without nested if() and without duplicating message writing lines > + if (store.length() > MAX_PACKET_LENGTH) > + { > + net->error= 1; > + net->last_errno= ER_NET_OK_PACKET_TOO_LARGE; > + my_error(ER_NET_OK_PACKET_TOO_LARGE, MYF(0)); > + DBUG_PRINT("info", ("OK packet too large")); > + DBUG_RETURN(1); 1. That's kinda unexpected. ER_NET_OK_PACKET_TOO_LARGE when sending an OK packet? 2. does it work at all? I mean, my_error() from inside net_send_ok() - please verify that it's supported. > + } > + error= my_net_write(net, (const unsigned char*)store.ptr(), store.length()); > + if (!error && (!skip_flush || is_eof)) Hmm, so you're still flushing for the EOF packet? Why? > error= net_flush(net); > > + thd->server_status&= ~SERVER_SESSION_STATE_CHANGED; > > thd->get_stmt_da()->set_overwrite_status(false); > DBUG_PRINT("info", ("OK sent, so no more error sending allowed")); > @@ -858,14 +928,19 @@ bool Protocol::send_result_set_metadata(List<Item> *list, uint flags) > > if (flags & SEND_EOF) > { > - /* > - Mark the end of meta-data result set, and store thd->server_status, > - to show that there is no cursor. > - Send no warning information, as it will be sent at statement end. > - */ > - if (write_eof_packet(thd, &thd->net, thd->server_status, > - thd->get_stmt_da()->current_statement_warn_count())) > - DBUG_RETURN(1); > + > + /* if it is new client do not send EOF packet */ > + if (!(thd->client_capabilities & CLIENT_DEPRECATE_EOF)) > + { > + /* > + Mark the end of meta-data result set, and store thd->server_status, > + to show that there is no cursor. what about "to show that there is no cursor"? new clients won't have this information? > + Send no warning information, as it will be sent at statement end. > + */ > + if (write_eof_packet(thd, &thd->net, thd->server_status, > + thd->get_stmt_da()->current_statement_warn_count())) > + DBUG_RETURN(1); > + } > } > DBUG_RETURN(prepare_for_send(list->elements)); > > diff --git a/sql/session_tracker.h b/sql/session_tracker.h > new file mode 100644 > index 0000000..5d529b8 > --- /dev/null > +++ b/sql/session_tracker.h > @@ -0,0 +1,299 @@ > +#ifndef SESSION_TRACKER_INCLUDED > +#define SESSION_TRACKER_INCLUDED > + > +/* Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved. > + Copyright (c) 2016, MariaDB > + > + 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 "m_string.h" > +#include "thr_lock.h" > + > +/* forward declarations */ > +class THD; > +class set_var; > +class String; > + > + > +enum enum_session_tracker > +{ > + SESSION_SYSVARS_TRACKER, /* Session system variables */ > + CURRENT_SCHEMA_TRACKER, /* Current schema */ > + SESSION_STATE_CHANGE_TRACKER, > + SESSION_GTIDS_TRACKER, /* Tracks GTIDs */ > + TRANSACTION_INFO_TRACKER /* Transaction state */ > +}; > + > +#define SESSION_TRACKER_END TRANSACTION_INFO_TRACKER > + > + > +/** > + State_tracker > + > + An abstract class that defines the interface for any of the server's > + 'session state change tracker'. A tracker, however, is a sub- class of > + this class which takes care of tracking the change in value of a part- > + icular session state type and thus defines various methods listed in this > + interface. The change information is later serialized and transmitted to > + the client through protocol's OK packet. > + > + Tracker system variables :- > + A tracker is normally mapped to a system variable. So in order to enable, > + disable or modify the sub-entities of a tracker, the user needs to modify > + the respective system variable either through SET command or via command > + line option. As required in system variable handling, this interface also > + includes two functions to help in the verification of the supplied value > + (ON_CHECK) and the updation (ON_UPDATE) of the tracker system variable, > + namely - check() and update(). Why do you still have check() method here? It isn't used anywhere. (I've checked out your branch and removed all check() methods and it still compiled fine) > +*/ > + > +class State_tracker > +{ > +protected: > + /** > + Is tracking enabled for a particular session state type ? > + > + @note: It is cache to avoid virtual functions and checking thd "it is a cache of the corresponding thd->variables.session_track_xxx variable" > + when we want mark tracker as changed. > + */ > + bool m_enabled; > + > + /** Has the session state type changed ? */ > + bool m_changed; > + > +public: > + /** Constructor */ > + State_tracker() : m_enabled(false), m_changed(false) > + {} > + > + /** Destructor */ > + virtual ~State_tracker() > + {} > + > + /** Getters */ > + bool is_enabled() const > + { return m_enabled; } > + > + bool is_changed() const > + { return m_changed; } > + > + /** Called in the constructor of THD*/ > + virtual bool enable(THD *thd)= 0; > + > + /** To be invoked when the tracker's system variable is checked (ON_CHECK). */ > + virtual bool check(THD *thd, set_var *var)= 0; > + > + /** To be invoked when the tracker's system variable is updated (ON_UPDATE).*/ > + virtual bool update(THD *thd)= 0; > + > + /** Store changed data into the given buffer. */ > + virtual bool store(THD *thd, String *buf)= 0; > + > + /** Mark the entity as changed. */ > + virtual void mark_as_changed(THD *thd, LEX_CSTRING *name)= 0; > +}; > + > +bool sysvartrack_validate_value(THD *thd, const char *str, size_t len); > +bool sysvartrack_reprint_value(THD *thd, char *str, size_t len); > +bool sysvartrack_update(THD *thd); > +size_t sysvartrack_value_len(THD *thd); > +bool sysvartrack_value_construct(THD *thd, char *val, size_t len); > + > + > +/** > + Session_tracker > + > + This class holds an object each for all tracker classes and provides > + methods necessary for systematic detection and generation of session > + state change information. > +*/ > + > +class Session_tracker > +{ > +private: > + State_tracker *m_trackers[SESSION_TRACKER_END + 1]; > + > + /* The following two functions are private to disable copying. */ > + Session_tracker(Session_tracker const &other) > + { > + DBUG_ASSERT(FALSE); > + } > + Session_tracker& operator= (Session_tracker const &rhs) > + { > + DBUG_ASSERT(FALSE); > + return *this; > + } > + > +public: > + > + Session_tracker(); > + ~Session_tracker() > + { > + deinit(); > + } > + > + /* trick to make happy memory accounting system */ > + void deinit() > + { > + for (int i= 0; i <= SESSION_TRACKER_END; i ++) > + { > + if (m_trackers[i]) > + delete m_trackers[i]; > + m_trackers[i]= NULL; > + } > + } > + > + void enable(THD *thd); > + bool server_boot_verify(const CHARSET_INFO *char_set); no "const CHARSET_INFO", it's MySQL-ism. We have put "const" into the CHARSET_INFO type definiton. > + > + /** Returns the pointer to the tracker object for the specified tracker. */ > + inline State_tracker *get_tracker(enum_session_tracker tracker) const > + { > + return m_trackers[tracker]; > + } > + > + inline void mark_as_changed(THD *thd, enum enum_session_tracker tracker, > + LEX_CSTRING *data) > + { > + if (m_trackers[tracker]->is_enabled()) > + m_trackers[tracker]->mark_as_changed(thd, data); > + } > + > + > + void store(THD *thd, String *main_buf); > +}; > + > + > +/* > + Transaction_state_tracker > +*/ > + > +/** > + Transaction state (no transaction, transaction active, work attached, etc.) > +*/ > +enum enum_tx_state { > + TX_EMPTY = 0, ///< "none of the below" > + TX_EXPLICIT = 1, ///< an explicit transaction is active > + TX_IMPLICIT = 2, ///< an implicit transaction is active > + TX_READ_TRX = 4, ///< transactional reads were done > + TX_READ_UNSAFE = 8, ///< non-transaction reads were done > + TX_WRITE_TRX = 16, ///< transactional writes were done > + TX_WRITE_UNSAFE = 32, ///< non-transactional writes were done > + TX_STMT_UNSAFE = 64, ///< "unsafe" (non-deterministic like UUID()) stmts > + TX_RESULT_SET = 128, ///< result-set was sent s/result-set/result set/ > + TX_WITH_SNAPSHOT= 256, ///< WITH CONSISTENT SNAPSHOT was used > + TX_LOCKED_TABLES= 512 ///< LOCK TABLES is active > +}; > + > + > +/** > + Transaction access mode > +*/ > +enum enum_tx_read_flags { > + TX_READ_INHERIT = 0, ///< not explicitly set, inherit session.tx_read_only > + TX_READ_ONLY = 1, ///< START TRANSACTION READ ONLY, or tx_read_only=1 > + TX_READ_WRITE = 2, ///< START TRANSACTION READ WRITE, or tx_read_only=0 > +}; > + > + > +/** > + Transaction isolation level > +*/ > +enum enum_tx_isol_level { > + TX_ISOL_INHERIT = 0, ///< not explicitly set, inherit session.tx_isolation > + TX_ISOL_UNCOMMITTED = 1, > + TX_ISOL_COMMITTED = 2, > + TX_ISOL_REPEATABLE = 3, > + TX_ISOL_SERIALIZABLE= 4 > +}; > + > + > +/** > + Transaction tracking level > +*/ > +enum enum_session_track_transaction_info { > + TX_TRACK_NONE = 0, ///< do not send tracker items on transaction info > + TX_TRACK_STATE = 1, ///< track transaction status > + TX_TRACK_CHISTICS = 2 ///< track status and characteristics > +}; > + > + > +/** > + This is a tracker class that enables & manages the tracking of > + current transaction info for a particular connection. > +*/ > + > +class Transaction_state_tracker : public State_tracker > +{ > +public: > + /** Constructor */ > + Transaction_state_tracker(); > + bool enable(THD *thd) > + { return update(thd); } > + bool check(THD *thd, set_var *var) > + { return false; } > + bool update(THD *thd); > + bool store(THD *thd, String *buf); > + void mark_as_changed(THD *thd, LEX_CSTRING *tracked_item_name); > + > + /** Change transaction characteristics */ > + void set_read_flags(THD *thd, enum enum_tx_read_flags flags); > + void set_isol_level(THD *thd, enum enum_tx_isol_level level); > + > + /** Change transaction state */ > + void clear_trx_state(THD *thd, uint clear); > + void add_trx_state(THD *thd, uint add); > + void add_trx_state_from_thd(THD *thd); > + void end_trx(THD *thd); > + > + /** Helper function: turn table info into table access flag */ > + enum_tx_state calc_trx_state(THD *thd, thr_lock_type l, bool has_trx); > + > +private: > + enum enum_tx_changed { > + TX_CHG_NONE = 0, ///< no changes from previous stmt > + TX_CHG_STATE = 1, ///< state has changed from previous stmt > + TX_CHG_CHISTICS = 2 ///< characteristics have changed from previous stmt > + }; Huh? I've just seen an identical enum just above this class definition? > + > + /** any trackable changes caused by this statement? */ > + uint tx_changed; > + > + /** transaction state */ > + uint tx_curr_state, tx_reported_state; > + > + /** r/w or r/o set? session default? */ > + enum enum_tx_read_flags tx_read_flags; > + > + /** isolation level */ > + enum enum_tx_isol_level tx_isol_level; > + > + void reset(); > + > + inline void update_change_flags(THD *thd) > + { > + tx_changed &= ~TX_CHG_STATE; > + tx_changed |= (tx_curr_state != tx_reported_state) ? TX_CHG_STATE : 0; > + if (tx_changed != TX_CHG_NONE) > + mark_as_changed(thd, NULL); > + } > +}; > + > +#define TRANSACT_TRACKER(X) \ > + do { if (thd->variables.session_track_transaction_info > TX_TRACK_NONE) \ > + {((Transaction_state_tracker *) \ > + thd->session_tracker.get_tracker(TRANSACTION_INFO_TRACKER)) \ > + ->X; } } while(0) > + > +#endif /* SESSION_TRACKER_INCLUDED */ > diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc > new file mode 100644 > index 0000000..ae7be5f > --- /dev/null > +++ b/sql/session_tracker.cc > @@ -0,0 +1,1631 @@ > +/* Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved. > + Copyright (c) 2016, MariaDB > + > + 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_plugin.h" > +#include "session_tracker.h" > + > +#include "hash.h" > +#include "table.h" > +#include "rpl_gtid.h" > +#include "sql_class.h" > +#include "sql_show.h" > +#include "sql_plugin.h" > +#include "set_var.h" > + > +class Not_implemented_tracker : public State_tracker > +{ > +public: > + bool enable(THD *thd) > + { return false; } > + bool check(THD *, set_var *) > + { return false; } > + bool update(THD *) > + { return false; } > + bool store(THD *, String *) > + { return false; } > + void mark_as_changed(THD *, LEX_CSTRING *tracked_item_name) > + {} > + > +}; > + > +static my_bool name_array_filler(void *ptr, void *data_ptr); > +/** > + Session_sysvars_tracker > + > + This is a tracker class that enables & manages the tracking of session > + system variables. It internally maintains a hash of user supplied variable > + references and a boolean field to store if the variable was changed by the > + last statement. > +*/ > + > +class Session_sysvars_tracker : public State_tracker > +{ > +private: > + > + struct sysvar_node_st { > + sys_var *m_svar; > + bool *test_load; > + bool m_changed; > + }; because you use sysvar_node_st, you need to allocate nodes all the time - many small mallocs. I think this can be avoided, like this: 1. you don't need test_load pointer, if a variable is not loaded, it cannot be changed either, so it's okay to use only m_changed and not look at *test_load. It means that for plugin sysvars you cannot store sys_var* in the hash, you need a new method "void *sys_var::safe_ptr()". Or, may be, "intptr sys_var::tracker_key()". It'll return this for server sysvars and a bookmark for plugin sysvars. 2. as for m_changed, it's just one bit, you can use the lowest bit of the key for this. sys_var::safe_ptr() (or sys_var::tracker_key()) is always aligned, so few lowest bits are free. You can use the second bit to mark all plugin variables (to know whether a pointer is to sysvar or to a bookmark). alternatively, you don't store m_changed in the hash, but keep a list of all modified sysvars. this will also be useful in the store method (and solves the problem or growing hash when '*' is specified) Then you won't need to allocate anything and you can store the key directly in the hash. > + > + class vars_list > + { > + private: > + /** > + Registered system variables. (@@session_track_system_variables) > + A hash to store the name of all the system variables specified by the > + user. > + */ > + HASH m_registered_sysvars; > + /** Size of buffer for string representation */ > + size_t buffer_length; > + myf m_mem_flag; > + /** > + If TRUE then we want to check all session variable. > + */ > + bool track_all; > + void init() > + { > + my_hash_init(&m_registered_sysvars, > + &my_charset_bin, > + 4, 0, 0, (my_hash_get_key) sysvars_get_key, > + my_free, MYF(HASH_UNIQUE | > + ((m_mem_flag & MY_THREAD_SPECIFIC) ? > + HASH_THREAD_SPECIFIC : 0))); > + } > + void free_hash() > + { > + if (my_hash_inited(&m_registered_sysvars)) > + { > + my_hash_free(&m_registered_sysvars); > + } > + } > + > + uchar* search(const sys_var *svar) > + { > + return (my_hash_search(&m_registered_sysvars, (const uchar *)&svar, > + sizeof(sys_var *))); > + } > + > + public: > + vars_list() : > + buffer_length(0) > + { > + m_mem_flag= current_thd ? MY_THREAD_SPECIFIC : 0; > + init(); > + } > + > + size_t get_buffer_length() > + { > + DBUG_ASSERT(buffer_length != 0); // asked earlier then should > + return buffer_length; > + } > + ~vars_list() > + { > + /* free the allocated hash. */ > + if (my_hash_inited(&m_registered_sysvars)) > + { > + my_hash_free(&m_registered_sysvars); > + } > + } > + > + uchar* search(sysvar_node_st *node, const sys_var *svar) may be, rename to insert_or_search ? > + { > + uchar *res; > + res= search(svar); > + if (!res) > + { > + if (track_all) will the hash grow all the time when track_all is true? > + { > + insert(node, svar, m_mem_flag); > + return search(svar); > + } > + } > + return res; > + } > + > + uchar* operator[](ulong idx) > + { > + return my_hash_element(&m_registered_sysvars, idx); > + } > + bool insert(sysvar_node_st *node, const sys_var *svar, myf mem_flag); > + void reset(); > + void copy(vars_list* from, THD *thd); > + bool parse_var_list(THD *thd, LEX_STRING var_list, bool throw_error, > + const CHARSET_INFO *char_set, bool session_created); > + bool construct_var_list(char *buf, size_t buf_len); > + }; > + /** > + Two objects of vars_list type are maintained to manage > + various operations. > + */ > + vars_list *orig_list, *tool_list; > + > +public: > + Session_sysvars_tracker() > + { > + orig_list= new (std::nothrow) vars_list(); > + tool_list= new (std::nothrow) vars_list(); > + } > + > + ~Session_sysvars_tracker() > + { > + if (orig_list) > + delete orig_list; > + if (tool_list) > + delete tool_list; > + } > + > + size_t get_buffer_length() > + { > + return orig_list->get_buffer_length(); > + } > + bool construct_var_list(char *buf, size_t buf_len) > + { > + return orig_list->construct_var_list(buf, buf_len); > + } > + > + /** > + Method used to check the validity of string provided > + for session_track_system_variables during the server > + startup. > + */ > + static bool server_init_check(THD *thd, const CHARSET_INFO *char_set, > + LEX_STRING var_list) > + { > + vars_list dummy; > + bool result; > + result= dummy.parse_var_list(thd, var_list, false, char_set, false); > + return result; > + } > + static bool server_init_process(THD *thd, const CHARSET_INFO *char_set, > + LEX_STRING var_list) > + { > + vars_list dummy; > + bool result; > + result= dummy.parse_var_list(thd, var_list, false, char_set, false); > + if (!result) > + dummy.construct_var_list(var_list.str, var_list.length + 1); > + return result; > + } > + > + void reset(); > + bool enable(THD *thd); > + bool check(THD *thd, set_var *var); > + bool check_str(THD *thd, LEX_STRING val); > + bool update(THD *thd); > + bool store(THD *thd, String *buf); > + void mark_as_changed(THD *thd, LEX_CSTRING *tracked_item_name); > + /* callback */ > + static uchar *sysvars_get_key(const char *entry, size_t *length, > + my_bool not_used __attribute__((unused))); > + > + friend my_bool name_array_filler(void *ptr, void *data_ptr); why wouldn't you make this one a class method too? > +}; > + > + > + > +/** > + Current_schema_tracker, > + > + This is a tracker class that enables & manages the tracking of current > + schema for a particular connection. > +*/ > + > +class Current_schema_tracker : public State_tracker > +{ > +private: > + bool schema_track_inited; > + void reset(); > + > +public: > + > + Current_schema_tracker() > + { > + schema_track_inited= false; > + } > + > + bool enable(THD *thd) > + { return update(thd); } > + bool check(THD *thd, set_var *var) > + { return false; } > + bool update(THD *thd); > + bool store(THD *thd, String *buf); > + void mark_as_changed(THD *thd, LEX_CSTRING *tracked_item_name); > +}; > + > +/* > + Session_state_change_tracker > + > + This is a boolean tracker class that will monitor any change that contributes > + to a session state change. > + Attributes that contribute to session state change include: > + - Successful change to System variables > + - User defined variables assignments > + - temporary tables created, altered or deleted > + - prepared statements added or removed > + - change in current database > + - change of current role > +*/ > + > +class Session_state_change_tracker : public State_tracker > +{ > +private: > + > + void reset(); > + > +public: > + Session_state_change_tracker(); > + bool enable(THD *thd) > + { return update(thd); }; > + bool check(THD *thd, set_var *var) > + { return false; } > + bool update(THD *thd); > + bool store(THD *thd, String *buf); > + void mark_as_changed(THD *thd, LEX_CSTRING *tracked_item_name); > + bool is_state_changed(THD*); > + void ensure_enabled(THD *thd) > + {} this method is never used > +}; > + > + > +/* To be used in expanding the buffer. */ > +static const unsigned int EXTRA_ALLOC= 1024; > + > + > +void Session_sysvars_tracker::vars_list::reset() > +{ > + buffer_length= 0; > + track_all= 0; > + if (m_registered_sysvars.records) > + my_hash_reset(&m_registered_sysvars); > +} > + > +/** > + Copy the given list. > + > + @param from Source vars_list object. > + @param thd THD handle to retrive the charset in use. > + > + @retval true there is something to track > + @retval false nothing to track > +*/ > + > +void Session_sysvars_tracker::vars_list::copy(vars_list* from, THD *thd) > +{ > + reset(); > + track_all= from->track_all; > + free_hash(); > + buffer_length= from->buffer_length; > + m_registered_sysvars= from->m_registered_sysvars; > + from->init(); > +} > + > +/** > + Inserts the variable to be tracked into m_registered_sysvars hash. > + > + @param node Node to be inserted. > + @param svar address of the system variable > + > + @retval false success > + @retval true error > +*/ > + > +bool Session_sysvars_tracker::vars_list::insert(sysvar_node_st *node, > + const sys_var *svar, > + myf mem_flag) > +{ > + if (!node) > + { > + if (!(node= (sysvar_node_st *) my_malloc(sizeof(sysvar_node_st), > + MYF(MY_WME | mem_flag)))) > + { > + reset(); > + return true; > + } > + } > + > + node->m_svar= (sys_var *)svar; > + node->test_load= node->m_svar->test_load; > + node->m_changed= false; > + if (my_hash_insert(&m_registered_sysvars, (uchar *) node)) > + { > + my_free(node); > + if (!search((sys_var *)svar)) > + { > + //EOF (error is already reported) > + reset(); > + return true; > + } > + } > + return false; > +} > + > +/** > + Parse the specified system variables list. > + > + @Note In case of invalid entry a warning is raised per invalid entry. > + This is done in order to handle 'potentially' valid system > + variables from uninstalled plugins which might get installed in > + future. > + > + > + @param thd [IN] The thd handle. > + @param var_list [IN] System variable list. > + @param throw_error [IN] bool when set to true, returns an error > + in case of invalid/duplicate values. > + @param char_set [IN] charecter set information used for string > + manipulations. > + @param session_created [IN] bool variable which says if the parse is > + already executed once. The mutex on variables > + is not acquired if this variable is false. why? > + > + @return > + true Error > + false Success > +*/ > +bool Session_sysvars_tracker::vars_list::parse_var_list(THD *thd, > + LEX_STRING var_list, > + bool throw_error, > + const CHARSET_INFO *char_set, > + bool session_created) > +{ > + const char separator= ','; > + char *token, *lasts= NULL; > + size_t rest= var_list.length; > + > + if (!var_list.str || var_list.length == 0) > + { > + buffer_length= 1; > + return false; > + } > + > + if(!strcmp(var_list.str,(const char *)"*")) > + { > + track_all= true; > + buffer_length= 2; > + return false; > + } > + > + buffer_length= var_list.length + 1; > + token= var_list.str; > + > + track_all= false; > + /* > + If Lock to the plugin mutex is not acquired here itself, it results > + in having to acquire it multiple times in find_sys_var_ex for each > + token value. Hence the mutex is handled here to avoid a performance > + overhead. > + */ > + if (!thd || session_created) > + mysql_mutex_lock(&LOCK_plugin); > + for (;;) > + { > + sys_var *svar; > + LEX_STRING var; > + > + lasts= (char *) memchr(token, separator, rest); > + > + var.str= token; > + if (lasts) > + { > + var.length= (lasts - token); > + rest-= var.length + 1; > + } > + else > + var.length= rest; > + > + /* Remove leading/trailing whitespace. */ > + trim_whitespace(char_set, &var); > + > + if ((svar= find_sys_var_ex(thd, var.str, var.length, throw_error, true))) > + { > + if (insert(NULL, svar, m_mem_flag) == TRUE) > + goto error; > + } > + else if (throw_error && session_created && thd) > + { > + push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, > + ER_WRONG_VALUE_FOR_VAR, > + "%.*s is not a valid system variable and will" > + "be ignored.", (int)var.length, token); > + } > + else > + goto error; > + > + if (lasts) > + token= lasts + 1; > + else > + break; > + } > + if (!thd || session_created) > + mysql_mutex_unlock(&LOCK_plugin); > + > + return false; > + > +error: > + if (!thd || session_created) > + mysql_mutex_unlock(&LOCK_plugin); > + return true; > +} > + > +struct name_array_filler_data > +{ > + LEX_CSTRING **names; > + uint idx; > + > +}; > + > +/** Collects variable references into array */ > +static my_bool name_array_filler(void *ptr, void *data_ptr) > +{ > + Session_sysvars_tracker::sysvar_node_st *node= > + (Session_sysvars_tracker::sysvar_node_st *)ptr; > + name_array_filler_data *data= (struct name_array_filler_data *)data_ptr; > + if (*node->test_load) > + data->names[data->idx++]= &node->m_svar->name; > + return FALSE; > +} > + > +/* Sorts variable references array */ > +static int name_array_sorter(const void *a, const void *b) > +{ > + LEX_CSTRING **an= (LEX_CSTRING **)a, **bn=(LEX_CSTRING **)b; > + size_t min= MY_MIN((*an)->length, (*bn)->length); > + int res= strncmp((*an)->str, (*bn)->str, min); > + if (res == 0) > + res= ((int)(*bn)->length)- ((int)(*an)->length); > + return res; > +} > + > +/** > + Construct variable list by internal hash with references > +*/ > + > +bool Session_sysvars_tracker::vars_list::construct_var_list(char *buf, > + size_t buf_len) > +{ > + struct name_array_filler_data data; > + size_t left= buf_len; > + size_t names_size= m_registered_sysvars.records * sizeof(LEX_CSTRING *); > + const char separator= ','; > + > + if (unlikely(buf_len < 1)) > + return true; > + > + if (unlikely(track_all)) > + { > + if (buf_len < 2) > + return true; > + buf[0]= '*'; > + buf[1]= '\0'; > + return false; > + } > + > + if (m_registered_sysvars.records == 0) > + { > + buf[0]= '\0'; > + return false; > + } > + > + data.names= (LEX_CSTRING**)my_safe_alloca(names_size); > + > + if (unlikely(!data.names)) > + return true; > + > + data.idx= 0; > + > + mysql_mutex_lock(&LOCK_plugin); > + my_hash_iterate(&m_registered_sysvars, &name_array_filler, &data); > + DBUG_ASSERT(data.idx <= m_registered_sysvars.records); > + > + > + if (m_registered_sysvars.records == 0) you could've done that before my_hash_iterate :) > + { > + mysql_mutex_unlock(&LOCK_plugin); > + buf[0]= '\0'; > + return false; > + } > + > + my_qsort(data.names, data.idx, sizeof(LEX_CSTRING *), > + &name_array_sorter); > + > + for(uint i= 0; i < data.idx; i++) > + { > + LEX_CSTRING *nm= data.names[i]; > + size_t ln= nm->length + 1; > + if (ln > left) > + { > + mysql_mutex_unlock(&LOCK_plugin); > + my_safe_afree(data.names, names_size); > + return true; > + } > + memcpy(buf, nm->str, nm->length); > + buf[nm->length]= separator; > + buf+= ln; > + left-= ln; > + } > + mysql_mutex_unlock(&LOCK_plugin); > + > + buf--; buf[0]= '\0'; > + my_safe_afree(data.names, names_size); > + > + return false; > +} > + > +/** > + Enable session tracker by parsing global value of tracked variables. > + > + @param thd [IN] The thd handle. > + > + @retval true Error > + @retval false Success > +*/ > + > +bool Session_sysvars_tracker::enable(THD *thd) > +{ > + sys_var *svar; > + > + mysql_mutex_lock(&LOCK_plugin); > + svar= find_sys_var_ex(thd, SESSION_TRACK_SYSTEM_VARIABLES_NAME.str, > + SESSION_TRACK_SYSTEM_VARIABLES_NAME.length, > + false, true); > + DBUG_ASSERT(svar); > + > + set_var tmp(thd, SHOW_OPT_GLOBAL, svar, &null_lex_str, NULL); > + svar->session_save_default(thd, &tmp); > + > + if (tool_list->parse_var_list(thd, tmp.save_result.string_value, > + true, thd->charset(), false) == true) > + { > + mysql_mutex_unlock(&LOCK_plugin); > + return true; > + } > + mysql_mutex_unlock(&LOCK_plugin); > + orig_list->copy(tool_list, thd); > + m_enabled= true; This is the only tracker where enable() is different from update(). And the reason for that is that in update() you reuse results of the check, so you only need to parse the string in enable(). But this is wrong. You cannot reuse results of the check(). The example is kind of artificial, but still: SET @@a=1, @@b=2, @@a=3; if the update() method of @@b will fail, @@a final value should be 1. but in your case (@@a is @@session_track_system_variables) it will be 3, because you remember the value of the last check only. So, the correct behavior is (for all sysvars): in check it should only do the check and store the value in the provided set_var::save_result union. in update it should use this value. and if you change session_track_system_variables to behave as other sysvars do, then your enable() will be the same as update(), and you can remove enable() method completely from all trackers. > + > + return false; > +} > + > + > +/** > + Check system variable name(s). > + > + @note This function is called from the ON_CHECK() function of the > + session_track_system_variables' sys_var class. > + > + @param thd [IN] The thd handle. > + @param var [IN] A pointer to set_var holding the specified list of > + system variable names. > + > + @retval true Error > + @retval false Success > +*/ > + > +inline bool Session_sysvars_tracker::check(THD *thd, set_var *var) > +{ > + return check_str(thd, var->save_result.string_value); > +} > + > +inline bool Session_sysvars_tracker::check_str(THD *thd, LEX_STRING val) > +{ > + tool_list->reset(); > + return tool_list->parse_var_list(thd, val, true, > + thd->charset(), true); > +} > + > + > +/** > + Once the value of the @@session_track_system_variables has been > + successfully updated, this function calls > + Session_sysvars_tracker::vars_list::copy updating the hash in orig_list > + which represents the system variables to be tracked. > + > + @note This function is called from the ON_UPDATE() function of the > + session_track_system_variables' sys_var class. > + > + @param thd [IN] The thd handle. > + > + @retval true Error > + @retval false Success > +*/ > + > +bool Session_sysvars_tracker::update(THD *thd) > +{ > + orig_list->copy(tool_list, thd); > + return false; > +} > + > + > +/** > + Store the data for changed system variables in the specified buffer. > + Once the data is stored, we reset the flags related to state-change > + (see reset()). > + > + @param thd [IN] The thd handle. > + @paran buf [INOUT] Buffer to store the information to. > + > + @retval true Error > + @retval false Success > +*/ > + > +bool Session_sysvars_tracker::store(THD *thd, String *buf) > +{ > + char val_buf[SHOW_VAR_FUNC_BUFF_SIZE]; > + SHOW_VAR show; > + const char *value; > + sysvar_node_st *node; > + const CHARSET_INFO *charset; > + size_t val_length, length; > + int idx= 0; > + > + /* As its always system variable. */ > + show.type= SHOW_SYS; > + > + while ((node= (sysvar_node_st *) (*orig_list)[idx])) Let's be consistent. Either you use my_hash_iterate in construct_var_list() and here. Or you use [] in both functions. Pick the one that you like more and use it. > + { > + if (node->m_changed) > + { > + mysql_mutex_lock(&LOCK_plugin); > + if (!*node->test_load) > + { > + mysql_mutex_unlock(&LOCK_plugin); > + continue; > + } > + sys_var *svar= node->m_svar; > + show.name= svar->name.str; > + show.value= (char *) svar; > + > + value= get_one_variable(thd, &show, OPT_SESSION, SHOW_SYS, NULL, > + &charset, val_buf, &val_length); if you have a sysvar already, you can simply do svar->val_str() > + mysql_mutex_unlock(&LOCK_plugin); > + > + length= net_length_size(svar->name.length) + > + svar->name.length + > + net_length_size(val_length) + > + val_length; > + > + compile_time_assert(SESSION_TRACK_SYSTEM_VARIABLES < 251); > + buf->prep_alloc(1 + net_length_size(length) + length, EXTRA_ALLOC); > + > + /* Session state type (SESSION_TRACK_SYSTEM_VARIABLES) */ > + buf->q_net_store_length((ulonglong)SESSION_TRACK_SYSTEM_VARIABLES); > + > + /* Length of the overall entity. */ > + buf->q_net_store_length((ulonglong)length); > + > + /* System variable's name (length-encoded string). */ > + buf->q_net_store_data((const uchar*)svar->name.str, svar->name.length); > + > + /* System variable's value (length-encoded string). */ > + buf->q_net_store_data((const uchar*)value, val_length); > + } > + ++ idx; > + } > + > + reset(); > + > + return false; > +} > + > + > +/** > + Mark the system variable as changed. > + > + @param [IN] pointer on a variable > +*/ > + > +void Session_sysvars_tracker::mark_as_changed(THD *thd, > + LEX_CSTRING *var) > +{ > + sysvar_node_st *node= NULL; > + sys_var *svar= (sys_var *)var; > + /* > + Check if the specified system variable is being tracked, if so > + mark it as changed and also set the class's m_changed flag. > + */ > + if ((node= (sysvar_node_st *) (orig_list->search(node, svar)))) > + { > + node->m_changed= true; > + m_changed= true; > + /* do not cache the statement when there is change in session state */ > + thd->lex->safe_to_cache_query= 0; > + thd->server_status|= SERVER_SESSION_STATE_CHANGED; > + } > +} > + > + > +/** > + Supply key to a hash. > + > + @param entry [IN] A single entry. > + @param length [OUT] Length of the key. > + @param not_used Unused. > + > + @return Pointer to the key buffer. > +*/ > + > +uchar *Session_sysvars_tracker::sysvars_get_key(const char *entry, > + size_t *length, > + my_bool not_used __attribute__((unused))) > +{ > + *length= sizeof(sys_var *); > + return (uchar *) &(((sysvar_node_st *) entry)->m_svar); > +} > + > + > +/** > + Prepare/reset the m_registered_sysvars hash for next statement. > +*/ > + > +void Session_sysvars_tracker::reset() > +{ > + sysvar_node_st *node; > + int idx= 0; > + > + while ((node= (sysvar_node_st *) (*orig_list)[idx])) > + { > + node->m_changed= false; > + ++ idx; > + } > + m_changed= false; > +} > + > +static Session_sysvars_tracker* sysvar_tracker(THD *thd) > +{ > + return (Session_sysvars_tracker*) > + thd->session_tracker.get_tracker(SESSION_SYSVARS_TRACKER); > +} > + > +bool sysvartrack_validate_value(THD *thd, const char *str, size_t len) > +{ > + LEX_STRING tmp= {(char *)str, len}; > + if (thd && sysvar_tracker(thd)->is_enabled()) > + return sysvar_tracker(thd)->check_str(thd, tmp); > + return Session_sysvars_tracker::server_init_check(thd, system_charset_info, > + tmp); this if() won't be needed, if Session_sysvars_tracker::check_str will not store the parsed hash, as I commented earier > +} > +bool sysvartrack_reprint_value(THD *thd, char *str, size_t len) > +{ > + LEX_STRING tmp= {str, len}; > + return Session_sysvars_tracker::server_init_process(thd, > + system_charset_info, > + tmp); > +} > +bool sysvartrack_update(THD *thd) > +{ > + return sysvar_tracker(thd)->update(thd); > +} > +size_t sysvartrack_value_len(THD *thd) > +{ > + return sysvar_tracker(thd)->get_buffer_length(); > +} > +bool sysvartrack_value_construct(THD *thd, char *val, size_t len) > +{ > + return sysvar_tracker(thd)->construct_var_list(val, len); > +} > + > +/////////////////////////////////////////////////////////////////////////////// > + > +/** > + Enable/disable the tracker based on @@session_track_schema's value. > + > + @param thd [IN] The thd handle. > + > + @return > + false (always) > +*/ > + > +bool Current_schema_tracker::update(THD *thd) > +{ > + m_enabled= thd->variables.session_track_schema; > + return false; > +} > + > + > +/** > + Store the schema name as length-encoded string in the specified buffer. > + > + @param thd [IN] The thd handle. > + @paran buf [INOUT] Buffer to store the information to. > + > + @reval false Success > + @retval true Error > +*/ > + > +bool Current_schema_tracker::store(THD *thd, String *buf) > +{ > + ulonglong db_length, length; > + > + /* > + Protocol made (by unknown reasons) redundant: > + It saves length of database name and name of database name + > + length of saved length of database length. > + */ > + length= db_length= thd->db_length; > + length += net_length_size(length); > + > + compile_time_assert(SESSION_TRACK_SCHEMA < 251); > + compile_time_assert(NAME_LEN < 251); > + DBUG_ASSERT(net_length_size(length) < 251); eh? net_length_size < 251? may be length < 251? > + if (buf->prep_alloc(1 + 1 + length, EXTRA_ALLOC)) > + return true; > + > + /* Session state type (SESSION_TRACK_SCHEMA) */ > + buf->q_net_store_length((ulonglong)SESSION_TRACK_SCHEMA); you can use buf->q_append((char)SESSION_TRACK_SCHEMA) instead. It's faster and you already know that SESSION_TRACK_SCHEMA will be encoded in one byte. This applies to other similar places too. > + > + /* Length of the overall entity. */ > + buf->q_net_store_length(length); > + > + /* Length and current schema name */ > + buf->q_net_store_data((const uchar *)thd->db, thd->db_length); > + > + reset(); > + > + return false; > +} > + > + > +/** > + Mark the tracker as changed. > +*/ > + > +void Current_schema_tracker::mark_as_changed(THD *thd, LEX_CSTRING *) > +{ > + m_changed= true; > + thd->lex->safe_to_cache_query= 0; > + thd->server_status|= SERVER_SESSION_STATE_CHANGED; put this code into the parent State_tracker class then all other trackers won't need to implement it. Only Session_sysvars_tracker will, but still it'll call State_tracker::mark_as_changed(...). > +} > + > + > +/** > + Reset the m_changed flag for next statement. > + > + @return void > +*/ > + > +void Current_schema_tracker::reset() > +{ > + m_changed= false; > +} > + > + > +/////////////////////////////////////////////////////////////////////////////// > + > + > +Transaction_state_tracker::Transaction_state_tracker() > +{ > + m_enabled = false; > + tx_changed = TX_CHG_NONE; > + tx_curr_state = > + tx_reported_state= TX_EMPTY; > + tx_read_flags = TX_READ_INHERIT; > + tx_isol_level = TX_ISOL_INHERIT; > +} > + > +/** > + Enable/disable the tracker based on @@session_track_transaction_info. > + > + @param thd [IN] The thd handle. > + > + @retval true if updating the tracking level failed > + @retval false otherwise > +*/ > + > +bool Transaction_state_tracker::update(THD *thd) > +{ > +#ifdef EMBEDDED_LIBRARY why you don't do that for other trackers? or, in other words, why do you do it for this tracker? > + return true; > + > +#else > + if (thd->variables.session_track_transaction_info != TX_TRACK_NONE) > + { > + /* > + If we only just turned reporting on (rather than changing between > + state and characteristics reporting), start from a defined state. > + */ > + if (!m_enabled) > + { > + tx_curr_state = > + tx_reported_state = TX_EMPTY; > + tx_changed |= TX_CHG_STATE; > + m_enabled= true; > + } > + if (thd->variables.session_track_transaction_info == TX_TRACK_CHISTICS) > + tx_changed |= TX_CHG_CHISTICS; > + mark_as_changed(thd, NULL); > + } > + else > + m_enabled= false; > + > + return false; > +#endif > +} > + > + > +/** > + Store the transaction state (and, optionally, characteristics) > + as length-encoded string in the specified buffer. Once the data > + is stored, we reset the flags related to state-change (see reset()). > + > + > + @param thd [IN] The thd handle. > + @paran buf [INOUT] Buffer to store the information to. > + > + @retval false Success > + @retval true Error > +*/ > + > +static LEX_CSTRING isol[]= { > + { STRING_WITH_LEN("READ UNCOMMITTED") }, > + { STRING_WITH_LEN("READ COMMITTED") }, > + { STRING_WITH_LEN("REPEATABLE READ") }, > + { STRING_WITH_LEN("SERIALIZABLE") } > +}; > + > +bool Transaction_state_tracker::store(THD *thd, String *buf) > +{ > + /* STATE */ > + if (tx_changed & TX_CHG_STATE) > + { > + uchar *to= (uchar *) buf->prep_append(11, EXTRA_ALLOC); > + > + to= net_store_length((uchar *) to, > + (ulonglong) SESSION_TRACK_TRANSACTION_STATE); > + > + to= net_store_length((uchar *) to, (ulonglong) 9); > + to= net_store_length((uchar *) to, (ulonglong) 8); > + > + *(to++)= (tx_curr_state & TX_EXPLICIT) ? 'T' : > + ((tx_curr_state & TX_IMPLICIT) ? 'I' : '_'); > + *(to++)= (tx_curr_state & TX_READ_UNSAFE) ? 'r' : '_'; > + *(to++)= ((tx_curr_state & TX_READ_TRX) || > + (tx_curr_state & TX_WITH_SNAPSHOT)) ? 'R' : '_'; > + *(to++)= (tx_curr_state & TX_WRITE_UNSAFE) ? 'w' : '_'; > + *(to++)= (tx_curr_state & TX_WRITE_TRX) ? 'W' : '_'; > + *(to++)= (tx_curr_state & TX_STMT_UNSAFE) ? 's' : '_'; > + *(to++)= (tx_curr_state & TX_RESULT_SET) ? 'S' : '_'; > + *(to++)= (tx_curr_state & TX_LOCKED_TABLES) ? 'L' : '_'; This doesn't make much sense, but I suppose that's what MySQL does :( btw, why don't you use String methods here? q_net_store_length and q_append? That'd be consistent with the rest of the code. > + } > + > + /* CHARACTERISTICS -- How to restart the transaction */ > + > + if ((thd->variables.session_track_transaction_info == TX_TRACK_CHISTICS) && > + (tx_changed & TX_CHG_CHISTICS)) > + { > + bool is_xa= (thd->transaction.xid_state.xa_state != XA_NOTR); > + size_t start; > + > + /* 2 length by 1 byte and code */ > + buf->prep_alloc(1 + 1 + 1, EXTRA_ALLOC); > + > + /* Session state type (SESSION_TRACK_TRANSACTION_CHARACTERISTICS) */ > + buf->q_net_store_length((ulonglong) > + SESSION_TRACK_TRANSACTION_CHARACTERISTICS); > + compile_time_assert(SESSION_TRACK_TRANSACTION_CHARACTERISTICS < 251); > + > + /* Whole length: Track result will fit in 251 byte (in worst case 110) */ > + buf->append('\0'); > + > + /* String length: Track result will fit in 251 byte (in worst case 110) */ > + buf->append('\0'); remove these two comments, use q_append, add a comment, like /* placeholders for lengths. will be filled in at the end */ > + > + start= buf->length(); > + > + { > + /* > + We have four basic replay scenarios: > + > + a) SET TRANSACTION was used, but before an actual transaction > + was started, the load balancer moves the connection elsewhere. > + In that case, the same one-shots should be set up in the > + target session. (read-only/read-write; isolation-level) > + > + b) The initial transaction has begun; the relevant characteristics > + are the session defaults, possibly overridden by previous > + SET TRANSACTION statements, possibly overridden or extended > + by options passed to the START TRANSACTION statement. > + If the load balancer wishes to move this transaction, > + it needs to be replayed with the correct characteristics. > + (read-only/read-write from SET or START; > + isolation-level from SET only, snapshot from START only) > + > + c) A subsequent transaction started with START TRANSACTION > + (which is legal syntax in lieu of COMMIT AND CHAIN in MySQL) > + may add/modify the current one-shots: > + > + - It may set up a read-only/read-write one-shot. > + This one-shot will override the value used in the previous > + transaction (whether that came from the default or a one-shot), > + and, like all one-shots currently do, it will carry over into > + any subsequent transactions that don't explicitly override them > + in turn. This behavior is not guaranteed in the docs and may > + change in the future, but the tracker item should correctly > + reflect whatever behavior a given version of mysqld implements. > + > + - It may also set up a WITH CONSISTENT SNAPSHOT one-shot. > + This one-shot does not currently carry over into subsequent > + transactions (meaning that with "traditional syntax", WITH > + CONSISTENT SNAPSHOT can only be requested for the first part > + of a transaction chain). Again, the tracker item should reflect > + mysqld behavior. > + > + d) A subsequent transaction started using COMMIT AND CHAIN > + (or, for that matter, BEGIN WORK, which is currently > + legal and equivalent syntax in MySQL, or START TRANSACTION > + sans options) will re-use any one-shots set up so far > + (with SET before the first transaction started, and with > + all subsequent STARTs), except for WITH CONSISTANT SNAPSHOT, > + which will never be chained and only applies when explicitly > + given. > + > + It bears noting that if we switch sessions in a follow-up > + transaction, SET TRANSACTION would be illegal in the old > + session (as a transaction is active), whereas in the target > + session which is being prepared, it should be legal, as no > + transaction (chain) should have started yet. > + > + Therefore, we are free to generate SET TRANSACTION as a replay > + statement even for a transaction that isn't the first in an > + ongoing chain. Consider > + > + SET TRANSACTION ISOLATION LEVEL READ UNCOMMITED; > + START TRANSACTION READ ONLY, WITH CONSISTENT SNAPSHOT; > + # work > + COMMIT AND CHAIN; > + > + If we switch away at this point, the replay in the new session > + needs to be > + > + SET TRANSACTION ISOLATION LEVEL READ UNCOMMITED; > + START TRANSACTION READ ONLY; > + > + When a transaction ends (COMMIT/ROLLBACK sans CHAIN), all > + per-transaction characteristics are reset to the session's > + defaults. > + > + This also holds for a transaction ended implicitly! (transaction.cc) > + Once again, the aim is to have the tracker item reflect on a > + given mysqld's actual behavior. > + */ > + > + /* > + "ISOLATION LEVEL" > + Only legal in SET TRANSACTION, so will always be replayed as such. > + */ > + if (tx_isol_level != TX_ISOL_INHERIT) > + { > + /* > + Unfortunately, we can't re-use tx_isolation_names / > + tx_isolation_typelib as it hyphenates its items. > + */ > + buf->append(STRING_WITH_LEN("SET TRANSACTION ISOLATION LEVEL ")); > + buf->append(isol[tx_isol_level - 1].str, isol[tx_isol_level - 1].length); this could be simply buf->append(&isol[tx_isol_level - 1]); or even buf->append(isol + tx_isol_level); if you add a dummy element to isol. but whatever... long version works too. > + buf->append(STRING_WITH_LEN("; ")); > + } > + > + /* > + Start transaction will usually result in TX_EXPLICIT (transaction > + started, but no data attached yet), except when WITH CONSISTENT > + SNAPSHOT, in which case we may have data pending. > + If it's an XA transaction, we don't go through here so we can > + first print the trx access mode ("SET TRANSACTION READ ...") > + separately before adding XA START (whereas with START TRANSACTION, > + we can merge the access mode into the same statement). > + */ > + if ((tx_curr_state & TX_EXPLICIT) && !is_xa) > + { > + buf->append(STRING_WITH_LEN("START TRANSACTION")); > + > + /* > + "WITH CONSISTENT SNAPSHOT" > + Defaults to no, can only be enabled. > + Only appears in START TRANSACTION. > + */ > + if (tx_curr_state & TX_WITH_SNAPSHOT) > + { > + buf->append(STRING_WITH_LEN(" WITH CONSISTENT SNAPSHOT")); > + if (tx_read_flags != TX_READ_INHERIT) > + buf->append(STRING_WITH_LEN(",")); > + } > + > + /* > + "READ WRITE / READ ONLY" can be set globally, per-session, > + or just for one transaction. > + > + The latter case can take the form of > + START TRANSACTION READ (WRITE|ONLY), or of > + SET TRANSACTION READ (ONLY|WRITE). > + (Both set thd->read_only for the upcoming transaction; > + it will ultimately be re-set to the session default.) > + > + As the regular session-variable tracker does not monitor the one-shot, > + we'll have to do it here. > + > + If READ is flagged as set explicitly (rather than just inherited > + from the session's default), we'll get the actual bool from the THD. > + */ > + if (tx_read_flags != TX_READ_INHERIT) > + { > + if (tx_read_flags == TX_READ_ONLY) > + buf->append(STRING_WITH_LEN(" READ ONLY")); > + else > + buf->append(STRING_WITH_LEN(" READ WRITE")); > + } > + buf->append(STRING_WITH_LEN("; ")); > + } > + else if (tx_read_flags != TX_READ_INHERIT) > + { > + /* > + "READ ONLY" / "READ WRITE" > + We could transform this to SET TRANSACTION even when it occurs > + in START TRANSACTION, but for now, we'll resysynthesize the original > + command as closely as possible. > + */ > + buf->append(STRING_WITH_LEN("SET TRANSACTION ")); > + if (tx_read_flags == TX_READ_ONLY) > + buf->append(STRING_WITH_LEN("READ ONLY; ")); > + else > + buf->append(STRING_WITH_LEN("READ WRITE; ")); > + } > + > + if ((tx_curr_state & TX_EXPLICIT) && is_xa) > + { > + XID *xid= &thd->transaction.xid_state.xid; > + long glen, blen; > + > + buf->append(STRING_WITH_LEN("XA START")); > + > + if ((glen= xid->gtrid_length) > 0) > + { > + buf->append(STRING_WITH_LEN(" '")); > + buf->append(xid->data, glen); > + > + if ((blen= xid->bqual_length) > 0) > + { > + buf->append(STRING_WITH_LEN("','")); > + buf->append(xid->data + glen, blen); > + } > + buf->append(STRING_WITH_LEN("'")); > + > + if (xid->formatID != 1) > + { > + buf->append(STRING_WITH_LEN(",")); > + buf->append_ulonglong(xid->formatID); > + } > + } > + > + buf->append(STRING_WITH_LEN("; ")); > + } > + > + // discard trailing space > + if (buf->length() > start) > + buf->length(buf->length() - 1); > + } > + > + { > + ulonglong length= buf->length() - start; > + uchar *place= (uchar *)(buf->ptr() + (start - 2)); > + DBUG_ASSERT(length < 249); // in fact < 110 > + DBUG_ASSERT(start >= 3); > + > + DBUG_ASSERT((place - 1)[0] == SESSION_TRACK_TRANSACTION_CHARACTERISTICS); > + /* Length of the overall entity. */ > + place[0]= length + 1; > + /* Transaction characteristics (length-encoded string). */ > + place[1]= length; > + } > + } > + > + reset(); > + > + return false; > +} > + > + > +/** > + Mark the tracker as changed. > +*/ > + > +void Transaction_state_tracker::mark_as_changed(THD *thd, > + LEX_CSTRING *tracked_item_nam) > +{ > + m_changed= true; > + thd->lex->safe_to_cache_query= 0; > + thd->server_status|= SERVER_SESSION_STATE_CHANGED; > +} > + > + > +/** > + Reset the m_changed flag for next statement. > +*/ > + > +void Transaction_state_tracker::reset() > +{ > + m_changed= false; > + tx_reported_state= tx_curr_state; > + tx_changed= TX_CHG_NONE; > +} > + > + > +/** > + Helper function: turn table info into table access flag. > + Accepts table lock type and engine type flag (transactional/ > + non-transactional), and returns the corresponding access flag > + out of TX_READ_TRX, TX_READ_UNSAFE, TX_WRITE_TRX, TX_WRITE_UNSAFE. > + > + @param thd [IN] The thd handle > + @param set [IN] The table's access/lock type > + @param set [IN] Whether the table's engine is transactional > + > + @return The table access flag > +*/ > + > +enum_tx_state Transaction_state_tracker::calc_trx_state(THD *thd, > + thr_lock_type l, > + bool has_trx) > +{ > + enum_tx_state s; > + bool read= (l <= TL_READ_NO_INSERT); > + > + if (read) > + s= has_trx ? TX_READ_TRX : TX_READ_UNSAFE; > + else > + s= has_trx ? TX_WRITE_TRX : TX_WRITE_UNSAFE; I'd do an array lookup, like return arr[l <= TL_READ_NO_INSERT][has_trx]; but whatever... > + > + return s; > +} > + > + > +/** > + Register the end of an (implicit or explicit) transaction. > + > + @param thd [IN] The thd handle > +*/ > +void Transaction_state_tracker::end_trx(THD *thd) > +{ > + DBUG_ASSERT(thd->variables.session_track_transaction_info > TX_TRACK_NONE); > + > + if ((!m_enabled) || (thd->state_flags & Open_tables_state::BACKUPS_AVAIL)) > + return; > + > + if (tx_curr_state != TX_EMPTY) > + { > + if (tx_curr_state & TX_EXPLICIT) > + tx_changed |= TX_CHG_CHISTICS; > + tx_curr_state &= TX_LOCKED_TABLES; > + } > + update_change_flags(thd); > +} > + > + > +/** > + Clear flags pertaining to the current statement or transaction. > + May be called repeatedly within the same execution cycle. > + > + @param thd [IN] The thd handle. > + @param set [IN] The flags to clear > +*/ > + > +void Transaction_state_tracker::clear_trx_state(THD *thd, uint clear) > +{ > + if ((!m_enabled) || (thd->state_flags & Open_tables_state::BACKUPS_AVAIL)) > + return; > + > + tx_curr_state &= ~clear; > + update_change_flags(thd); > +} > + > + > +/** > + Add flags pertaining to the current statement or transaction. > + May be called repeatedly within the same execution cycle, > + e.g. to add access info for more tables. > + > + @param thd [IN] The thd handle. > + @param set [IN] The flags to add > +*/ > + > +void Transaction_state_tracker::add_trx_state(THD *thd, uint add) > +{ > + if ((!m_enabled) || (thd->state_flags & Open_tables_state::BACKUPS_AVAIL)) > + return; > + > + if (add == TX_EXPLICIT) > + { > + /* Always send characteristic item (if tracked), always replace state. */ > + tx_changed |= TX_CHG_CHISTICS; > + tx_curr_state = TX_EXPLICIT; > + } > + > + /* > + If we're not in an implicit or explicit transaction, but > + autocommit==0 and tables are accessed, we flag "implicit transaction." > + */ > + else if (!(tx_curr_state & (TX_EXPLICIT|TX_IMPLICIT)) && > + (thd->variables.option_bits & OPTION_NOT_AUTOCOMMIT) && > + (add & > + (TX_READ_TRX | TX_READ_UNSAFE | TX_WRITE_TRX | TX_WRITE_UNSAFE))) > + tx_curr_state |= TX_IMPLICIT; > + > + /* > + Only flag state when in transaction or LOCK TABLES is added. > + */ > + if ((tx_curr_state & (TX_EXPLICIT | TX_IMPLICIT)) || > + (add & TX_LOCKED_TABLES)) > + tx_curr_state |= add; > + > + update_change_flags(thd); > +} > + > + > +/** > + Add "unsafe statement" flag if applicable. > + > + @param thd [IN] The thd handle. > + @param set [IN] The flags to add > +*/ > + > +void Transaction_state_tracker::add_trx_state_from_thd(THD *thd) > +{ > + if (m_enabled) > + { > + if (thd->lex->is_stmt_unsafe()) > + add_trx_state(thd, TX_STMT_UNSAFE); > + } > +} > + > + > +/** > + Set read flags (read only/read write) pertaining to the next > + transaction. > + > + @param thd [IN] The thd handle. > + @param set [IN] The flags to set > +*/ > + > +void Transaction_state_tracker::set_read_flags(THD *thd, > + enum enum_tx_read_flags flags) > +{ > + if (m_enabled && (tx_read_flags != flags)) > + { > + tx_read_flags = flags; > + tx_changed |= TX_CHG_CHISTICS; > + mark_as_changed(thd, NULL); > + } > +} > + > + > +/** > + Set isolation level pertaining to the next transaction. > + > + @param thd [IN] The thd handle. > + @param set [IN] The isolation level to set > +*/ > + > +void Transaction_state_tracker::set_isol_level(THD *thd, > + enum enum_tx_isol_level level) > +{ > + if (m_enabled && (tx_isol_level != level)) > + { > + tx_isol_level = level; > + tx_changed |= TX_CHG_CHISTICS; > + mark_as_changed(thd, NULL); > + } > +} > + > + > +/////////////////////////////////////////////////////////////////////////////// > + > +Session_state_change_tracker::Session_state_change_tracker() > +{ > + m_changed= false; > +} > + > +/** > + @Enable/disable the tracker based on @@session_track_state_change value. > + > + @param thd [IN] The thd handle. > + @return false (always) > + > +**/ > + > +bool Session_state_change_tracker::update(THD *thd) > +{ > + m_enabled= thd->variables.session_track_state_change; > + return false; > +} > + > +/** > + Store the '1' in the specified buffer when state is changed. > + > + @param thd [IN] The thd handle. > + @paran buf [INOUT] Buffer to store the information to. > + > + @reval false Success > + @retval true Error > +**/ > + > +bool Session_state_change_tracker::store(THD *thd, String *buf) > +{ > + if (buf->prep_alloc(1 + 1 + 1, EXTRA_ALLOC)) > + return true; > + > + compile_time_assert(SESSION_TRACK_STATE_CHANGE < 251); > + /* Session state type (SESSION_TRACK_STATE_CHANGE) */ > + buf->q_net_store_length((ulonglong)SESSION_TRACK_STATE_CHANGE); > + > + /* Length of the overall entity (1 byte) */ > + buf->q_append('\1'); > + > + DBUG_ASSERT(is_state_changed(thd)); > + buf->q_append('1'); > + > + reset(); > + > + return false; > +} > + > +/** > + Mark the tracker as changed and associated session > + attributes accordingly. > +*/ > + > +void Session_state_change_tracker::mark_as_changed(THD *thd, LEX_CSTRING *) > +{ > + m_changed= true; > + thd->lex->safe_to_cache_query= 0; > + thd->server_status|= SERVER_SESSION_STATE_CHANGED; > +} > + > +/** > + Reset the m_changed flag for next statement. > +*/ > + > +void Session_state_change_tracker::reset() > +{ > + m_changed= false; > +} > + > +/** > + Find if there is a session state change. > +*/ > + > +bool Session_state_change_tracker::is_state_changed(THD *) > +{ > + return m_changed; > +} > + > +/////////////////////////////////////////////////////////////////////////////// > + > +/** > + @brief Initialize session tracker objects. > +*/ > + > +Session_tracker::Session_tracker() > +{ > + for (int i= 0; i <= SESSION_TRACKER_END; i ++) > + m_trackers[i]= NULL; > +} > + > + > +/** > + @brief Enables the tracker objects. > + > + @param thd [IN] The thread handle. > + > + @return void > +*/ > + > +void Session_tracker::enable(THD *thd) > +{ > + /* > + Originally and correctly this allocation was in the constructor and > + deallocation in the destructor, but in this case memory counting > + system works incorrectly (for example in INSERT DELAYED thread) > + */ > + deinit(); > + m_trackers[SESSION_SYSVARS_TRACKER]= > + new (std::nothrow) Session_sysvars_tracker(); > + m_trackers[CURRENT_SCHEMA_TRACKER]= > + new (std::nothrow) Current_schema_tracker; > + m_trackers[SESSION_STATE_CHANGE_TRACKER]= > + new (std::nothrow) Session_state_change_tracker; > + m_trackers[SESSION_GTIDS_TRACKER]= > + new (std::nothrow) Not_implemented_tracker; > + m_trackers[TRANSACTION_INFO_TRACKER]= > + new (std::nothrow) Transaction_state_tracker; why do you need to allocate them dynamically at all? Just put them inside Session_tracker object. > + > + for (int i= 0; i <= SESSION_TRACKER_END; i ++) > + m_trackers[i]->enable(thd); > +} > + > + > +/** > + Method called during the server startup to verify the contents > + of @@session_track_system_variables. > + > + @retval false Success > + @retval true Failure > +*/ > + > +bool Session_tracker::server_boot_verify(const CHARSET_INFO *char_set) > +{ > + Session_sysvars_tracker *server_tracker; > + bool result; > + sys_var *svar= find_sys_var_ex(NULL, SESSION_TRACK_SYSTEM_VARIABLES_NAME.str, > + SESSION_TRACK_SYSTEM_VARIABLES_NAME.length, > + false, true); > + DBUG_ASSERT(svar); > + set_var tmp(NULL, SHOW_OPT_GLOBAL, svar, &null_lex_str, NULL); > + svar->session_save_default(NULL, &tmp); > + server_tracker= new (std::nothrow) Session_sysvars_tracker(); > + result= server_tracker->server_init_check(NULL, char_set, > + tmp.save_result.string_value); > + delete server_tracker; grrrr. really? svar->session_save_default() and new/delete a tracker? just to verify the value? > + return result; > +} > + > + > +/** > + @brief Store all change information in the specified buffer. > + > + @param thd [IN] The thd handle. > + @param buf [OUT] Reference to the string buffer to which the state > + change data needs to be written. > +*/ > + > +void Session_tracker::store(THD *thd, String *buf) > +{ > + size_t start; > + > + /* > + Probably most track result will fit in 251 byte so lets made it at > + least efficient. We allocate 1 byte for length and then will move > + string if there is more. > + */ > + buf->append('\0'); > + start= buf->length(); > + > + /* Get total length. */ > + for (int i= 0; i <= SESSION_TRACKER_END; i ++) > + { > + if (m_trackers[i]->is_changed() && > + m_trackers[i]->store(thd, buf)) > + { > + buf->length(start); // it is safer to have 0-length block in case of error > + return; > + } > + } > + > + size_t length= buf->length() - start; > + uchar *data= (uchar *)(buf->ptr() + start); > + uint size; > + > + if ((size= net_length_size(length)) != 1) > + { > + if (buf->prep_alloc(size - 1, EXTRA_ALLOC)) > + { > + buf->length(start); // it is safer to have 0-length block in case of error > + return; > + } > + memmove(data + (size - 1), data, length); > + } > + > + net_store_length(data - 1, length); > +} > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt > index 7dfc622..c9e83ae 100644 > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -7168,6 +7167,10 @@ skip-to-error-number 3000 > ER_MYSQL_57_TEST > eng "5.7 test" > > +ER_NET_OK_PACKET_TOO_LARGE 08S01 > + eng "OK packet too large" > + ukr "Пакет OK надто великий" as I've said above, this is a rather strange error. > + > # MariaDB extra error numbers starts from 4000 > skip-to-error-number 4000 > > diff --git a/sql/lock.cc b/sql/lock.cc > index 2e44786..e2d2da0 100644 > --- a/sql/lock.cc > +++ b/sql/lock.cc > @@ -89,6 +89,7 @@ extern HASH open_cache; > > static int lock_external(THD *thd, TABLE **table,uint count); > static int unlock_external(THD *thd, TABLE **table,uint count); > +static void track_table_access(THD *thd, TABLE **tables, size_t count); this forward declaration is not needed > > /* Map the return value of thr_lock to an error from errmsg.txt */ > static int thr_lock_errno_to_mysql[]= > @@ -244,6 +245,39 @@ void reset_lock_data(MYSQL_LOCK *sql_lock, bool unlock) > > > /** > + Scan array of tables for access types; update transaction tracker > + accordingly. > + > + @param thd The current thread. > + @param tables An array of pointers to the tables to lock. > + @param count The number of tables to lock. > +*/ > + > +static void track_table_access(THD *thd, TABLE **tables, size_t count) > +{ > + if (thd->variables.session_track_transaction_info > TX_TRACK_NONE) > + { > + Transaction_state_tracker *tst= (Transaction_state_tracker *) > + thd->session_tracker.get_tracker(TRANSACTION_INFO_TRACKER); > + enum enum_tx_state s; > + > + while (count--) > + { > + TABLE *t= tables[count]; > + > + if (t) > + { > + s= tst->calc_trx_state(thd, t->reginfo.lock_type, > + t->file->has_transactions()); don't do calc_trx_state, turn it into a calc_and_add_trx_state. or, may be, tst->add_trx_state(thd, t->reginfo.lock_type, t->file->has_transactions()) because in all cases where calc_trx_state is used, the caller calls add_trx_state immediately after that. > + tst->add_trx_state(thd, s); > + } > + } > + } > +} > + > + > + > +/** > Lock tables. > > @param thd The current thread. > diff --git a/sql/set_var.cc b/sql/set_var.cc > index 5392a00..5f2bc93 100644 > --- a/sql/set_var.cc > +++ b/sql/set_var.cc > @@ -204,8 +209,29 @@ bool sys_var::update(THD *thd, set_var *var) > (on_update && on_update(this, thd, OPT_GLOBAL)); > } > else > - return session_update(thd, var) || > + { > + bool ret= session_update(thd, var) || > (on_update && on_update(this, thd, OPT_SESSION)); > + > + /* > + Make sure we don't session-track variables that are not actually > + part of the session. tx_isolation and and tx_read_only for example > + exist as GLOBAL, SESSION, and one-shot ("for next transaction only"). > + */ > + if ((var->type == OPT_SESSION) && (!ret)) first, s/var->type/type/ second, when type is not OPT_SESSION? only for tx_isolation and tx_read_only? btw, why that many parentheses? > + { > + thd->session_tracker.mark_as_changed(thd, SESSION_SYSVARS_TRACKER, > + (LEX_CSTRING*)var->var); > + /* > + Here MySQL sends variable name to avoid reporting change of > + the tracker itself, but we decided that it is not needed > + */ > + thd->session_tracker.mark_as_changed(thd, SESSION_STATE_CHANGE_TRACKER, > + NULL); > + } > + > + return ret; > + } > } > > uchar *sys_var::session_value_ptr(THD *thd, const LEX_STRING *base) > diff --git a/sql/sp_head.cc b/sql/sp_head.cc > index d58b51a..396f308 100644 > --- a/sql/sp_head.cc > +++ b/sql/sp_head.cc > @@ -2975,6 +2977,16 @@ sp_lex_keeper::reset_lex_and_exec_core(THD *thd, uint *nextp, > > reinit_stmt_before_use(thd, m_lex); > > +#ifndef EMBEDDED_LIBRARY > + /* > + if there was instruction which changed tracking state before, result > + can go with this command OK packet, so better do not cache the result. sorry, I cannot parse that :( > + */ > + if ((thd->client_capabilities & CLIENT_SESSION_TRACK) && > + (thd->server_status & SERVER_SESSION_STATE_CHANGED)) > + thd->lex->safe_to_cache_query= 0; > +#endif > + > if (open_tables) > res= instr->exec_open_and_lock_tables(thd, m_lex->query_tables); > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 08a9647..924e9d4 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -5013,6 +5018,20 @@ static bool check_lock_and_start_stmt(THD *thd, > table_list->table->file->print_error(error, MYF(0)); > DBUG_RETURN(1); > } > + > + /* > + Record in transaction state tracking > + */ > + if (thd->variables.session_track_transaction_info > TX_TRACK_NONE) > + { > + Transaction_state_tracker *tst= (Transaction_state_tracker *) > + thd->session_tracker.get_tracker(TRANSACTION_INFO_TRACKER); > + enum enum_tx_state s= > + tst->calc_trx_state(thd, lock_type, > + table_list->table->file->has_transactions()); > + tst->add_trx_state(thd, s); after you get rid of a separatre calc_trx_state call, you'll be able to use TRANSACT_TRACKER here too > + } > + > DBUG_RETURN(0); > } > > diff --git a/sql/sql_cache.cc b/sql/sql_cache.cc > index 42b80d9..b0dacca 100644 > --- a/sql/sql_cache.cc > +++ b/sql/sql_cache.cc > @@ -1381,6 +1381,19 @@ void Query_cache::store_query(THD *thd, TABLE_LIST *tables_used) > DBUG_VOID_RETURN; > } > > + /* > + Do not store queries while tracking transaction state. > + The tracker already flags queries that actually have > + transaction tracker items, but this will make behavior > + more straight forward. why? > + */ > + if (thd->variables.session_track_transaction_info != TX_TRACK_NONE) > + { > + DBUG_PRINT("qcache", ("Do not work with transaction tracking")); > + DBUG_VOID_RETURN; > + } > + > + > /* The following assert fails if we haven't called send_result_to_client */ > DBUG_ASSERT(thd->base_query.is_alloced() || > thd->base_query.ptr() == thd->query()); > @@ -1719,6 +1732,18 @@ Query_cache::send_result_to_client(THD *thd, char *org_sql, uint query_length) > goto err; > } > > + /* > + Don't allow serving from Query_cache while tracking transaction > + state. This is a safeguard in case an otherwise matching query > + was added to the cache before tracking was turned on. how could that be possible? > + */ > + if (thd->variables.session_track_transaction_info != TX_TRACK_NONE) > + { > + DBUG_PRINT("qcache", ("Do not work with transaction tracking")); > + goto err; > + } > + > + > thd->query_cache_is_applicable= 1; > sql= org_sql; sql_end= sql + query_length; > > diff --git a/sql/sql_db.cc b/sql/sql_db.cc > index 2ba67cb..1780a81 100644 > --- a/sql/sql_db.cc > +++ b/sql/sql_db.cc > @@ -1034,7 +1034,10 @@ mysql_rm_db_internal(THD *thd,char *db, bool if_exists, bool silent) > it to 0. > */ > if (thd->db && cmp_db_names(thd->db, db) && !error) > + { > mysql_change_db_impl(thd, NULL, 0, thd->variables.collation_server); > + thd->session_tracker.mark_as_changed(thd, CURRENT_SCHEMA_TRACKER, NULL); another point for simply including all trackers into Session_tracker. this method will be inlined, the check for is_enabled will be inlined too, so in the normal case this will become much cheaper. > + } > my_dirend(dirp); > DBUG_RETURN(error); > } > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index f540c26..0342a88 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -269,6 +269,7 @@ struct st_bookmark > uint name_len; > int offset; > uint version; > + bool loaded; I'm not commenting on changes in this file, because I hope they will go away completely, see my earlier comment. > char key[1]; > }; > > @@ -322,6 +323,8 @@ static void unlock_variables(THD *thd, struct system_variables *vars); > static void cleanup_variables(struct system_variables *vars); > static void plugin_vars_free_values(sys_var *vars); > static void restore_ptr_backup(uint n, st_ptr_backup *backup); > +#define my_intern_plugin_lock(A,B) intern_plugin_lock(A,B) > +#define my_intern_plugin_lock_ci(A,B) intern_plugin_lock(A,B) nope. remove those macros, as I've already wrote in a previous review. I introduced them to track memory allocations (all _ci macros pass "caller info" down the stack, __FILE__ and __LINE__, so that safemalloc would show not the line where malloc was called, but where, say, init_dynamic_array was called). in MariaDB this is not needed, because I've changed safemalloc to remember stack traces. So all _ci macros are long gone now, don't add them back from MySQL code. > static plugin_ref intern_plugin_lock(LEX *lex, plugin_ref plugin); > static void intern_plugin_unlock(LEX *lex, plugin_ref plugin); > static void reap_plugins(void); > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index f41fb39..b6ed6e6 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -3272,109 +3398,21 @@ static bool show_status_array(THD *thd, const char *wild, > name_buffer, wild))) && > (!cond || cond->val_int())) > { > - void *value=var->value; > - const char *pos, *end; // We assign a lot of const's > + const char *pos; // We assign a lot of const's > + size_t length; > > if (show_type == SHOW_SYS) > - { > - sys_var *var= (sys_var *) value; > - show_type= var->show_type(); > mysql_mutex_lock(&LOCK_global_system_variables); > - value= var->value_ptr(thd, scope, &null_lex_str); > - charset= var->charset(thd); > - } > + pos= get_one_variable(thd, var, scope, show_type, status_var, > + &charset, buff, &length); this wasn't needed, if you won't use get_one_variable() anywhere else > > - pos= end= buff; > - /* > - note that value may be == buff. All SHOW_xxx code below > - should still work in this case > - */ > - switch (show_type) { > - case SHOW_DOUBLE_STATUS: > - value= ((char *) status_var + (intptr) value); > - /* fall through */ > - case SHOW_DOUBLE: > - /* 6 is the default precision for '%f' in sprintf() */ > - end= buff + my_fcvt(*(double *) value, 6, buff, NULL); > - break; > - case SHOW_LONG_STATUS: > - value= ((char *) status_var + (intptr) value); > - /* fall through */ > - case SHOW_ULONG: > - case SHOW_LONG_NOFLUSH: // the difference lies in refresh_status() > - end= int10_to_str(*(long*) value, buff, 10); > - break; > - case SHOW_LONGLONG_STATUS: > - value= ((char *) status_var + (intptr) value); > - /* fall through */ > - case SHOW_ULONGLONG: > - end= longlong10_to_str(*(longlong*) value, buff, 10); > - break; > - case SHOW_HA_ROWS: > - end= longlong10_to_str((longlong) *(ha_rows*) value, buff, 10); > - break; > - case SHOW_BOOL: > - end= strmov(buff, *(bool*) value ? "ON" : "OFF"); > - break; > - case SHOW_MY_BOOL: > - end= strmov(buff, *(my_bool*) value ? "ON" : "OFF"); > - break; > - case SHOW_UINT: > - end= int10_to_str((long) *(uint*) value, buff, 10); > - break; > - case SHOW_SINT: > - end= int10_to_str((long) *(int*) value, buff, -10); > - break; > - case SHOW_SLONG: > - end= int10_to_str(*(long*) value, buff, -10); > - break; > - case SHOW_SLONGLONG: > - end= longlong10_to_str(*(longlong*) value, buff, -10); > - break; > - case SHOW_HAVE: > - { > - SHOW_COMP_OPTION tmp= *(SHOW_COMP_OPTION*) value; > - pos= show_comp_option_name[(int) tmp]; > - end= strend(pos); > - break; > - } > - case SHOW_CHAR: > - { > - if (!(pos= (char*)value)) > - pos= ""; > - end= strend(pos); > - break; > - } > - case SHOW_CHAR_PTR: > - { > - if (!(pos= *(char**) value)) > - pos= ""; > - > - end= strend(pos); > - break; > - } > - case SHOW_LEX_STRING: > - { > - LEX_STRING *ls=(LEX_STRING*)value; > - if (!(pos= ls->str)) > - end= pos= ""; > - else > - end= pos + ls->length; > - break; > - } > - case SHOW_UNDEF: > - break; // Return empty string > - case SHOW_SYS: // Cannot happen > - default: > - DBUG_ASSERT(0); > - break; > - } > - table->field[1]->store(pos, (uint32) (end - pos), charset); > + table->field[1]->store(pos, (uint32) length, charset); > + thd->count_cuted_fields= CHECK_FIELD_IGNORE; > table->field[1]->set_notnull(); > - > - if (var->type == SHOW_SYS) > + if (show_type == SHOW_SYS) > mysql_mutex_unlock(&LOCK_global_system_variables); > > + > if (schema_table_store_record(thd, table)) > { > res= TRUE; > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index 4bf2028..3be2e55 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -5365,3 +5377,72 @@ static Sys_var_ulong Sys_log_tc_size( > DEFAULT(my_getpagesize() * 6), > BLOCK_SIZE(my_getpagesize())); > #endif > + > +const LEX_CSTRING SESSION_TRACK_SYSTEM_VARIABLES_NAME= > + {STRING_WITH_LEN("session_track_system_variables")}; I think you don't need that^^^. You shouldn't search for Sys_var_sesvartrack by name anyway. > + > +static Sys_var_sesvartrack Sys_track_session_sys_vars( > + SESSION_TRACK_SYSTEM_VARIABLES_NAME.str, > + "Track changes in registered system variables.", > + CMD_LINE(REQUIRED_ARG), IN_SYSTEM_CHARSET, > + DEFAULT("autocommit,character_set_client,character_set_connection," > + "character_set_results,time_zone"), > + NO_MUTEX_GUARD); > + > +static bool update_session_track_schema(sys_var *self, THD *thd, > + enum_var_type type) > +{ > + DBUG_ENTER("update_session_track_schema"); > + DBUG_RETURN(thd->session_tracker.get_tracker(CURRENT_SCHEMA_TRACKER)->update(thd)); > +} > + > +static Sys_var_mybool Sys_session_track_schema( > + "session_track_schema", > + "Track changes to the 'default schema'.", > + SESSION_VAR(session_track_schema), > + CMD_LINE(OPT_ARG), DEFAULT(TRUE), > + NO_MUTEX_GUARD, NOT_IN_BINLOG, > + ON_CHECK(0), > + ON_UPDATE(update_session_track_schema)); > + > + > +static bool update_session_track_tx_info(sys_var *self, THD *thd, > + enum_var_type type) > +{ > + DBUG_ENTER("update_session_track_tx_info"); > + DBUG_RETURN(thd->session_tracker.get_tracker(TRANSACTION_INFO_TRACKER)->update(thd)); > +} > + > +static const char *session_track_transaction_info_names[]= > + { "OFF", "STATE", "CHARACTERISTICS", NullS }; > + > +static Sys_var_enum Sys_session_track_transaction_info( > + "session_track_transaction_info", > + "Track changes to the transaction attributes. OFF to disable; " > + "STATE to track just transaction state (Is there an active transaction? " > + "Does it have any data? etc.); CHARACTERISTICS to track transaction " > + "state " > + "and report all statements needed to start a transaction with the same " > + "characteristics (isolation level, read only/read write, snapshot - " > + "but not any work done / data modified within the transaction).", > + SESSION_VAR(session_track_transaction_info), > + CMD_LINE(REQUIRED_ARG), session_track_transaction_info_names, > + DEFAULT(0), NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(0), > + ON_UPDATE(update_session_track_tx_info)); > + > + > +static bool update_session_track_state_change(sys_var *self, THD *thd, > + enum_var_type type) > +{ > + DBUG_ENTER("update_session_track_state_change"); > + DBUG_RETURN(thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->update(thd)); > +} > + > +static Sys_var_mybool Sys_session_track_state_change( > + "session_track_state_change", > + "Track changes to the 'session state'.", > + SESSION_VAR(session_track_state_change), > + CMD_LINE(OPT_ARG), DEFAULT(FALSE), > + NO_MUTEX_GUARD, NOT_IN_BINLOG, > + ON_CHECK(0), > + ON_UPDATE(update_session_track_state_change)); > diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic > index 2488e80..cf01835 100644 > --- a/sql/sys_vars.ic > +++ b/sql/sys_vars.ic > @@ -535,6 +537,105 @@ class Sys_var_charptr: public sys_var > } > }; > > +class Sys_var_charptr: public Sys_var_charptr_base > +{ > +public: > + Sys_var_charptr(const char *name_arg, > + const char *comment, int flag_args, ptrdiff_t off, size_t size, > + CMD_LINE getopt, > + enum charset_enum is_os_charset_arg, > + const char *def_val, PolyLock *lock=0, > + enum binlog_status_enum binlog_status_arg=VARIABLE_NOT_IN_BINLOG, > + on_check_function on_check_func=0, > + on_update_function on_update_func=0, > + const char *substitute=0) : > + Sys_var_charptr_base(name_arg, comment, flag_args, off, size, getopt, > + is_os_charset_arg, def_val, lock, binlog_status_arg, > + on_check_func, on_update_func, substitute) > + { > + SYSVAR_ASSERT(scope() == GLOBAL); > + SYSVAR_ASSERT(size == sizeof(char *)); > + } > + > + bool session_update(THD *thd, set_var *var) > + { > + DBUG_ASSERT(FALSE); > + return true; > + } > + void session_save_default(THD *thd, set_var *var) > + { DBUG_ASSERT(FALSE); } > +}; > + > +class Sys_var_sesvartrack: public Sys_var_charptr_base > +{ > +public: > + Sys_var_sesvartrack(const char *name_arg, > + const char *comment, > + CMD_LINE getopt, > + enum charset_enum is_os_charset_arg, > + const char *def_val, PolyLock *lock) : > + Sys_var_charptr_base(name_arg, comment, > + SESSION_VAR(session_track_system_variables), > + getopt, is_os_charset_arg, def_val, lock, > + VARIABLE_NOT_IN_BINLOG, 0, 0, 0) > + {} > + bool do_check(THD *thd, set_var *var) > + { > + if (Sys_var_charptr_base::do_check(thd, var) || > + sysvartrack_validate_value(thd, var->save_result.string_value.str, > + var->save_result.string_value.length)) > + return TRUE; > + return FALSE; > + } > + bool global_update(THD *thd, set_var *var) > + { > + char *new_val= global_update_prepare(thd, var); > + if (new_val) > + { > + if (sysvartrack_reprint_value(thd, new_val, > + var->save_result.string_value.length)) > + new_val= 0; > + } > + global_update_finish(new_val); > + return (new_val == 0 && var->save_result.string_value.str != 0); > + } > + bool session_update(THD *thd, set_var *var) > + { > + return sysvartrack_update(thd); > + } > + void session_save_default(THD *thd, set_var *var) > + { > + var->save_result.string_value.str= global_var(char*); > + var->save_result.string_value.length= > + strlen(var->save_result.string_value.str); > + /* parse and feel list with default values */ > + if (thd) > + { > + bool res= > + sysvartrack_validate_value(thd, > + var->save_result.string_value.str, > + var->save_result.string_value.length); > + DBUG_ASSERT(res == 0); > + } > + } > + uchar *session_value_ptr(THD *thd, const LEX_STRING *base) > + { > + DBUG_ASSERT(thd != NULL); > + size_t len= sysvartrack_value_len(thd); > + char *res= 0; > + char *buf= (char *)my_safe_alloca(len); > + if (buf && !sysvartrack_value_construct(thd, buf, len)) > + { > + size_t len= strlen(buf) + 1; > + res= (char*) thd->alloc(len + sizeof(char *)); why don't you use thd->alloc up front? why alloca and then memcpy? > + if (res) > + memcpy((*((char**) res)= res + sizeof(char *)), buf, len); > + my_safe_afree(buf, len); > + } > + return (uchar *)res; > + } > +}; > + > > class Sys_var_proxy_user: public sys_var > { Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] Binary is built, but does it have Cassandra-v2 (Was: Re: June, 30th meeting: outcome)
by Sergey Petrunia 02 Jun '16

02 Jun '16

On Wed, Jun 01, 2016 at 08:46:45PM +0100, Charles Muurmu wrote: > Hi Sergei, > > make now runs to completion and make install as well I'm a bit surprised. I'm looking at https://github.com/charlesmuurmu/server/blob/MDEV-8947/storage/cassandra-v2… And I still see two "IF(...)" lines but three "ENDIF" lines. Are you sure cassandra-v2 gets built and linked? As I said earlier: >> Do you know how to check if mysqld binary has storage engine linked? >> >> I think this would work on OS X, too: >> >> "nm mysqld | grep cassandra" >> >> this command must produce non-empty output (for an example, try "nm >> mysqld | grep myisam"). If you've made any changes to storage/cassandra-v2, please commit them and push. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0