Re: [Maria-developers] 0402f6dcb67: MDEV-25477 Auto-create breaks replication when triggering event was not replicated
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. 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.
+ 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?
/* [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
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
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.
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 hate it that there're so many ways to force binlogging a statement, and they're all, of course, are subtly different.
+ my_ok(thd, 0); DBUG_RETURN(0); }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
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
participants (2)
-
Aleksey Midenkov -
Sergei Golubchik