Hi Oystein,
Thanks for the speedy review! Here are my replies to your comments. A
new patch is on the way. Anything not specifically mentioned was fixed
without comment.
> 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)
I have made a note in WL#4209 about WL#4612 (testing) and a note in
WL#4612 about adding this form of test.
> 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.
We must ensure the entries written to the backup logs are not replicated
to the slave. They should never be replicated. See R06.
> 3. Please, verify that restore may not enable slave connections after
> restore when it was disabled before restore. [3]
This is ok. We set disable_slaves = FALSE regardless of whether we
turned it on (set to TRUE) or not. So this is safe to call without a
guard or checking to see if it was set to TRUE previously.
> 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]
Done. This must be a new and recent change to MTR or something because
it never caused this problem before. Can you please check other backup
tests and if you find them in those tests issue a bug report to fix
them? Thanks.
> 5. I do not understand output in test "Compare should be 1" etc. I
> do not find any comparison at that point. [6]
This was left over from a failed attempt to test one of the items that
is now in WL#4612. I have removed it.
> 6. Typo [7]
Fixed.
> SUGGESTIONS
> ===========
> 1. Use one if-statement instead of three. [2]
Done.
> 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.
Yes. I want to show that the backup_id's generated on the master are
different from those on the slave. Thus, I use 500+ for the master and
600+ for the slave.
> 3. Test: Why both counting and displaying all content? Seems like
> latter should suffice as a count check, too. [5]
Ok, I picked the more detailed of the two.
> 4. See [8]. Seems like a new test case is started, but comments does
> not highlight that.
Actually, it's a subcase really. I added comments to explain it better.
> 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.
FLUSH BACKUP LOGS ensures all of the cached log writes are written to
the file and/or table. It is a recent addition done in WL#4296 which
Paul is in the process of revising. See
http://dev.mysql.com/doc/refman/6.0/en/backup-database-restore-monitoring.html
for the work to date. I have sent an email asking Paul to document it.
...
> > + /*
> > + 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?
A single statement is used now.
> > 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?
Yes, we are safe. Default value is FALSE so setting to FALSE again won't
affect anything. One less guard... ;)
> > + /*
> > + 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.
Rafal has suggested an alternative and I have implemented it.
Chuck
