STATUS
------
Undecided.
REQUESTS
--------
1. Please explain why the code is duplicating the error. I don't think
it should be doing this.
2. Spelling errors.
3. Too many characters on one line.
DETAILS
-------
> Bug#47940 - Backup: Implementation of save_vp_info() violates its
> specification.
>
> This patch fixes problems in the backup synchronization logic:
>
> - In case of error, all drivers which have ben locked, should be
[2] been
> unlocked before we abort operation.
>
> - Potentially time consuming error reporting should not be done inside
> the synchronization phase, but only after all drivers have been
> unlocked.
> @ mysql-test/suite/backup/r/backup_errors_debug_3.result
> This test case triggers errors during unlock() of backup drivers. Since two
> drivers are used, the unlock error should be reported twice. This patch
> fixes it.
[1] I don't think we should report the same error twice. I understand
this comment and after reading the code it is fine, however, I think we
should be generating an error that includes the driver name or some
such. That is, so the user knows these errors came from two different
places. Either that or only report the error once.
> @ sql/backup/data_backup.cc
> - Add new LOCKED driver state.
> - Add Scheduler::report_vp_errors() method.
> - Add Scheduler members m_failed_lock and m_vp_info_error which
> store information about errors encountered in the synchronization
> phase.
> - Remove error reporting from time critical methods.
> - Refactoring of the synchronization phase.
> - Add new operator and fix comments in Pump_iterator class.
> - Refactor Pump_iterator::lock()/unlock() methods: make sure all
> drivers which have been locked are unlocked in case of error.
> - Refactor Backup_pump methods: remove error reporting and
> handle the new LOCKED state.
Commentary: This makes me nervous when we include so much refactoring in
a bug report. But I don't see any logic errors, just questioning the
need to do this.
>
> modified:
> mysql-test/suite/backup/r/backup_errors_debug_3.result
> sql/backup/data_backup.cc
> === modified file 'mysql-test/suite/backup/r/backup_errors_debug_3.result'
> --- a/mysql-test/suite/backup/r/backup_errors_debug_3.result 2009-11-24 16:47:23
> +0000
> +++ b/mysql-test/suite/backup/r/backup_errors_debug_3.result 2009-11-25 10:37:26
> +0000
> @@ -626,6 +626,7 @@ ERROR HY000: Can't unlock DEBUG TEST bac
> SELECT * FROM mysql.backup_progress WHERE error_num <> 0;
> backup_id object error_num notes
> # #### Can't unlock DEBUG TEST backup driver after creating the validity point
> +# #### Can't unlock DEBUG TEST backup driver after creating the validity point
> PURGE BACKUP LOGS;
> #
> # Turn off debug SESSION.
>
> === modified file 'sql/backup/data_backup.cc'
> --- a/sql/backup/data_backup.cc 2009-10-21 13:32:24 +0000
> +++ b/sql/backup/data_backup.cc 2009-11-25 10:37:26 +0000
> @@ -42,6 +42,7 @@ struct backup_state {
> WAITING, ///< Waiting for others to finish init ph. (phase 2).
> PREPARING, ///< Preparing for @c lock() call (phase 3).
> READY, ///< Ready for @c lock() call (phase 4)
> + LOCKED, ///< After @c lock() and before @c unlock().
> FINISHING, ///< Final data transfer (phase 7).
> DONE, ///< Backup complete.
> SHUT_DOWN, ///< After @c end() call.
> @@ -67,6 +68,7 @@ struct backup_state {
> name[WAITING]= "WAITING";
> name[PREPARING]= "PREPARING";
> name[READY]= "READY";
> + name[LOCKED]= "LOCKED";
> name[FINISHING]= "FINISHING";
> name[DONE]= "DONE";
> name[SHUT_DOWN]= "SHUT DOWN";
> @@ -256,6 +258,7 @@ public:
> int prepare();
> int lock();
> int unlock();
> + int report_vp_errors();
>
> uint init_count; ///< Nr of drivers sending init data.
> uint prepare_count; ///< Nr of drivers preparing for lock.
> @@ -286,11 +289,15 @@ private:
> Output_stream &m_str; ///< Stream to which we write.
> bool cancelled; ///< True if backup process was cancelled.
>
> + Backup_pump *m_failed_lock; ///< If not NULL, points at driver which failed in
> lock() call.
> + bool m_vp_info_error; ///< TRUE if errors were detected while collecting VP
> info in the synchronization phase.
[3] Long line please fix.
...
Chuck
