Hi Luís,
This is a *partial* review of your worklog. I haven't had time to look
at everything, but since I'm away Thu and Fri, I send it now to make the
work more pipelined. If you prefer to have the whole review in one
email, let me know and I'll just extend on this when I continue with it
next week.
Btw, thanks for the improvements, I think it became much easier to
follow the logic now.
(S11) Test the default value of binlog_row_image (see comment inline)
(S12) Move diff_tables2.inc to diff_tables.inc (see comment inline)
(S13) Remove duplicate test rpl_record_find.test (see comment inline)
(S14) Refactor rpl_row_img_sanity_set_img.test (see comment inline)
(S15) Please don't refer to review comments in test cases. The comments
should be self-contained and just describe the current version of the
test. How it worked in previous versions of the tests and how it changed
after review comments will not be interesting for anyone else than you
and me or at any other point in time than now. (see comment inline)
(S16) Ensure that the matching part of the output from mysqlbinlog
really belongs to the previous statement in
rpl_row_mysqlbinlog_img_check.inc. (See comments inline.)
(S17) Move code repeated in rpl_row_img_sanity.test into
rpl_row_mysqlbinlog_img_check.inc to avoid code repetition. (see comment
inline)
(S18) Add comments to the top of all files (see comments inline)
(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?
(S20) Suggested simplification (?) of rpl_row_mysqlbinlog_img_check.inc
(see comments inline)
Luís Soares wrote:
>> Suggestions:
>>
>> (S1) Please state for each test (1) what server property is tested; (2)
>> how is it tested (see comments inline)
>>
>> (S2) I think a little refactoring of rpl_row_img_*.test would make the
>> test even more transparent (see comments inline)
>>
>> (S3) I refuse to review rpl_row_sanity :-) Seriously, I'm impressed that
>> you could write something that looks so complex, but I think it is
>> possible to refactor it to make it more easy to decipher. See comments
>> inline.
>
> Ok.
>
>> (S4) I think some tests are missing. In particular, can you add tests
>> for the following:
>>
>> (S4-1) In rpl_row_sanity, please make sure all cases listed in the
>> tables in WL#5072 are tested. Currently, it misses e.g., the autoinc case.
>
> Hmm... OK. WL#5092 does not change anything regarding autoincs, but we
> should check them anyway. Done.
>
>> (S4-2) Test that a slave with missing columns does not write columns
>> that it does not have
>
> OK.
>
>> (S4-3) Test that if master has binlog_row_image=minimal and slave has
>> binlog_row_image=noblob|full, then slave logs the correct columns.
>
> OK.
>
>> (S4-4) Test that an error is generated when the slave does not have
>> the columns included in the AI.
>
> OK.
>
>> (S4-5) Test that correct default values are inserted in extra slave
>> columns.
>
> OK
>
>> (S4-6) Test that extra master columns are correctly ignored by the
>> slave as long as there is at least one usable column on the slave. E.g.:
>>
>> master> CREATE TABLE t1 (a INT NOT NULL, b INT, c INT
>> PRIMARY KEY (a,c), UNIQUE KEY (a));
>> slave> CREATE TABLE t1 (a INT NOT NULL, b INT,
>> UNIQUE KEY (a));
>>
>> AIUI, the above should always replicate correctly even with
>> binlog_row_image=minimal?
>
> Yes, it should. The slave will receive a BI with data for columns
> 'a' and 'c'. Since it knows nothing about 'c' it will use the available
> data for 'a' so that the unique index is used. I have added a case
> to cover this.
>
>> (S4-7) Test that the default value for binlog_row_image is 'full'.
>
> Hmmm.... I believe that this was already covered implicitly in the
> sanity test. However, I've made it explicit.
(S11) I meant that the test should do:
SELECT @@global.binlog_row_image;
I couldn't see this in the new patch.
>
>> (S5) Instead of the connect/disconnect hack in
>> rpl_chained_3_hosts_diff_tables.inc , could you make
>> include/diff_tables.inc capable of diffing tables in arbitrary
>> connections? I think diff_tables.inc should also reset the original
>> connection at the end. (Ah, finally! I've been waiting for a chance to
>> scratch this itch ever since BUG#35701 was fixed :-)
>
> I can, and I have. However, I would prefer to create diff_tables2.inc,
> so that we don't cause much disruption with what is currently accepted
> among developers wrt diff_tables.inc.
(S12) All use cases of the old version are covered by the new version,
so I don't think there is any need to fork. For test cases, we don't
need to maintain backward compatibility, we can just make it work in the
best possible way. Could you remove the old test case and rename the new
one to diff_tables.inc?
>
>> (S6) Some tests seem redundant and/or it is unclear what is being
>> tested. See comments inline.
>
> OK.
>
>> (S7) Possible typo, see comments inline.
>
> OK.
>
>> (S8) Remove DELAYED from INSERT DELAYED (see comments inline)
>
> No, I don't want to remove it. I'll explain inline.
OK
>
>> (S9) It looks like a lot of code is duplicated between
>> rpl_record_find.test and rpl_row_img.test. Isn't rpl_record_find.test a
>> special case of rpl_row_img.test, only with one server less? If not,
>> please explain!
>
> rpl_record_find.test was part of BUG#40045 which I had to backport. As
> such, I want to keep it.
(S13) AFAICS all that we test for BUG#40045 in rpl_record_find.test is
also tested in rpl_row_img.test. I think there is too much duplication
between the tests to keep both. If rpl_record_find.test has anything
that rpl_row_img.test does not have, can it be merged to rpl_row_img.test?
rpl_row_img_sanity_set_img:
> -- let $old_con= $CURRENT_CONNECTION
>
> if (`SELECT $iteration = 1`)
> {
> -- connection master
> -- echo ON MASTER
> # global so that flush tables restarts the
> # insert dealyed thread
> SET GLOBAL binlog_row_image='FULL';
> SET SESSION binlog_row_image='FULL';
> SHOW VARIABLES LIKE 'binlog_row_image';
> FLUSH TABLES;
>
> -- connection slave
> -- echo ON SLAVE
> # global so that SQL thread also gets the change
> SET GLOBAL binlog_row_image='FULL';
> SET SESSION binlog_row_image='FULL';
> SHOW VARIABLES LIKE 'binlog_row_image';
> }
[...]
(S14) The code above is duplicated three times. I would suggest to refactor:
if (`SELECT $iteration = 1`) {
--set $img_type= 'FULL'
}
if (`SELECT $iteration = 2`) {
--set $img_type= 'NOBLOB'
}
if (`SELECT $iteration = 3`) {
--set $img_type= 'MINIMAL'
}
[...]
--eval SET GLOBAL binlog_row_image= $img_type
etc. Also, this file is only used once. Can't it be copied to the place
where it's used? It is slightly confusing since this file does almost
exactly the same as rpl_row_img_set.inc
rpl_row_img_set.inc:
> -- connection mysqld_a
(S18) Please add a (short) description of what this file does and how to
use it.
> -- eval SET SESSION binlog_row_image= $row_image_param
> -- eval SET GLOBAL binlog_row_image= $row_image_param
> FLUSH TABLES;
> SHOW VARIABLES LIKE 'binlog_row_image';
>
> -- connection mysqld_b
> -- eval SET SESSION binlog_row_image= $row_image_param
> -- eval SET GLOBAL binlog_row_image= $row_image_param
> -- source include/stop_slave.inc
> -- source include/start_slave.inc
> FLUSH TABLES;
> SHOW VARIABLES LIKE 'binlog_row_image';
>
> -- connection mysqld_c
> -- eval SET SESSION binlog_row_image= $row_image_param
> -- eval SET GLOBAL binlog_row_image= $row_image_param
> -- source include/stop_slave.inc
> -- source include/start_slave.inc
> FLUSH TABLES;
> SHOW VARIABLES LIKE 'binlog_row_image';
>
> -- connection mysqld_a
>
rpl_row_mysqlbinlog_img_check.inc:
> -- 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.
(S18) Please also add a comment to the top of the file, explaining what
it does and how to use it.
> sub match_imgs
> {
> my ($contents, $ai_ref, $bi_ref, $stmt_type) = @_;
>
> my $match= 0;
> my $error= 0;
>
> %ai= %{$ai_ref};
> %bi= %{$bi_ref};
>
> my $composed_bi= "";
> for my $k (sort (keys(%bi)))
> {
> $composed_bi = $composed_bi . "### \@$k=" . $bi{$k} . "\n";
> }
>
> my $composed_ai= "";
> for my $k (sort (keys(%ai)))
> {
> $composed_ai = $composed_ai . "### \@$k=" . $ai{$k} . "\n";
> }
>
> if($stmt_type=~/INSERT/)
> {
> $match= ($contents =~ /### INSERT.*\n### SET\n$composed_ai# at/m) ? 1 : 0;
> }
> elsif ($stmt_type=~/UPDATE/)
> {
> $match= ($contents =~ /### UPDATE.*\n### WHERE\n$composed_bi###
> SET\n$composed_ai# at/m) ? 1 : 0;
> }
> elsif ($stmt_type=~/DELETE/)
> {
> $match= ($contents =~ /### DELETE.*\n### WHERE\n$composed_bi# at/m) ? 1 : 0;
> }
> else
> {
> $error= 1;
> }
(S20) This is mostly a matter of style, but here is a version that takes
a shorter syntax as input:
# 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);
# Generate regular expression
if ($ai) {
if ($bi) {
$pattern= "### UPDATE.*\n### WHERE\n$bi### SET\n$ai";
} else {
$pattern= "### INSERT.*\n### SET\n$ai";
}
} else {
$pattern= "### DELETE.*\n### WHERE\n$bi";
}
$match= ($contents =~ /$pattern/);
The point is, we could abbreviate this:
-- let IMG_STMT_TYPE=UPDATE
-- let IMG_BI_EXPECTED={1=>1, 3=>"'foo'"}
-- let IMG_AI_EXPECTED={2=>4}
... to this:
-- let IMG_EXPECTED=1:1 3:'foo' | 2:4
... which is more brief.
Hmm, I don't know if this is better or anything, use whatever version
you prefer. :-)
>
> if ($error)
> {
> print "Unexpected statement type: $stmt_type.\n"
> }
>
> if (!$match)
> {
> print "Unexpected columns in $stmt_type.\n";
> print "EXPECTED BEFORE IMAGE:\n$composed_bi\nEXPECTED AFTER
> IMAGE:\n$composed_ai\nBINLOG CONTENTS:" . $contents;
> }
>
> return $match;
> }
>
>
> my $ai;
> my $bi;
>
> if (not defined $ENV{'IMG_AI_EXPECTED'})
> {
> $ai= {};
> }
> else
> {
> $ai= eval ($ENV{'IMG_AI_EXPECTED'}) or die("Invalid AI parameter: ".
> $ENV{'IMG_AI_EXPECTED'});
> }
>
> if (not defined $ENV{'IMG_BI_EXPECTED'})
> {
> $bi= {};
> }
> else
> {
> $bi= eval ($ENV{'IMG_BI_EXPECTED'}) or die("Invalid BI parameter: ".
> $ENV{'IMG_BI_EXPECTED'});
> }
>
> my $binlog= $ENV{'IMG_BINLOG_FILE'};
> my $stmt_type= $ENV{'IMG_STMT_TYPE'};
>
> open(FILE, "$binlog") or die("Unable to open $binlog: $!\n");
> my $contents = do { local $/; <FILE> };
> close(FILE) or die("Unable to close file.");
>
> match_imgs($contents, $ai, $bi, $stmt_type);
>
> END_OF_FILE
(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.
rpl_row_img_sanity.test:
> #
> # Description
> # ===========
> #
> # This test case checks whether binlog files contain
> # Before and After image values as expected.
> #
> # Configuration is done using the --binlog-row-image
> # variable.
> #
> # How it works
> # ============
> #
> # The test case is implemented such that master and slave
> # create a table with different types of keys. Then, deletes
> # and updates are performed in the master. The resulting
> # output of mysqlbinlog -v issued on the binlogs generated
> # is then searched for the pattern expected wrt before and
> # after images.
> #
> # Some special cases are also considered. A small description
> # for each one is provided inline.
> #
> # See also WL#5096.
> #
> # NOTE: cases marked with S#(-#) address review comments in:
> # http://lists.mysql.com/commits/89844
(S15) Please don't refer to review comments. Instead, state explicitly
what properties we test.
>
> -- source include/master-slave.inc
> -- source include/have_binlog_format_row.inc
>
> -- connection slave
> call mtr.add_suppression("Slave: Can\'t find record in \'t\' Error_code: 1032");
> -- connection master
>
> -- let $TMP_FILE= $MYSQLTEST_VARDIR/tmp/rpl_row_img_sanity.tmp
> -- let IMG_BINLOG_FILE= $TMP_FILE
>
> -- source include/master-slave-reset.inc
> -- connection master
>
> -- echo #####################################################
> -- echo # (S4-7) basic assertion that binlog_row_image='FULL' is the
> -- echo ######################################################
(S15) Please avoid the reference to (S4-7), it's not understandable
unless you read the review email. Better to describe what is tested.
>
> CREATE TABLE t (c1 int, c2 int, c3 blob, primary key(c1));
> INSERT INTO t(c1,c3) VALUES (1, 'a');
> UPDATE t SET c1=2 WHERE c1=1;
> DELETE FROM t;
>
> -- sync_slave_with_master
>
> -- let $MYSQLD_DATADIR= `select @@datadir;`
> -- exec $MYSQL_BINLOG -v $MYSQLD_DATADIR/slave-bin.000001 > $TMP_FILE
>
> -- let IMG_STMT_TYPE=INSERT
> -- let IMG_AI_EXPECTED={1=>1, 2=>NULL, 3=>"\'a\'"}
(Just a style comment: single quotes withing double quotes don't need to
be escaped in perl. We can just write 3=>"'a'".)
> -- source suite/rpl/include/rpl_row_mysqbinlog_img_check.inc
(S16) Note that the test currently does not check *which* statement
generated the INSERT in the binlog. If we execute
rpl_row_mysqlbinlog_img_check.inc after we have executed N statements,
then it will report succes if any of the N statements matches.
I suggest that we instead execute rpl_row_mysqlbinlog_img_check.inc
immediately after the INSERT statement, and run RESET MASTER at the end
of rpl_row_mysqlbinlog_img_check.inc. Then we are sure that the matching
output from mysqlbinlog really belongs to the last INSERT statement and
not to any other INSERT statement further up in the file. See also
comment (S16) in rpl_row_mysqlbinlog_img_check.inc, above.
>
> -- let IMG_STMT_TYPE=UPDATE
> -- let IMG_BI_EXPECTED={1=>1, 2=>NULL, 3=>"\'a\'"}
> -- let IMG_AI_EXPECTED={1=>2, 2=>NULL, 3=>"\'a\'"}
> -- source suite/rpl/include/rpl_row_mysqbinlog_img_check.inc
>
> -- let IMG_STMT_TYPE=DELETE
> -- let IMG_BI_EXPECTED={1=>2, 2=>NULL, 3=>"\'a\'"}
> -- source suite/rpl/include/rpl_row_mysqbinlog_img_check.inc
>
[...]
--
Sven Sandberg, Software Engineer
MySQL AB, www.mysql.com
