Hi Sergei, On Wed, Mar 2, 2022 at 10:33 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey,
On Feb 15, Aleksey Midenkov wrote:
On Feb 12, Aleksey Midenkov wrote:
commit 0402f6dcb67 Author: Aleksey Midenkov <midenok@gmail.com> Date: Tue Feb 8 22:54:03 2022 +0300
diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc index 0403d6e73c3..06068290247 100644 --- a/sql/sql_delete.cc +++ b/sql/sql_delete.cc @@ -507,6 +508,18 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, if (thd->lex->describe || thd->lex->analyze_stmt) goto produce_explain_and_leave;
+ if (thd->log_current_statement) + { + thd->reset_unsafe_warnings(); + if (thd->binlog_query(THD::STMT_QUERY_TYPE, + thd->query(), thd->query_length(), + transactional_table, FALSE, FALSE, 0) > 0) + { + my_error(ER_ERROR_ON_WRITE, MYF(0), "binary log", -1); + DBUG_RETURN(1); + } + }
perhaps, move this block into a function? It's repeated four times here.
Moved.
Thanks! The new method, binlog_for_noop_dml() looks very puzzling.
I understand what it does, for now, but somebody who wasn't involved in this bugfixing, likely won't have any idea. The name is correct, I agree, it says what, but it doesn't explain why.
Could you add a comment there? To say that "noop dml" is "an insert(odku)/update/delete that doesn't change the table" and that it is normally not logged, but needs to be logged if it auto-created a partition as a side effect.
Added.
by the way, you don't seem to have a test for a zero-rows insert. That is INSERT ... SELECT with an impossible where or limit 0.
In fact, you can have a zero-row insert with a normal insert INSERT t1 VALUES (1); if this (1) is a unique key violation. I believe your code handles this case, thought, just a test case is missing.
INSERT does not trigger history. But INSERT .. ODKU does. Added test cases for INSERT .. ODKU and INSERT .. SELECT .. ODKU. That required to add THD::vers_created_partitions as log_current_statement is also used in CREATE TABLE (and select_insert::prepare_eof() is used for CREATE TABLE .. SELECT) and that broke some replication tests:
rpl.create_or_replace_mix rpl.create_or_replace_row rpl.create_or_replace_statement
Indeed, I see.
Can you use OPTION_KEEP_LOG instead? Or transaction->stmt.modified_non_trans_table ?
I don't think it is a good idea to intermix this semantic into these variables/flags. log_current_statement was designed specifically for this.
I hate it that there're so many ways to force binlogging a statement, and they're all, of course, are subtly different.
That is pretty normal. It is the business logic that is complex and therefore its realization should be complex too. Compressing many semantics into single control variables only makes development harder.
+ my_ok(thd, 0); DBUG_RETURN(0); }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- @midenok