maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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

> 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 :)

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.

>      if (thd->spcont)
>      {
>        m_statement_warn_count= 0;
> +      m_affected_rows= 0;

why do you reset m_affected_rows too?

>      }
>      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?

>  
> -  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" ?

> +      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() ?

> +    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