STATUS
======
Not approved, yet. Some chranges requested.
REQUESTS
========
1. Is a test that checks that correct master binlog pos is recorded
part of future plans? (By the way, I could not find any reference
to the test work log in WL#4209. Please add)
2. I do not understand why the binlog handling is necessary in
LOGGER::backup_history_log_write/backup_progress_log_write.
Please, explain. See [1] below.
3. Please, verify that restore may not enable slave connections after
restore when it was disabled before restore. [3]
4. Please, use SET SESSION debug="-d" to turn off set_backup_id.
Otherwise, test will generate 20MB of master.err which takes a very
long time to parse at the end of the test. [4]
5. I do not understand output in test "Compare should be 1" etc. I
do not find any comparison at that point. [6]
6. Typo [7]
SUGGESTIONS
===========
1. Use one if-statement instead of three. [2]
2. Is it strictly necessary to have different behavior for the
set_backup_id debug on master and slave? I think can create
confusion since many people may assume that the behavior will be
the same.
3. Test: Why both counting and displaying all content? Seems like
latter should suffice as a count check, too. [5]
4. See [8]. Seems like a new test case is started, but comments does
not highlight that.
COMMENTARY
==========
What is the purpose of FLUSH BACKUP LOGS, and why is it needed in the
new test? FLUSH BACKUP LOGS does not seem to be documented in manual.
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
>
> per-file messages:
> mysql-test/suite/rpl/r/rpl_backup.result
> New result file.
> mysql-test/suite/rpl/t/rpl_backup.test
> New test for backup and replication integration.
> sql/backup/backup_kernel.h
> Added attribute to allow close() to reengage the binlog if it was
turned off.
> sql/backup/data_backup.cc
> Add code to record master's binlog information when backup run on
a slave.
> sql/backup/kernel.cc
> Add calls to service interface to allow the following during restore:
> * Disable slave connections when running with connected slaves
> * Issue incident report when binlog is engaged
> * Issue error if restore run on a slave
> * Enable slave connections after restore
> * Save master's binlog information when restore run on a
(disconnected) slave :
> used when slave is connected to master after restore begins.
> sql/backup/logger.h
> Added new method to report master's binlog position in
backup_progress log.
> sql/log.cc
> Add code to disable binlog of backup log entries.
> sql/share/errmsg.txt
...
> +# We are using debug insertion to set the first backup_id to
> +# 500 so we can expect the output of this operation to be 500.
> +--echo Backup_id = 500.
> +BACKUP DATABASE rpl_backup TO 'rpl_bup_m1.bak';
> +
> +SET SESSION debug="d";
[4] Please use SET SESSION debug="-d" to avoid turning on tracing.
> +
> +#
> +# Now check slave to see if backup logs are affected.
> +#
> +connection slave;
> +
> +--echo Should have count(*) = 0.
> +SELECT count(*) FROM mysql.backup_history;
> +
> +--echo Check backup log. Compare should be 1
> +--echo Perform backup on slave and examine backup log.
> +--echo Check backup log. Compare should be NULL
[6] What does this mean? I do not see any comparison here.
> +FLUSH BACKUP LOGS;
> +PURGE BACKUP LOGS;
> +
> +#
> +# Use Case 3 - Backup performed on a slave (part 1 of 2)
> +# Test backup on slave but slave has not slaves.
> +#
> +
> +#
> +# Now test read of backupid with known id using debug insertion
> +#
> +
> +SET SESSION debug="d,set_backup_id";
> +
> +# We are using debug insertion to set the first backup_id to
> +# 600 so we can expect the output of this operation to be 500.
[7] "to be 600", I guess.
> +--echo Backup_id = 600.
> +BACKUP DATABASE rpl_backup TO 'rpl_bup_s1.bak';
> +
> +SET SESSION debug="d";
[4] Please use SET SESSION debug="-d" to avoid turning on tracing.
> +
> +--echo Check saving of master's binlog information.
> +--echo Should have count(*) = 1.
> +SELECT count(*) FROM mysql.backup_progress
> +WHERE backup_id = 600 AND notes LIKE 'Recording master binlog
information%';
> +
> +--echo Should have count(*) = 1.
> +SELECT count(*) FROM mysql.backup_history;
> +
> +--echo Verify backup run on master does not advance binlog pos.
[8] Seems like this starts a new test case. Please, use comments
similar to when other test cases start.
...
> +--echo Checking affect on replication.
> +INSERT INTO rpl_backup.t1 VALUES (44), (55), (66);
> +SELECT count(*) FROM rpl_backup.t1;
> +SELECT * FROM rpl_backup.t1 ORDER BY a;
[5] Does not seem necesssary to both count and display content.
> +
> +sync_slave_with_master;
> +connection slave;
> +SELECT count(*) FROM rpl_backup.t1;
> +SELECT * FROM rpl_backup.t1 ORDER BY a;
[5] Ditto
...
> @@ -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())
> + {
> + m_engage_binlog= TRUE;
> + obs::engage_binlog(FALSE);
> + }
> +
[2] Why not use a single if statement here?
> return info;
> }
>
> @@ -923,6 +956,17 @@ int Backup_restore_ctx::close()
>
> using namespace backup;
>
> + /*
> + Allow slaves connect after restore is complete.
> + */
> + obs::disable_slave_connections(FALSE);
[3] Are you sure it was not TRUE before restore started?
...
>
> + /*
> + If binlog is turned on, turn it off and don't replicate the
> + updates to the backup logs.
> + */
> + if (obs::is_binlog_engaged())
> + {
> + obs::engage_binlog(FALSE);
> + reengage_binlog= TRUE;
> + }
[1] Is this the right place for this code? Should not this been turned
off for the entire connection doing backup/restore? Does not seem
like the right level to me.
--
Øystein
