maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #09690
Re: 56a2835: MDEV-5535: Cannot reopen temporary table
Hi, Nirbhay!
Just a couple of questions/comments, see below.
And please pay attention to Kristian's email about locking and parallel
replication. I don't think I can confidently review it in a patch, so
you may want to spend more time testing or analyzing that code.
Ok to push, when you're ready! Thanks!
On May 26, Nirbhay Choubey wrote:
> revision-id: 56a2835872c4ac7296ec0ae2ff618822855b0fc0 (mariadb-10.1.8-82-g56a2835)
> parent(s): 28c289626f318631d707f85b057a90af99018b06
> author: Nirbhay Choubey
> committer: Nirbhay Choubey
> timestamp: 2016-05-26 20:42:47 -0400
> message:
>
> MDEV-5535: Cannot reopen temporary table
>
> mysqld maintains a list of TABLE objects for all temporary
> tables in THD, where each table is represented by a TABLE
> object. A query referencing a temporary table for more than
> once, however, failed with ER_CANT_REOPEN_TABLE error.
"because a TABLE_SHARE was allocate together with the TABLE, so
temporary tables always had one TABLE per TABLE_SHARE"
> This patch lift this restriction by preserving the TABLE_SHARE
> object of each created temporary table, so that multiple instances
> of TABLE objects can be created.
better "by separating TABLE and TABLE_SHARE objects and storing
TABLE_SHARE's in the list in a THD, and TABLE's in the list in
the corresponding TABLE_SHARE"
> diff --git a/sql/rpl_rli.cc b/sql/rpl_rli.cc
> index 7bdd9c1..77f458b 100644
> --- a/sql/rpl_rli.cc
> +++ b/sql/rpl_rli.cc
> @@ -1060,24 +1061,43 @@ void Relay_log_info::inc_group_relay_log_pos(ulonglong log_pos,
>
> void Relay_log_info::close_temporary_tables()
> {
> - TABLE *table,*next;
> DBUG_ENTER("Relay_log_info::close_temporary_tables");
>
> - for (table=save_temporary_tables ; table ; table=next)
> + TMP_TABLE_SHARE *share;
> + TABLE *table;
> +
> + while ((share= save_temporary_tables.pop_front()))
> {
> - next=table->next;
> + /*
> + Iterate over the list of tables for this TABLE_SHARE and close them.
> + */
> + while ((table= share->all_tmp_tables.pop_front()))
> + {
> + DBUG_PRINT("tmptable", ("closing table: '%s'.'%s'",
> + table->s->db.str, table->s->table_name.str));
> +
> + /* Reset in_use as the table may have been created by another thd */
> + table->in_use= 0;
> + free_io_cache(table);
> + /*
> + Lets not free TABLE_SHARE here as there could be multiple TABLEs opened
> + for the same table (TABLE_SHARE).
> + */
> + closefrm(table, false);
> + my_free(table);
> + }
>
> - /* Reset in_use as the table may have been created by another thd */
> - table->in_use=0;
> /*
> Don't ask for disk deletion. For now, anyway they will be deleted when
> slave restarts, but it is a better intention to not delete them.
> */
> - DBUG_PRINT("info", ("table: 0x%lx", (long) table));
> - close_temporary(table, 1, 0);
> +
> + free_table_share(share);
> + my_free(share);
> }
> - save_temporary_tables= 0;
> - slave_open_temp_tables= 0;
> +
> + save_temporary_tables.empty();
Is it needed?
you've popped all shares from the list, it's empty.
I'd rather add an assert instead
> +
> DBUG_VOID_RETURN;
> }
>
> diff --git a/sql/sql_class.h b/sql/sql_class.h
> index ae240ae..a70c4a9 100644
> --- a/sql/sql_class.h
> +++ b/sql/sql_class.h
> @@ -1247,6 +1247,61 @@ enum enum_locked_tables_mode
> LTM_PRELOCKED_UNDER_LOCK_TABLES
> };
>
> +/**
> + The following structure is an extension to TABLE_SHARE and is
> + exclusively for temporary tables.
> +
> + @note:
> + Although, TDC_element has data members (like next, prev &
> + all_tables) to store the list of TABLE_SHARE & TABLE objects
> + related to a particular TABLE_SHARE, they cannot be moved to
> + TABLE_SHARE in order to be reused for temporary tables. This
> + is because, as concurrent threads iterating through hash of
> + TDC_element's may need access to all_tables, but if all_tables
> + is made part of TABLE_SHARE, then TDC_element->share->all_tables
> + is not always guaranteed to be valid, as TDC_element can live
> + longer than TABLE_SHARE.
> +*/
> +struct TMP_TABLE_SHARE : public TABLE_SHARE
> +{
> +private:
> + /*
> + Link to all temporary table shares. Declared as private to
> + avoid direct manipulation with those objects. One should
> + use methods of I_P_List template instead.
> + */
> + TMP_TABLE_SHARE *tmp_next;
> + TMP_TABLE_SHARE **tmp_prev;
> +
> + friend struct All_tmp_table_shares;
> +
> +public:
> + /*
> + Doubly-linked (back-linked) lists of used and unused TABLE objects
> + for this share.
and unused? you don't free them but keep in the list?
> + */
> + All_share_tables_list all_tmp_tables;
> +};
> +
> +/**
> + Helper class which specifies which members of TMP_TABLE_SHARE are
> + used for participation in the list of temporary tables.
> +*/
> +
> +struct All_tmp_table_shares
> +{
> + static inline TMP_TABLE_SHARE **next_ptr(TMP_TABLE_SHARE *l)
> + {
> + return &l->tmp_next;
> + }
> + static inline TMP_TABLE_SHARE ***prev_ptr(TMP_TABLE_SHARE *l)
> + {
> + return &l->tmp_prev;
> + }
> +};
> +
> +/* Also used in rpl_rli.h. */
> +typedef I_P_List <TMP_TABLE_SHARE, All_tmp_table_shares> All_tmp_tables_list;
>
> /**
> Class that holds information about tables which were opened and locked
> diff --git a/sql/temporary_tables.cc b/sql/temporary_tables.cc
> new file mode 100644
> index 0000000..1514b72
> --- /dev/null
> +++ b/sql/temporary_tables.cc
...
> +TABLE *THD::find_temporary_table(const char *db,
> + const char *table_name)
> +{
> + DBUG_ENTER("THD::find_temporary_table");
> +
> + TABLE *table;
> + char key[MAX_DBKEY_LENGTH];
> + uint key_length;
> + bool locked;
> +
> + if (!(has_temporary_tables() || (rgi_slave && has_slave_temporary_tables())))
hmm, really? you had here is_empty(true) before
and your is_empty() was working like this:
if (flag_is_true)
{ do rgi_slave }
else
{ do thd }
your old is_empty was not checking thd if the argument was true,
your new code does, as if your is_empty() was
do thd;
if (flag_is_true)
{ do rgi_slave}
> + {
> + DBUG_RETURN(NULL);
> + }
> +
> + key_length= create_tmp_table_def_key(key, db, table_name);
Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx
Follow ups