maria-developers team mailing list archive

[Oleksandr Byelkin <sanja@xxxxxxxxxxx>]

HI, Sergei!

On Wed, Jul 29, 2020 at 7:47 PM Sergei Golubchik <serg@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > committer: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> > 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@xxxxxxxxxxx
>