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.

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

mysql-test/r/trigger-trans.result

@@ -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

mysql-test/t/trigger-trans.test

@@ -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;

sql/handler.cc

@@ -2165,7 +2165,12 @@ prev_insert_id(ulonglong nr, struct system_variables *variables)
   - 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 &&
+      if ((auto_inc_intervals_count == 0) && (estimation_rows_to_insert > 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 * 
+          nb_desired_values= AUTO_INC_DEFAULT_NB_ROWS *
-            (1 << nb_already_reserved_intervals);
+            (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;

sql/handler.h

@@ -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)
   {

sql/log.cc

@@ -4011,11 +4011,6 @@ bool MYSQL_BIN_LOG::write(Log_event *event_info)
           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());

sql/sql_class.h

@@ -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 */