Hi Luís,
Wow, great work! You reworked it quite a bit. The test logic is *much*
easier to understand now.
In this email, I first review rpl_row_img.test and files that it uses.
Then I reply to your previous email.
In rpl_chained_3_hosts_sync.inc, there's an extra "save_master_pos". I
suggest to remove this.
rpl_row_img_diff_keys.inc:
> -- echo ### Master with Key, slaves with no PKs on different fields
Remove the word 'no' (cases $i=9, $i=10).
In rpl_row_img_blobs.test, the comments in the beginning of each case
are not really understandable: see comments below
rpl_row_img_blobs.test:
> #
> # The comments below (on create table) must be read with the SQL
> # instructions issued later in mind. Declaring a table obviously is
> # not enough to assert anything.
> #
> # Also, the tests in this file make more sense when performed with
> # binlog_row_image configured as NOBLOB.
> #
>
> if (`SELECT $i=1`) {
> -- echo ### R1: replicate blobs always because they are part of PK
This probably refers to R1 in WL#5096. So the comment is not
self-contained. (There are also four different R1 in the WL.) Better to
state explicitly what we test. The comment in case $i=3 is good - can we
use a similar comment for case $i=2 and $i=1 too?
> -- connection mysqld_a
> --eval CREATE TABLE t (c1 int, c2 blob, c3 int, primary key(c2(512))) engine=
> $mysqld_a_engine;
> -- connection mysqld_b
> --eval CREATE TABLE t (c1 int, c2 blob, c3 int, primary key(c2(512))) engine=
> $mysqld_b_engine;
> -- connection mysqld_c
> --eval CREATE TABLE t (c1 int, c2 blob, c3 int, primary key(c2(512))) engine=
> $mysqld_c_engine;
> }
> if (`SELECT $i=2`)
> {
> -- echo ### Asserts that R1 does not break replication
See case $i=1
> -- connection mysqld_a
> --eval CREATE TABLE t (c1 int, c2 blob NOT NULL, c3 int, unique key(c2(512)))
> engine= $mysqld_a_engine;
> -- connection mysqld_b
> --eval CREATE TABLE t (c1 int, c2 blob NOT NULL, c3 int, unique key(c2(512)))
> engine= $mysqld_b_engine;
> -- connection mysqld_c
> --eval CREATE TABLE t (c1 int, c2 blob NOT NULL, c3 int, unique key(c2(512)))
> engine= $mysqld_c_engine;
> }
> if (`SELECT $i=3`)
> {
> -- echo ### Asserts that declaring a blob in a key does not break replication
Good.
> -- connection mysqld_a
> --eval CREATE TABLE t (c1 int, c2 blob, c3 int, key(c2(512))) engine=
> $mysqld_a_engine;
> -- connection mysqld_b
> --eval CREATE TABLE t (c1 int, c2 blob, c3 int, key(c2(512))) engine=
> $mysqld_b_engine;
> -- connection mysqld_c
> --eval CREATE TABLE t (c1 int, c2 blob, c3 int, key(c2(512))) engine=
> $mysqld_c_engine;
>
> }
> if (`SELECT $i=4`) {
> -- echo ### This asserts that R2 does not break replication
This comment too is not self-contained. But does this test really cover
anything that the other cases don't cover? I think a blob outside a PK
is covered by $i=5.
> -- connection mysqld_a
> --eval CREATE TABLE t (c1 int, c2 blob, c3 int, primary key(c1)) engine=
> $mysqld_a_engine;
> -- connection mysqld_b
> --eval CREATE TABLE t (c1 int, c2 blob, c3 int, primary key(c1)) engine=
> $mysqld_b_engine;
> -- connection mysqld_c
> --eval CREATE TABLE t (c1 int, c2 blob, c3 int, primary key(c1)) engine=
> $mysqld_c_engine;
>
> }
> if (`SELECT $i=5`)
> {
> -- echo ### Asserts R4 - when using NOBLOB c2 shall not be replicated if it is
> not updated
We don't check that c2 is not replicated (the test would succeed even if
c2 was replicated), so this comment is wrong. What I think we do test is
something like "replication works when a blob column is left out due to
the setting binlog_row_image=NOBLOB." Same thing for case $i=6 and $i=7.
> -- connection mysqld_a
> --eval CREATE TABLE t (c1 int NOT NULL, c2 blob NOT NULL, c3 int, unique key(c1))
> engine= $mysqld_a_engine;
> -- connection mysqld_b
> --eval CREATE TABLE t (c1 int NOT NULL, c2 blob NOT NULL, c3 int, unique key(c1))
> engine= $mysqld_b_engine;
> -- connection mysqld_c
> --eval CREATE TABLE t (c1 int NOT NULL, c2 blob NOT NULL, c3 int, unique key(c1))
> engine= $mysqld_c_engine;
>
> }
> if (`SELECT $i=6`)
> {
> -- echo ### Asserts R4 - when using NOBLOB c2 shall not be replicated if it is
> not updated/written
See above.
> -- connection mysqld_a
> --eval CREATE TABLE t (c1 int, c2 blob, c3 int, key(c1)) engine=
> $mysqld_a_engine;
> -- connection mysqld_b
> --eval CREATE TABLE t (c1 int, c2 blob, c3 int, key(c1)) engine=
> $mysqld_b_engine;
> -- connection mysqld_c
> --eval CREATE TABLE t (c1 int, c2 blob, c3 int, key(c1)) engine=
> $mysqld_c_engine;
>
> }
> if (`SELECT $i=7`)
> {
> -- echo ### Asserts R4 - when using NOBLOB c2 shall not be replicated if it is
> not updated/written
See above.
> -- connection mysqld_a
> --eval CREATE TABLE t (c1 int, c2 blob, c3 int) engine= $mysqld_a_engine;
> -- connection mysqld_b
> --eval CREATE TABLE t (c1 int, c2 blob, c3 int) engine= $mysqld_b_engine;
> -- connection mysqld_c
> --eval CREATE TABLE t (c1 int, c2 blob, c3 int) engine= $mysqld_c_engine;
> }
Luís Soares wrote:
[...]
>> (S19) rpl_row_img_set.inc is a generic file used for the chained 3 hosts
>> setup - maybe it should be named rpl_chained_3_hosts_set_img.inc or
>> something like that? Also, what's the logic of the division into
>> mysql-test/extra/rpl_tests vs mysql-test/suite/rpl/include ? Can't we
>> have all files in one place?
>
> rpl_row_img_set.inc is now usable in all scenarios.
> I usually reason like this:
> - accessories/auxiliar stuff put in mysql-test/suite/rpl/include
> - more strictly related to testing (assertion wise) put in
> mysql-test/extra/rpl_tests and mysql-test/suite/rpl/t
This doesn't sound like a bad principle. But it doesn't seem to work
this way for existing files. suite/rpl/include is quite new. Before we
had this, we used include/ for generic auxiliary files
(master-slave.inc, wait_for_slave_* etc) and extra/rpl_tests/ for
test-specific include files with assertions etc. Also, e.g.,
mysql-test/suite/rpl/include/rpl_mixed_dml.inc isn't a generic auxiliary
file, but more like a test with assertions.
[...]
>>> -- perl END_OF_FILE
>>>
>> (S17) I suggest to add this to the top of rpl_row_mysqlbinlog_img_check.inc:
>>
>> -- sync_slave_with_master
>> -- let $MYSQLD_DATADIR= `select @@datadir;`
>> -- exec $MYSQL_BINLOG -v $MYSQLD_DATADIR/slave-bin.000001 > $TMP_FILE
>>
>> Then these commands can be eliminated from the rpl_row_img_sanity.test,
>> where they are repeated a lot.
>
> Done. However, I must note that now mysqlbinlog is called on every
> rpl_row_img_parts_assertion.inc include. I think that's probably one
> thing we can't escape if we want to reduce the scope of the pattern
> matching (I provide details later on in this email).
I think that's ok. Correctness wins over optimization :-)
[...]
>> # Save IMG_EXPECTED in $img and check if it has correct format
>> my $img = $ENV{'IMG_EXPECTED'};
>> $img =~ /^([0-9]+:\S+ )*\|( [0-9]+:\S+)*$/
>> or die "Invalid format of IMG_EXPECTED parameter";
>> # Turn $img into the format of the binlog, and get BI and AI
>> $img =~ s/ *([0-9]+):(\S*) */### \@$1=$2\n/g;
>> my ($ai, $bi)= split(/\|/, $img);
>
> I reversed ($ai and $bi) here. Makes it more intuitive while
> reading/writing the expected values before the include...
> Any special reason for you to put it $ai, $bi ?
It was a mistake. I probably just put them in alphabetic order :-)
Thanks for correcting it.
[...]
>> (S16) I suggest to add "RESET MASTER" to the bottom of
>> rpl_row_mysqlbinlog_img_check.inc. See comment (S16) in
>> rpl_row_img_sanity.test, below.
>
> I think it's better to take other approach. Let this script
> take the start position of the binary log for the event one
> is scanning. (If we have the need we can later had the stop
> position as well).
Sounds good too.
[...]
--
Sven Sandberg, Software Engineer
MySQL AB, www.mysql.com
