Hi Chuck,
Thanks for all your comments. At this juncture, I want to update you few
things. Lars, I request you to pitch in and add more clarity or if I'm
missing out any details here.
1. It was decided sometime ago that we will have only BACKUP SUITE and Lars
is aware of that. Also regarding the usage of combinations file, the history
goes like this. (I started with hard coded engines in the script -> Lars
suggested .opt file -> Other folks pointed out the hitches using it -> Moved
to combinations file approach [based on feedback from Lars, Magnus]).
2. Magnus had sent an email (copied you also) indicating that all test cases
will be moved to backup suite and thats the reason I created this suite and
added test case. I will also add the test cases for other backup related WL
in this suite only.
3. I kept resolving issues, one after other for pushing my tests (commit
issues, .opt, combination file, bzr migration)into the suite, though I had
them ready and I had them tested before manually. Still I agree that there
was little bit of delay and I shall diligently work on this and get the test
cases into suite asap.
Please read in line for my replies and let me know, if you are fine. Based
on your final approval I will push the test suite.
>
> Hema,
>
> Patch not approved. I have comments.
>
> 1) The patch is malformed and includes an extra bit at the
> top which should not be there. .bzr-mysql...
Hema: Agreed, will make necessary modifications.
> 2) The tests will not run.
>
> Unexpected line ' ' found in
> 'd:/source/bzr/mysql-6.0-wl-4222/mysql-test/suite/b
> ackup/combinations' at lib//My/Config.pm line 203, <GEN12> line 3.
>
> This was due to extra characters in the file. I changed it to:
>
> --- a/mysql-test/suite/backup/combinations 1970-01-01
> 00:00:00 +0000
> +++ b/mysql-test/suite/backup/combinations 2008-06-05
> 18:18:32 +0000
> @@ -0,0 +1,12 @@
> +[innodb]
> +--default-storage-engine=innodb
> +[falcon]
> +--default-storage-engine=falcon
> +[myisam]
> +--default-storage-engine=myisam
> +[memory]
> +--default-storage-engine=memory
> +
>
Hema: Ok.
> 3) Why are these tests in a backup suite but the rest are
> not? Is there some other patch that moves the backup tests to
> the backup suite? Seems silly to have some in one place and
> others in another.
Hema: Explained above.
>
> 4) I don't like the names of some of the tests. The names
> start out meaningful but the naming scheme is inconsistent.
> For example, shouldn't 'backup_objectsfall' be
> 'backup_functions'? That would seem to make more sense. When
> I first saw the test name I though it was a test that is only
> run in the Fall...as opposed to Spring, Summer.... :P Same
> goes for the other two tests... backup_procedures, backup_triggers.
>
Hema: Agreed. Will change it :-)
> 5) You need to include the 'must have' list to include
> falcon, innodb in all of your tests. Not every build will
> build these engines -- especially falcon.
Hema: Will inlcude have_falcon.inc in all my tests.
>
> 6) Why are all of the statements on one line? It would make
> for easier reading if, for example, CREATE were written like this:
>
> CREATE TABLE teams (
> teamno int not null primary key,
> playerno int,
> division char(10),
> foreign key(playerno)
> references players(playerno)
> );
>
> ...you get the idea.
Hema : Agreed. Will change it.
>
> 7) It has been requested of me in the past by QA folk that
> there is a standard for case usage. For example, all SQL
> command keywords are uppercase while identifiers are
> lowercase. Above should be:
>
> CREATE TABLE teams (
> teamno INT NOT NULL PRIMARY KEY,
> playerno INT,
> division CHAR(10),
> FOREIGN KEY(playerno)
> REFERENCES players(playerno)
> );
>
> ...please check with Jeb to confirm this.
>
Hema: I have already shown the same script to Jeb, he did not give me any
specific review comments on SQL command keywords and identifiers. Anyways I
have changed it in accordance to your suggestion :-)
> 8) It has also been suggested to me that we should avoid
> using 'USE db' and prefix all statements with the db name.
> SELECT * FROM db.t1; ...please check with Jeb to confirm this.
Hema: I was feeling comfortable to use "USE db". But if thats not the
suggested option, I can change it.
> 9) There are spacing issues. For example:
>
> + INSERT INTO matches
> values(6,1,3,3,1),(7,2,1,4,2),(8,1,5,0,1),(9,2,2,3,0),(10,2,2,
> 2,2) ; ^ ^
>
Hema: Agreed, will make necessary modifications.
> 10) Watch line breaks. This should be formatted so that words
> are broken across lines at 'legal' points:
>
> +# Purpose: To test the metadata consistency by using object
> trigger for
> +all
> sto
> +# rage engines
>
> What's a sto rage engine? Is it a really angry sto?
Hema: :-)! Oops missed the line breaks, will modify that.
>
> 11) Do you really need to mask so many columns?
>
> +--replace_column 2 # 3 # 4 # 5 # 6 # 7 # 8 # 9 # 10 # 11 #
> 12 # 13 #
> +14 #
> 16 # 17 # 18 #
> +SHOW TABLE STATUS;
>
> If so, then perhaps there is a different command you can use...
Hema: I wanted to maintain a uniqueness in result file for all engines. I
realise that this command isn't required and will remove it from the script.
>
> 12) Once the patch and combinations file was fixed, I got this error:
>
> backup.backup_objectsfall 'innodb' [ fail ]
>
> mysqltest: At line 9: Could not open
> '.\data2\bzr6.04june\mysql-6.0-backup\mysql
> -test\suite\backup\include\backup_engine.inc' for reading
>
> Which is caused by a hard-coded path in your test file:
>
> --source
> /data2/bzr6.04june/mysql-6.0-backup/mysql-test/suite/backup/in
> clude/backup_e
> ngine.inc
>
> The above should be:
>
> --source suite/backup/include/backup_engine.inc
Hema: Sorry for hardcoding the path name. I have changed it.
>
> 13) Once the patch issue was resolved, the result file was
> different but this was due to the patch problem. The same is
> true for the other two tests.
>
> 14) Once all those problems were solved, I got this. I
> surrender. Please fix and resubmit a new patch.
>
> backup.backup_objectts 'innodb' [ fail ]
>
> mysqltest: At line 181: query 'RESTORE FROM
> 'bup_objectts.bak'' failed: 7:
> Error
> on rename of '.\bup_objectts\cap.TRG~' to '.\bup_objectts\cap.TRG'
> (Errcode: 17
> )
Hema: Oops ! I didn't get this error when I run it in my machine and thats
why I committed the test. I don't know why it is failing in your machine,
please help me out to figure out the delta b/w these 2 systems.Also this
error is pertinent to "ALTER TABLE" operation, which I haven't done in my
test case. I don't know from where this error pops up?
>
> The result from queries just before the failure was:
> < snip >
> Table t2
> Create Table CREATE TABLE `t2` (
> `a` char(4) DEFAULT NULL
> ) ENGINE=ENGINE DEFAULT CHARSET=latin1
> SHOW TABLE STATUS;
> Name Engine Version Row_format Rows Avg_row_length
> Data_lengthMax_d
> ata_length Index_length Data_free Auto_increment
> Create_timeUpdat
> e_time Check_time Collation Checksum Create_options
> Comment
> cap # # # # # # # #
> #####lat
> in1_swedish_ci # # #
> city # # # # # # # #
> #####lat
> in1_swedish_ci # # #
> dropcity # # # # # # #
> ######la
> tin1_swedish_ci # # #
> t2 # # # # # # # #
> #####lat
> in1_swedish_ci # # #
> t3 # # # # # # # #
> #####lat
> in1_swedish_ci # # #
> t4 # # # # # # # #
> #####lat
> in1_swedish_ci # # #
> backup data
> BACKUP DATABASE bup_objectts TO 'bup_objectts.bak'; backup_id
> # dropping database.
> DROP DATABASE bup_objectts;
> RESTORE FROM bup_objectts.bak;
> RESTORE FROM 'bup_objectts.bak';
>
Hema: Once again, I don't see this error and the test runs fine in my
ambience / system. Please see the result that I obtained.
=======================================================
TEST RESULT TIME (ms)
-------------------------------------------------------
backup.backup_functions 'innodb' [ pass ] 1336
backup.backup_procedures 'innodb' [ pass ] 420
backup.backup_triggers 'innodb' [ pass ] 1831
backup.backup_functions 'falcon' [ pass ] 1767
backup.backup_procedures 'falcon' [ pass ] 459
backup.backup_triggers 'falcon' [ pass ] 2277
backup.backup_functions 'myisam' [ pass ] 1042
backup.backup_procedures 'myisam' [ pass ] 422
backup.backup_triggers 'myisam' [ pass ] 1462
backup.backup_functions 'memory' [ pass ] 1008
backup.backup_procedures 'memory' [ pass ] 394
backup.backup_triggers 'memory' [ skipped ] Temporarily Disabled Bug#35249
---------------------------------------
> Chuck
>
>
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe: http://lists.mysql.com/commits?unsub=1
>
>
