Hi Valentine,

Thanks for the quick turnaround with the test case. Some comments below:

On 07.02.11 8:36, Valentine Gostev wrote:
> Valentine Gostev has proposed merging lp:~percona-dev/percona-xtrabackup/lp713799_test into lp:percona-xtrabackup.
> 
> Requested reviews:
>   Alexey Kopytov (akopytov)
> Related bugs:
>   #712441 Test backup during intensive create/drop/alter table statements
>   https://bugs.launchpad.net/bugs/712441
>   #713799 xtrabackup: race condition when trying to open an already removed tablespace
>   https://bugs.launchpad.net/bugs/713799
> 
> For more details, see:
> https://code.launchpad.net/~percona-dev/percona-xtrabackup/lp713799_test/+merge/48750
> 
> Added test case for bug lp:713799

> === added file 'test/inc/race_table_create.sh'
> --- test/inc/race_table_create.sh	1970-01-01 00:00:00 +0000
> +++ test/inc/race_table_create.sh	2011-02-07 05:36:05 +0000
> @@ -0,0 +1,20 @@
> +. inc/common.sh
> +
> +COMMD="mysql --no-defaults --socket=/tmp/xtrabackup.mysql.sock --user=root"
> +run_cmd  ${COMMD} -e "create database race;"
> +

All other tests use " ${MYSQL} ${MYSQL_ARGS} -e ..." to run the mysql
client. It looks like the same should work here. Or am I missing something?

> +rows=1000

I can imaging that on a fast machine (and with a bit of luck) the
create/drop loop can finish before the parent process actually starts to
backup. Which makes the test unreliable.

> +rowc=0
> +while [ "$rows" -gt "$rowc" ]
> +do
> +        t1=tr$RANDOM
> +        t2=tr$RANDOM
> +        t3=tr$RANDOM
> +        ${COMMD} -e "create table $t1 (a int) ENGINE=InnoDB;" race
> +        ${COMMD} -e "create table $t2 (a int) ENGINE=InnoDB;" race
> +        ${COMMD} -e "create table $t3 (a int) ENGINE=InnoDB;" race
> +        ${COMMD} -e "drop table $t1;" race
> +        ${COMMD} -e "drop table $t2;" race
> +        ${COMMD} -e "drop table $t3;" race
> +        let "rowc=rowc+1"
> +done
> 
> === added file 'test/t/xb_table_race.sh'
> --- test/t/xb_table_race.sh	1970-01-01 00:00:00 +0000
> +++ test/t/xb_table_race.sh	2011-02-07 05:36:05 +0000
> @@ -0,0 +1,20 @@
> +. inc/common.sh
> +
> +init
> +run_mysqld --innodb_file_per_table
> +
> +run_cmd bash inc/race_table_create.sh &
> +

I would make race_table_create a function rather than a separate script.

> +sleep 3
> +

Same comments as before. Using sleeps to synchronize asynchronous
process is generally a bad idea.

> +# Full backup
> +# Full backup folder
> +mkdir -p $topdir/data/full
> +vlog "Starting backup"
> +
> +xtrabackup --datadir=$mysql_datadir --backup --target-dir=$topdir/data/full
> +
> +vlog "Full backup done"
> +

It would also make sense to actually test restoring from the backup to
verify its integrity. That would require creating some permanent
table(s) in addition to the transient ones being created in
race_table_create.

> +stop_mysqld
> +clean
> 

On a general level, I think this test is good for an ad-hoc testing of
the fix for bug #713799 (when you can manually verify the results), but
it's too flaky to include it into the test suite. We either need to add
some synchronization facilities to the xtrabackup test framework, or
create a separate stress test suite for such tests.