Re: [Maria-developers] 17548c8a8b6: MDEV-25477 Auto-create breaks replication when triggering event was not replicated
Hi, Aleksey, Basically, it's fine, but I still didn't like that there's a new flag in THD to mark a statement that has to be logged. So I tried to change to log_current_statement. I know you tried that and you told me what tests fails, rpl.create_or_replace_*. I debugged that a bit, and I think the problem is that select_create inherits from select_insert, so CREATE ... SELECT runs your code that is only supposed to be run for INSERT ODKU or REPLACE. This change makes all tests pass (after removing vers_created_partitions): @@ -4225,7 +4225,8 @@ bool select_insert::prepare_eof() thd->clear_error(); else errcode= query_error_code(thd, killed_status == NOT_KILLED); - StatementBinlog stmt_binlog(thd, thd->binlog_need_stmt_format(trans_table)); + StatementBinlog stmt_binlog(thd, !can_rollback_data() && + thd->binlog_need_stmt_format(trans_table)); res= thd->binlog_query(THD::ROW_QUERY_TYPE, thd->query(), thd->query_length(), trans_table, FALSE, FALSE, errcode); On Apr 10, Aleksey Midenkov wrote:
revision-id: 17548c8a8b6 (mariadb-10.6.1-250-g17548c8a8b6) parent(s): 6282e025a0e author: Aleksey Midenkov committer: Aleksey Midenkov timestamp: 2022-03-07 23:49:35 +0300 message:
MDEV-25477 Auto-create breaks replication when triggering event was not replicated
diff --git a/sql/sql_class.h b/sql/sql_class.h index 686e6e70766..4fed1bf7355 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -2914,6 +2914,13 @@ class THD: public THD_count, /* this must be first */ int binlog_flush_pending_rows_event(bool stmt_end, bool is_transactional); int binlog_remove_pending_rows_event(bool clear_maps, bool is_transactional);
+ bool binlog_need_stmt_format(bool is_transactional) const + { + return vers_created_partitions && !binlog_get_pending_rows_event(is_transactional); + } + + bool binlog_for_noop_dml(bool transactional_table); + /** Determine the binlog format of the current statement.
@@ -3557,6 +3564,7 @@ class THD: public THD_count, /* this must be first */ bool tablespace_op; /* This is TRUE in DISCARD/IMPORT TABLESPACE */ /* True if we have to log the current statement */ bool log_current_statement; + uint vers_created_partitions; /** True if a slave error. Causes the slave to stop. Not the same as the statement execution error (is_error()), since diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index 707b8a0d3bf..25551dfa197 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -4223,6 +4225,7 @@ bool select_insert::prepare_eof() thd->clear_error(); else errcode= query_error_code(thd, killed_status == NOT_KILLED); + StatementBinlog stmt_binlog(thd, thd->binlog_need_stmt_format(trans_table)); res= thd->binlog_query(THD::ROW_QUERY_TYPE, thd->query(), thd->query_length(), trans_table, FALSE, FALSE, errcode);
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei! On Tue, Apr 12, 2022 at 12:58 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey,
Basically, it's fine, but I still didn't like that there's a new flag in THD to mark a statement that has to be logged. So I tried to change to log_current_statement. I know you tried that and you told me what tests fails, rpl.create_or_replace_*. I debugged that a bit, and I think the problem is that select_create inherits from select_insert, so CREATE ... SELECT runs your code that is only supposed to be run for INSERT ODKU or REPLACE. This change makes all tests pass (after removing vers_created_partitions):
I attached the patch and this still fails rpl.create_or_replace_row. Why don't you like vers_created_partitions? It might be useful info for debugging and for new features.
@@ -4225,7 +4225,8 @@ bool select_insert::prepare_eof() thd->clear_error(); else errcode= query_error_code(thd, killed_status == NOT_KILLED); - StatementBinlog stmt_binlog(thd, thd->binlog_need_stmt_format(trans_table)); + StatementBinlog stmt_binlog(thd, !can_rollback_data() && + thd->binlog_need_stmt_format(trans_table)); res= thd->binlog_query(THD::ROW_QUERY_TYPE, thd->query(), thd->query_length(), trans_table, FALSE, FALSE, errcode);
On Apr 10, Aleksey Midenkov wrote:
revision-id: 17548c8a8b6 (mariadb-10.6.1-250-g17548c8a8b6) parent(s): 6282e025a0e author: Aleksey Midenkov committer: Aleksey Midenkov timestamp: 2022-03-07 23:49:35 +0300 message:
MDEV-25477 Auto-create breaks replication when triggering event was not replicated
diff --git a/sql/sql_class.h b/sql/sql_class.h index 686e6e70766..4fed1bf7355 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -2914,6 +2914,13 @@ class THD: public THD_count, /* this must be first */ int binlog_flush_pending_rows_event(bool stmt_end, bool is_transactional); int binlog_remove_pending_rows_event(bool clear_maps, bool is_transactional);
+ bool binlog_need_stmt_format(bool is_transactional) const + { + return vers_created_partitions && !binlog_get_pending_rows_event(is_transactional); + } + + bool binlog_for_noop_dml(bool transactional_table); + /** Determine the binlog format of the current statement.
@@ -3557,6 +3564,7 @@ class THD: public THD_count, /* this must be first */ bool tablespace_op; /* This is TRUE in DISCARD/IMPORT TABLESPACE */ /* True if we have to log the current statement */ bool log_current_statement; + uint vers_created_partitions; /** True if a slave error. Causes the slave to stop. Not the same as the statement execution error (is_error()), since diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index 707b8a0d3bf..25551dfa197 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -4223,6 +4225,7 @@ bool select_insert::prepare_eof() thd->clear_error(); else errcode= query_error_code(thd, killed_status == NOT_KILLED); + StatementBinlog stmt_binlog(thd, thd->binlog_need_stmt_format(trans_table)); res= thd->binlog_query(THD::ROW_QUERY_TYPE, thd->query(), thd->query_length(), trans_table, FALSE, FALSE, errcode);
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- @midenok
Hi, Aleksey,
I attached the patch and this still fails rpl.create_or_replace_row.
small mistake in the patch:
--- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -2916,7 +2916,7 @@ class THD: public THD_count, /* this must be first */
bool binlog_need_stmt_format(bool is_transactional) const { - return vers_created_partitions && !binlog_get_pending_rows_event(is_transactional); + return !binlog_get_pending_rows_event(is_transactional);
if you put here log_current_statement && !binlog_get_pending_rows_event(is_transactional) then all tests pass.
}
bool binlog_for_noop_dml(bool transactional_table);
Why don't you like vers_created_partitions? It might be useful info for debugging and for new features.
I have a second patch that renames flags to have a very well-defined meaning and name. OPTION_KEEP_LOG -> OPTION_BINLOG_THIS_TRX log_current_statement -> OPTION_BINLOG_THIS_STMT the first flag means that when a transaction is rolled back, it's written to binlog with ROLLBACK at the end, not discarded. The second means that the statement is written to binlog (or binlog trx cache) even if it otherwise wouldn't be. So, two flags. Clear names and semantics. "vers_created_partitions" fits into the second use case, so one should use OPTION_BINLOG_THIS_STMT for it. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei, On Sun, Apr 17, 2022 at 8:48 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey,
I attached the patch and this still fails rpl.create_or_replace_row.
small mistake in the patch:
--- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -2916,7 +2916,7 @@ class THD: public THD_count, /* this must be first */
bool binlog_need_stmt_format(bool is_transactional) const { - return vers_created_partitions && !binlog_get_pending_rows_event(is_transactional); + return !binlog_get_pending_rows_event(is_transactional);
if you put here
log_current_statement && !binlog_get_pending_rows_event(is_transactional)
then all tests pass.
}
bool binlog_for_noop_dml(bool transactional_table);
Why don't you like vers_created_partitions? It might be useful info for debugging and for new features.
I have a second patch that renames flags to have a very well-defined meaning and name.
OPTION_KEEP_LOG -> OPTION_BINLOG_THIS_TRX log_current_statement -> OPTION_BINLOG_THIS_STMT
Where is your patch?
the first flag means that when a transaction is rolled back, it's written to binlog with ROLLBACK at the end, not discarded.
The second means that the statement is written to binlog (or binlog trx cache) even if it otherwise wouldn't be.
So, two flags. Clear names and semantics. "vers_created_partitions" fits into the second use case, so one should use OPTION_BINLOG_THIS_STMT for it.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- @midenok
Hi, Aleksey, On Apr 18, Aleksey Midenkov wrote:
I have a second patch that renames flags to have a very well-defined meaning and name.
OPTION_KEEP_LOG -> OPTION_BINLOG_THIS_TRX log_current_statement -> OPTION_BINLOG_THIS_STMT
Where is your patch?
It's really just renaming, almost nothing else. But here it is, attached. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei! On Tue, Apr 19, 2022 at 11:13 AM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey,
On Apr 18, Aleksey Midenkov wrote:
I have a second patch that renames flags to have a very well-defined meaning and name.
OPTION_KEEP_LOG -> OPTION_BINLOG_THIS_TRX log_current_statement -> OPTION_BINLOG_THIS_STMT
Where is your patch?
It's really just renaming, almost nothing else. But here it is, attached.
I'd keep log_current_statement() like this: --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -3562,4 +3562,8 @@ class THD: public THD_count, /* this must be first */ /* set during loop of derived table processing */ bool derived_tables_processing; bool tablespace_op; /* This is TRUE in DISCARD/IMPORT TABLESPACE */ + bool log_current_statement() const + { + return variables.option_bits & OPTION_BINLOG_THIS_STMT; + } /**
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- @midenok
Hi, Aleksey, Okay, sure. I was going to push it after the release. And in 10.3 to simplify future merges. On Apr 20, Aleksey Midenkov wrote:
Hi Sergei!
On Tue, Apr 19, 2022 at 11:13 AM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey,
On Apr 18, Aleksey Midenkov wrote:
I have a second patch that renames flags to have a very well-defined meaning and name.
OPTION_KEEP_LOG -> OPTION_BINLOG_THIS_TRX log_current_statement -> OPTION_BINLOG_THIS_STMT
Where is your patch?
It's really just renaming, almost nothing else. But here it is, attached.
I'd keep log_current_statement() like this:
--- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -3562,4 +3562,8 @@ class THD: public THD_count, /* this must be first */ /* set during loop of derived table processing */ bool derived_tables_processing; bool tablespace_op; /* This is TRUE in DISCARD/IMPORT TABLESPACE */ + bool log_current_statement() const + { + return variables.option_bits & OPTION_BINLOG_THIS_STMT; + } /**
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Sergei, I updated the branch accordingly. There is your patch fa444975d0f where I added log_current_statement(). On Wed, Apr 20, 2022 at 1:12 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey,
Okay, sure. I was going to push it after the release. And in 10.3 to simplify future merges.
On Apr 20, Aleksey Midenkov wrote:
Hi Sergei!
On Tue, Apr 19, 2022 at 11:13 AM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey,
On Apr 18, Aleksey Midenkov wrote:
I have a second patch that renames flags to have a very well-defined meaning and name.
OPTION_KEEP_LOG -> OPTION_BINLOG_THIS_TRX log_current_statement -> OPTION_BINLOG_THIS_STMT
Where is your patch?
It's really just renaming, almost nothing else. But here it is, attached.
I'd keep log_current_statement() like this:
--- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -3562,4 +3562,8 @@ class THD: public THD_count, /* this must be first */ /* set during loop of derived table processing */ bool derived_tables_processing; bool tablespace_op; /* This is TRUE in DISCARD/IMPORT TABLESPACE */ + bool log_current_statement() const + { + return variables.option_bits & OPTION_BINLOG_THIS_STMT; + } /**
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- @midenok
Hi, Aleksey, ok to push, thanks! On Apr 20, Aleksey Midenkov wrote:
Sergei,
I updated the branch accordingly. There is your patch fa444975d0f where I added log_current_statement().
On Wed, Apr 20, 2022 at 1:12 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey,
Okay, sure. I was going to push it after the release. And in 10.3 to simplify future merges.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Aleksey Midenkov -
Sergei Golubchik