List:Commits« Previous MessageNext Message »
From:Olav Sandstaa Date:May 21 2010 1:57pm
Subject:bzr commit into mysql-next-mr-bugfixing branch (olav:3182) Bug#41029
View as plain text  
#At file:///export/home/tmp/mysql/opt-bug41029-opt-back/ based on revid:epotemkin@stripped

 3182 Olav Sandstaa	2010-05-21
      Fix for Bug#41029 MRR: SELECT FOR UPDATE fails to lock gaps (InnoDB table)
      
      This problem occurs due to MRR is creating a second handler
      object. This handler object is a "clone" of the first/initial handler
      object and the MRR code use this second handler object for executing
      some of the operations.
      
      The second handler object is created in DsMRR_impl::dsmrr_init() (in
      handler.cc) by "cloning" the initial handler object:
      
         if (!(new_h2= h->clone(thd->mem_root)) || 
              new_h2->ha_external_lock(thd, F_RDLCK))
      
      The important thing about the above code is that on this cloned
      handler object a call to ha_external_lock() is done with a request to
      use a read lock (F_RDLCK). When this handler object (h2) is used in
      DsMrr_impl::dsmrr_fill_buffer() for doing the initial search for the
      first record in the range interval, the index search done by InnoDB
      will be done without setting any exclusive lock on the search
      interval. This allows a second transaction to succeed to insert data
      within the "read interval" of the first transaction.
      
      The fix for this problem is to use the same lock type when calling
      new_h2->ha_external_lock() as was done on the original handler
      object. To do this we store (in handler::ha_external_lock()) the
      original lock type in a new member variable in the handler object
      (m_lock_type) and use this lock type when "cloning" the handler object
      in DsMRR_impl::dsmrr_init().
      
      Note to reviewers (and myself):
      
      1. The included test case should be will be included in the innodb_mrr
         test before the fix is pushed but due to innodb_mrr test being unstable
         I have included as a separate test case.
      
      2. The final commit/push should also remove the test and result files for
         this bug from the optimizer_unfixed_bugs suite.
     @ mysql-test/r/bug41029.result
        Result file for test of Bug#41029 MRR: SELECT FOR UPDATE fails to lock gaps (InnoDB table)
     @ mysql-test/t/bug41029-master.opt
        Option file for reducing the default lock timeout in order to reduce the time for running the test.
     @ mysql-test/t/bug41029.test
        Test for Bug#41029 MRR: SELECT FOR UPDATE fails to lock gaps (InnoDB table)
     @ sql/handler.cc
        When MRR is used a second handler object is cloned based on the original 
        handler object. This second handler object is used for the initial navigation 
        of the underlying storage engine's data. When this second handler object
        was created a call to ha_external_lock() was done with F_RDLCK as the
        lock type. Since F_RDLCK was always used this made InnoDB
        not put gap lock on the search interval and allowed concurrent transactions
        to insert data into the search interval of transasactions running SELECT 
        FOR UPDATE statements in REPEATABLE READ mode.
        
        To ensure the second handler object created when MRR is used is getting the
        same lock type as the initial handler the following changes are implemented:
        
        1. Extend handler::ha_external_lock() to save the lock type in a member variable
           (m_lock_type).
        2. Extend DsMRR_impl::dsmrr_init() so that it uses the same lock type when
           cloning a second handler object as used for the initial handler object
           (instead of always using F_RDLCK).
     @ sql/handler.h
        Add a new member variable to the handler class to store the lock type set by
        the call to handler::ha_external_lock(). This lock type is needed by the MRR
        implementation when creating a second handler object by cloning the initial
        handler.

    added:
      mysql-test/r/bug41029.result
      mysql-test/t/bug41029-master.opt
      mysql-test/t/bug41029.test
    modified:
      sql/handler.cc
      sql/handler.h
=== added file 'mysql-test/r/bug41029.result'

=== added file 'mysql-test/r/bug41029.result'
--- a/mysql-test/r/bug41029.result	1970-01-01 00:00:00 +0000
+++ b/mysql-test/r/bug41029.result	2010-05-21 13:57:37 +0000
@@ -0,0 +1,30 @@
+#
+# Bug#41029 "MRR: SELECT FOR UPDATE fails to lock gaps (InnoDB table)"
+#
+SET AUTOCOMMIT=0;
+CREATE TABLE t1 (
+dummy INT PRIMARY KEY,
+a INT UNIQUE,
+b INT
+) ENGINE=InnoDB;
+INSERT INTO t1 VALUES (1,1,1),(3,3,3),(5,5,5);
+COMMIT;
+SET SESSION TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+SELECT @@tx_isolation;
+@@tx_isolation
+REPEATABLE-READ
+START TRANSACTION;
+EXPLAIN SELECT * FROM t1 WHERE a > 2 FOR UPDATE;
+id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
+1	SIMPLE	t1	range	a	a	5	NULL	1	Using where; Using MRR
+SELECT * FROM t1 WHERE a > 2 FOR UPDATE;
+dummy	a	b
+3	3	3
+5	5	5
+SET AUTOCOMMIT=0;
+START TRANSACTION;
+INSERT INTO t1 VALUES (2,2,2);
+ERROR HY000: Lock wait timeout exceeded; try restarting transaction
+ROLLBACK;
+ROLLBACK;
+DROP TABLE t1;

=== added file 'mysql-test/t/bug41029-master.opt'
--- a/mysql-test/t/bug41029-master.opt	1970-01-01 00:00:00 +0000
+++ b/mysql-test/t/bug41029-master.opt	2010-05-21 13:57:37 +0000
@@ -0,0 +1,1 @@
+--loose-innodb_lock_wait_timeout=3

=== added file 'mysql-test/t/bug41029.test'
--- a/mysql-test/t/bug41029.test	1970-01-01 00:00:00 +0000
+++ b/mysql-test/t/bug41029.test	2010-05-21 13:57:37 +0000
@@ -0,0 +1,53 @@
+--echo #
+--echo # Bug#41029 "MRR: SELECT FOR UPDATE fails to lock gaps (InnoDB table)"
+--echo #
+
+--source include/have_innodb.inc
+
+# This test verifies that a SELECT FOR UPDATE statement executed in
+# REPEATABLE READ isolation will lock the entire read interval by verifying
+# that a second transaction trying to update data within this interval will
+# be blocked.
+
+connect (con1,localhost,root,,);
+connect (con2,localhost,root,,);
+
+connection con1;
+
+SET AUTOCOMMIT=0;
+
+CREATE TABLE t1 (
+  dummy INT PRIMARY KEY,
+  a INT UNIQUE,
+  b INT
+) ENGINE=InnoDB;
+
+INSERT INTO t1 VALUES (1,1,1),(3,3,3),(5,5,5);
+COMMIT;
+
+SET SESSION TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+SELECT @@tx_isolation;
+START TRANSACTION;
+
+EXPLAIN SELECT * FROM t1 WHERE a > 2 FOR UPDATE;
+
+SELECT * FROM t1 WHERE a > 2 FOR UPDATE;
+
+connection con2;
+
+SET AUTOCOMMIT=0;
+START TRANSACTION;
+
+--error ER_LOCK_WAIT_TIMEOUT
+INSERT INTO t1 VALUES (2,2,2);
+ROLLBACK;
+
+connection con1;
+
+ROLLBACK;
+DROP TABLE t1;
+
+connection default;
+disconnect con1;
+disconnect con2;
+

=== modified file 'sql/handler.cc'
--- a/sql/handler.cc	2010-05-14 10:58:39 +0000
+++ b/sql/handler.cc	2010-05-21 13:57:37 +0000
@@ -4464,7 +4464,7 @@
     uint mrr_keyno= h->active_index;
 
     if (!(new_h2= h->clone(thd->mem_root)) || 
-        new_h2->ha_external_lock(thd, F_RDLCK))
+        new_h2->ha_external_lock(thd, h->m_lock_type))
     {
       delete new_h2;
       DBUG_RETURN(1);
@@ -5505,8 +5505,16 @@
   int error= external_lock(thd, lock_type);
 
   if (error == 0)
+  {
     cached_table_flags= table_flags();
 
+    /*
+      The lock type is needed by MRR when creating a clone of this handler
+      object.
+    */
+    m_lock_type= lock_type;
+  }
+
   if (MYSQL_HANDLER_RDLOCK_DONE_ENABLED() ||
       MYSQL_HANDLER_WRLOCK_DONE_ENABLED() ||
       MYSQL_HANDLER_UNLOCK_DONE_ENABLED())

=== modified file 'sql/handler.h'
--- a/sql/handler.h	2010-05-14 11:22:27 +0000
+++ b/sql/handler.h	2010-05-21 13:57:37 +0000
@@ -1487,6 +1487,14 @@
   */
   PSI_table *m_psi;
 
+  /**
+    The lock type set by when calling::ha_external_lock(). This is 
+    propagated down to the storage engine. The reason for also storing 
+    this here is when doing MRR we need to create/clone a second handler 
+    object. This cloned handler object needs to know about the lock_type used.
+  */
+  int m_lock_type;
+
   handler(handlerton *ht_arg, TABLE_SHARE *share_arg)
     :table_share(share_arg), table(0),
     estimation_rows_to_insert(0), ht(ht_arg),
@@ -1498,7 +1506,7 @@
     pushed_cond(0), pushed_idx_cond(NULL), pushed_idx_cond_keyno(MAX_KEY),
     next_insert_id(0), insert_id_for_cur_row(0),
     auto_inc_intervals_count(0),
-    m_psi(NULL)
+    m_psi(NULL), m_lock_type(F_UNLCK)
     {}
   virtual ~handler(void)
   {


Attachment: [text/bzr-bundle] bzr/olav@sun.com-20100521135737-2pv0d5cf8fmbf52e.bundle
Thread
bzr commit into mysql-next-mr-bugfixing branch (olav:3182) Bug#41029Olav Sandstaa21 May
  • Re: bzr commit into mysql-next-mr-bugfixing branch (olav:3182)Bug#41029Øystein Grøvlen27 May
    • Re: bzr commit into mysql-next-mr-bugfixing branch (olav:3182)Bug#41029Olav Sandstaa27 May
    • Re: bzr commit into mysql-next-mr-bugfixing branch (olav:3182)Bug#41029Olav Sandstaa7 Jun
  • Re: bzr commit into mysql-next-mr-bugfixing branch (olav:3182)Bug#41029Øystein Grøvlen27 May