Re: [Maria-developers] 256753e8ae8: Clean up and speed up interfaces for binary row logging
Hi, Michael! Just a few comments, see below On Feb 28, Michael Widenius wrote:
revision-id: 256753e8ae8 (mariadb-10.5.0-269-g256753e8ae8) parent(s): 9fd961cf687 author: Michael Widenius <monty@mariadb.com> committer: Michael Widenius <monty@mariadb.com> timestamp: 2020-02-26 16:05:53 +0200 message:
Clean up and speed up interfaces for binary row logging ... diff --git a/mysql-test/suite/rpl/r/create_or_replace_mix.result b/mysql-test/suite/rpl/r/create_or_replace_mix.result index 661278aa7ef..6c83d27eef9 100644 --- a/mysql-test/suite/rpl/r/create_or_replace_mix.result +++ b/mysql-test/suite/rpl/r/create_or_replace_mix.result @@ -223,26 +226,12 @@ Log_name Pos Event_type Server_id End_log_pos Info slave-bin.000001 # Gtid # # GTID #-#-# slave-bin.000001 # Query # # use `test`; create table t1 (a int) slave-bin.000001 # Gtid # # BEGIN GTID #-#-# -slave-bin.000001 # Annotate_rows # # insert into t1 values (0),(1),(2) -slave-bin.000001 # Table_map # # table_id: # (test.t1) -slave-bin.000001 # Write_rows_v1 # # table_id: # flags: STMT_END_F +slave-bin.000001 # Query # # use `test`; insert into t1 values (0),(1),(2)
why is it STATEMENT now?
slave-bin.000001 # Query # # COMMIT -slave-bin.000001 # Gtid # # BEGIN GTID #-#-# -slave-bin.000001 # Query # # use `test`; CREATE TABLE `t2` ( - `a` int(11) DEFAULT NULL -) ENGINE=MyISAM -slave-bin.000001 # Annotate_rows # # create table t2 engine=myisam select * from t1 -slave-bin.000001 # Table_map # # table_id: # (test.t2) -slave-bin.000001 # Write_rows_v1 # # table_id: # flags: STMT_END_F -slave-bin.000001 # Query # # COMMIT -slave-bin.000001 # Gtid # # BEGIN GTID #-#-# -slave-bin.000001 # Query # # use `test`; CREATE OR REPLACE TABLE `t2` ( - `a` int(11) DEFAULT NULL -) ENGINE=InnoDB -slave-bin.000001 # Annotate_rows # # create or replace table t2 engine=innodb select * from t1 -slave-bin.000001 # Table_map # # table_id: # (test.t2) -slave-bin.000001 # Write_rows_v1 # # table_id: # flags: STMT_END_F -slave-bin.000001 # Xid # # COMMIT /* XID */ +slave-bin.000001 # Gtid # # GTID #-#-# +slave-bin.000001 # Query # # use `test`; create table t2 engine=myisam select * from t1 +slave-bin.000001 # Gtid # # GTID #-#-# +slave-bin.000001 # Query # # use `test`; create or replace table t2 engine=innodb select * from t1 connection server_1; drop table t1; # diff --git a/mysql-test/suite/rpl/t/rpl_foreign_key.test b/mysql-test/suite/rpl/t/rpl_foreign_key.test new file mode 100644 index 00000000000..50be97af24d --- /dev/null +++ b/mysql-test/suite/rpl/t/rpl_foreign_key.test @@ -0,0 +1,18 @@ +--source include/have_innodb.inc +--source include/have_binlog_format_row.inc + +CREATE TABLE t1 ( + id INT, + k INT, + c CHAR(8), + KEY (k), + PRIMARY KEY (id), + FOREIGN KEY (id) REFERENCES t1 (k) +) ENGINE=InnoDB; +LOCK TABLES t1 WRITE; +SET SESSION FOREIGN_KEY_CHECKS= OFF; +SET AUTOCOMMIT=OFF; +INSERT INTO t1 VALUES (1,1,'foo'); +DROP TABLE t1; +SET SESSION FOREIGN_KEY_CHECKS= ON; +SET AUTOCOMMIT=ON;
What kind of replication test is it? You don't check binlog events, you don't compare master and slave, you don't even run anything on the slave to check whether your staments were replicated correctly. In fact, you don't have any slave, you have not included master-slave.inc, you only have binlog, so this test should be in the binlog suite, not in the rpl suite - it's a binlog test, not replication test. And even in the binlog suite it would make sense to see what's actually in a binlog. Just as a test that it's ok.
diff --git a/sql/ha_sequence.cc b/sql/ha_sequence.cc index 6cb9937ebb4..71da208d775 100644 --- a/sql/ha_sequence.cc +++ b/sql/ha_sequence.cc @@ -202,7 +202,11 @@ int ha_sequence::write_row(const uchar *buf) DBUG_ENTER("ha_sequence::write_row"); DBUG_ASSERT(table->record[0] == buf);
- row_already_logged= 0; + /* + Log to binary log even if this function has been called before + (The function ends by setting row_logging to 0) + */ + row_logging= row_logging_init;
this is a sequence-specific hack, so you should define row_logging_init in ha_sequence class, not in the base handler class
if (unlikely(sequence->initialized == SEQUENCE::SEQ_IN_PREPARE)) { /* This calls is from ha_open() as part of create table */ diff --git a/sql/handler.cc b/sql/handler.cc index 1e3f987b4e5..4096ae8b90f 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -6224,32 +6225,37 @@ bool ha_show_status(THD *thd, handlerton *db_type, enum ha_stat_type stat) 1 Row needs to be logged */
-bool handler::check_table_binlog_row_based(bool binlog_row) +bool handler::check_table_binlog_row_based() { - if (table->versioned(VERS_TRX_ID)) - return false; - if (unlikely((table->in_use->variables.sql_log_bin_off))) - return 0; /* Called by partitioning engine */ #ifdef WITH_WSREP - if (!table->in_use->variables.sql_log_bin && - wsrep_thd_is_applying(table->in_use)) - return 0; /* wsrep patch sets sql_log_bin to silence binlogging - from high priority threads */ #endif /* WITH_WSREP */
That's an empty #ifdef :)
if (unlikely((!check_table_binlog_row_based_done))) { check_table_binlog_row_based_done= 1; check_table_binlog_row_based_result= - check_table_binlog_row_based_internal(binlog_row); + check_table_binlog_row_based_internal(); } return check_table_binlog_row_based_result; }
-bool handler::check_table_binlog_row_based_internal(bool binlog_row) +bool handler::check_table_binlog_row_based_internal() { THD *thd= table->in_use;
+#ifdef WITH_WSREP + if (!thd->variables.sql_log_bin && + wsrep_thd_is_applying(table->in_use)) + { + /* + wsrep patch sets sql_log_bin to silence binlogging from high + priority threads + */ + return 0; + } +#endif return (table->s->can_do_row_logging && + !table->versioned(VERS_TRX_ID) && + !(thd->variables.option_bits & OPTION_BIN_TMP_LOG_OFF) && thd->is_current_stmt_binlog_format_row() && /* Wsrep partially enables binary logging if it have not been @@ -6769,13 +6718,17 @@ int handler::ha_write_row(const uchar *buf) { error= write_row(buf); })
MYSQL_INSERT_ROW_DONE(error); - if (likely(!error) && !row_already_logged) + if (likely(!error)) { rows_changed++; - error= binlog_log_row(table, 0, buf, log_func); + if (row_logging) + { + Log_func *log_func= Write_rows_log_event::binlog_row_logging_function; + error= binlog_log_row(table, 0, buf, log_func); + } #ifdef WITH_WSREP - if (table_share->tmp_table == NO_TMP_TABLE && - WSREP(ha_thd()) && (error= wsrep_after_row(ha_thd()))) + if (WSREP_NNULL(ha_thd()) && table_share->tmp_table == NO_TMP_TABLE &&
why did you swap tests? NO_TMP_TABLE check is cheaper (same in update and delete)
+ !error && (error= wsrep_after_row(ha_thd()))) { DBUG_RETURN(error); } diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 102416ec0a6..9e40c2ae8c8 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -10506,10 +10506,10 @@ do_continue:; No additional logging of query is needed */ binlog_done= 1; + DBUG_ASSERT(new_table->file->row_logging); new_table->mark_columns_needed_for_insert(); thd->binlog_start_trans_and_stmt(); - binlog_write_table_map(thd, new_table, - thd->variables.binlog_annotate_row_events); + thd->binlog_write_table_map(new_table, 1);
does it mean you force annotations for ALTER TABLE even if they were configured off? And why would ALTER TABLE generate row events anyway?
} if (copy_data_between_tables(thd, table, new_table, alter_info->create_list, ignore,
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi! On Fri, Feb 28, 2020 at 10:00 PM Sergei Golubchik <serg@mariadb.org> wrote: ...
diff --git a/mysql-test/suite/rpl/r/create_or_replace_mix.result b/mysql-test/suite/rpl/r/create_or_replace_mix.result index 661278aa7ef..6c83d27eef9 100644 --- a/mysql-test/suite/rpl/r/create_or_replace_mix.result +++ b/mysql-test/suite/rpl/r/create_or_replace_mix.result @@ -223,26 +226,12 @@ Log_name Pos Event_type Server_id End_log_pos Info slave-bin.000001 # Gtid # # GTID #-#-# slave-bin.000001 # Query # # use `test`; create table t1 (a int) slave-bin.000001 # Gtid # # BEGIN GTID #-#-# -slave-bin.000001 # Annotate_rows # # insert into t1 values (0),(1),(2) -slave-bin.000001 # Table_map # # table_id: # (test.t1) -slave-bin.000001 # Write_rows_v1 # # table_id: # flags: STMT_END_F +slave-bin.000001 # Query # # use `test`; insert into t1 values (0),(1),(2)
why is it STATEMENT now?
" - In the original code, the master could come into a state where row logging is enforced for all future events if statement could be used. This is now partly fixed." The test is run in mix mode. Insert are normally run in statement mode for simple tables like the above. The old code assumed that if you have created a temporary table, then all future statements until the temporary table is dropped will always be in binary logging mode. I have now fixed so that only statements that are actually using the temporary table will use binary logging. <cut>
diff --git a/mysql-test/suite/rpl/t/rpl_foreign_key.test b/mysql-test/suite/rpl/t/rpl_foreign_key.test new file mode 100644 index 00000000000..50be97af24d --- /dev/null +++ b/mysql-test/suite/rpl/t/rpl_foreign_key.test @@ -0,0 +1,18 @@ +--source include/have_innodb.inc +--source include/have_binlog_format_row.inc + +CREATE TABLE t1 ( + id INT, + k INT, + c CHAR(8), + KEY (k), + PRIMARY KEY (id), + FOREIGN KEY (id) REFERENCES t1 (k) +) ENGINE=InnoDB; +LOCK TABLES t1 WRITE; +SET SESSION FOREIGN_KEY_CHECKS= OFF; +SET AUTOCOMMIT=OFF; +INSERT INTO t1 VALUES (1,1,'foo'); +DROP TABLE t1; +SET SESSION FOREIGN_KEY_CHECKS= ON; +SET AUTOCOMMIT=ON;
What kind of replication test is it? You don't check binlog events, you don't compare master and slave, you don't even run anything on the slave to check whether your staments were replicated correctly.
The test case is from mdev-21617. It caused a crash in one version of the code as such. I agree that it would be better to check the result, but as this was never a problem I didn't think about adding it.
In fact, you don't have any slave, you have not included master-slave.inc, you only have binlog, so this test should be in the binlog suite, not in the rpl suite - it's a binlog test, not replication test.
And even in the binlog suite it would make sense to see what's actually in a binlog. Just as a test that it's ok. I have now moved it to binlog and check the binary log, even if it's not really needed for the MDEV.
diff --git a/sql/ha_sequence.cc b/sql/ha_sequence.cc index 6cb9937ebb4..71da208d775 100644 --- a/sql/ha_sequence.cc +++ b/sql/ha_sequence.cc @@ -202,7 +202,11 @@ int ha_sequence::write_row(const uchar *buf) DBUG_ENTER("ha_sequence::write_row"); DBUG_ASSERT(table->record[0] == buf);
- row_already_logged= 0; + /* + Log to binary log even if this function has been called before + (The function ends by setting row_logging to 0) + */ + row_logging= row_logging_init;
this is a sequence-specific hack, so you should define row_logging_init in ha_sequence class, not in the base handler class
This would force me to make prepare_for_row_logging() and ha_reset() virtual or add extra virtual functions that would have to be called for all handlers in both of the above cases. The overhead is much bigger than having two set of the above variable in the two above functions.
index 1e3f987b4e5..4096ae8b90f 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -6224,32 +6225,37 @@ bool ha_show_status(THD *thd, handlerton *db_type, enum ha_stat_type stat) 1 Row needs to be logged */
-bool handler::check_table_binlog_row_based(bool binlog_row) +bool handler::check_table_binlog_row_based() { - if (table->versioned(VERS_TRX_ID)) - return false; - if (unlikely((table->in_use->variables.sql_log_bin_off))) - return 0; /* Called by partitioning engine */ #ifdef WITH_WSREP - if (!table->in_use->variables.sql_log_bin && - wsrep_thd_is_applying(table->in_use)) - return 0; /* wsrep patch sets sql_log_bin to silence binlogging - from high priority threads */ #endif /* WITH_WSREP */
That's an empty #ifdef :)
Strange that I didn't notice that. Fixed!
@@ -6769,13 +6718,17 @@ int handler::ha_write_row(const uchar *buf) { error= write_row(buf); })
MYSQL_INSERT_ROW_DONE(error); - if (likely(!error) && !row_already_logged) + if (likely(!error)) { rows_changed++; - error= binlog_log_row(table, 0, buf, log_func); + if (row_logging) + { + Log_func *log_func= Write_rows_log_event::binlog_row_logging_function; + error= binlog_log_row(table, 0, buf, log_func); + } #ifdef WITH_WSREP - if (table_share->tmp_table == NO_TMP_TABLE && - WSREP(ha_thd()) && (error= wsrep_after_row(ha_thd()))) + if (WSREP_NNULL(ha_thd()) && table_share->tmp_table == NO_TMP_TABLE &&
why did you swap tests? NO_TMP_TABLE check is cheaper (same in update and delete)
Actually, it's more expensive as it's almost almost true while WSREP_NNULL is only true if galera is used. In other words, the new test favors normal server, not galera which I think is right.
+++ b/sql/sql_table.cc @@ -10506,10 +10506,10 @@ do_continue:; No additional logging of query is needed */ binlog_done= 1; + DBUG_ASSERT(new_table->file->row_logging); new_table->mark_columns_needed_for_insert(); thd->binlog_start_trans_and_stmt(); - binlog_write_table_map(thd, new_table, - thd->variables.binlog_annotate_row_events); + thd->binlog_write_table_map(new_table, 1);
does it mean you force annotations for ALTER TABLE even if they were configured off?
No. This means just that he binlog_write_table_map is the first call and it should consider writing an annotate event. The function bool THD::binlog_write_annotated_row(Log_event_writer *writer) will check that variables.binlog_annotate_row_events is true (in addition to other things)
And why would ALTER TABLE generate row events anyway?
ALTER TABLE s3_table engine=InnoDB. As the s3_table is not available on slaves, it has to row logged. Same should happen if one converts a clustrix table to InnoDB. We did discuss this a couple of times... Thanks for the review. All fixed and also rebased on latest 10.5 is now pushed into bb-10.5-monty Regards, Monty
participants (2)
-
Michael Widenius -
Sergei Golubchik