Merge lp:~laurynas-biveinis/percona-server/i-s-temp-tables-fixes-5.1 into lp:percona-server/5.1

[Laurynas Biveinis]

No BT or ST but QA blocker.

Fix three INFORMATION_SCHEMA.TEMPORARY_TABLES and
INFORMATION_SCHEMA.GLOBAL_TEMPORARY_TABLES bugs:

- bug 1222709 (Bug 951588 fix needs revert and re-fixing);
- bug 1113388 (field.cc:3822: virtual longlong Field_long::val_int():
  Assertion `table->in_use == _current_thd()' failed);
- bug 1223335 (fill_temporary_tables() does not protect
  thd->temporary_tables access with LOCK_temporary_tables).

The bug 1222709 and its duplicats stem from the bug 951588 fix calling
handler::clone() on a temp table handler in an attempt to make
handler::info() call safe for concurrency as it would be called on a
non-shared handler then. The problem with this approach is that
handler::clone() itself is not safe for concurrent calls. Thus remove
its call, and fix the original bug 951588 issue by only calling
handler::info() when it is safe to do so, that is, when the current
I_S-query-executing thread is the same as the temp table owning one.
For the rest, do not call handler::info() and just read the existing
statistic values.

At the same time, if zeroes are encountered in the stat file fields,
assume that handler::info() has never been called on the temp table
handler and return NULL instead of 0.

At the same time revert the ha_innodb::clone() changes that were
required to support the previous bug 951588 fix use.

Fix bug 1113388 by removing the temporary THD::in_use patching in
fill_global_temporary_tables(), which now server no purpose and only
introduces a race condition as the field might be read by its owning
thread.

Fix bug 1223335 by locking thd->LOCK_temporary_tables around
thd->temporary_tables iteration in fill_temporary_tables().

http://jenkins.percona.com/job/percona-server-5.1-param/572/

[Laurynas Biveinis (laurynas-biveinis)]

"// Assume that invoking handler::table_type() on a shared handler is safe" comment should be removed as well from store_temporary_table_record(). Too little an issue for a repush, thus will either fix if review requires repush otherwise, either before merging.

review: Needs Fixing

[Alexey Kopytov (akopytov)]

  - bug #1223335 is invalid, so references and the “fix” should be
    removed
  - the revision comments provide almost no information on the problem
    being addressed. So “The problem with this approach is that
    handler::clone() itself is not safe for concurrent calls.” Why?
  - even if there are reasons to not use handler::clone(), I don’t like
    the fix. The fix introduces a rather important change in
    functionality: I_S.GLOBAL_TEMPORARY_TABLES will not show table
    sizes. Which basically makes it useless. So I’d like to be sure
    there are no alternative solutions. But first I need to understand
    the problem (see above).
  - “At the same time, if zeroes are encountered in the stat file fields,
    assume that handler::info() has never been called on the temp table
    handler and return NULL instead of 0.” — that won’t work. Couple of
    issues:
      - in [GLOBAL_]TEMPORARY_TABLES there are only 2 nullable columns:
        CREATE_TIME and UPDATE_TIME. So assuming that the value will be
        NULL if set_not_null() is not called is wrong. And the call to
        set_not_null() is pointless.
      - even if that worked, assuming zero values to indicate that
        info() was not called is wrong. They will be 0 for an empty
        table, for example.
  - spurious change at line 84

review: Needs Fixing

[Laurynas Biveinis (laurynas-biveinis)]

Alexey -

> being addressed. So “The problem with this approach is that
> handler::clone() itself is not safe for concurrent calls.” Why?
> - even if there are reasons to not use handler::clone(), I don’t like
> the fix. The fix introduces a rather important change in
> functionality: I_S.GLOBAL_TEMPORARY_TABLES will not show table
> sizes. Which basically makes it useless. So I’d like to be sure
> there are no alternative solutions. But first I need to understand
> the problem (see above).

My (probably incorrect) guess on ::clone() being unsafe for concurrent calls was based on the sheer number of different crashing bugs found with the RQG testing, listed in the bug 1222709 description, and the fact that ha_innobase::clone() in the upstream had code that needed to be removed in order for it to work when called from another thread. Previous attempts to fix those crashing bugs resulted in elaborate patching throughout ::clone() methods of many storage engines, and even in the handler core - see https://code.launchpad.net/~laurynas-biveinis/percona-server/i-s-temp-tables-fixes-1-5.1/+merge/178092. Even with those fixes I couldn't figure out how to create fully safe conditions for clone(), that's why I thought that clone() was never meant to be used like that.

But now, in addition to your review, I also just found a recent push [1]. Which suggests that a safe clone() from another thread should be possible, and that I should be able to find how it's supposed to be used in PFS sources. I will look into this.

[1]

5.6$ bzr log -r 5356
------------------------------------------------------------
revno: 5356
committer: Annamalai Gurusami <email address hidden>
branch nick: mysql-5.6
timestamp: Wed 2013-08-14 14:57:39 +0530
message:
  Bug #17001980 RQG_PERFORMANCE_SCHEMA STOPPED WITH A CRASH IN
  HA_INNOBASE::CLONE.

  Problem:

  In the function ha_innobase::clone(), it was asserted that a thread
  cannot clone a table handler that is used by another thread. It was
  also asserted that the original table handler and the cloned table
  handler must belong to the same transaction. Both are wrong.

  Solution:

  It is OK for a thread to clone a table handler that is used by another
  thread. The original table handler and the cloned table handler can
  belong to different transactions. So both asserts are removed.

  rb#3066 approved by Marko

[Laurynas Biveinis (laurynas-biveinis)]

> fix those crashing bugs resulted in elaborate patching throughout ::clone()
> methods of many storage engines, and even in the handler core

Errr, that's not fully correct, while the crashes fixed were in many storage engines, the patching in that MP was more localized.

Unmerged revisions

578. By Laurynas Biveinis on 2013-09-13

Fix three INFORMATION_SCHEMA.TEMPORARY_TABLES and
INFORMATION_SCHEMA.GLOBAL_TEMPORARY_TABLES bugs:

- bug 1222709 (Bug 951588 fix needs revert and re-fixing);
- bug 1113388 (field.cc:3822: virtual longlong Field_long::val_int():
  Assertion `table->in_use == _current_thd()' failed);
- bug 1223335 (fill_temporary_tables() does not protect
  thd->temporary_tables access with LOCK_temporary_tables).

The bug 1222709 and its duplicats stem from the bug 951588 fix calling
handler::clone() on a temp table handler in an attempt to make
handler::info() call safe for concurrency as it would be called on a
non-shared handler then. The problem with this approach is that
handler::clone() itself is not safe for concurrent calls. Thus remove
its call, and fix the original bug 951588 issue by only calling
handler::info() when it is safe to do so, that is, when the current
I_S-query-executing thread is the same as the temp table owning one.
For the rest, do not call handler::info() and just read the existing
statistic values.

At the same time, if zeroes are encountered in the stat file fields,
assume that handler::info() has never been called on the temp table
handler and return NULL instead of 0.

At the same time revert the ha_innodb::clone() changes that were
required to support the previous bug 951588 fix use.

Fix bug 1113388 by removing the temporary THD::in_use patching in
fill_global_temporary_tables(), which now server no purpose and only
introduces a race condition as the field might be read by its owning
thread.

Fix bug 1223335 by locking thd->LOCK_temporary_tables around
thd->temporary_tables iteration in fill_temporary_tables().

=== modified file 'Percona-Server/sql/sql_show.cc'
--- Percona-Server/sql/sql_show.cc	2013-05-31 12:43:01 +0000
+++ Percona-Server/sql/sql_show.cc	2013-09-13 13:36:27 +0000
@@ -3642,13 +3642,20 @@
   @param[in]      table                    I_S table
   @param[in]      tmp_table                temporary table
   @param[in]      db                       database name
+  @param[in]      table_name_only          whether to return only session_id,
+                                           database, and table fields
+  @param[in]      thd_is_current_thd       whether the temp table belongs to
+                                           the current connection
 
   @return         Operation status
     @retval       0                        success
     @retval       1                        error
 */
 
-static int store_temporary_table_record(THD *thd, TABLE *table, TABLE *tmp_table, const char *db, bool table_name_only)
+static int store_temporary_table_record(THD *thd, TABLE *table,
+                                        TABLE *tmp_table, const char *db,
+                                        bool table_name_only,
+                                        bool thd_is_current_thd)
 {
   CHARSET_INFO *cs= system_charset_info;
   DBUG_ENTER("store_temporary_table_record");
@@ -3688,11 +3695,6 @@
 
     MYSQL_TIME time;
 
-    /* We have only one handler object for a temp table globally and it might
-    be in use by other thread.  Do not trash it by invoking handler methods on
-    it but rather clone it. */
-    file = file->clone(tmp_table->s->normalized_path.str, thd->mem_root);
-
     /**
         TODO: InnoDB stat(file) checks file on short names within data dictionary
         rather than using full path, because of that, temp files created in
@@ -3700,14 +3702,33 @@
 
         The fix is to patch InnoDB to use full path
     */
-    file->info(HA_STATUS_VARIABLE | HA_STATUS_TIME | HA_STATUS_NO_LOCK);
-
-    table->field[5]->store((longlong) file->stats.records, TRUE);
-    table->field[5]->set_notnull();
-
-    table->field[6]->store((longlong) file->stats.mean_rec_length, TRUE);
-    table->field[7]->store((longlong) file->stats.data_file_length, TRUE);
-    table->field[8]->store((longlong) file->stats.index_file_length, TRUE);
+    if (thd_is_current_thd)
+      file->info(HA_STATUS_VARIABLE | HA_STATUS_TIME | HA_STATUS_NO_LOCK);
+
+    if (file->stats.records)
+    {
+      table->field[5]->store((longlong) file->stats.records, TRUE);
+      table->field[5]->set_notnull();
+    }
+
+    if (file->stats.mean_rec_length)
+    {
+      table->field[6]->store((longlong) file->stats.mean_rec_length, TRUE);
+      table->field[6]->set_notnull();
+    }
+
+    if (file->stats.data_file_length)
+    {
+      table->field[7]->store((longlong) file->stats.data_file_length, TRUE);
+      table->field[7]->set_notnull();
+    }
+
+    if (file->stats.index_file_length)
+    {
+      table->field[8]->store((longlong) file->stats.index_file_length, TRUE);
+      table->field[8]->set_notnull();
+    }
+
     if (file->stats.create_time)
     {
       thd->variables.time_zone->gmt_sec_to_TIME(&time,
@@ -3715,6 +3736,7 @@
       table->field[9]->store_time(&time, MYSQL_TIMESTAMP_DATETIME);
       table->field[9]->set_notnull();
     }
+
     if (file->stats.update_time)
     {
       thd->variables.time_zone->gmt_sec_to_TIME(&time,
@@ -3722,8 +3744,6 @@
       table->field[10]->store_time(&time, MYSQL_TIMESTAMP_DATETIME);
       table->field[10]->set_notnull();
     }
-
-    file->close();
   }
 
   DBUG_RETURN(schema_table_store_record(thd, table));
@@ -3773,17 +3793,14 @@
       }
 #endif
 
-      THD *t= tmp->in_use;
-      tmp->in_use= thd;
-
-      if (store_temporary_table_record(thd_item, tables->table, tmp, thd->lex->select_lex.db, table_names_only)) {
-        tmp->in_use= t;
+      if (store_temporary_table_record(thd_item, tables->table, tmp,
+                                       thd->lex->select_lex.db,
+                                       table_names_only, thd_item == thd)) {
         pthread_mutex_unlock(&thd_item->LOCK_temporary_tables);
         pthread_mutex_unlock(&LOCK_thread_count); 
         DBUG_RETURN(1);
       }
 
-      tmp->in_use= t;
     }
     pthread_mutex_unlock(&thd_item->LOCK_temporary_tables);
   }
@@ -3814,11 +3831,17 @@
   bool table_names_only= (thd->lex->sql_command == SQLCOM_SHOW_TEMPORARY_TABLES) ? 1 : 0;
   TABLE *tmp;
 
+  pthread_mutex_lock(&thd->LOCK_temporary_tables);
+
   for (tmp=thd->temporary_tables; tmp; tmp=tmp->next) {
-    if (store_temporary_table_record(thd, tables->table, tmp, thd->lex->select_lex.db, table_names_only)) {
+    if (store_temporary_table_record(thd, tables->table, tmp,
+                                     thd->lex->select_lex.db,
+                                     table_names_only, true)) {
+      pthread_mutex_unlock(&thd->LOCK_temporary_tables);
       DBUG_RETURN(1);
     }
   }
+  pthread_mutex_unlock(&thd->LOCK_temporary_tables);
   DBUG_RETURN(0);
 }
 

=== modified file 'Percona-Server/storage/innodb_plugin/handler/ha_innodb.cc'
--- Percona-Server/storage/innodb_plugin/handler/ha_innodb.cc	2013-08-23 07:34:57 +0000
+++ Percona-Server/storage/innodb_plugin/handler/ha_innodb.cc	2013-09-13 13:36:27 +0000
@@ -4537,6 +4537,10 @@
 	new_handler = static_cast<ha_innobase*>(handler::clone(name,
 							       mem_root));
 	if (new_handler) {
+		DBUG_ASSERT(new_handler->prebuilt != NULL);
+		DBUG_ASSERT(new_handler->user_thd == user_thd);
+		DBUG_ASSERT(new_handler->prebuilt->trx == prebuilt->trx);
+
 		new_handler->prebuilt->select_lock_type
 			= prebuilt->select_lock_type;
 	}