maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Hi Serg,

Here is my review of your MDEV-5115 patch.

First, a general comment:

Another way to fix this would be a smaller patch. It would not attempt to
implement anything related to the new event types. Instead, it would just have
a small bit of code that allows to read the binary format of the new event
types, but it would represent them internally as the old type, just ignoring
any of the extra information, which appears to be currently unused outside of
NDB. Probably then it would also make sense to not implement the renaming of
the old event types. There would just be three extra cases in
Log_event::read_log_event(), and a bit of extra code in the *_rows_log_event
construstructors to be able to ignore any extra info.

However, I do not really have much against your approach of doing a partial
merge to get code that is closer to what is in MySQL, if you prefer that. I
mention it because we discussed briefly already on IRC whether a smaller patch
could be enough.

A few detailed comments below. Otherwise, while I would have probably done the
minimal patch myself, I am ok with this way also. So ok to push after fixing
the few commit and code comments mentioned below.

 - Kristian.

> ------------------------------------------------------------
> revno: 3921
> revision-id: sergii@xxxxxxxxx-20131203192301-afokgpslfq0f3xg8
> parent: sergii@xxxxxxxxx-20131201111624-xe3f1ul6otcyvyom
> fixes bug: https://mariadb.atlassian.net/browse/MDEV-5115
> committer: Sergei Golubchik <sergii@xxxxxxxxx>
> branch nick: 10.0
> timestamp: Tue 2013-12-03 20:23:01 +0100
> message:
>   MDEV-5115 RBR from MySQL 5.6 to MariaDB 10.0 does not work
>   
>   Patially merge WL#5917, to understand v2 row events

"Partially merge WL#5917, which introduces new event types for row-based
binlogging. This is needed to be able to replicate from a >=5.6 MySQL master
to a MariaDB slave."

> === modified file 'mysql-test/suite/binlog/t/binlog_old_versions.test'
> --- mysql-test/suite/binlog/t/binlog_old_versions.test	2013-04-09 21:27:41 +0000
> +++ mysql-test/suite/binlog/t/binlog_old_versions.test	2013-12-03 19:23:01 +0000
> @@ -24,6 +24,17 @@
>  
>  source include/not_embedded.inc;
>  
> +--echo ==== Read binlog with v2 row events ====
> +
> +# Read binlog.
> +--exec $MYSQL_BINLOG --local-load=$MYSQLTEST_VARDIR/tmp/ suite/binlog/std_data/ver_trunk_row_v2.001 | $MYSQL --local-infile=1
> +# Show result.
> +SELECT * FROM t1 ORDER BY a;
> +SELECT * FROM t2 ORDER BY a;
> +SELECT COUNT(*) FROM t3;
> +# Reset.
> +DROP TABLE t1, t2, t3;
> +
>  
>  --echo ==== Read modern binlog (version 5.1.23) ====
>  

Is this the only test we have of reading the v2 row event types?
I think it is not a particularly good test, as it tests mysqlbinlog reading
them, while the interesting thing is a slave receiving them from the
master. Even though some code is shared between the two there are lots of
differences as well.

The better test would be to install the pre-generated binlog file manually on
a master and have the slave replicate it. However, it is easy to end up
spending too much time on setting up these replication tests and getting them
to work reliably on all platforms, so maybe not worth it.

Maybe a good compromise is to instead ask Elena to test it manually against a
real MySQL 5.6 server - after all, that is the actual case we want to work.

> === modified file 'sql/log_event.h'
> --- sql/log_event.h	2013-11-01 11:00:11 +0000
> +++ sql/log_event.h	2013-12-03 19:23:01 +0000

> @@ -659,11 +663,11 @@ enum Log_event_type
>    PRE_GA_DELETE_ROWS_EVENT = 22,
>  
>    /*
> -    These event numbers are used from 5.1.16 and forward
> +    These event numbers are used from 5.1.16 until mysql-trunk-xx

Please change this comment to explain the actual situation. Ie. that they are
used in MariaDB, but not in MySQL >= 5.6.

> @@ -677,6 +681,20 @@ enum Log_event_type
>    HEARTBEAT_LOG_EVENT= 27,
>    
>    /*
> +    In some situations, it is necessary to send over ignorable
> +    data to the slave: data that a slave can handle in case there
> +    is code for handling it, but which can be ignored if it is not
> +    recognized.
> +  */
> +  IGNORABLE_LOG_EVENT= 28,
> +  ROWS_QUERY_LOG_EVENT= 29,

Add a comment that these are MySQL 5.6 events that are unimplemented in
MariaDB.

(Do we need another MDEV to be able to replicate against a MySQL master that
produces ROWS_QUERY_LOG_EVENT? It is quite similar to our ANNOTATE_ROWS_EVENT,
IIRC).

> +  /* Version 2 of the Row events */
> +  WRITE_ROWS_EVENT = 30,
> +  UPDATE_ROWS_EVENT = 31,
> +  DELETE_ROWS_EVENT = 32,

Add comment that these are only generated by MySQL 5.6.