maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #09668
Re: [Commits] c285dbe: MDEV-5535: Cannot reopen temporary table
Hi Serg,
On Thu, May 19, 2016 at 12:07 PM, Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
> 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 :)
>
Sure, I will.
>
> > 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
>
That's the fun of copying existing stuff. :)
> 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 :)
>
Right. In fact, I just realized that mtr allots a separate tmpdir for all
server
instances. So, use of this opt file is redundant. Removed.
> > 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)
>
In create_table_impl() after the table is opened:
..
4839 create_info->table= table; // Store pointer to
table
..
The new code does a THD::create_and_open_tmp_table() in
create_table_impl(), and
simply assigns the new TABLE to create_table->table. And thus,
open_temporary_table()
and assert for equality is no longer needed, IMO.
>
> > + 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?
>
Explained in the above comment.
>
> > }
> > }
> > 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.
>
I have committed this (and related) hunk in a separate commit on top of
main commit. But, I have left the ER_CANT_REOPEN_TABLE related
code in THD::drop_remporary_table in the main commit, as it was part of
the original code. I hope that's ok.
>
> > + 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)
>
The old drop_temporary_table never returned 1, but only 0 and -1. So, the
comment above was incorrect.
I chose to change it to return bool, instead.
>
> > }
> > 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?
>
Yea, removed now.
>
> >
> > /** 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.
>
Agree. I have split the method into two.
>
> > +{
> > + 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?
>
Yes, but not the other way around. For instance, if the table was created
successfully,
but the subsequent malloc failed while an attempt was made to allocate
TABLE object
later in open_temporary_table().
>
> > +
> > +
> > + @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?
>
Since, in previous version of the patch, all temporary TABLE objects were
added to a single list, I had it locked untile the TABLE got added. But now
that's not the case, so moved the lock/unlock to
THD::create_temporary_table()
like in the original code.
> > +
> > + 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' :)
>
Fixed.
>
> > +*/
> > +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
>
IIRC, I added them to fix some hanging/failing parallel replication tests,
but now since the approach of storing temporary TABLE_SHARE and
TABLE objects has changed, I will rerun the tests to see how they behave.
> > + {
> > + DBUG_RETURN(true);
> > + }
> > +
> > + locked= lock_tables();
>
> neither was this
>
Removed.
>
> > +
> > + /*
> > + 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?
>
Perhaps its related to TDC? But I do not understand why will it be
needed 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." ?
>
Sorry, my bad. Fixed.
>
> > + {
> > + 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?
>
Removed.
>
> > +
> > + 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);
> }
>
That's a good idea. Done.
> > +
> > + /* 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.
>
Fixed.
>
> > + */
> > + 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)
>
I added these additional waits to fix some failing replication tests. But,
whit new TMP_TABLE_SHARE approach, this could have changed.
I have now reverted them to the original state and will run rpl test to
assure if we do not need additional waits.
> > + {
> > + 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;
>
Right, There is now a new list, TMP_TABLE_SHARE::all_tmp_tables,
that uses these pointers to maintain a list of TABLEs for a particular
TMP_TABLE_SHARE.
> > +
> > + /* 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?
>
Added.
>
> > +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?
>
>
I was calling it in open_table() before calling find_and_use_table().
Restored it now.
> + /*
> > + 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)
>
Okay
>
> > + 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.
>
I discussed it with Svoj. We originally thought that all_tables, next
and prev members can be moved from TDC_element to TABLE_SHARE
and can be reused to link temporary table shares and table instances.
But, since share can be freed concurrently, accessing all_tables via share
in TDC code can lead to segfault (TDC_element->share->all_tables).
So, we decided not to move them and stick to the TMP_TABLE_SHARE
approach with additional members for next/prev shares and all_temp_tables
list using share_all_next & share_all_prev.
> > +
> > + /* 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.
>
I agree, done.
>
> > +
> > + uint tmpkeyval(TMP_TABLE_SHARE *share)
> > + {
> > + return uint4korr(share->table_cache_key.str +
> > + share->table_cache_key.length - 4);
> > + }
> > +};
> > +
> > +#endif /* TEMPORARY_TABLES_H */
>
>
Best,
Nirbhay
> Regards,
> Sergei
> Chief Architect MariaDB
> and security@xxxxxxxxxxx
>
Follow ups
References
