maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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.

> 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@xxxxxxxxxxx