Re: [Maria-developers] f854ac4d9e0: MDEV-21916: COM_STMT_BULK_EXECUTE with RETURNING insert wrong values
Hi, Oleksandr! On Jul 29, Oleksandr Byelkin wrote:
revision-id: f854ac4d9e0 (mariadb-10.5.4-20-gf854ac4d9e0) parent(s): 6cee9b1953b author: Oleksandr Byelkin <sanja@mariadb.com> committer: Oleksandr Byelkin <sanja@mariadb.com> timestamp: 2020-07-03 13:39:49 +0200 message:
MDEV-21916: COM_STMT_BULK_EXECUTE with RETURNING insert wrong values
To allocate new net buffer to avoid changing bufer we are reading.
please add a more descriptive comment. I'm still not sure I understood correctly what the bug was.
diff --git a/sql/net_serv.cc b/sql/net_serv.cc --- a/sql/net_serv.cc +++ b/sql/net_serv.cc @@ -178,6 +178,14 @@ my_bool my_net_init(NET *net, Vio *vio, void *thd, uint my_flags) DBUG_RETURN(0); }
+ +inline void net_reset_new_packet(NET *net)
static
+{ + net->buff_end= net->buff + net->max_packet; + net->write_pos= net->read_pos= net->buff; +} + + my_bool net_allocate_new_packet(NET *net, void *thd, uint my_flags) { DBUG_ENTER("net_allocate_new_packet"); @@ -186,11 +194,26 @@ my_bool net_allocate_new_packet(NET *net, void *thd, uint my_flags) NET_HEADER_SIZE + COMP_HEADER_SIZE + 1, MYF(MY_WME | my_flags)))) DBUG_RETURN(1); - net->buff_end=net->buff+net->max_packet; - net->write_pos=net->read_pos = net->buff; + net_reset_new_packet(net); DBUG_RETURN(0); }
+unsigned char *net_try_allocate_new_packet(NET *net, void *thd, uint my_flags) +{ + unsigned char * old_buff= net->buff; + if (net_allocate_new_packet(net, thd, my_flags)) + { + /* + We failed to allocate a new buffer => return the old buffer and + return an error + */ + net->buff= old_buff; + net_reset_new_packet(net); + old_buff= NULL;
wouldn't it be better to make net_allocate_new_packet not to change net->buff on failure? because currently you have exactly the same problem in sql_parse.cc (in 10.4) and your patch doesn't fix it. simply if (!(buff= (uchar*) my_malloc(...))) DBUG_RETURN(1); net->buff= buff; net->buff_end= net->buff + net->max_packet; net->write_pos= net->read_pos= net->buff; DBUG_RETURN(0);
+ } + return old_buff; +} +
void net_end(NET *net) { diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc --- a/sql/sql_delete.cc +++ b/sql/sql_delete.cc @@ -685,7 +685,7 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, !table->prepare_triggers_for_delete_stmt_or_event()) will_batch= !table->file->start_bulk_delete();
- if (returning) + if (returning && !thd->get_stmt_da()->is_set())
how are result sets for bulk delete returning sent?
{ if (result->send_result_set_metadata(returning->item_list, Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF)) diff --git a/sql/sql_error.cc b/sql/sql_error.cc --- a/sql/sql_error.cc +++ b/sql/sql_error.cc @@ -380,16 +380,33 @@ Diagnostics_area::set_eof_status(THD *thd) if (unlikely(is_error() || is_disabled())) return;
- /* - If inside a stored procedure, do not return the total - number of warnings, since they are not available to the client - anyway. - */ - m_statement_warn_count= (thd->spcont ? - 0 : - current_statement_warn_count()); + if (m_status == DA_EOF_BULK) + { + /* + If inside a stored procedure, do not return the total + number of warnings, since they are not available to the client + anyway. + */ + if (!thd->spcont) + m_statement_warn_count+= current_statement_warn_count(); + } + else + { + /* + If inside a stored procedure, do not return the total + number of warnings, since they are not available to the client + anyway. + */
did you have to repeat the same comment twice?
+ if (thd->spcont) + { + m_statement_warn_count= 0; + m_affected_rows= 0;
affected rows too? old code didn't do that
+ } + else + m_statement_warn_count= current_statement_warn_count(); + m_status= (is_bulk_op() ? DA_EOF_BULK : DA_EOF); + }
- m_status= DA_EOF; DBUG_VOID_RETURN; }
diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc --- a/sql/sql_prepare.cc +++ b/sql/sql_prepare.cc @@ -4364,24 +4368,37 @@ Prepared_statement::execute_bulk_loop(String *expanded_query, if (state == Query_arena::STMT_ERROR) { my_message(last_errno, last_error, MYF(0)); - thd->set_bulk_execution(0); - return TRUE; + goto err; } /* Check for non zero parameter count*/ if (param_count == 0) { DBUG_PRINT("error", ("Statement with no parameters for bulk execution.")); my_error(ER_UNSUPPORTED_PS, MYF(0)); - thd->set_bulk_execution(0); - return TRUE; + goto err; }
if (!(sql_command_flags[lex->sql_command] & CF_SP_BULK_SAFE)) { DBUG_PRINT("error", ("Command is not supported in bulk execution.")); my_error(ER_UNSUPPORTED_PS, MYF(0)); - thd->set_bulk_execution(0); - return TRUE; + goto err; + } + /* + Here second buffer for not optimized commands, + optimized commands do it inside thier internal loop. + */ + if (!(sql_command_flags[lex->sql_command] & CF_SP_BULK_OPTIMIZED) && + this->lex->has_returning()) + { + // Above check can be true for SELECT in future + DBUG_ASSERT(lex->sql_command != SQLCOM_SELECT); + if ((readbuff= + net_try_allocate_new_packet(&thd->net, thd, + MYF(MY_THREAD_SPECIFIC))) == NULL)
sorry, I'm confused. You allocate a buffer here, in Prepared_statement::execute_bulk_loop, and then again in mysql_insert() ?
+ { + goto err; + } }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
HI, Sergei! On Wed, Jul 29, 2020 at 7:47 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Oleksandr!
On Jul 29, Oleksandr Byelkin wrote:
revision-id: f854ac4d9e0 (mariadb-10.5.4-20-gf854ac4d9e0) parent(s): 6cee9b1953b author: Oleksandr Byelkin <sanja@mariadb.com> committer: Oleksandr Byelkin <sanja@mariadb.com> timestamp: 2020-07-03 13:39:49 +0200 message:
MDEV-21916: COM_STMT_BULK_EXECUTE with RETURNING insert wrong values
To allocate new net buffer to avoid changing bufer we are reading.
please add a more descriptive comment. I'm still not sure I understood correctly what the bug was.
OK. But the idea is simple, to avoid reading and writing the same buffer we should allocate a new buffer for writing.
diff --git a/sql/net_serv.cc b/sql/net_serv.cc --- a/sql/net_serv.cc +++ b/sql/net_serv.cc @@ -178,6 +178,14 @@ my_bool my_net_init(NET *net, Vio *vio, void *thd, uint my_flags) DBUG_RETURN(0); }
+ +inline void net_reset_new_packet(NET *net)
static
OK
+{ + net->buff_end= net->buff + net->max_packet; + net->write_pos= net->read_pos= net->buff; +} + + my_bool net_allocate_new_packet(NET *net, void *thd, uint my_flags) { DBUG_ENTER("net_allocate_new_packet"); @@ -186,11 +194,26 @@ my_bool net_allocate_new_packet(NET *net, void *thd, uint my_flags) NET_HEADER_SIZE + COMP_HEADER_SIZE + 1, MYF(MY_WME | my_flags)))) DBUG_RETURN(1); - net->buff_end=net->buff+net->max_packet; - net->write_pos=net->read_pos = net->buff; + net_reset_new_packet(net); DBUG_RETURN(0); }
+unsigned char *net_try_allocate_new_packet(NET *net, void *thd, uint my_flags) +{ + unsigned char * old_buff= net->buff; + if (net_allocate_new_packet(net, thd, my_flags)) + { + /* + We failed to allocate a new buffer => return the old buffer and + return an error + */ + net->buff= old_buff; + net_reset_new_packet(net); + old_buff= NULL;
wouldn't it be better to make net_allocate_new_packet not to change net->buff on failure?
because currently you have exactly the same problem in sql_parse.cc (in 10.4) and your patch doesn't fix it.
simply
if (!(buff= (uchar*) my_malloc(...))) DBUG_RETURN(1); net->buff= buff; net->buff_end= net->buff + net->max_packet; net->write_pos= net->read_pos= net->buff; DBUG_RETURN(0);
OK, I just tried not to touch it really, just divide it into several functions.
+ } + return old_buff; +} +
void net_end(NET *net) { diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc --- a/sql/sql_delete.cc +++ b/sql/sql_delete.cc @@ -685,7 +685,7 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, !table->prepare_triggers_for_delete_stmt_or_event()) will_batch= !table->file->start_bulk_delete();
- if (returning) + if (returning && !thd->get_stmt_da()->is_set())
how are result sets for bulk delete returning sent?
Above is just a check for the first cycle. After the first cycle it will be set to DA_EOF_BULK or DA_OK_BULK. I will add a comment about it.
{ if (result->send_result_set_metadata(returning->item_list, Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF)) diff --git a/sql/sql_error.cc b/sql/sql_error.cc --- a/sql/sql_error.cc +++ b/sql/sql_error.cc @@ -380,16 +380,33 @@ Diagnostics_area::set_eof_status(THD *thd) if (unlikely(is_error() || is_disabled())) return;
- /* - If inside a stored procedure, do not return the total - number of warnings, since they are not available to the client - anyway. - */ - m_statement_warn_count= (thd->spcont ? - 0 : - current_statement_warn_count()); + if (m_status == DA_EOF_BULK) + { + /* + If inside a stored procedure, do not return the total + number of warnings, since they are not available to the client + anyway. + */ + if (!thd->spcont) + m_statement_warn_count+= current_statement_warn_count(); + } + else + { + /* + If inside a stored procedure, do not return the total + number of warnings, since they are not available to the client + anyway. + */
did you have to repeat the same comment twice?
I will remove
+ if (thd->spcont) + { + m_statement_warn_count= 0; + m_affected_rows= 0;
affected rows too? old code didn't do that
made for safety, but you are right maybe it is too much
+ } + else + m_statement_warn_count= current_statement_warn_count(); + m_status= (is_bulk_op() ? DA_EOF_BULK : DA_EOF); + }
- m_status= DA_EOF; DBUG_VOID_RETURN; }
diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc --- a/sql/sql_prepare.cc +++ b/sql/sql_prepare.cc @@ -4364,24 +4368,37 @@ Prepared_statement::execute_bulk_loop(String *expanded_query, if (state == Query_arena::STMT_ERROR) { my_message(last_errno, last_error, MYF(0)); - thd->set_bulk_execution(0); - return TRUE; + goto err; } /* Check for non zero parameter count*/ if (param_count == 0) { DBUG_PRINT("error", ("Statement with no parameters for bulk execution.")); my_error(ER_UNSUPPORTED_PS, MYF(0)); - thd->set_bulk_execution(0); - return TRUE; + goto err; }
if (!(sql_command_flags[lex->sql_command] & CF_SP_BULK_SAFE)) { DBUG_PRINT("error", ("Command is not supported in bulk execution.")); my_error(ER_UNSUPPORTED_PS, MYF(0)); - thd->set_bulk_execution(0); - return TRUE; + goto err; + } + /* + Here second buffer for not optimized commands, + optimized commands do it inside thier internal loop. + */ + if (!(sql_command_flags[lex->sql_command] & CF_SP_BULK_OPTIMIZED) && + this->lex->has_returning()) + { + // Above check can be true for SELECT in future + DBUG_ASSERT(lex->sql_command != SQLCOM_SELECT); + if ((readbuff= + net_try_allocate_new_packet(&thd->net, thd, + MYF(MY_THREAD_SPECIFIC))) == NULL)
sorry, I'm confused. You allocate a buffer here, in Prepared_statement::execute_bulk_loop, and then again in mysql_insert() ?
there are 2 way to execute "bulk" operation: 1) optimized (it is only INSERT (REPLACE) for now) where all is done inside insert procedure (one table open, one transaction) 2) many calls of the procedure (DELETE/UPDATE) which as output make "picture" of one call (one OK/EOF and maybe result set) (as side effect many time tables opened and it is different transaction) so it is in fact 2 independent ways and that is why it is in 2 places.
+ { + goto err; + } }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Oleksandr! On Jul 30, Oleksandr Byelkin wrote:
On Wed, Jul 29, 2020 at 7:47 PM Sergei Golubchik <serg@mariadb.org> wrote:
+ /* + Here second buffer for not optimized commands, + optimized commands do it inside thier internal loop. + */ + if (!(sql_command_flags[lex->sql_command] & CF_SP_BULK_OPTIMIZED) && + this->lex->has_returning()) + { + // Above check can be true for SELECT in future + DBUG_ASSERT(lex->sql_command != SQLCOM_SELECT); + if ((readbuff= + net_try_allocate_new_packet(&thd->net, thd, + MYF(MY_THREAD_SPECIFIC))) == NULL)
sorry, I'm confused. You allocate a buffer here, in Prepared_statement::execute_bulk_loop, and then again in mysql_insert() ?
there are 2 way to execute "bulk" operation: 1) optimized (it is only INSERT (REPLACE) for now) where all is done inside insert procedure (one table open, one transaction) 2) many calls of the procedure (DELETE/UPDATE) which as output make "picture" of one call (one OK/EOF and maybe result set) (as side effect many time tables opened and it is different transaction)
so it is in fact 2 independent ways and that is why it is in 2 places.
May be, then, add DBUG_ASSERT(lex->sql_command != SQLCOM_INSERT); ? Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Oleksandr Byelkin -
Sergei Golubchik