Hi Jorgen,
STATUS
------
Not approved.
COMMENTS
--------
My general concern is that the patch you propose is incomplete. I think the
intention of the bug report is to point out that various error branches in the
code are not covered in tests. A patch fixing this should try to achieve
completeness in some sense. I'm not even sure how to handle it properly. Should
we fix it in small job batches like this or make a general plan for covering all
branches in the code, split it into small tasks and then do it one by one.
I think I prefer the second approach. This would mean creating a WL for code
coverage testing, do the planning there and then work on resulting tasks. In
that case I'd cancel this bug with a note that it will be handled by the WL.
If you disagree and think that it is better to work on this bug as a separate
task then please try to define some criteria for the completeness of the
injections you add. I.e., I'm asking for defining more precisely what this fix
is supposed to do.
As I see it now, you are covering branches inside
Backup_restore_ctx::do_restore() but not all of them (e.g., you do not simulate
error(s) in restore_table_data()). Also, input stream is read in
Backup_restore_ctx::prepare_for_restore() and you don't cover this code. This is
why I think that the patch is not complete enough.
DETAILS
-------
See some more detailed comments/suggestions inlined below.
Jorgen Loland wrote:
> #At file:///localhome/jl208045/mysql/mysql-6.0-backup-41012/
>
> 2739 Jorgen Loland 2008-12-16
> Bug#41012 - Error checks when reading backup image not covered by tests.
>
> Previously, there were no tests of RESTORE when errors occured. This patch adds
> debug hooks that can be used to inject error into the RESTORE code.
> modified:
> mysql-test/suite/backup/r/backup_errors.result
> mysql-test/suite/backup/t/backup_errors.test
> sql/backup/be_default.cc
> sql/backup/data_backup.cc
> sql/backup/kernel.cc
> sql/backup/stream.cc
> sql/backup/stream.h
>
> per-file messages:
> mysql-test/suite/backup/r/backup_errors.result
> Add tests that inject errors into RESTORE code
> mysql-test/suite/backup/t/backup_errors.test
> Add tests that inject errors into RESTORE code
> sql/backup/be_default.cc
> Add debug hook for testing purposes that can be used to inject error into
> Restore::send_data
> sql/backup/data_backup.cc
> Add debug hook for testing purposes that can be used to inject error into RESTORE
> method restore_table_data
> sql/backup/kernel.cc
> Add debug hook for testing purposes that can be used to inject error into various
> RESTORE methods
> sql/backup/stream.cc
> Add debug hook for testing purposes that can be used to inject error into method
> Input_stream::next_chunk
> sql/backup/stream.h
> Add debug hook for testing purposes that can be used to inject error into method
> read_summary
...
> === modified file 'sql/backup/data_backup.cc'
> --- a/sql/backup/data_backup.cc 2008-12-15 09:22:24 +0000
> +++ b/sql/backup/data_backup.cc 2008-12-16 09:30:16 +0000
...
> @@ -1564,6 +1567,10 @@ int restore_table_data(THD *thd, Restore
> */
> DBUG_ASSERT(snap && drvr);
>
> + /*
> + Note: For testing, error can be injected in default driver
> + send_data by using dbug hook 'backup_default_send_data'
> + */
> ret= drvr->send_data(buf);
Wouldn't it be better to inject error here: DBUG_EXECUTE_IF(..., ret= ERROR).
Then it can be used independent of the type of restore driver.
> switch (ret) {
>
>
> === modified file 'sql/backup/kernel.cc'
> --- a/sql/backup/kernel.cc 2008-12-10 15:53:06 +0000
> +++ b/sql/backup/kernel.cc 2008-12-16 09:30:16 +0000
...
> @@ -1131,6 +1136,17 @@ int Backup_restore_ctx::restore_triggers
>
> case BSTREAM_IT_TRIGGER:
> DBUG_ASSERT(obj->m_obj_ptr);
> +
> + DBUG_EXECUTE_IF("restore_trigger",
> + {
> + /* Mimic error in restore of trigger */
> + delete it;
> + delete dbit;
> + int err= report_error(ER_BACKUP_CANT_RESTORE_TRIGGER,
> + obj->describe(buf));
> + DBUG_RETURN(fatal_error(err));
> + });
> +
> if (obj->m_obj_ptr->create(m_thd))
Suggestion 1: Inject error inside si_objects?
Suggestion 2: Do it like this
int ret= obj->m_obj_ptr->create(m_thd);
DBUG_EXECUTE_IF("restore_trigger", ret= ERROR);
if (ret)
...
Then the code handling error condition does not have to be repeated which is
less error prone.
> {
> delete it;
> @@ -1219,6 +1235,14 @@ int Backup_restore_ctx::do_restore(bool
>
> disable_fkey_constraints(); // Never errors
>
> + DBUG_EXECUTE_IF("restore_read_meta_data",
> + /* Mimic behavior of a failure in read_meta_data */
> + {
> + if (!error_reported())
> + report_error(ER_BACKUP_READ_META);
> + DBUG_RETURN(fatal_error(ERROR));
Better, fatal_error(ER_BACKUP_READ_META).
> + });
> +
I think it is better to do it the same as with read_summary(), i.e., do error
injection inside read_meta_data() function.
> err= read_meta_data(info, s); // Can log errors via callback functions.
> if (err)
> {
Rafal
