Hi,
I have comments and questions below.
Chuck
> 2632 Oystein Grovlen 2008-06-09
> BUG#36586 Online backup stream library can miss end of a stream.
> Make sure end-of-stream is detected.
> added:
> mysql-test/r/backup_multi_blocks.result
> mysql-test/t/backup_multi_blocks.test
> modified:
> mysql-test/lib/mtr_report.pl
> sql/backup/stream_v1_transport.c
Commit email has 2 bug reports on it. Which one does this patch go with?
Both? The scrubber didn't attach the commit to the bug report. I am
reviewing this for BUG#36586.
>
> per-file messages:
> mysql-test/lib/mtr_report.pl
> Ignore expected restore failure in that backup_multi_blocks test
> mysql-test/r/backup_multi_blocks.result
> Output for new test.
> mysql-test/t/backup_multi_blocks.test
> Test that a restore of a multi-block backup is succesful.
> Prior to this bug fix, test would hang.
> Until bug 36624 is fixed the restore in this test will fail,
> and currently the test is written to expect that.
So when BUG#36624 is fixed the patch will correct the test, right?
> sql/backup/stream_v1_transport.c
> Make sure unexpected end-of-stream is detected in load_buffer()
> I have verified that all calls to as_read() now checks
> for end-of-stream.
> === modified file 'mysql-test/lib/mtr_report.pl'
> --- a/mysql-test/lib/mtr_report.pl 2008-05-05 19:38:05 +0000
> +++ b/mysql-test/lib/mtr_report.pl 2008-06-09 08:25:26 +0000
> @@ -333,6 +333,13 @@ sub mtr_report_stats ($) {
> (
> /Backup:/ or /Restore:/ or /Can't open the
> online backup progress tables/
> ) or
> +
> + # Filter expected Restore error in
> backup_multi_blocks
> + ($testname eq 'main.backup_multi_blocks') and
> + (
> + /Restore: Error when reading summary
> section of backup image/
> + ) or
> +
> # The tablespace test triggers error below on purpose
> ($testname eq 'main.backup_tablespace') and
> (
>
> === added file 'mysql-test/r/backup_multi_blocks.result'
> --- a/mysql-test/r/backup_multi_blocks.result 1970-01-01
> 00:00:00 +0000
> +++ b/mysql-test/r/backup_multi_blocks.result 2008-06-09
> 08:25:26 +0000
> @@ -0,0 +1,33 @@
> +create database mysqltest;
> +use mysqltest;
> +CREATE TABLE t1 (a longtext) engine=myisam;
> +use mysqltest;
> +insert into t1 values ("text");
> +update t1 set a=concat(a,a);
> +update t1 set a=concat(a,a);
> +update t1 set a=concat(a,a);
> +update t1 set a=concat(a,a);
> +update t1 set a=concat(a,a);
> +update t1 set a=concat(a,a);
> +update t1 set a=concat(a,a);
> +update t1 set a=concat(a,a);
> +update t1 set a=concat(a,a);
> +update t1 set a=concat(a,a);
> +update t1 set a=concat(a,a);
> +update t1 set a=concat(a,a);
> +update t1 set a=concat(a,a);
> +select length(a) from t1;
> +length(a)
> +32768
> +BACKUP DATABASE mysqltest TO 'test.ba';
> +backup_id
> +#;
> +DROP DATABASE mysqltest;
> +RESTORE FROM 'test.ba';
> +ERROR HY000: Error when reading summary section of backup image
> +select length(a) from t1;
> +length(a)
> +checksum table t1;
> +Table Checksum
> +mysqltest.t1 0
> +drop database mysqltest;
>
> === added file 'mysql-test/t/backup_multi_blocks.test'
> --- a/mysql-test/t/backup_multi_blocks.test 1970-01-01
> 00:00:00 +0000
> +++ b/mysql-test/t/backup_multi_blocks.test 2008-06-09
> 08:25:26 +0000
> @@ -0,0 +1,26 @@
All backup tests should have at least the following in the preamble. This is
because backup is not included in embeddded.
--source include/not_embedded.inc
Similarly, if you need a debug build you should include this (but I don't
think you need it for this patch).
--source include/have_debug.inc
Required addition: Please add the following. We do this in case another test
(or this one) doesn't cleanup properly. Always good to start every test with
this sort of thing.
--disable_warnings
DROP DATABASE IF EXISTS mysqltest;
--enable_warnings
> +create database mysqltest;
> +use mysqltest;
> +
> +CREATE TABLE t1 (a longtext) engine=myisam;
A word on style...once upon a time I used to type in lowercase until I was
told all commands in tests should be UPPERCASE and only identifiers should
be lowercase. So the above should be:
CREATE DATABASE mysqltests;
USE mysqltest;
CREATE TABLE t1 (a LONGTEXT) ENGINE=MYISAM;
> +
> +use mysqltest;
> +insert into t1 values ("text");
> +let $1=13;
> +while ($1)
> +{
> + update t1 set a=concat(a,a);
> + dec $1;
> +}
> +select length(a) from t1;
> +
> +--replace_column 1 #;
> +BACKUP DATABASE mysqltest TO 'test.ba';
> +
> +DROP DATABASE mysqltest;
> +
> +--error 1698
> +RESTORE FROM 'test.ba';
> +
> +select length(a) from t1;
> +checksum table t1;
> +drop database mysqltest;
Please make use of the comments '#' and add, at a minimum, a short
description of the test that includes the rationale and basic things the
test is, er, testing.
Lastly, please use the --echo command to print out steps for debugging
purposes. For example, this is an excerpt from another test that prints out
statements letting the person reviewing the result file to see what is about
to happen (or should happen).
# Set the breakpoint for the commit blocker.
--echo con5: Getting lock on commit blocker.
SELECT get_lock("backup_commit_blocker", 0);
>
> === modified file 'sql/backup/stream_v1_transport.c'
> --- a/sql/backup/stream_v1_transport.c 2007-12-03
> 20:28:32 +0000
> +++ b/sql/backup/stream_v1_transport.c 2008-06-09
> 08:25:26 +0000
> @@ -1480,7 +1480,7 @@ int bstream_read_part(backup_stream *s,
> saved= *data;
> data->end= data->begin + howmuch;
>
> - as_read(&s->stream,data,buf);
> + if (as_read(&s->stream, data, buf) == BSTREAM_EOS) s->state= EOS;
Style issue: Please format the code as follows:
if (something)
do_something();
Please see coding guidelines at:
http://forge.mysql.com/wiki/MySQL_Internals_Coding_Guidelines
>
> s->buf.begin += data->begin - saved.begin;
> s->buf.pos= s->buf.begin;
