Hi Sergei! Updated branch preview-10.8-MDEV-17554-auto-create-partition On Sat, Feb 12, 2022 at 2:31 AM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey,
This was quite good, thanks!
Many cases covered, not just ROLLBACK, but also LIMIT 0 and impossible WHERE.
See a couple of comments below. Nothing big.
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.
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
+ my_ok(thd, 0); DBUG_RETURN(0); } @@ -932,8 +958,8 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, else errcode= query_error_code(thd, killed_status == NOT_KILLED);
- ScopedStatementReplication scoped_stmt_rpl( - table->versioned(VERS_TRX_ID) ? thd : NULL); + StatementBinlog stmt_binlog(thd, table->versioned(VERS_TRX_ID) || + thd->binlog_need_stmt_format(transactional_table));
I know that's not part of this patch, but still. Just trying to understand. Why did it change to statement-based if VERS_TRX_ID?
Because we don't need replicated TRX_ID as they are different on slave. The related commit is d998da03069 and the issue is https://github.com/tempesta-tech/mariadb/issues/234
/* [binlog]: If 'handler::delete_all_rows()' was called and the storage engine does not inject the rows itself, we replicate
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- @midenok