Bug #52966 Backup kernel: wrong logic for handling backup drivers.
Submitted: 20 Apr 2010 7:24 Modified: 22 Apr 2010 10:24
Reporter: Rafal Somla Email Updates:
Status: Verified Impact on me:
None 
Category:MySQL Server: Backup Severity:S3 (Non-critical)
Version:mysql-next-mr-backup OS:Any
Assigned to: CPU Architecture:Any
Triage: Triaged: D2 (Serious)

[20 Apr 2010 7:24] Rafal Somla
Description:
A READY reply from drv->get_data() method indicates end of initial or prepare phase of the backup synchronization protocol. This is not correctly captured in the Backup_pump::pump() method (data_backup.cc:1540). The logic wrongly intercepts READY reply if the pump is in WAITING state, but it should rather do it when the pump is in PREPARING state.

This error in the logic does not affect the current behaviour, because current drivers do not rely on a correct handling of READY reply. But in principle, it is possible that someone writes a backup driver according to the specifications and the system will not work correctly with that driver (this has happened when I worked on a prototype backup driver).

How to repeat:
Code inspection: Backup_pump::pump() method, after a call to m_drv->get_data() (data_backup.cc:1536).

Suggested fix:
=== modified file 'sql/backup/data_backup.cc'
--- sql/backup/data_backup.cc   2009-12-21 07:38:02 +0000
+++ sql/backup/data_backup.cc   2010-04-16 12:23:42 +0000
@@ -1508,7 +1508,7 @@ int Backup_pump::pump(size_t *howmuch)
 
         if( state == backup_state::INIT )
           state= backup_state::WAITING;
-        else if( state == backup_state::WAITING )
+        else if( state == backup_state::PREPARING )
           state= backup_state::READY;
 
       case OK:
[22 Apr 2010 9:50] Rafal Somla
Here is yet another error in the backup driver handling logic which I report 
under this bug.

A backup driver can send DONE reply when sending last chunk of snapshot 
data. Currently, the chunk received from the driver is ignored if the driver 
sends DONE reply at the same time. This is wrong: backup kernel should save 
the chunk in the image and then mark that the driver has finished its job.

Again, this error does not affect the current system since existing backup 
drivers never send data and DONE reply together. But it can show up when new 
backup drivers are developed.

A fix for this error:

=== modified file 'sql/backup/data_backup.cc'
--- sql/backup/data_backup.cc   2010-04-19 14:08:16 +0000
+++ sql/backup/data_backup.cc   2010-04-22 09:47:59 +0000
@@ -1512,7 +1512,7 @@ int Backup_pump::pump(size_t *howmuch)
           state= backup_state::READY;
 
       case OK:
-
+      case DONE:
         if (m_buf.last)
         {
           mark_stream_closed(m_buf.table_num);
@@ -1530,6 +1530,8 @@ int Backup_pump::pump(size_t *howmuch)
         else
           m_bw.drop_buf(m_buf);         // Never errors.
 
+        if (res == DONE)
+          state= backup_state::DONE;
         break;
 
       case PROCESSING:
@@ -1543,8 +1545,6 @@ int Backup_pump::pump(size_t *howmuch)
         state= backup_state::ERROR;
         return ERROR;
 
-      case DONE:
-        state= backup_state::DONE;
 
       case BUSY:
         m_bw.drop_buf(m_buf);           // Never errors.
@@ -1554,8 +1554,7 @@ int Backup_pump::pump(size_t *howmuch)
     } // if (mode == READING)
 
     if (mode == WRITING
-        && state != backup_state::ERROR
-        && state != backup_state::DONE)
+        && state != backup_state::ERROR)
     {
       backup::Block_writer::result_t res= m_bw.write_buf(m_buf);
[22 Apr 2010 9:52] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/106346

2948 Rafal Somla	2010-04-22
      Fix of the wrong logic in backup kernel for handling DONE reply
      from a backup driver (see BUG#52966).
[22 Apr 2010 10:24] Rafal Somla
Changing state - wrong patch picked-up by the bug database.