Hi Sergei! On Mon, Oct 12, 2020 at 5:36 PM Sergei Golubchik wrote:

Hi, Oleksandr!

On Oct 12, Oleksandr Byelkin wrote:

...

revision-id: 27060eb6ba5 (mariadb-10.5.4-220-g27060eb6ba5) parent(s): 861cd4ce286 author: Oleksandr Byelkin committer: Oleksandr Byelkin 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@mariadb.org