Code review comment for lp:~percona-dev/percona-xtrabackup/ib_csm_csv

Hi Valentine,

Some comments on the patch:

On 18.02.11 23:20, Valentine Gostev wrote:
> Valentine Gostev has proposed merging lp:~percona-dev/percona-xtrabackup/ib_csm_csv into lp:percona-xtrabackup.
>
> Requested reviews:
> Vadim Tkachenko (vadim-tk)
> Alexey Kopytov (akopytov)
> Related bugs:
> #420181 innobackupex fails to backup CSV tables causing mysql_upgrade to fail
> https://bugs.launchpad.net/bugs/420181
> #597384 innobackupex --include does not apply to the non-InnoDB tables
> https://bugs.launchpad.net/bugs/597384
>
> For more details, see:
> https://code.launchpad.net/~percona-dev/percona-xtrabackup/ib_csm_csv/+merge/50390
>
> 1. Added fix for bug 420181
> 2. Added test for bug 420181
> 3. Added test for bug 597384

>
> === modified file 'innobackupex'
> --- innobackupex 2011-02-12 06:14:27 +0000
> +++ innobackupex 2011-02-18 20:19:33 +0000
> @@ -322,7 +322,7 @@
> command makes a complete backup of all MyISAM and InnoDB tables and
> indexes in all databases or in all of the databases specified with the
> --databases option. The created backup contains .frm, .MRG, .MYD,
> -.MYI, .TRG, .TRN, .ARM, .ARZ, .opt, .par, and InnoDB data and log files.
> +.MYI, .TRG, .TRN, .ARM, .ARZ, .CSM, .CSV, .opt, .par, and InnoDB data and log files.
> The MY.CNF options file defines the location of the database. This command
> connects to the MySQL server using mysql client program, and runs
> ibbackup (InnoDB Hot Backup program) as a child process.
> @@ -599,7 +599,7 @@
>
> }
>
> - # backup .frm, .MRG, .MYD, .MYI, .TRG, .TRN, .ARM, .ARZ and .opt files
> + # backup .frm, .MRG, .MYD, .MYI, .TRG, .TRN, .ARM, .ARZ, .CSM, .CSV and .opt files
> backup_files();
>
> # resume ibbackup and wait till it has finished
> @@ -775,7 +775,7 @@
> opendir(DIR, $backup_dir)
> || Die "Can't open directory '$backup_dir': $!\n";
> print STDERR "$prefix Starting to copy MyISAM tables, indexes,\n";
> - print STDERR "$prefix .MRG, .TRG, .TRN, .ARM, .ARZ, .opt, and .frm files\n";
> + print STDERR "$prefix .MRG, .TRG, .TRN, .ARM, .ARZ, .CSM, .CSV, .opt, and .frm files\n";
> print STDERR "$prefix in '$backup_dir'\n";
> print STDERR "$prefix back to original data directory '$orig_datadir'\n";
> while (defined($file = readdir(DIR))) {
> @@ -2009,13 +2009,13 @@
> my @list;
> my $file;
> my $database;
> - my $wildcard = '*.{frm,MYD,MYI,MRG,TRG,TRN,ARM,ARZ,opt,par}';
> + my $wildcard = '*.{frm,MYD,MYI,MRG,TRG,TRN,ARM,ARZ,CSM,CSV,opt,par}';
>
> opendir(DIR, $source_dir)
> || Die "Can't open directory '$source_dir': $!\n";
> $now = current_time();
> print STDERR "\n$now $prefix Starting to backup .frm, .MRG, .MYD, .MYI,\n";
> - print STDERR "$prefix .TRG, .TRN, .ARM, .ARZ and .opt files in\n";
> + print STDERR "$prefix .TRG, .TRN, .ARM, .ARZ, .CSM, .CSV and .opt files in\n";
> print STDERR "$prefix subdirectories of '$source_dir'\n";
> # loop through all database directories
> while (defined($database = readdir(DIR))) {
> @@ -2042,7 +2042,7 @@
>
> # copy files of this database
> opendir(DBDIR, "$source_dir/$database");
> - @list = grep(/\.(frm)|(MYD)|(MYI)|(MRG)|(TRG)|(TRN)|(ARM)|(ARZ)|(opt)|(par)$/, readdir(DBDIR));
> + @list = grep(/\.(frm)|(MYD)|(MYI)|(MRG)|(TRG)|(TRN)|(ARM)|(ARZ)|(CSM)|(CSV)|(opt)|(par)$/, readdir(DBDIR));
> closedir DBDIR;
> $file_c = @list;
> if ($file_c <= $backup_file_print_limit) {
> @@ -2098,7 +2098,7 @@
> closedir(DIR);
>
> $now = current_time();
> - print STDERR "$now $prefix Finished backing up .frm, .MRG, .MYD, .MYI, .TRG, .TRN, .ARM, .ARZ and .opt files\n\n";
> + print STDERR "$now $prefix Finished backing up .frm, .MRG, .MYD, .MYI, .TRG, .TRN, .ARM, .ARZ, .CSV, .CSM and .opt files\n\n";
> }
>
>
>
> === added file 'test/t/ib_csm_csv.sh'
> --- test/t/ib_csm_csv.sh 1970-01-01 00:00:00 +0000
> +++ test/t/ib_csm_csv.sh 2011-02-18 20:19:33 +0000
> @@ -0,0 +1,64 @@
> +. inc/common.sh
> +OUTFILE=results/ib_csm_csv_innobackupex.out
> +init
> +run_mysqld --innodb_file_per_table
> +load_dbase_schema incremental_sample

Why do you need to load incremental_sample here? Just to be able to pass
that as a DB name in all commands?

> +run_cmd ${MYSQL} ${MYSQL_ARGS} -e "create table csm (a int NOT NULL ) ENGINE=CSV" incremental_sample
> +# creating my.cnf for innobackupex
> +echo "
> +[mysqld]
> +datadir=$mysql_datadir" > $topdir/my.cnf
> +# Adding initial rows
> +vlog "Adding initial rows to database..."
> +numrow=100
> +count=0
> +while [ "$numrow" -gt "$count" ]
> +do
> + ${MYSQL} ${MYSQL_ARGS} -e "insert into csm values ($count);" incremental_sample
> + let "count=count+1"
> +done
> +vlog "Initial rows added"
> +
> +# Full backup
> +# backup root directory
> +mkdir -p $topdir/backup
> +
> +vlog "Starting backup"
> +innobackupex --user=root --socket=$mysql_socket --defaults-file=$topdir/my.cnf $topdir/backup > $OUTFILE 2>&1 || die "innobackupex died with exit code $?"
> +full_backup_dir=`grep "innobackupex: Backup created in directory" $OUTFILE | awk -F\' '{print $2}'`
> +vlog "Full backup done to folder $full_backup_dir"
> +

"folder" is Windows terminology. It's called "directory" on any other OS.

> +# Saving the checksum of original table
> +checksum_a=`${MYSQL} ${MYSQL_ARGS} -Ns -e "checksum table csm;" incremental_sample | awk '{print $2}'`
> +vlog "Table checksum is $checksum_a"
> +
> +vlog "Preparing backup"
> +# Prepare backup
> +echo "##############" >> $OUTFILE
> +echo "# PREPARE #1 #" >> $OUTFILE

Why does it say "#1" if there is no "#2"? Any specific reasons for not
using vlog?

> +echo "##############" >> $OUTFILE
> +innobackupex --defaults-file=$topdir/my.cnf --apply-log $full_backup_dir >> $OUTFILE 2>&1 || die "innobackupex died with exit code $?"
> +vlog "Data prepared for restore"
> +
> +# Destroying mysql data
> +stop_mysqld
> +rm -rf $mysql_datadir/*
> +vlog "Data destroyed"
> +
> +# Restore backup
> +vlog "Copying files"
> +run_cmd innobackupex --defaults-file=$topdir/my.cnf --copy-back $full_backup_dir

Any specific reasons for using run_cmd here, but not using it in all
previous invokations of innobackupex?

> +vlog "Data restored"
> +
> +run_mysqld --innodb_file_per_table
> +vlog "Cheking checksums"

And we are still "cheking" the checksums :)

> +checksum_b=`${MYSQL} ${MYSQL_ARGS} -Ns -e "checksum table csm" incremental_sample | awk '{print $2}'`
> +
> +if [ $checksum_a -ne $checksum_b ]
> +then
> + vlog "Checksums are not equal"
> + exit -1
> +fi
> +vlog "Checksums are OK"
> +stop_mysqld
> +clean
>
> === added file 'test/t/ib_include.sh'
> --- test/t/ib_include.sh 1970-01-01 00:00:00 +0000
> +++ test/t/ib_include.sh 2011-02-18 20:19:33 +0000
> @@ -0,0 +1,51 @@
> +. inc/common.sh
> +OUTFILE=results/xb_basic_innobackupex_out
> +

Wrong output filename.

> +init
> +run_mysqld
> +run_cmd ${MYSQL} ${MYSQL_ARGS} -e "create database include;"
> +run_cmd ${MYSQL} ${MYSQL_ARGS} -e "create table t1 (a int) ENGINE=MyISAM;" include
> +run_cmd ${MYSQL} ${MYSQL_ARGS} -e "create table t2 (a int) ENGINE=InnoDB;" include
> +run_cmd ${MYSQL} ${MYSQL_ARGS} -e "insert into t1 values (1),(2),(3);" include
> +run_cmd ${MYSQL} ${MYSQL_ARGS} -e "insert into t2 values (1),(2),(3);" include
> +# Take backup
> +echo "
> +[mysqld]
> +datadir=$mysql_datadir" > $topdir/my.cnf
> +mkdir -p $topdir/backup
> +checksum_t1=`${MYSQL} ${MYSQL_ARGS} -Ns -e "checksum table t1" include | awk '{print $2}'`
> +checksum_t2=`${MYSQL} ${MYSQL_ARGS} -Ns -e "checksum table t2" include | awk '{print $2}'`
> +vlog "checksum_t1 is $checksum_t1"
> +vlog "checksum_t2 is $checksum_t2"
> +innobackupex-1.5.1 --user=root --socket=$mysql_socket --defaults-file=$topdir/my.cnf --include="^include[.]t" $topdir/backup > $OUTFILE 2>&1 || die "innobackupex-1.5.1 died with exit code $?"

Please use "innobackupex" as in ib_csm_csv.sh. Please also consider
using run_cmd to avoid handling the exit code on your own.

> +backup_dir=`grep "innobackupex-1.5.1: Backup created in directory" $OUTFILE | awk -F\' '{ print $2}'`
> +stop_mysqld
> +# Remove datadir
> +rm -r $mysql_datadir
> +# Restore data
> +vlog "Applying log"
> +innobackupex-1.5.1 --apply-log --defaults-file=$topdir/my.cnf $backup_dir
> +vlog "Restoring MySQL datadir"
> +mkdir -p $mysql_datadir
> +innobackupex-1.5.1 --copy-back --defaults-file=$topdir/my.cnf $backup_dir
> +run_mysqld
> +checksum_tt1=`${MYSQL} ${MYSQL_ARGS} -Ns -e "checksum table t1" include | awk '{print $2}'`
> +checksum_tt2=`${MYSQL} ${MYSQL_ARGS} -Ns -e "checksum table t2" include | awk '{print $2}'`
> +vlog "checksum_tt1 is $checksum_tt1"
> +vlog "checksum_tt2 is $checksum_tt2"
> +if [ $checksum_t1 -eq $checksum_tt1 ]
> +then
> + vlog "Checksums are OK"
> +else
> + vlog "Checksums are not equal"
> + exit -1
> +fi
> +if [ $checksum_t2 -eq $checksum_tt2 ]
> +then
> + vlog "Checksums are OK"
> +else
> + vlog "Checksums are not equal"
> + exit -1
> +fi

Why not combine the above statements using the "-a" (and) operator?

> +stop_mysqld
> +clean
>