MySQL Bugs: #42999: If BACKUP to pipe fails, pipe will be removed.

Bug #42999	If BACKUP to pipe fails, pipe will be removed.
Submitted:	19 Feb 2009 7:04	Modified:	29 Aug 2009 23:15
Reporter:	Rafal Somla	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: Backup	Severity:	S3 (Non-critical)
Version:	6.0	OS:	Any
Assigned to:	Rafal Somla	CPU Architecture:	Any

Description:
If BACKUP to a named pipe fails, the pipe will be removed.

How to repeat:
When executing this test case, the --file_exists at the end fails, indicating that the piep was removed.

===================================
let $bdir= `select @@datadir`;
--exec mkfifo $bdir/db1_pipe
connect (con1,localhost,root,,);

--connection default
send BACKUP DATABASE test TO 'db1_pipe';

--connection con1
--exec cat $bdir/db1_pipe > /dev/null

--connection default
--error 0,1049
reap;

--file_exists $bdir/db1_pipe

Suggested fix:
Revise the logic for removing backup file in case of failed backup (in sql/backup/kernel.cc).

Here is a fixed test case for repeating the problem:

let $bdir= `select @@datadir`;
--exec mkfifo $bdir/db1_pipe
connect (con1,localhost,root,,);

--connection default
send BACKUP DATABASE db1 TO 'db1_pipe';

--connection con1
--exec cat $bdir/db1_pipe > /dev/null

--connection default
--error 0,1049
reap;

--file_exists $bdir/db1_pipe
--exit

REFINED PROBLEM DESCRIPTION
===========================
In case BACKUP operation is aborted before completion (e.g., due to an error), Backup_restore_ctx::close() method removes the backup image file (line 1155 in backup/kernel.cc). However, this should be done only if the file was previously created, not if it existed beforehand as is the case for named pipes.

PROPOSED SOLUTION
=================
Add backup::Stream::remove() method which is supposed to not only close the stream but also remove the underlying file if it was created when the stream was opened. Use it (instead of backup::Stram::close()) in the Backup_restore_ctx::close() method to remove file after incomplete backup operation. The remove() method will ensure that named pipes are not deleted.

Normally, it is possible to open a backup::Output_stream only if there is no file with the given path. In that case the file is created. The only exception is opening a named pipe, in which case the pipe should exist and is not created.

The remove() method will remove a file which was created in open() but will not remove a named pipe, which was not created in open(). The two situations can be distinguished by looking at m_flags member. It contains O_CREAT flag iff a regular file was crated and opened (see backup::Output_stream::open()). Thus the logic in remove() will be as follows:

Output_stream::remove()
{
  close(); // close the file first
  if (m_flags & O_CREAT)
  {
    // delete the path
  }
}

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/80568

2858 Rafal Somla	2009-08-11
      Bug #42999 - If BACKUP to pipe fails, pipe will be removed
      
      In case BACKUP operation is aborted before completion (e.g., due to an error),
      Backup_restore_ctx::close() method removes the backup image file which was 
      created when backup stream was opened. An exception is backup to a named pipe 
      which is never created but should exist prior to invoking BACKUP command.
      
      Before this patch, the code did not make exception for named pipes which were 
      removed like regular files. The patch adds logic for detecting this exception 
      and correctly handling named pipes.
     @ mysql-test/suite/backup/t/backup_pipe.test
        Added scenario testing the issue.
     @ sql/backup/kernel.cc
        In Backup_restore_ctx::close() call Stream::close() or Stream::remove() as appropriate. Move error reporting code into the latter methods.
     @ sql/backup/stream.cc
        - Make stream methods report status via integer return code, not a bool value.
        - Remove unused rewind() methods.
        - Move error reporting from kernel.cc to close() methods.
        - Implementation of Output_stream::remove() method.
     @ sql/backup/stream.h
        - Make stream methods report status via integer return code, not a bool value.
        - Add remove() method to stream class.
        - Delete unused rewind() methods.

Patch approved.

Good to push

Pushed to team tree (server version 5.4.4).
revision-id:rafal.somla@sun.com-20090811110931-piwv6pc448r1i70h

Pushed into 5.4.4-alpha (revid:alik@sun.com-20090828100112-r73xkx8dhekz5bbb) (version source revid:jorgen.loland@sun.com-20090818084812-rlq2mh37241doswu) (merge vers: 5.4.4-alpha) (pib:11)

Not in any released version. No changelog entry needed.