Hi Andrei,
Nice Work. Please find my review comments below. I have a few
suggestions but not something that would prevent you from
pushing. Please, when pushing to main, remove the entry from
mysql-next-mr.push as we will be adding it to other
collection (as discussed previously).
STATUS
------
Approved.
REQUIRED CHANGES
----------------
n/a
REQUESTS
--------
n/a
SUGGESTIONS
-----------
S1. I would suggest to enforce the assertion in the test case
like this:
set @save_binlog_checksum= @@global.binlog_checksum;
set @@global.binlog_checksum = default;
if (`select @@global.binlog_checksum <> NONE`)
{
--echo Although we set the default for global.binlog_checksum, it does has not
changed!
--die
}
S2. I would use a collection for the feature tree only:
1. copy mysql-next-mr.push to mysql-next-mr-wl2540.push
2. add to mysql-next-mr-wl2540.push the extra tests one
wants in the feature tree.
S3. I would probably make use of mtr to run rpl,binlog tests
with CRC and not the checksum.pl script. Something
like (note the absence of setting the binlog_format and no
tests are skipped as well with --skip-test-list=):
+perl mysql-test-run.pl --timer --force --parallel=auto
--experimental=collections/default.experimental
--comment=rpl_binlog_checksum --vardir=var-rpl_binlog_checksum
--mysqld=--binlog-checksum=CRC32 --suite=binlog,rpl
DETAILS
-------
n/a
On 11/02/2010 03:20 PM, Andrei Elkin wrote:
> #At file:///home/andrei/MySQL/BZR/2a-23May/WL/mysql-next-mr-wl2540/ based on
> revid:andrei.elkin@stripped
>
> 3190 Andrei Elkin 2010-11-02
> Bug #57893 rpl_row_ignorable_event fails when --binlog-checksum=CRC32
>
> There were two issues with the failure:
>
> 1. The reported explicitly CRC32 `val' in the header needs filtering out via
> --replace_regex
>
> 2. More serious is that at verbose mode of event decoding the checksum
> trailing part
> of Rows-log-event:s was not cut off so the decoder decides there is more
> data.
>
> Fixed with deploying --replace_regex in the test and correcting the actual
> event size
> inside the verbose decoder of Log_event::print_base64().
>
> Side effects of this patch include:
> forcing PB2 to run rpl,binlog suites with checksum per push additionally to
> the default no-checksum.
> and fixes for binlog_checksum_basic.
> @ mysql-test/suite/rpl/t/rpl_row_ignorable_event.test
> replace_regex-ing CRC32 `val' to empty in the header to comply with the
> no-checksum
> version results of the test.
> @ sql/log_event.cc
> correcting the actual event size of Rows-events in case of checksum is
> present.
>
> modified:
> mysql-test/collections/mysql-next-mr.push
> mysql-test/suite/rpl/t/rpl_row_ignorable_event.test
> mysql-test/suite/sys_vars/r/binlog_checksum_basic.result
> mysql-test/suite/sys_vars/t/binlog_checksum_basic.test
> sql/log_event.cc
> === modified file 'mysql-test/collections/mysql-next-mr.push'
> --- a/mysql-test/collections/mysql-next-mr.push 2010-10-27 10:47:25 +0000
> +++ b/mysql-test/collections/mysql-next-mr.push 2010-11-02 15:20:42 +0000
> @@ -1,6 +1,6 @@
> +perl suite/rpl/extension/checksum.pl --timer --force --parallel=auto
> --experimental=collections/default.experimental --comment=rpl_binlog_checksum
> --vardir=var-rpl_binlog_checksum --suite=rpl,binlog --percent=99
> perl mysql-test-run.pl --timer --force --parallel=auto
> --experimental=collections/default.experimental --comment=n_mix
> --vardir=var-n_mix --mysqld=--binlog-format=mixed
> --suite=main,binlog,innodb,federated,rpl,sys_vars,perfschema
> --skip-test-list=collections/disabled-per-push.list
> perl mysql-test-run.pl --timer --force --parallel=auto
> --experimental=collections/default.experimental --comment=ps_row
> --vardir=var-ps_row --ps-protocol --mysqld=--binlog-format=row
> --suite=main,binlog,innodb,federated,rpl,sys_vars,perfschema
> --skip-test-list=collections/disabled-per-push.list
> perl mysql-test-run.pl --timer --force --parallel=auto
> --experimental=collections/default.experimental --comment=embedded
> --vardir=var-emebbed --embedded
> --suite=main,binlog,innodb,federated,rpl,sys_vars,perfschema
> perl mysql-test-run.pl --timer --force --parallel=auto
> --experimental=collections/default.experimental --comment=rpl_binlog_row
> --vardir=var-rpl_binlog_row --mysqld=--binlog-format=row
> --suite=rpl,binlog --skip-test-list=collections/disabled-per-push.list
> perl mysql-test-run.pl --timer --force --parallel=auto
> --experimental=collections/default.experimental --comment=funcs_1
> --vardir=var-funcs_1 --suite=funcs_1
> -perl suite/rpl/extension/checksum.pl --timer --force --parallel=auto
> --experimental=collections/default.experimental --comment=rpl_binlog_checksum
> --vardir=var-rpl_binlog_checksum --suite=rpl,binlog
>
> === modified file 'mysql-test/suite/rpl/t/rpl_row_ignorable_event.test'
> --- a/mysql-test/suite/rpl/t/rpl_row_ignorable_event.test 2010-08-06 02:20:44 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_row_ignorable_event.test 2010-11-02 15:20:42 +0000
> @@ -73,7 +73,7 @@ source include/show_binlog_events.inc;
> --echo # MYSQL_BINLOG output base on master-bin.000001
> let $MYSQLD_DATADIR= `select @@datadir`;
> --replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
> ---replace_regex /TIMESTAMP=[0-9]*/TIMESTAMP=t/ /#[0-9]*[ ]*[0-9]*:[0-9]*:[0-9]*
> server id [0-9]*/#server id #/ /exec_time=[0-9]*/exec_time=#/
> /error_code=[0-9]*/error_code=#/ /end_log_pos [0-9]*/end_log_pos #/ /# at [0-9]*/# at #/
> /Xid = [0-9]*/Xid = #/ /thread_id=[0-9]*/thread_id=#/ /table id [0-9]*/table id #/ /mapped
> to number [0-9]*/mapped to number #/ /server v [^ ]*/server v #.##.##/ /Start: binlog v
> [0-9]*/Start: binlog v#/ /created [0-9]*[ ]*[0-9]*:[0-9]*:[0-9]* at startup/created #
> #:#:# at startup/
> +--replace_regex /TIMESTAMP=[0-9]*/TIMESTAMP=t/ /#[0-9]*[ ]*[0-9]*:[0-9]*:[0-9]*
> server id [0-9]*/#server id #/ /exec_time=[0-9]*/exec_time=#/
> /error_code=[0-9]*/error_code=#/ /end_log_pos [0-9]*/end_log_pos #/ /# at [0-9]*/# at #/
> /Xid = [0-9]*/Xid = #/ /thread_id=[0-9]*/thread_id=#/ /table id [0-9]*/table id #/ /mapped
> to number [0-9]*/mapped to number #/ /server v [^ ]*/server v #.##.##/ /Start: binlog v
> [0-9]*/Start: binlog v#/ /created [0-9]*[ ]*[0-9]*:[0-9]*:[0-9]* at startup/created #
> #:#:# at startup/ /[ ]CRC32 0x[abcdef0-9]+//
> --exec $MYSQL_BINLOG --base64-output=decode-rows -v -v
> $MYSQLD_DATADIR/$master_binlog
>
> let $binlog_start= query_get_value(SHOW MASTER STATUS, Position, 1);
>
> === modified file 'mysql-test/suite/sys_vars/r/binlog_checksum_basic.result'
> --- a/mysql-test/suite/sys_vars/r/binlog_checksum_basic.result 2010-10-28 17:09:41
> +0000
> +++ b/mysql-test/suite/sys_vars/r/binlog_checksum_basic.result 2010-11-02 15:20:42
> +0000
> @@ -1,10 +1,10 @@
> set @save_binlog_checksum= @@global.binlog_checksum;
> +set @@global.binlog_checksum = default;
> select @@global.binlog_checksum as 'must be NONE by default';
> must be NONE by default
> NONE
> select @@session.binlog_checksum as 'no session var';
> ERROR HY000: Variable 'binlog_checksum' is a GLOBAL variable
> -set @@global.binlog_checksum = default;
> set @@global.binlog_checksum = CRC32;
> set @@global.binlog_checksum = CRC32;
> set @@global.master_verify_checksum = 0;
>
> === modified file 'mysql-test/suite/sys_vars/t/binlog_checksum_basic.test'
> --- a/mysql-test/suite/sys_vars/t/binlog_checksum_basic.test 2010-10-28 17:09:41
> +0000
> +++ b/mysql-test/suite/sys_vars/t/binlog_checksum_basic.test 2010-11-02 15:20:42
> +0000
> @@ -4,12 +4,12 @@
> # all checksum related system variables.
>
> set @save_binlog_checksum= @@global.binlog_checksum;
> +set @@global.binlog_checksum = default;
>
> select @@global.binlog_checksum as 'must be NONE by default';
> --error ER_INCORRECT_GLOBAL_LOCAL_VAR
> select @@session.binlog_checksum as 'no session var';
>
> -set @@global.binlog_checksum = default;
>
> # testing lack of side-effects in non-effective update of binlog_checksum:
> set @@global.binlog_checksum = CRC32;
>
> === modified file 'sql/log_event.cc'
> --- a/sql/log_event.cc 2010-10-25 19:02:24 +0000
> +++ b/sql/log_event.cc 2010-11-02 15:20:42 +0000
> @@ -2338,6 +2338,9 @@ void Log_event::print_base64(IO_CACHE* f
> if (print_event_info->verbose)
> {
> Rows_log_event *ev= NULL;
> + if (checksum_alg != BINLOG_CHECKSUM_ALG_UNDEF&&
> + checksum_alg != BINLOG_CHECKSUM_ALG_OFF)
> + size-= BINLOG_CHECKSUM_LEN; // checksum is displayed through the header
>
> if (ptr[4] == TABLE_MAP_EVENT)
> {
>
>
>
>
>
