HI, Sergei! On Wed, Jul 29, 2020 at 7:47 PM Sergei Golubchik <[email protected]> wrote:
> Hi, Oleksandr! > > On Jul 29, Oleksandr Byelkin wrote: > > revision-id: f854ac4d9e0 (mariadb-10.5.4-20-gf854ac4d9e0) > > parent(s): 6cee9b1953b > > author: Oleksandr Byelkin <[email protected]> > > committer: Oleksandr Byelkin <[email protected]> > > 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 [email protected] >
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
