Rafal,
This is a formal reply to your first review. I have a new patch ready
and will submit that soon for review.
I have also asked that this be put on the backup decision meeting agenda
in anticipation of your rejection of my replies.
Note: I have implemented all of Jorgen's requests and suggestions where
possible.
> REQUESTS
> --------
> 1. Use a different method for testing errors during insertion of various
> object types into the catalogue (ER_BACKUP_CATALOG_ADD_*) and when
> restoring objects (ER_BACKUP_CANT_RESTORE_*/ER_BACKUP_GET_META_*).
I see your request as an alternative implementation that is actually
more complex that my original patch and not a case where the code is
wrong. Your version is more complex because the three areas where this
occurs will mean three copies of something very similar to what I have
with sync points instead of debug execute if. Furthermore, your version
will make the test cases much more complex. Therefore, I must reject
this alternative as it does not invalidate nor improve the original patch.
STATUS: alternative implementation is rejected
> 2. Do not set Backup_pump::m_name member upon error insertion - it
> should be already correctly initialized.
As I have explained, this is so that the tests will be deterministic.
Reminder: This form of testing can only show the error handling for a
given error is possible, not that it is 100% error free.
Since you do not believe me that the data I have masked is
non-deterministic, consider this modification:
(data_backup.cc @ 1279)
from:
// Error code insertion
DBUG_EXECUTE_IF("ER_BACKUP_PREPARE_DRIVER",
res= ERROR; m_name= "Default";);
to:
// Error code insertion
DBUG_EXECUTE_IF("ER_BACKUP_PREPARE_DRIVER", res= ERROR;);
As recorded without m_name change:
# Show warning/error from progress and history log
# if they exist.
#
SELECT * FROM mysql.backup_progress WHERE error_num <> 0;
backup_id object error_num notes
# #### Default backup driver can't create its validity point
PURGE BACKUP LOGS;
And after several runs, this happens. Ooops, who'da thunk it...it really
is non-deterministic!
CURRENT_TEST: backup.backup_errors_debug_2
---
d:/source/bzr/mysql-6.0-wl-4722/mysql-test/suite/backup/r/backup_errors_deb
g_2.result 2009-08-20 21:44:42.337506400 +0300
+++
d:\source\bzr\mysql-6.0-wl-4722\mysql-test\suite\backup\r\backup_errors_deb
g_2.reject 2009-08-20 21:45:39.293106400 +0300
@@ -53,7 +53,7 @@
# Execute backup
#
BACKUP DATABASE backup_test TO 'backup_test_err.bak';
-ERROR HY000: Default backup driver can't create its validity point
+ERROR HY000: MyISAM backup driver can't create its validity point
#
# Test case for error ER_BACKUP_CREATE_VP PASSED.
#
@@ -63,7 +63,7 @@
#
SELECT * FROM mysql.backup_progress WHERE error_num <> 0;
backup_id object error_num notes
-# #### Default backup driver can't create its validity
point
+# #### MyISAM backup driver can't create its validity point
PURGE BACKUP LOGS;
You may believe it is a bug and if so you're welcome to open a bug
report but I do not agree and see this as merely expected behavior. I
have explained this in another email so I will not rehash that here.
STATUS: rejected
> 3. Remove driver list cutting during error insertion in
> restore_table_data() or explain why it is needed.
I do this to avoid the non-deterministic problem.
STATUS: rejected
> 4. Directly trigger error when testing for ER_BACKUP_STOP_RESTORE_DRIVERS.
This does *not* work. I have changed the code exactly as you say and the
test case fails. That is because we are simulating an error here and the
normal path of execution does not enter the section you requested I move
the code to.
STATUS: rejected
> 5. Also trigger direct error in send_reply() method (ER_BACKUP_SEND_REPLY).
Done. BUG#46594 set to 'not a bug'.
STATUS: code changed
> 6. Make sure that errors are simulated directly after a call to function
> reporting error, so that all code following it is executed.
Done.
STATUS: code changed
> 7. Remove extra variable if a single one can be used.
Done.
STATUS: code changed
> 8. When triggering ER_BACKUP_RESTORE_2 do not change type of the
> operation stored in the context class.
Done.
STATUS: code changed
> 9. Remove assertion where error should be reported
> (ER_BACKUP_WRONG_TABLE_BE).
I have made the change as you requested and it does *not* work. In the
case of snap= NULL, the server crashes. In the case of snap_num + 1 the
test fails.
STATUS: rejected
> 10. Remove error insertion for ER_BACKUP_INTERRUPTION since it is
> already extensively tested in backup_interruption test from
> backup_engines suite and this without using error injection.
There are no tests that test this error message or its conditions. I
have searched the latest tree and there is also not test result that
contains this error message.
STATUS: rejected
> SUGGESTIONS
> -----------
> 11. Whenever possible, test the original error branch, not a copy of it.
Your specific example for the binlog test still crashes the server. See
BUG#46574.
Your specific example for ER_BACKUP_UNEXPECTED_DATA will not work
because, again, we are executing through the normal non-error portion of
the code and your request has me move the debug insertion to inside an
error condition. This will not work and the test case fails.
STATUS: rejected
> 12. Other minor suggestions.
All were changed except the first which forced the server to crash and
the third which I do not consider relevant.
STATUS: partially accepted
> 13. Make include/basic_data.inc database independent so that it can be
> easily used from other tests.
This is not so easy to do. Consider the event, trigger, procedure,
function, and grant statement. Parameterizing this would require extra
work. I choose to leave it as it is.
Chuck
