maria-developers team mailing list archive

[Oleksandr Byelkin <sanja@xxxxxxxxxxx>]

Hi Sergei!

On Mon, Oct 12, 2020 at 5:36 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> Hi, Oleksandr!
>
> On Oct 12, Oleksandr Byelkin wrote:
> > revision-id: 27060eb6ba5 (mariadb-10.5.4-220-g27060eb6ba5)
> > parent(s): 861cd4ce286
> > author: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> > committer: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> > timestamp: 2020-10-07 15:22:41 +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.
>
> You still didn't clarify the commit comment
>
> fixed.

> > diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc
> > index 7280236e43f..5aaff3cf623 100644
> > --- a/sql/sql_delete.cc
> > +++ b/sql/sql_delete.cc
> > @@ -685,8 +685,14 @@ 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)
> > +  /*
> > +    thd->get_stmt_da()->is_set() means first iteration of prepared
> statement
> > +    with array binding operation execution (non optimized so it is not
> > +    INSERT)
> > +  */
> > +  if (returning && !thd->get_stmt_da()->is_set())
> >    {
> > +    DBUG_ASSERT(thd->lex->sql_command != SQLCOM_INSERT);
>
> a strange assert to see in sql_delete.cc :)
>

you asked to show that there is 2 different ways execute the statement and
INSERT do it in other place it was how I showed, there is no problem to
remove it

>
> can one even reach mysql_delete() with SQLCOM_INSERT?
> not just with returning and !thd->get_stmt_da()->is_set(), anywhere?
>
> >      if (result->send_result_set_metadata(returning->item_list,
> >                                  Protocol::SEND_NUM_ROWS |
> Protocol::SEND_EOF))
> >        goto cleanup;
> > diff --git a/sql/sql_error.cc b/sql/sql_error.cc
> > index b3ef0d89a98..a753af2b34d 100644
> > --- 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 (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.
> > +    */
>
> I don't think it helps to duplicate the comment. You could just put it
> once before the if.
>
fixed

>
> >      if (thd->spcont)
> >      {
> >        m_statement_warn_count= 0;
> > +      m_affected_rows= 0;
>
> why do you reset m_affected_rows too?
>
it is what returned as the result so they goes together

>
> >      }
> >      else
> >        m_statement_warn_count= current_statement_warn_count();
> > +    m_status= (is_bulk_op() ? DA_EOF_BULK : DA_EOF);
> > +  }
>
> do we have tests for bulk operations and CALL?
>

no, CALL is not supported for array binding


>
> >
> > -  m_status= DA_EOF;
> >    DBUG_VOID_RETURN;
> >  }
> >
> > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc
> > index ecb56e70f88..c144d3a8d7e 100644
> > --- a/sql/sql_prepare.cc
> > +++ b/sql/sql_prepare.cc
> > @@ -4357,24 +4361,37 @@ Prepared_statement::execute_bulk_loop(String
> *expanded_query,
> >
> > +  /*
> > +     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) &&
>
> why "SP" and not "PS" ?
>

It is because the fix is very old. the constant was renamed already.


>
> > +      this->lex->has_returning())
>
> what about CALL? It won't have lex->has_returning(). What about SELECT?
> Can you bind an array to a parameter in SELECT or CALL?
>
> > +  {
> > +    // Above check can be true for SELECT in future
> > +    DBUG_ASSERT(lex->sql_command != SQLCOM_SELECT);
>
> how can SQLCOM_SELECT have lex->has_returning() ?
>
by mistake for example or a new feature  or by moving all processors of
INMSERT/DELETE for the same procedures (Igor going to do it) and also some
bugs possible

>
> > +    readbuff= thd->net.buff; // old buffer
> > +    if (net_allocate_new_packet(&thd->net, thd,
> MYF(MY_THREAD_SPECIFIC)))
> > +    {
> > +      readbuff= NULL; // failure, net_allocate_new_packet keeps old
> buffer
> > +      goto err;
> > +    }
> >    }
> >
> >  #ifndef EMBEDDED_LIBRARY
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx
>