Re: [Maria-developers] 23959479689: MDEV-11419: Report all INSERT ID for bulk operation INSERT
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 security@mariadb.org
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 <serg@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 security@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
Hi, Jean-Francois! Am 14.03.2017 um 20:31 schrieb Jean-Francois B. Gagne:
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/
I answered from my phone, but not sure that it worked, so I'll repeat. The idea is to report IDs as if they reported by separate call of INSERT with each parameter, so EXECUTE INSERT with 5 parameters got back 5 IDs. The other variant is to all that command 5 times and get back 5 IDs via 5 server status report for each command. Array parameters binding return 1 status so it can report all IDs there. [skip]
Hi, Sergei!
Here is better answer (I had time to dig in the sources)
Am 14.03.2017 um 12:27 schrieb Sergei Golubchik:
> 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?
We discussed it, will be the test.
>
>> 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
It is flags and it happened that they are about cursors, but field of
flags can be used for other flags also (IMHO)
>
>> };
>>
>>
>> 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.
Problem with thd->alloc is that MEM_ROOT of THD freed before we can send
the data. Idea of preallocation is that it is hardly exceed it in most
realistic scenario.
>> + {
>> + 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.
OK.
/**
Collecting sequences of INSERT ID and storing them in the format:
<first ID, number of ID in the sequence> with the step of
collect_auto_increment_increment
*/
>
>> +
>> +
>> +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?
It is what we need INSERT ID as if the command would be executed with
the parameters separately, i.e
ID correspond to PARAMETER SET.
>
> 2. What about INSERT ... SELECT?
It is not supported by ARRAY binding.
>
> 3. What about a insert-id generated in one statement but for many
> tables? (stored function, trigger, whatever)
As above we need what usual INSERT send back to client in single execution.
>
>> +
>> } while (iteration < bulk_iterations);
> Regards,
> Sergei
> Chief Architect MariaDB
> and security@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
Hi, Oleksandr!
On Mar 15, Oleksandr Byelkin wrote:
> >> 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
> It is flags and it happened that they are about cursors, but field of
> flags can be used for other flags also (IMHO)
Yes. I saw that it only flags. I said just "don't put this flag into
enum_cursor_type". That is, you can store it in the same flag variable
together with cursor flags. But don't put it in the same enum.
Either
#define INSERT_ID_REQUEST 128
or a separate enum with non-cursor flags.
> >> };
> >>
> >>
> >> 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
> >> @@ -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.
> Problem with thd->alloc is that MEM_ROOT of THD freed before we can send
> the data. Idea of preallocation is that it is hardly exceed it in most
> realistic scenario.
Why wouldn't you send your data before the the MEM_ROOT is freed?
Preallocation is fine, just don't do it with multi-malloc.
my_init_dynamic_array2 can preallocate internally.
and init=10, increment=100 looks rather weird.
> >> + {
> >> + if (insert_ids)
> >> + my_free(insert_ids);
> >> + insert_ids= NULL;
> >> + return TRUE;
> >> + }
> >> + collect_auto_increment_increment= variables.auto_increment_increment;
> >> + }
> >> + return FALSE;
> >> +}
> >> +
> >> 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?
>
> It is what we need INSERT ID as if the command would be executed with
> the parameters separately, i.e
> ID correspond to PARAMETER SET.
I'm sorry, I don't understand that.
> > 2. What about INSERT ... SELECT?
> It is not supported by ARRAY binding.
What does INSERT ID generation has to do with array binding?
> > 3. What about a insert-id generated in one statement but for many
> > tables? (stored function, trigger, whatever)
> As above we need what usual INSERT send back to client in single execution.
Right, but if there's a trigger that inserts into another table, then
you might have two insert ids, for two different tables, in one
statement.
Regards,
Sergei
Chief Architect MariaDB
and security@mariadb.org
participants (3)
-
Jean-Francois B. Gagne -
Oleksandr Byelkin -
Sergei Golubchik