From 84c1fffabbf89a7c1ed3a768c81dc9060516255c Mon Sep 17 00:00:00 2001 From: Guilhem Bichot Date: Mon, 6 Oct 2008 16:06:59 +0200 Subject: [PATCH] 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 | 27 +++++++++++++++++++++++++++ mysql-test/t/trigger-trans.test | 13 +++++++++++++ sql/handler.cc | 27 ++++++++++++++++----------- sql/handler.h | 10 +++++++++- sql/log.cc | 5 ----- sql/sql_class.h | 3 +++ 6 files changed, 68 insertions(+), 17 deletions(-) diff --git a/mysql-test/r/trigger-trans.result b/mysql-test/r/trigger-trans.result index 9e0f1e2c351..29df8cbe06d 100644 --- a/mysql-test/r/trigger-trans.result +++ b/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 diff --git a/mysql-test/t/trigger-trans.test b/mysql-test/t/trigger-trans.test index 5db5b982773..ae223b2666e 100644 --- a/mysql-test/t/trigger-trans.test +++ b/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; diff --git a/sql/handler.cc b/sql/handler.cc index f26e84bdfdf..600991aeec8 100644 --- a/sql/handler.cc +++ 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 && - (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; diff --git a/sql/handler.h b/sql/handler.h index 3c5db8a0dff..b943e188962 100644 --- a/sql/handler.h +++ b/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) { diff --git a/sql/log.cc b/sql/log.cc index 7775fb44b65..b5539525ea6 100644 --- a/sql/log.cc +++ b/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()); diff --git a/sql/sql_class.h b/sql/sql_class.h index 60051ed02cc..82c162f923f 100644 --- a/sql/sql_class.h +++ b/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 */