Hi Sergei and Oleksandr, if we implement this (MDEV-11419: Report all INSERT ID for bulk operation INSERT), does this mean that we could allow innodb_autoinc_lock_mode=2 and still be safe for SBR ?
[1]: https://mariadb.com/kb/en/mariadb/auto_increment-handling-in-xtradbinnodb/ Many thanks, JFG On 14 March 2017 at 12:27, Sergei Golubchik <s...@mariadb.org> wrote: > Hi, Oleksandr! > > On Jan 17, Oleksandr Byelkin wrote: > > revision-id: 23959479689f47bdfe5aa33cb6cd5e1171b5f8a8 > (mariadb-10.2.2-128-g23959479689) > > parent(s): 9ea5de30963dd16cec7190b8b2f77858f4a82545 > > committer: Oleksandr Byelkin > > timestamp: 2017-01-17 15:47:17 +0100 > > message: > > > > MDEV-11419: Report all INSERT ID for bulk operation INSERT > > > > Send all Insert IDs of the buld operation to client (JDBC need it) > > > > --- > > include/mysql.h.pp | 3 +- > > include/mysql_com.h | 3 +- > > sql/protocol.cc | 22 +++++++--- > > sql/protocol.h | 6 +++ > > sql/sql_class.cc | 116 ++++++++++++++++++++++++++++++ > ++++++++++++++++++++++ > > sql/sql_class.h | 8 ++++ > > sql/sql_insert.cc | 11 ++++- > > sql/sql_prepare.cc | 21 ++++++++-- > > Tests are in C/C repository, I hope? > > > diff --git a/include/mysql_com.h b/include/mysql_com.h > > index c399520022d..9fac5edd1bc 100644 > > --- a/include/mysql_com.h > > +++ b/include/mysql_com.h > > @@ -551,7 +551,8 @@ enum enum_cursor_type > > CURSOR_TYPE_NO_CURSOR= 0, > > CURSOR_TYPE_READ_ONLY= 1, > > CURSOR_TYPE_FOR_UPDATE= 2, > > - CURSOR_TYPE_SCROLLABLE= 4 > > + CURSOR_TYPE_SCROLLABLE= 4, > > + INSERT_ID_REQUEST= 128 > > That's very weird. Why would "INSERT_ID_REQUEST" be a flag in the > "cursor_type"? > > Don't put this flag into enum_cursor_type > > > }; > > > > > > diff --git a/sql/protocol.cc b/sql/protocol.cc > > index f8b68c02fff..608c06da2df 100644 > > --- a/sql/protocol.cc > > +++ b/sql/protocol.cc > > @@ -562,6 +562,7 @@ void Protocol::end_statement() > > > > switch (thd->get_stmt_da()->status()) { > > case Diagnostics_area::DA_ERROR: > > + thd->stop_collecting_insert_id(); > > /* The query failed, send error to log and abort bootstrap. */ > > error= send_error(thd->get_stmt_da()->sql_errno(), > > thd->get_stmt_da()->message(), > > @@ -573,12 +574,21 @@ void Protocol::end_statement() > > break; > > case Diagnostics_area::DA_OK: > > case Diagnostics_area::DA_OK_BULK: > > - error= send_ok(thd->server_status, > > - thd->get_stmt_da()->statement_warn_count(), > > - thd->get_stmt_da()->affected_rows(), > > - thd->get_stmt_da()->last_insert_id(), > > - thd->get_stmt_da()->message(), > > - thd->get_stmt_da()->skip_flush()); > > + if (thd->report_collected_insert_id()) > > + if (thd->is_error()) > > + error= send_error(thd->get_stmt_da()->sql_errno(), > > + thd->get_stmt_da()->message(), > > + thd->get_stmt_da()->get_sqlstate()); > > + else > > + error= send_eof(thd->server_status, > > + thd->get_stmt_da()->statement_warn_count()); > > + else > > + error= send_ok(thd->server_status, > > + thd->get_stmt_da()->statement_warn_count(), > > + thd->get_stmt_da()->affected_rows(), > > + thd->get_stmt_da()->last_insert_id(), > > + thd->get_stmt_da()->message(), > > + thd->get_stmt_da()->skip_flush()); > > break; > > case Diagnostics_area::DA_DISABLED: > > break; > > diff --git a/sql/protocol.h b/sql/protocol.h > > index 6397e3dd5e6..bf74f52fa98 100644 > > --- a/sql/protocol.h > > +++ b/sql/protocol.h > > @@ -30,6 +30,12 @@ class Item_param; > > typedef struct st_mysql_field MYSQL_FIELD; > > typedef struct st_mysql_rows MYSQL_ROWS; > > > > +struct insert_id_desc > > +{ > > + ulonglong first_id; > > + ulonglong sequence; > > +}; > > + > > class Protocol > > { > > protected: > > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > > index 8fabc8f593e..53591c371c0 100644 > > --- a/sql/sql_class.cc > > +++ b/sql/sql_class.cc > > @@ -1474,6 +1474,7 @@ void THD::init(void) > > #endif //EMBEDDED_LIBRARY > > > > apc_target.init(&LOCK_thd_data); > > + insert_ids= NULL; > > DBUG_VOID_RETURN; > > } > > > > @@ -7365,4 +7366,119 @@ bool > > Discrete_intervals_list::append(Discrete_interval > *new_interval) > > DBUG_RETURN(0); > > } > > > > +bool THD::init_collecting_insert_id() > > +{ > > + if (!insert_ids) > > + { > > + void *buff; > > + if (!(my_multi_malloc(MYF(MY_WME), &insert_ids, > sizeof(DYNAMIC_ARRAY), > > + &buff, sizeof(insert_id_desc) * 10, > > + NullS)) || > > + my_init_dynamic_array2(insert_ids, sizeof(insert_id_desc), > > + buff, 10, 100, MYF(MY_WME))) > > Huh? > 1. You allocate DYNAMIC_ARRAY, on the heap, for every (insert-id > generating) statement. Why? > > 2. You preallocate a buffer, but dynamic array cannot reallocate it, so > on the 11'th element, it'll allocate *another* buffer and won't > use yours. > > Better: put DYNAMIC_ARRAY in the THD, or (even better) allocate it with > thd->alloc. And let DYNAMIC_ARRAY do it's own memory management. > > > + { > > + if (insert_ids) > > + my_free(insert_ids); > > + insert_ids= NULL; > > + return TRUE; > > + } > > + collect_auto_increment_increment= variables.auto_increment_ > increment; > > + } > > + return FALSE; > > +} > > + > > +void THD::stop_collecting_insert_id() > > +{ > > + if (insert_ids) > > + { > > + delete_dynamic(insert_ids); > > + my_free(insert_ids); > > + insert_ids= NULL; > > + } > > +} > > + > > +bool THD::collect_insert_id(ulonglong id) > > +{ > > + if (insert_ids) > > + { > > + if (insert_ids->elements) > > + { > > + insert_id_desc *last= > > + (insert_id_desc *)dynamic_array_ptr(insert_ids, > > + insert_ids->elements - 1); > > + if (id == last->first_id) > > + { > > + return FALSE; // no new insert id > > + } > > + if (id == last->first_id + (last->sequence * > > + collect_auto_increment_increment)) > > + { > > + last->sequence++; > > + return FALSE; > > + } > > + } > > + insert_id_desc el; > > + el.first_id= id; > > + el.sequence= 1; > > + if (insert_dynamic(insert_ids, &el)) > > + { > > + return TRUE; > > + } > > + } > > + return FALSE; > > +} > > This definitely needs a comment, explaining what you're doing and how > you're storing your insert id values. > > > + > > + > > +bool THD::report_collected_insert_id() > > +{ > > + if (insert_ids) > > + { > > + List<Item> field_list; > > + MEM_ROOT tmp_mem_root; > > + Query_arena arena(&tmp_mem_root, Query_arena::STMT_INITIALIZED), > backup; > > + > > + init_alloc_root(arena.mem_root, 2048, 4096, MYF(0)); > > + set_n_backup_active_arena(&arena, &backup); > > + DBUG_ASSERT(mem_root == &tmp_mem_root); > > + > > + field_list.push_back(new (mem_root) > > + Item_int(this, "Id", 0, > MY_INT64_NUM_DECIMAL_DIGITS), > > + mem_root); > > + field_list.push_back(new (mem_root) > > + Item_int(this, "Len", 0, > MY_INT64_NUM_DECIMAL_DIGITS), > > + mem_root); > > + field_list.push_back(new (mem_root) > > + Item_return_int(this, "Inc", 0, > MYSQL_TYPE_LONG), > > + mem_root); > > + > > + if (protocol_binary.send_result_set_metadata(&field_list, > > + > Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF)) > > + goto error; > > + > > + for (ulonglong i= 0; i < insert_ids->elements; i++) > > + { > > + insert_id_desc *last= > > + (insert_id_desc *)dynamic_array_ptr(insert_ids, i); > > + if (insert_ids->elements == 1 && last->first_id == 0 && > > + get_stmt_da()->affected_rows() != 1) > > + continue; // No insert IDs > > + protocol_binary.prepare_for_resend(); > > + protocol_binary.store_longlong(last->first_id, TRUE); > > + protocol_binary.store_longlong(last->sequence, TRUE); > > + protocol_binary.store_long(collect_auto_increment_increment); > > + if (protocol_binary.write()) > > + goto error; > > + } > > +error: > > + restore_active_arena(&arena, &backup); > > + DBUG_ASSERT(arena.mem_root == &tmp_mem_root); > > + // no need free Items because they was only constants > > + free_root(arena.mem_root, MYF(0)); > > + stop_collecting_insert_id(); > > + return TRUE; > > + } > > + return FALSE; > > + > > +} > > + > > #endif /* !defined(MYSQL_CLIENT) */ > > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > > index 7c0c1b68a2b..20b775c9273 100644 > > --- a/sql/sql_insert.cc > > +++ b/sql/sql_insert.cc > > @@ -1007,6 +1007,12 @@ bool mysql_insert(THD *thd,TABLE_LIST *table_list, > > } > > its.rewind(); > > iteration++; > > + > > + if (!error && thd->bulk_param) > > + { > > + thd->collect_insert_id(table->file->insert_id_for_cur_row); > > + } > > 1. Why are you doing it here, and not per row, inside > `while ((values= its++))` loop? > > 2. What about INSERT ... SELECT? > > 3. What about a insert-id generated in one statement but for many > tables? (stored function, trigger, whatever) > > > + > > } while (iteration < bulk_iterations); > > Regards, > Sergei > Chief Architect MariaDB > and secur...@mariadb.org > > _______________________________________________ > Mailing list: https://launchpad.net/~maria-developers > Post to : maria-developers@lists.launchpad.net > Unsubscribe : https://launchpad.net/~maria-developers > More help : https://help.launchpad.net/ListHelp >
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp