List:Commits« Previous MessageNext Message »
From:Konstantin Osipov Date:October 5 2010 12:47pm
Subject:Re: bzr commit into mysql-5.5-runtime branch (magne.mahre:3138)
Bug#56452
View as plain text  
* Magne Mahre <magne.mahre@stripped> [10/09/16 02:40]:

>  3138 Magne Mahre	2010-09-15
>       Bug#56452 Assertion failed: thd->transaction.stmt.is_empty() || 
>                 thd->in_sub_stmt
>       
>       In a precursor patch for Bug#52044 
>       (revid:bzr/kostja@stripped), a
>       number of reorganizations of code was made. In addition some
>       assertions were added to ensure the correct transactional state.
>       
>       The reorganization had a small glitch so statements that was
>       active in the query cache was not followed by a
>       statement commit/rollback (this code was removed). A section
>       in the trans_commit_stmt/trans_rollback_stmt code is to
>       clear the thd->transaction.stmt list of affected storage
>       engines.  When a new statement is initiated, an assert
>       introduced by the 523044 patch checks if this list is cleared.
>       When the query cache is accessed, this list may be populated,
>       and since it's not committed it will not be cleared.
>       
>       This fix adds explicit statement commit or rollback for
>       statements that is contained in the query cache.
>      @ sql/sql_parse.cc
>         Ensure that even statements that the query cache provides
>         results for, are committed properly.

I agree with the approach, however, I have a stylistic comment,
best represented as a patch (in the end of the mail).

> === modified file 'mysql-test/r/cache_innodb.result'
> --- a/mysql-test/r/cache_innodb.result	2009-12-22 09:35:56 +0000
> +++ b/mysql-test/r/cache_innodb.result	2010-09-15 21:47:50 +0000
> @@ -220,3 +220,14 @@ Variable_name	Value
>  Qcache_hits	1
>  set GLOBAL query_cache_size=1048576;
>  drop table t2;
> +CREATE TABLE t1 (a INT) ENGINE=InnoDB;
> +BEGIN;
> +INSERT INTO t1 VALUES(1);
> +ROLLBACK WORK AND CHAIN NO RELEASE;
> +SELECT a FROM t1;
> +a
> +ROLLBACK WORK AND CHAIN NO RELEASE;
> +SELECT a FROM t1;
> +a
> +ROLLBACK;
> +DROP TABLE t1;

This, somewhat esoteric test case puzzled me. In theory,
you would require virtually any statement that follows a SELECT
from the query cache to repeat the bug. In practice,
this is not the case, see Bug#57243 Inconsistent use of
trans_register_ha() API in InnoDB.


=== modified file 'mysql-test/r/cache_innodb.result'
--- mysql-test/r/cache_innodb.result	2009-12-22 09:35:56 +0000
+++ mysql-test/r/cache_innodb.result	2010-10-05 08:42:55 +0000
@@ -220,3 +220,14 @@ Variable_name	Value
 Qcache_hits	1
 set GLOBAL query_cache_size=1048576;
 drop table t2;
+CREATE TABLE t1 (a INT) ENGINE=InnoDB;
+BEGIN;
+INSERT INTO t1 VALUES(1);
+ROLLBACK WORK AND CHAIN NO RELEASE;
+SELECT a FROM t1;
+a
+ROLLBACK WORK AND CHAIN NO RELEASE;
+SELECT a FROM t1;
+a
+ROLLBACK;
+DROP TABLE t1;

=== modified file 'mysql-test/t/cache_innodb.test'
--- mysql-test/t/cache_innodb.test	2006-08-16 12:58:49 +0000
+++ mysql-test/t/cache_innodb.test	2010-10-05 08:42:55 +0000
@@ -14,3 +14,18 @@ let $engine_type= InnoDB;
 let $test_foreign_keys= 1;
 
 --source include/query_cache.inc
+
+#
+# Bug#56452 Assertion failed: thd->transaction.stmt.is_empty() || 
+#           thd->in_sub_stmt
+#
+CREATE TABLE t1 (a INT) ENGINE=InnoDB;
+BEGIN;
+  INSERT INTO t1 VALUES(1);
+ROLLBACK WORK AND CHAIN NO RELEASE;
+SELECT a FROM t1;
+ROLLBACK WORK AND CHAIN NO RELEASE;
+SELECT a FROM t1;
+ROLLBACK;
+DROP TABLE t1;
+

=== modified file 'sql/sql_cache.cc'
--- sql/sql_cache.cc	2010-09-24 12:12:09 +0000
+++ sql/sql_cache.cc	2010-10-05 12:42:26 +0000
@@ -341,6 +341,7 @@ TODO list:
 #include "../storage/myisammrg/ha_myisammrg.h"
 #include "../storage/myisammrg/myrg_def.h"
 #include "probes_mysql.h"
+#include "transaction.h"
 
 #ifdef EMBEDDED_LIBRARY
 #include "emb_qcache.h"
@@ -1683,6 +1684,8 @@ def_week_frmt: %lu, in_trans: %d, autoco
       }
       else
         thd->lex->safe_to_cache_query= 0;       // Don't try to cache this
+      /* End the statement transaction potentially started by engine. */
+      trans_rollback_stmt(thd);
       goto err_unlock;				// Parse query
     }
     else
@@ -1724,6 +1727,13 @@ def_week_frmt: %lu, in_trans: %d, autoco
 
   thd->limit_found_rows = query->found_rows();
   thd->status_var.last_query_cost= 0.0;
+  /*
+    End the statement transaction potentially started by an
+    engine callback. We ignore the return value for now,
+    since as long as EOF packet is part of the query cache
+    response, we can't handle it anyway.
+  */
+  (void) trans_commit_stmt(thd);
   if (!thd->stmt_da->is_set())
     thd->stmt_da->disable_status();
 

-- 
Thread
bzr commit into mysql-5.5-runtime branch (magne.mahre:3138) Bug#56452Magne Mahre15 Sep
  • Re: bzr commit into mysql-5.5-runtime branch (magne.mahre:3138)Bug#56452Konstantin Osipov5 Oct