bk commit into 5.1 tree (guilhem:1.2596) BUG#31612

[Guilhem Bichot]

Below is the list of changes that have just been committed into a local
5.1 repository of guilhem. When guilhem does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet@stripped, 2007-10-22 20:52:05+02:00, guilhem@stripped +8 -0
  Fix for BUG#31612
  "Trigger fired multiple times leads to gaps in auto_increment sequence".
  Bug was never experienced by users because it occurs only with the fix
  for BUG31540 "incorrect auto_increment values used for multi-row insert
  trigger" which is not yet in the main 5.1.
  The bug was that if a trigger fired multiple times inside a top
  statement (for example top-statement is a multi-row INSERT, 
  and trigger is ON INSERT), and that trigger inserted into an auto_increment
  column, then gaps could be observed in the auto_increment sequence,
  even if there were no other users of the database (no concurrency).
  It was wrong usage of THD::auto_inc_intervals_in_cur_stmt_for_binlog.

  mysql-test/r/trigger-trans.result@stripped, 2007-10-22 20:52:00+02:00, guilhem@stripped
+27 -0
    result; before the bugfix, the sequence was 1,2,4,6,8,10,12...

  mysql-test/suite/rpl/include/rpl_mixed_dml.inc@stripped, 2007-10-22 20:52:00+02:00,
guilhem@stripped +1 -1
    because rpl_mixed.dat was read-only, the "rm" went into interactive mode
    ("/bin/rm: remove write-protected regular file?") and thus the test
    blocked and timed out.

  mysql-test/t/trigger-trans.test@stripped, 2007-10-22 20:52:00+02:00, guilhem@stripped
+13 -0
    test for BUG#31612

  sql/handler.cc@stripped, 2007-10-22 20:52:00+02:00, guilhem@stripped +16 -11
    See revision comment of handler.h.
    As THD::auto_inc_intervals_in_cur_stmt_for_binlog is cumulative
    over all trigger invokations by the top statement, the
    second invokation of the trigger arrived in handler::update_auto_increment()
    with already one interval in
    THD::auto_inc_intervals_in_cur_stmt_for_binlog. The method thus
    believed it had already reserved one interval for that invokation,
    thus reserved a twice larger interval (heuristic when we don't know
    how large the interval should be: we grow by powers of two). InnoDB
    thus increased its internal per-table auto_increment counter by 2
    while only one row was to be inserted. Hence a gap in the sequence.
    The fix is to use the new handler::auto_inc_intervals_count.
    Note that the trigger's statement knows how many rows it is going
    to insert, but provides estimation_rows_to_insert == 0 (see comments
    in sql_insert.cc why triggers don't call
    handler::ha_start_bulk_insert()).
    * removing white space at end of line
    * we don't need to maintain THD::auto_inc_intervals_in_cur_stmt_for_binlog
    if no binlogging or if row-based binlogging.

  sql/handler.h@stripped, 2007-10-22 20:52:00+02:00, guilhem@stripped +9 -1
    THD::auto_inc_intervals_in_cur_stmt_for_binlog served
    - for binlogging
    - as a heuristic when we have no estimation of how many records the
    statement will insert.
    But the first goal needs to be cumulative over all statements which
    form a binlog event, while the second one needs to be attached to each
    statement. THD::auto_inc_intervals_in_cur_stmt_for_binlog is cumulative,
    leading to BUG#31612. So we introduce handler::auto_inc_intervals_count
    for the second goal. See the revision comment of handler.cc.
    A smaller issue was that, even when the binlog event was only one
    statement (no triggers, no stored functions),
    THD::auto_inc_intervals_in_cur_stmt.nb_elements() could be lower than
    the number of reserved intervals (fooling the heuristic), because its
    append() method collapses two contiguous intervals in one.
    Note that as auto_inc_intervals_count is in class 'handler' and not
    in class 'THD', it does not need to be handled in
    THD::reset|restore_sub_statement_state()

  sql/log.cc@stripped, 2007-10-22 20:52:00+02:00, guilhem@stripped +0 -5
    Comment is wrong: if auto_increment is second, in handler::update_auto_increment()
    'append' is false and so auto_inc_intervals_in_cur_stmt_for_binlog
    is empty, we do not come here.

  sql/sql_class.h@stripped, 2007-10-22 20:52:00+02:00, guilhem@stripped +3 -0
    comment

  storage/innobase/handler/ha_innodb.cc@stripped, 2007-10-22 20:52:00+02:00,
guilhem@stripped +4 -0
    Importing a patch from InnoDB which is not yet in the main 5.1 tree
    (see BUG#31540). I should probably not commit this.

diff -Nrup a/mysql-test/r/trigger-trans.result b/mysql-test/r/trigger-trans.result
--- a/mysql-test/r/trigger-trans.result	2007-07-12 20:26:38 +02:00
+++ b/mysql-test/r/trigger-trans.result	2007-10-22 20:52:00 +02:00
@@ -141,3 +141,30 @@ c
 1
 drop table t1, t2, t3;
 End of 5.0 tests
+BUG#31612
+Trigger fired multiple times leads to gaps in auto_increment sequence
+create table t1 (a int, val char(1)) engine=InnoDB;
+create table t2 (b int auto_increment primary key,
+val char(1)) engine=InnoDB;
+create trigger t1_after_insert after
+insert on t1 for each row insert into t2 set val=NEW.val;
+insert into t1 values ( 123, 'a'), ( 123, 'b'), ( 123, 'c'),
+(123, 'd'), (123, 'e'), (123, 'f'), (123, 'g');
+insert into t1 values ( 654, 'a'), ( 654, 'b'), ( 654, 'c'),
+(654, 'd'), (654, 'e'), (654, 'f'), (654, 'g');
+select * from t2 order by b;
+b	val
+1	a
+2	b
+3	c
+4	d
+5	e
+6	f
+7	g
+8	a
+9	b
+10	c
+11	d
+12	e
+13	f
+14	g
diff -Nrup a/mysql-test/suite/rpl/include/rpl_mixed_dml.inc
b/mysql-test/suite/rpl/include/rpl_mixed_dml.inc
--- a/mysql-test/suite/rpl/include/rpl_mixed_dml.inc	2007-08-28 02:33:52 +02:00
+++ b/mysql-test/suite/rpl/include/rpl_mixed_dml.inc	2007-10-22 20:52:00 +02:00
@@ -53,7 +53,7 @@ DELETE FROM t2 WHERE a = 2;
 --echo ******************** LOAD DATA INFILE ********************
 --exec cp ./suite/rpl/data/rpl_mixed.dat $MYSQLTEST_VARDIR/tmp/
 LOAD DATA INFILE '../tmp/rpl_mixed.dat' INTO TABLE t1 FIELDS TERMINATED BY '|' ;
---exec rm $MYSQLTEST_VARDIR/tmp/rpl_mixed.dat
+remove_file $MYSQLTEST_VARDIR/tmp/rpl_mixed.dat;
 SELECT * FROM t1;
 --source suite/rpl/include/rpl_mixed_check_select.inc
 --source suite/rpl/include/rpl_mixed_clear_tables.inc
diff -Nrup a/mysql-test/t/trigger-trans.test b/mysql-test/t/trigger-trans.test
--- a/mysql-test/t/trigger-trans.test	2007-07-12 20:26:38 +02:00
+++ b/mysql-test/t/trigger-trans.test	2007-10-22 20:52:00 +02:00
@@ -130,3 +130,16 @@ disconnect connection_aux;
 
 
 --echo End of 5.0 tests
+
+--echo BUG#31612
+--echo Trigger fired multiple times leads to gaps in auto_increment sequence
+create table t1 (a int, val char(1)) engine=InnoDB;
+create table t2 (b int auto_increment primary key,
+ val char(1)) engine=InnoDB;
+create trigger t1_after_insert after
+ insert on t1 for each row insert into t2 set val=NEW.val;
+insert into t1 values ( 123, 'a'), ( 123, 'b'), ( 123, 'c'),
+ (123, 'd'), (123, 'e'), (123, 'f'), (123, 'g');
+insert into t1 values ( 654, 'a'), ( 654, 'b'), ( 654, 'c'),
+ (654, 'd'), (654, 'e'), (654, 'f'), (654, 'g');
+select * from t2 order by b;
diff -Nrup a/sql/handler.cc b/sql/handler.cc
--- a/sql/handler.cc	2007-10-19 23:20:33 +02:00
+++ b/sql/handler.cc	2007-10-22 20:52:00 +02:00
@@ -1730,7 +1730,12 @@ prev_insert_id(ulonglong nr, struct syst
   - In both cases, the reserved intervals are remembered in
     thd->auto_inc_intervals_in_cur_stmt_for_binlog if statement-based
     binlogging; the last reserved interval is remembered in
-    auto_inc_interval_for_cur_row.
+    auto_inc_interval_for_cur_row. The number of reserved intervals is
+    remembered in auto_inc_intervals_count. It differs from the number of
+    elements in thd->auto_inc_intervals_in_cur_stmt_for_binlog() because the
+    latter list is cumulative over all statements forming one binlog event
+    (when stored functions and triggers are used), and collapses two
+    contiguous intervals in one (see its append() method).
 
     The idea is that generated auto_increment values are predictable and
     independent of the column values in the table.  This is needed to be
@@ -1807,8 +1812,6 @@ int handler::update_auto_increment()
         handler::estimation_rows_to_insert was set by
         handler::ha_start_bulk_insert(); if 0 it means "unknown".
       */
-      uint nb_already_reserved_intervals=
-        thd->auto_inc_intervals_in_cur_stmt_for_binlog.nb_elements();
       ulonglong nb_desired_values;
       /*
         If an estimation was given to the engine:
@@ -1820,17 +1823,17 @@ int handler::update_auto_increment()
         start, starting from AUTO_INC_DEFAULT_NB_ROWS.
         Don't go beyond a max to not reserve "way too much" (because
         reservation means potentially losing unused values).
+        Note that in prelocked mode no estimation is given.
       */
-      if (nb_already_reserved_intervals == 0 &&
-          (estimation_rows_to_insert > 0))
+      if ((auto_inc_intervals_count == 0) && (estimation_rows_to_insert > 0))
         nb_desired_values= estimation_rows_to_insert;
       else /* go with the increasing defaults */
       {
         /* avoid overflow in formula, with this if() */
-        if (nb_already_reserved_intervals <= AUTO_INC_DEFAULT_NB_MAX_BITS)
+        if (auto_inc_intervals_count <= AUTO_INC_DEFAULT_NB_MAX_BITS)
         {
-          nb_desired_values= AUTO_INC_DEFAULT_NB_ROWS * 
-            (1 << nb_already_reserved_intervals);
+          nb_desired_values= AUTO_INC_DEFAULT_NB_ROWS *
+            (1 << auto_inc_intervals_count);
           set_if_smaller(nb_desired_values, AUTO_INC_DEFAULT_NB_MAX);
         }
         else
@@ -1843,7 +1846,7 @@ int handler::update_auto_increment()
                          &nb_reserved_values);
       if (nr == ~(ulonglong) 0)
         DBUG_RETURN(HA_ERR_AUTOINC_READ_FAILED);  // Mark failure
-      
+
       /*
         That rounding below should not be needed when all engines actually
         respect offset and increment in get_auto_increment(). But they don't
@@ -1854,7 +1857,7 @@ int handler::update_auto_increment()
       */
       nr= compute_next_insert_id(nr-1, variables);
     }
-    
+
     if (table->s->next_number_keypart == 0)
     {
       /* We must defer the appending until "nr" has been possibly truncated */
@@ -1898,8 +1901,9 @@ int handler::update_auto_increment()
   {
     auto_inc_interval_for_cur_row.replace(nr, nb_reserved_values,
                                           variables->auto_increment_increment);
+    auto_inc_intervals_count++;
     /* Row-based replication does not need to store intervals in binlog */
-    if (!thd->current_stmt_binlog_row_based)
+    if (mysql_bin_log.is_open() && !thd->current_stmt_binlog_row_based)
        
thd->auto_inc_intervals_in_cur_stmt_for_binlog.append(auto_inc_interval_for_cur_row.minimum(),
                                                              
auto_inc_interval_for_cur_row.values(),
                                                              
variables->auto_increment_increment);
@@ -2019,6 +2023,7 @@ void handler::ha_release_auto_increment(
   release_auto_increment();
   insert_id_for_cur_row= 0;
   auto_inc_interval_for_cur_row.replace(0, 0, 0);
+  auto_inc_intervals_count= 0;
   if (next_insert_id > 0)
   {
     next_insert_id= 0;
diff -Nrup a/sql/handler.h b/sql/handler.h
--- a/sql/handler.h	2007-09-07 15:41:46 +02:00
+++ b/sql/handler.h	2007-10-22 20:52:00 +02:00
@@ -1050,6 +1050,13 @@ public:
     inserter.
   */
   Discrete_interval auto_inc_interval_for_cur_row;
+  /**
+     @brief number of reserved auto-increment intervals. Serves as a heuristic
+     when we have no estimation of how many records the statement will insert:
+     the more intervals we have reserved, the bigger the next one. Reset in
+     handler::ha_release_auto_increment().
+  */
+  uint32 auto_inc_intervals_count;
 
   handler(handlerton *ht_arg, TABLE_SHARE *share_arg)
     :table_share(share_arg), table(0),
@@ -1058,7 +1065,8 @@ public:
     ref_length(sizeof(my_off_t)),
     ft_handler(0), inited(NONE),
     locked(FALSE), implicit_emptied(0),
-    pushed_cond(0), next_insert_id(0), insert_id_for_cur_row(0)
+    pushed_cond(0), next_insert_id(0), insert_id_for_cur_row(0),
+    auto_inc_intervals_count(0)
     {}
   virtual ~handler(void)
   {
diff -Nrup a/sql/log.cc b/sql/log.cc
--- a/sql/log.cc	2007-10-19 23:20:34 +02:00
+++ b/sql/log.cc	2007-10-22 20:52:00 +02:00
@@ -3643,11 +3643,6 @@ bool MYSQL_BIN_LOG::write(Log_event *eve
           DBUG_PRINT("info",("number of auto_inc intervals: %u",
                              thd->auto_inc_intervals_in_cur_stmt_for_binlog.
                              nb_elements()));
-          /*
-            If the auto_increment was second in a table's index (possible with
-            MyISAM or BDB) (table->next_number_keypart != 0), such event is
-            in fact not necessary. We could avoid logging it.
-          */
           Intvar_log_event e(thd, (uchar) INSERT_ID_EVENT,
                              thd->auto_inc_intervals_in_cur_stmt_for_binlog.
                              minimum());
diff -Nrup a/sql/sql_class.h b/sql/sql_class.h
--- a/sql/sql_class.h	2007-10-19 23:20:35 +02:00
+++ b/sql/sql_class.h	2007-10-22 20:52:00 +02:00
@@ -1296,6 +1296,9 @@ public:
     then the latter INSERT will insert no rows
     (first_successful_insert_id_in_cur_stmt == 0), but storing "INSERT_ID=3"
     in the binlog is still needed; the list's minimum will contain 3.
+    This variable is cumulative: if several statements are written to binlog
+    as one (stored functions or triggers are used) this list is the
+    concatenation of all intervals reserved by all statements.
   */
   Discrete_intervals_list auto_inc_intervals_in_cur_stmt_for_binlog;
   /* Used by replication and SET INSERT_ID */
diff -Nrup a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc
--- a/storage/innobase/handler/ha_innodb.cc	2007-10-10 22:15:06 +02:00
+++ b/storage/innobase/handler/ha_innodb.cc	2007-10-22 20:52:00 +02:00
@@ -6295,6 +6295,10 @@ ha_innobase::start_stmt(
 
 	trx = prebuilt->trx;
 
+        /* Reset the AUTOINC statement level counter for multi-row INSERTs.
+        */
+        trx->n_autoinc_rows = 0;
+
 	/* Here we release the search latch and the InnoDB thread FIFO ticket
 	if they were reserved. They should have been released already at the
 	end of the previous statement, but because inside LOCK TABLES the