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