From: Date: December 8 2008 4:04pm
Subject: Re: bzr commit into mysql-6.0-runtime branch (jorgen.austvik:2766)
	WL#4343
List-Archive: http://lists.mysql.com/commits/60954
Message-Id: <20081208150452.GX6011@bodhi>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7BIT

Hello,

The patch is OK to push. 

Please, before you push, address my requests below,
check with QA about the test suite and test file naming,
or some other general QA conventions that I'm unaware of.

* Jorgen Austvik <Jorgen.Austvik@stripped> [08/12/05 15:51]:

>  2766 Jorgen Austvik	2008-12-05
>       WL#4343: First part so that Kostja can review it

Please add meaningful changeset comments ;), something like:

WL#4343, Part 1. Add a test coverage that: (briefly describe what
and how you test).

> 
> === added file 'mysql-test/suite/ddl_lock/include/sync_lock.inc'
> --- a/mysql-test/suite/ddl_lock/include/sync_lock.inc	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/ddl_lock/include/sync_lock.inc	2008-12-05 12:42:24 +0000
> @@ -0,0 +1,63 @@
> +#
> +# SUMMARY
> +#
> +#   Locks a table in one thread, run an SQL statement in another thread,
> +#   and check that the SQL statement is locked.
> +#
> +# USAGE
> +#
> +#   let $lock_table= t1;          # The table to be locked
> +#   let $sql= DROP TABLE t1 ;     # The SQL statement that should lock
> +#
> +# OPTIONAL
> +#   let $sync= after_start_ddl;      # What to sync with to unlock the tables. Default: wait_for_lock
> +#   let $setup= HANDLER t1 OPEN;     # Statement to run before the locked statement
> +#   let $teardown= HANDLER t1 CLOSE; # Statement to run after the lock has been unlocked
> +#
> +# EXAMPLE
> +#   concurrent_ddl.test in ddl_lock suite.

Please use --echo # for comments, so that they make it in the
result file.

Format code and comments with 80 columns or less (preferably
less, I use 66 personally).

> === added file 'mysql-test/suite/ddl_lock/r/create_stress_tables.result'
> --- a/mysql-test/suite/ddl_lock/r/create_stress_tables.result	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/ddl_lock/r/create_stress_tables.result	2008-12-05 12:42:24 +0000
> @@ -0,0 +1,5007 @@
> +SET GLOBAL query_cache_type = OFF;
> +CREATE TABLE t1 (id INT PRIMARY KEY, b CHAR(100) DEFAULT "initial value") ENGINE=<engine_type>;

I suggest to use insert-select instead, to speed up this part. 
Then bulk insert mode is used, which is faster. 

The second option is to use a helper stored procedure. 
If you decide to use mysqltest language still, please use
--disable-query-log here, to make sure the simple queries
don't pollute the result log.

> +INSERT INTO t1 (id) VALUES (5000);
> +INSERT INTO t1 (id) VALUES (4999);
> +INSERT INTO t1 (id) VALUES (4998);

<~5000 inserts that shouldn't be in the changeset were cut>

> +INSERT INTO t1 (id) VALUES (1);
> +CREATE PROCEDURE t_proc (OUT b_p CHAR(100)) 
> +BEGIN 
> +SELECT b INTO @b_p FROM t1 WHERE Id = 1; 
> +END|
> +CREATE FUNCTION t_func (s CHAR(20)) RETURNS CHAR(30) DETERMINISTIC RETURN CONCAT('Hello, ', s, '!');
> 
> === added directory 'mysql-test/suite/ddl_lock/t'
> === added file 'mysql-test/suite/ddl_lock/t/concurrent_ddl.test'
> --- a/mysql-test/suite/ddl_lock/t/concurrent_ddl.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/ddl_lock/t/concurrent_ddl.test	2008-12-05 12:42:24 +0000
> @@ -0,0 +1,557 @@
> +##
> +## Test for WL #4343 - DDL locking for all metadata objects
> +##

Using --echo # for comments simplifies merges of test results,
reading them, and simply following what's going on when a test
breaks.

<cut>

Overall looks good.
I presume the stress part will be improved more.

Thank you for working on this!


--