Re: [Maria-developers] 27060eb6ba5: MDEV-21916: COM_STMT_BULK_EXECUTE with RETURNING insert wrong values
Hi, Oleksandr! On Oct 12, Oleksandr Byelkin wrote:
revision-id: 27060eb6ba5 (mariadb-10.5.4-220-g27060eb6ba5) parent(s): 861cd4ce286 author: Oleksandr Byelkin <sanja@mariadb.com> committer: Oleksandr Byelkin <sanja@mariadb.com> 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@mariadb.org
Hi Sergei! On Mon, Oct 12, 2020 at 5:36 PM Sergei Golubchik <serg@mariadb.org> 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@mariadb.com> committer: Oleksandr Byelkin <sanja@mariadb.com> 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
Hi, Sanja! One more question. Why did you introduce DA_EOF_BULK ? It doesn't seem to be related to net buffers. Is it to handle bulk UPDATE/DELETE, to calculate warnings over the whole batch? And only for that? On Jun 09, Oleksandr Byelkin wrote:
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.
Thanks, looks much clearer now.
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
I have to admit, asserting not SQLCOM_INSERT inside sql_delete.cc looks rather ridiculous. Sorry for this. As you like, basically, but in my opinion the comment explains enough, the DBUG_ASSERT only confuses a reader.
can one even reach mysql_delete() with SQLCOM_INSERT? not just with returning and !thd->get_stmt_da()->is_set(), anywhere?
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Oleksandr Byelkin -
Sergei Golubchik