Hi Chuck,
I had a quick look at your patch. I haven't checked the test case - only the
code changes. Please see my comments inlined below.
I also wonder about the way you satisfy R02. What you do is to note master's
binlog position in the backup_progress log. But the position is not saved in the
backup image and thus will not be shown on RESTORE.
Saving it in the backup image will be very difficult, because that requires
changing the format of the backup image. But if we don't do that, does that
still satisfy requirement R02?
Chuck Bell wrote:
> #At file:///C:/source/bzr/mysql-6.0-wl-4209/
>
> 2720 Chuck Bell 2008-10-28
> 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
(cut)
> === 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 00:10:42 +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);
> +
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().
> === modified file 'sql/backup/kernel.cc'
> --- a/sql/backup/kernel.cc 2008-10-28 14:17:05 +0000
> +++ b/sql/backup/kernel.cc 2008-10-29 00:10:42 +0000
(cut)
> @@ -785,6 +794,30 @@ Backup_restore_ctx::prepare_for_restore(
>
> m_state= PREPARED_FOR_RESTORE;
>
> + /*
> + Do not allow slaves to connect during a restore.
> + */
> + if (obs::is_binlog_engaged())
> + obs::disable_slave_connections(TRUE);
> +
> + DEBUG_SYNC(m_thd, "after_disable_slave_connections");
> +
> + /*
> + If the binlog is turned on, write a RESTORE_EVENT as an
> + incident report into the binary log.
> + */
> + if (obs::is_binlog_engaged())
> + obs::write_incident_event(m_thd, obs::RESTORE_EVENT);
> +
> + /*
> + Turn off binlog during restore.
> + */
> + if (obs::is_binlog_engaged())
> + {
Suggestion: Why not put disable_slave_connections() and write_incident_event() here?
(cut)
> @@ -1250,6 +1294,13 @@ int Backup_restore_ctx::do_restore()
> report_vp_time(info.get_vp_time(), FALSE); // FALSE = do not write to progress
> log
> if (info.flags & BSTREAM_FLAG_BINLOG)
> 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())
> + report_master_binlog_pos(active_mi);
>
Q: Why is it done here and in write_table_data()?
Note: Using global variable active_mi breaks encapsulation of server's
replication infrastructure behind services API.
> === 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 00:10:42 +0000
> @@ -5,6 +5,7 @@
> #include <backup_stream.h>
> #include <backup/error.h>
> #include "si_logs.h"
> +#include "rpl_mi.h"
>
> namespace backup {
>
> @@ -56,6 +57,7 @@ class Logger
> void report_state(enum_backup_state);
> void report_vp_time(time_t, bool);
> void report_binlog_pos(const st_bstream_binlog_pos&);
> + void report_master_binlog_pos(Master_info *active_mi);
Note: Using the Master_info structure breaks encapsulation of server's
replication behind services API.
> void report_driver(const char *driver);
> void report_backup_file(char * path);
> void report_stats_pre(const Image_info&);
> @@ -267,6 +269,23 @@ void Logger::report_binlog_pos(const st_
> DBUG_ASSERT(backup_log);
> backup_log->binlog_pos(pos.pos);
> backup_log->binlog_file(pos.file);
> +}
> +
> +/*
> + Get master's binlog information.
Wrong description - this function does not "get" master's binlog information, it
logs it.
> +*/
> +inline
> +void Logger::report_master_binlog_pos(Master_info *active_mi)
> +{
> + char buff[1024];
> +
> + if (active_mi)
> + {
> + sprintf(buff,
> + "Recording master binlog information. binlog file = '%s', position = %d.",
> + active_mi->master_log_name, active_mi->master_log_pos);
> + backup_log->write_progress(0, 0, 0, 0, 0, 0, (char *)&buff);
> + }
> }
(cut)
Rafal
