maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> committer: Michael Widenius <monty@xxxxxxxxxxx>
> 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@xxxxxxxxxxx