STATUS
======
Not approved. Saving master's binlog position and the test need to be improved.
COMMENTARY
==========
The proposed test script is not covering all the functionality implemented in
the code (e.g slaves should not be able to connect during on-going RESTORE on
master). It also uses doubtful testing strategies which would to be discussed
before I can accept them. I'll send a separate email with what I consider a
minimal set of properties which should be tested, before I can comfortably
accept the patch.
In my previous review I raised this issue:
"This might report incorrect position. We want to capture the exact position at
the VP time, i.e in between sch.lock() and sch.unlock() calls. Since we can't do
anything time consuming during that time, we should use the same trick as for
reporting binlog position:
sch.lock();
<read master's binlog position from active_mi structure and store it for
reporting later>;
sch.unlock();
<report the saved master's binlog position>
Please explain why report_master_binlog_pos() is called here and then also later
in Backup_restore_ctx::do_restore()."
It has not been addressed in this patch and I think it should. Here is my
proposition how to do it:
=== modified file 'sql/backup/data_backup.cc'
--- sql/backup/data_backup.cc 2008-10-14 12:08:56 +0000
+++ sql/backup/data_backup.cc 2008-10-30 10:34:00 +0000
@@ -600,6 +600,16 @@ int write_table_data(THD* thd, Backup_in
}
/*
+ If this is a slave, save its master binlog coordinates.
+ */
+ st_bstream_binlog_pos master_pos; // alternatively, add it to Image_info class
+ if (obs::is_slave())
+ {
+ master_pos.pos= active_mi->master_log_pos;
+ master_pos.file= active_mi->master_log_name;
+ }
+
+ /*
Save VP creation time.
*/
vp_time= my_time(0);
@@ -627,6 +637,9 @@ int write_table_data(THD* thd, Backup_in
info.m_ctx.report_binlog_pos(info.binlog_pos);
}
+ if (obs::is_slave())
+ info.m_ctx.report_master_binlog_pos(master_pos);
+
info.m_ctx.report_state(BUP_RUNNING);
DEBUG_SYNC(thd, "after_backup_binlog");
REQUESTS
========
1. Address the issue of reporting correct master's binlog position.
2. Fix test case to cover all the implemented functionality (requirementd for
the test to be detailed in a separate email).
DETAILS
=======
Chuck Bell wrote:
> #At file:///C:/source/bzr/mysql-6.0-wl-4209/
>
> 2720 Chuck Bell 2008-10-29
> WL#4209 : Integrate Backup with Replication
>
> This patch makes changes to the backup system to allow backup to
> be used with replication.
> added:
> mysql-test/suite/rpl/r/rpl_backup.result
> mysql-test/suite/rpl/t/rpl_backup.test
> modified:
> sql/backup/backup_kernel.h
> sql/backup/data_backup.cc
> sql/backup/kernel.cc
> sql/backup/logger.h
> sql/log.cc
> sql/share/errmsg.txt
> sql/si_logs.cc
> sql/si_logs.h
>
> === modified file 'sql/backup/data_backup.cc'
> --- a/sql/backup/data_backup.cc 2008-10-14 12:08:56 +0000
> +++ b/sql/backup/data_backup.cc 2008-10-29 19:10:00 +0000
> @@ -627,6 +627,13 @@ int write_table_data(THD* thd, Backup_in
> info.m_ctx.report_binlog_pos(info.binlog_pos);
> }
>
> + /*
> + If we are a connected slave, write master's binlog information to
> + the progress log for later use.
> + */
> + if (obs::is_slave())
> + info.m_ctx.report_master_binlog_pos(active_mi);
> +
Request: Address the issue of possible change of master's position between the
VP time and the time it is reported here.
> info.m_ctx.report_state(BUP_RUNNING);
> DEBUG_SYNC(thd, "after_backup_binlog");
>
>
(cut)
> === modified file 'sql/backup/logger.h'
> --- a/sql/backup/logger.h 2008-10-27 13:06:21 +0000
> +++ b/sql/backup/logger.h 2008-10-29 19:10:00 +0000
(cut)
> @@ -220,6 +222,10 @@ void Logger::report_stop(time_t when, bo
> backup_log->stop(when);
> backup_log->state(success ? BUP_COMPLETE : BUP_ERRORS);
> backup_log->write_history();
> + /*
> + Record master's binlog file and position if recorded earlier.
> + */
> + backup_log->write_master_binlog_info();
Suggestion: The backup_log->write_master_binlog_info() call writes an entry with
master binlog position to the progress log. I think this should happen when the
position is reported, not here in Logger::report_stop(). This is different from
the normal binlog position, which is stored in m_op_hist and then written to the
history log at the end of the operation.
> m_state= DONE;
> }
>
> @@ -267,6 +273,21 @@ void Logger::report_binlog_pos(const st_
> DBUG_ASSERT(backup_log);
> backup_log->binlog_pos(pos.pos);
> backup_log->binlog_file(pos.file);
> +}
> +
> +/**
> + Report master's binlog information.
> +
> + @todo Write this information to the backup image file.
> +*/
> +inline
> +void Logger::report_master_binlog_pos(Master_info *active_mi)
> +{
> + if (active_mi)
> + {
> + backup_log->master_binlog_pos((ulong)active_mi->master_log_pos);
> + backup_log->master_binlog_file(active_mi->master_log_name);
Suggestion: Call backup_log->write_master_binlog_info() here.
> + }
> }
>
> /**
>
Rafal
