Re: [Maria-developers] 56a2835: MDEV-5535: Cannot reopen temporary table

8 Jun 2016

Hi Serg!

On Fri, Jun 3, 2016 at 4:10 PM, Sergei Golubchik  wrote:
...
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.
Yes, I did add some related rpl test cases.
...
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"
That's better indeed. I will update that when I merge the consolidated
patches to the main branch.
...
...

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.
Right. I realized this later. Fixed.
...
...
+
   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?
Yes. We did agree to not free/close but reuse them.
...
...
+  */
+  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 
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);
Yea, there was a problem here. It should have checked for !rgi_slave for
thd before
proceeding check slave temporary tables. Its now :

(!rgi_slave && has_temporary_tables()) || (rgi_slave &&
unlikely(has_slave_temporary_tables())))

Best,
Nirbhay
...
Regards,
Sergei
Chief Architect MariaDB
and security@mariadb.org


    