Re: bzr commit into mysql-6.0-falcon branch (hky:2815)

[Hakan Kuecuekyilmaz]

On Di, 2008-09-09 at 13:07 +0200, John Embretsen wrote:
> Hakan,
> 
> Overall good changes, using the template and all :)
> 
> Patch OK to push, however please see my inline comments anyway...
> 
> Hakan Kuecuekyilmaz wrote:
> > #At file:///home/hakan/work/mysql/mysql-6.0-falcon-team/
> > 
> >  2815 Hakan Kuecuekyilmaz	2008-09-09
> >       Rescued 3 tests from falcon_team test suite.
> >       
> >       Cleaned up test cases and added one -big.test version of
> >       falcon_bug_22207.test.
> > added:
> >   mysql-test/suite/falcon/r/falcon_bug_22207-big.result
> >   mysql-test/suite/falcon/t/falcon_bug_22207-big.test
> > renamed:
> >   mysql-test/suite/falcon_team/r/falcon_bug_22189.result =>
> mysql-test/suite/falcon/r/falcon_bug_22189.result
> >   mysql-test/suite/falcon_team/r/falcon_bug_22207.result =>
> mysql-test/suite/falcon/r/falcon_bug_22207.result
> >   mysql-test/suite/falcon_team/r/falcon_bug_24024.result =>
> mysql-test/suite/falcon/r/falcon_bug_24024.result
> >   mysql-test/suite/falcon_team/t/falcon_bug_22189.test =>
> mysql-test/suite/falcon/t/falcon_bug_22189.test
> >   mysql-test/suite/falcon_team/t/falcon_bug_22207.test =>
> mysql-test/suite/falcon/t/falcon_bug_22207.test
> >   mysql-test/suite/falcon_team/t/falcon_bug_24024.test =>
> mysql-test/suite/falcon/t/falcon_bug_24024.test
> > modified:
> >   mysql-test/suite/falcon/r/falcon_bug_22972.result
> >   mysql-test/suite/falcon/t/disabled.def
> >   mysql-test/suite/falcon/t/falcon_bug_22972.test
> >   mysql-test/suite/falcon_team/r/falcon_bug_23945.result
> >   mysql-test/suite/falcon_team/t/falcon_bug_23945.test
> >   mysql-test/suite/falcon/r/falcon_bug_22189.result
> >   mysql-test/suite/falcon/r/falcon_bug_22207.result
> >   mysql-test/suite/falcon/r/falcon_bug_24024.result
> >   mysql-test/suite/falcon/t/falcon_bug_22189.test
> >   mysql-test/suite/falcon/t/falcon_bug_22207.test
> >   mysql-test/suite/falcon/t/falcon_bug_24024.test
> 
> Ideally I would like to see some commit comments explaining (the
> reasoning behind) the actual changes to each file, instead of just
> "cleaned up test cases". This makes reviews and future archeological
> investigations easier.
> 

I will consider it in my next commits to have full comments on every
file.

> 
> > === renamed file 'mysql-test/suite/falcon_team/r/falcon_bug_24024.result' =>
> 'mysql-test/suite/falcon/r/falcon_bug_24024.result'
> > --- a/mysql-test/suite/falcon_team/r/falcon_bug_24024.result	2008-04-20 00:05:17
> +0000
> > +++ b/mysql-test/suite/falcon/r/falcon_bug_24024.result	2008-09-09 08:30:18
> +0000
> 
> > @@ -14,10 +16,13 @@ a	b
> >  1	NULL
> >  7	NULL
> >  DROP TABLE t1;
> > -!!! Error message has to be decided.
> > -!!! Something like "Cannot drop locked table" or similar would be fine.
> > -!!! But never the misleading    ERROR 42S02: Unknown table 't1'
> > -COMMIT;
> > -# Switch to session default and disconnect session con1
> > +ERROR 42S02: Unknown table 't1'
> > +SHOW WARNINGS;
> > +Level	Code	Message
> > +Warning	178	Can't execute the given command because you have active locked
> tables or an active transaction
> 
> Although this change is OK, I don't really like the fact that a request
> of never using the misleading "Unknown table" error message for this
> issue is removed.

I put back the comment.

> Bug#22972 (duplicates Bug#24024) was closed after the following was
> added to the changelog:
> 
> "Falcon cannot drop a table for which there is a pending transaction,
> but the error message for such attempts was misleading."
> 
> It is clear that the error message is still misleading, so in my opinion
> the bug has not been completely fixed, although there is a descriptive
> warning message.
> 
> However, according to the bug report the issue seems to be fixed from
> the Falcon side, since the problem is that the server does not recognize
> these types of errors properly ("Falcon can report that the moon is
> green and this same error will occur.").
> 
> So I'm OK with this particular change to the test, but I wonder if there
> is a bug reported against the server regarding this?

There is no bug report against the server regarding this, which I am
aware of.

> ---
> 
> Please explain why you did the following change:
> 
> > --- a/mysql-test/suite/falcon_team/t/falcon_bug_22189.test	2008-04-28 11:21:52
> +0000
> > +++ b/mysql-test/suite/falcon/t/falcon_bug_22189.test	2008-09-09 08:30:18 +0000
> > @@ -68,8 +68,10 @@ SELECT * FROM x1;
> >  
> >  --echo # Switch to connection conn1
> >  connection conn1;
> > ---send UPDATE x1 SET x1 = 0, x2 = 5
> > ---send INSERT INTO x1 VALUES (0,6)
> > +--error ER_DUP_ENTRY
> > +UPDATE x1 SET x1 = 0, x2 = 5;
> > +--error ER_DUP_ENTRY
> > +INSERT INTO x1 VALUES (0,6);

I removed the two --send's, because the statements where not blocking
anymore. Having two consecutive --send's can lead to race conditions.
Removing the --send's makes the test more robust. 

  
> >  --echo # Switch to connection default
> >  connection default;
> > @@ -78,8 +80,6 @@ ROLLBACK;
> >  --echo # Switch to connection conn1
> >  connection conn1;
> >  --error ER_DUP_ENTRY
> > ---reap
> > ---error ER_DUP_ENTRY
> >  INSERT INTO x1 VALUES (0,6);
> >  --error ER_DUP_ENTRY
> >  INSERT INTO x1 VALUES (0,6);

Every --send needs a --reap, therefore I removed the --reap.

> 
> Changes to falcon_bug_23945.[test|result] look good.
> 
> This test is still problematic, since it causes subsequent tests in the
> test suite to fail ("1005: Can't create table 'test.t1' (errno: 156)").
> 
> Until that is fixed, we should at least add a note and/or echo statement
> about that in the test case, or possibly even disable it.
> 
> However, those things can be done later in a separate changeset if so
> desired.

Agreed. I think disabling it would be good, so that the other tests
don't suffer from the side effect. I think we should do it in a separate
commit.

[...]

> The rest looked all right. The patched falcon suite passed 100% on my
> Linux test host.


-- 
Hakan Küçükyılmaz, Senior Software Engineer DBTG/MySQL +49 160
98953296
Sun Microsystems GmbH     Sonnenallee 1, DE-85551 Kirchheim-Heimstetten
Geschaeftsfuehrer:  Thomas Schroeder, Wolfang Engels, Dr. Roland Boemer
Vorsitz d. Aufs.rat.: Martin Haering   HRB MUC 161028     49.011, 8.376