Hi Hema,
Thanks for the updated patch that adresses some of my concerns. In
this mail, I will mostly address new issues. I will also reply to
your reply to my first review about issues that have already been
discussed.
Comments are in-line:
Hema Sridharan wrote:
...
> +SET @@global.log_backup_output = 'TABLE,FILE';
Is this an attempt to satisfy my request of testing file output? I am
not sure it helps much unless you actually check the content of the
file.
...
...
> +CREATE TABLE backup_logs.t1_res (id BIGINT UNSIGNED NOT NULL)
ENGINE=MEMORY;
Here, you are re-introducing a table that was removed by Chuck's fix
of Bug#40160. Is this intentional?
...
> ---echo Start using a known backup id for a more definitive test.
> -SET SESSION debug="+d,set_backup_id";
> -
> ---echo con2: Send backup command.
> ---echo con2: Backup id = 500.
Any particular reason why you have removed this?
...
> -FLUSH BACKUP LOGS;
> -
Why have you removed the flushing?
> ---echo Turn off debugging session.
> -SET SESSION debug="-d";
> -
> connection con1;
...
> +#cat_file $MYSQLTEST_VARDIR/master-data/backup_history.log;
> +#cat_file $MYSQLTEST_VARDIR/master-data/backup_progress.log;
Do you have some future plans to use this? If not, I suggest removing
it.
>
> +#
> +# Now test read of backupid with known id using debug insertion
> +#
What are you trying to test here? That the debug flag is working? Is
that necessary to test? Or that backup_id numbers is a sequence with
out wholes? Is that really a requirement?
> +SET SESSION debug="d,set_backup_id";
> +
> --remove_file $MYSQLTEST_VARDIR/master-data/backup_logs_orig.bak
> ---echo The backup id for this command should be 502.
> +
> +#
> +# The first backup will cause the value to be set to 500 and written
to file.
> +# The second backup will read the value (500) and increment it.
> +#--replace_column 1 #
> BACKUP DATABASE backup_logs to 'backup_logs_orig.bak';
>
> +SET SESSION debug="d";
> +
This will turn on full debugging and create a lot of debug output.
Tests will take long to complete since a large master.err will have to
be parsed. (After I run the backup suite with your patch, master.err
is 25 MB). Please, do it the way it was done before your changes:
-SET SESSION debug="+d,set_backup_id";
...
-SET SESSION debug="-d";
> --remove_file $MYSQLTEST_VARDIR/master-data/backup_logs_orig.bak
> ---echo The backup id for this command should be 503.
> +--echo The backup id for this command should be 501.
> BACKUP DATABASE backup_logs to 'backup_logs_orig.bak';
>
> --remove_file $MYSQLTEST_VARDIR/master-data/backup_logs_orig.bak
> ---echo The backup id for this command should be 504.
> +--echo The backup id for this command should be 502.
> BACKUP DATABASE backup_logs to 'backup_logs_orig.bak';
>
> --remove_file $MYSQLTEST_VARDIR/master-data/backup_logs_orig.bak
> ---echo The backup id for this command should be 505.
> +--echo The backup id for this command should be 503.
> BACKUP DATABASE backup_logs to 'backup_logs_orig.bak';
I still miss what I requested in my previous review, that you test
that the backup_id return from the backup command matches the entries
in the backup tables. (I think I gave you an example of how to do
it.)
--
Øystein
