bzr commit into mysql-5.1 branch (guilhem:2689) Bug#31612

[Guilhem Bichot]

#At file:///home/mysql_src/bzrrepos/mysql-5.1-5.1.29-rc/

 2689 Guilhem Bichot	2008-10-06
      Fix for BUG#31612
      "Trigger fired multiple times leads to gaps in auto_increment sequence".
      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.
      Note that the fix changes "class handler", I'll tell the Storage Engine API team.
modified:
  mysql-test/r/trigger-trans.result
  mysql-test/t/trigger-trans.test
  sql/handler.cc
  sql/handler.h
  sql/log.cc
  sql/sql_class.h

per-file messages:
  mysql-test/r/trigger-trans.result
    result; before the bugfix, the sequence was 1,2,4,6,8,10,12...
  mysql-test/t/trigger-trans.test
    test for BUG#31612
  sql/handler.cc
    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. Using auto_inc_intervals_count in
    the heuristic makes the heuristic independent of binary logging, which is good.
  sql/handler.h
    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
    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
    comment
=== modified file 'mysql-test/r/trigger-trans.result'
--- a/mysql-test/r/trigger-trans.result	2008-08-11 18:02:03 +0000
+++ b/mysql-test/r/trigger-trans.result	2008-10-06 13:59:47 +0000
@@ -161,3 +161,30 @@ SELECT @a, @b;
 1	1
 DROP TABLE t2, t1;
 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

=== modified file 'mysql-test/t/trigger-trans.test'
--- a/mysql-test/t/trigger-trans.test	2008-03-12 13:13:33 +0000
+++ b/mysql-test/t/trigger-trans.test	2008-10-06 13:59:47 +0000
@@ -162,3 +162,16 @@ DROP TABLE t2, t1;
 
 
 --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;

=== modified file 'sql/handler.cc'
--- a/sql/handler.cc	2008-09-05 15:21:59 +0000
+++ b/sql/handler.cc	2008-10-06 13:59:47 +0000
@@ -2165,7 +2165,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
@@ -2249,8 +2254,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:
@@ -2262,17 +2265,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
@@ -2285,7 +2288,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
@@ -2296,7 +2299,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 */
@@ -2340,8 +2343,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);
@@ -2461,6 +2465,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;

=== modified file 'sql/handler.h'
--- a/sql/handler.h	2008-09-08 13:30:01 +0000
+++ b/sql/handler.h	2008-10-06 13:59:47 +0000
@@ -1129,6 +1129,13 @@ public:
     inserter.
   */
   Discrete_interval auto_inc_interval_for_cur_row;
+  /**
+     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().
+  */
+  uint auto_inc_intervals_count;
 
   handler(handlerton *ht_arg, TABLE_SHARE *share_arg)
     :table_share(share_arg), table(0),
@@ -1137,7 +1144,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)
   {

=== modified file 'sql/log.cc'
--- a/sql/log.cc	2008-09-03 20:04:07 +0000
+++ b/sql/log.cc	2008-10-06 13:59:47 +0000
@@ -4011,11 +4011,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());

=== modified file 'sql/sql_class.h'
--- a/sql/sql_class.h	2008-09-17 12:54:50 +0000
+++ b/sql/sql_class.h	2008-10-06 13:59:47 +0000
@@ -1524,6 +1524,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 */