Hi Sven,
On Tue, 2010-03-30 at 14:45 +0200, Sven Sandberg wrote:
> Hi Luís,
>
> Wow, great work! You reworked it quite a bit. The test logic is *much*
> easier to understand now.
Thank you. There's a lot of your work in it as well.
> In this email, I first review rpl_row_img.test and files that it uses.
> Then I reply to your previous email.
>
OK.
> In rpl_chained_3_hosts_sync.inc, there's an extra "save_master_pos". I
> suggest to remove this.
>
Removed.
> rpl_row_img_diff_keys.inc:
I think you meant: rpl_row_img_diff_indexes.inc
> > -- echo ### Master with Key, slaves with no PKs on different fields
>
> Remove the word 'no' (cases $i=9, $i=10).
Done.
>
> 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?
Fixed.
> > -- 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
Fixed.
> > -- 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.
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.
Yes, it does. In this case we have an explicit PK, while on $i==5
we have an implicit PK (UK NOT NULL). I want to keep this.
> > -- 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.
Fixed.
> > -- 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.
Fixed.
> > -- 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.
Fixed.
> > -- 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.
>
Moved everything, related to WL#5096, under suite/rpl/include
to include/ as we had agreed.
> [...]
> >>> -- 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 :-)
Good.
> [...]
> >> # 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.
No problem.
> [...]
> >> (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.
Great!
Cset: http://lists.mysql.com/commits/104642
Regards,
Luís
