#At file:///ext/mysql/bzr/backup/bug42999/ based on revid:jorgen.loland@stripped
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.
modified:
mysql-test/suite/backup/r/backup_pipe.result
mysql-test/suite/backup/t/backup_pipe.test
sql/backup/kernel.cc
sql/backup/stream.cc
sql/backup/stream.h
=== modified file 'mysql-test/suite/backup/r/backup_pipe.result'
--- a/mysql-test/suite/backup/r/backup_pipe.result 2009-02-16 12:20:31 +0000
+++ b/mysql-test/suite/backup/r/backup_pipe.result 2009-08-11 11:09:31 +0000
@@ -1,4 +1,7 @@
SET DEBUG_SYNC= 'reset';
+DROP DATABASE IF EXISTS db1;
+DROP DATABASE IF EXISTS db2;
+DROP DATABASE IF EXISTS db_non_existent;
Verify that backup can be performed to pipe
con1:
Create database, tables and load data.
@@ -25,6 +28,15 @@ con2:
Create pipe.
con1:
PURGE BACKUP LOGS;
+Start invalid backup to a pipe.
+BACKUP DATABASE db_non_existent TO 'db1_pipe';
+con2:
+Cat the output to a file.
+con1:
+ERROR 42000: Unknown database 'db_non_existent'
+Check that the pipe is still there and remove the file.
+con1:
+PURGE BACKUP LOGS;
Start backup to pipe.
BACKUP DATABASE db1 TO 'db1_pipe';
con2:
=== modified file 'mysql-test/suite/backup/t/backup_pipe.test'
--- a/mysql-test/suite/backup/t/backup_pipe.test 2009-02-13 01:00:22 +0000
+++ b/mysql-test/suite/backup/t/backup_pipe.test 2009-08-11 11:09:31 +0000
@@ -24,6 +24,9 @@ let $MYSQLD_DATADIR= `select @@datadir`;
--disable_warnings
SET DEBUG_SYNC= 'reset';
+DROP DATABASE IF EXISTS db1;
+DROP DATABASE IF EXISTS db2;
+DROP DATABASE IF EXISTS db_non_existent;
--enable_warnings
connect (con1,localhost,root,,);
@@ -65,6 +68,33 @@ connection con2;
--exec mkfifo $MYSQLD_DATADIR/db1_pipe
--exec chmod 666 $MYSQLD_DATADIR/db1_pipe
+#
+# First check that the pipe is not removed if BACKUP fails (BUG#42999).
+#
+--echo con1:
+connection con1;
+PURGE BACKUP LOGS;
+--echo Start invalid backup to a pipe.
+send BACKUP DATABASE db_non_existent TO 'db1_pipe';
+
+--echo con2:
+connection con2;
+--echo Cat the output to a file.
+--exec cat $MYSQLD_DATADIR/db1_pipe > $MYSQLD_DATADIR/db1_file
+
+--echo con1:
+connection con1;
+--replace_column 1 #
+--error ER_BAD_DB_ERROR
+reap;
+
+--echo Check that the pipe is still there and remove the file.
+--file_exists $MYSQLD_DATADIR/db1_pipe
+--remove_file $MYSQLD_DATADIR/db1_file
+
+#
+# Now test normal operation.
+#
--echo con1:
connection con1;
PURGE BACKUP LOGS;
=== modified file 'sql/backup/kernel.cc'
--- a/sql/backup/kernel.cc 2009-08-07 07:50:49 +0000
+++ b/sql/backup/kernel.cc 2009-08-11 11:09:31 +0000
@@ -1138,31 +1138,28 @@ int Backup_restore_ctx::close()
m_thd->options= m_thd_options;
- // close stream if not closed already (in which case m_steam is NULL)
-
- if (m_stream && !m_stream->close())
+ // close or remove stream if not closed already (in which case m_steam is NULL)
+ if (m_stream)
{
- // Note error, but complete clean-up
- fatal_error(report_error(log_level::ERROR, ER_BACKUP_CLOSE));
- }
-
- /*
- Remove the location if it is BACKUP operation and it has not completed
- successfully.
+ /*
+ If this is backup operation which is not completed (due to error or
+ interruption), call Stream::remove() so that the underlying file is
+ removed if it was earlier created. Otherwise, just close the stream.
- Important: RESTORE should never remove the specified backup image!
- */
- if (m_state == PREPARED_FOR_BACKUP && !m_completed)
- {
- int ret= my_delete(m_path.c_ptr(), MYF(0));
- DBUG_EXECUTE_IF("backup_remove_location_error", ret= TRUE;);
- /*
- Ignore ENOENT error since it is ok if the file doesn't exist.
- */
- if (ret && my_errno != ENOENT)
+ Important: RESTORE should never try to remove the specified backup
+ image!
+ */
+ if (!m_completed && m_state == PREPARED_FOR_BACKUP)
+ {
+ int ret= m_stream->remove(); // reports errors
+ if (ret != BSTREAM_OK)
+ fatal_error(ER_CANT_DELETE_FILE);
+ }
+ else
{
- fatal_error(report_error(log_level::ERROR, ER_CANT_DELETE_FILE,
- m_path.c_ptr(), my_errno));
+ int ret= m_stream->close(); // reports errors
+ if (ret != BSTREAM_OK)
+ fatal_error(ER_BACKUP_CLOSE);
}
}
=== modified file 'sql/backup/stream.cc'
--- a/sql/backup/stream.cc 2009-05-19 20:20:54 +0000
+++ b/sql/backup/stream.cc 2009-08-11 11:09:31 +0000
@@ -342,40 +342,25 @@ int Stream::open()
/**
Close stream
- @retval TRUE success
- @retval FALSE failure
+ @retval BSTREAM_OK success
+ @retval BSTREAM_ERROR failure
*/
-bool Stream::close()
+int Stream::close()
{
- bool ret= TRUE;
+ int ret= BSTREAM_OK;
if (m_fd >= 0)
{
if (my_close(m_fd, MYF(0)))
{
- ret= FALSE;
+ ret= BSTREAM_ERROR;
}
- DBUG_EXECUTE_IF("backup_stream_close_error", ret= FALSE;);
+ DBUG_EXECUTE_IF("backup_stream_close_error", ret= BSTREAM_ERROR;);
m_fd= -1;
}
return ret;
}
-/**
- Rewind stream
-
- @returns int result of seek to start of stream
-*/
-bool Stream::rewind()
-{
-#ifdef HAVE_COMPRESS
- /* Compressed stream cannot be rewound */
- if (m_with_compression)
- return FALSE;
-#endif
- return m_fd >= 0 && my_seek(m_fd, 0, SEEK_SET, MYF(0)) == 0;
-}
-
Output_stream::Output_stream(Logger &log, ::String *path,
bool with_compression)
@@ -415,6 +400,8 @@ int Output_stream::write_magic_and_versi
/**
Initialize backup stream after the underlying stream has been opened.
+
+ @return TRUE if initialization was successful, FALSE otherwise.
*/
bool Output_stream::init()
{
@@ -509,21 +496,20 @@ int Output_stream::open()
/**
Close backup stream
- If @c destroy is TRUE, the stream object is deleted.
-
- @retval TRUE Operation Succeeded
- @retval FALSE Operation Failed
+ @retval BSTREAM_OK success
+ @retval BSTREAM_ERROR failure
*/
-bool Output_stream::close()
+int Output_stream::close()
{
- bool ret= TRUE;
+ int ret= BSTREAM_OK;
+
if (m_fd < 0)
- return TRUE;
+ return BSTREAM_OK;
if (bstream_close(this) == BSTREAM_ERROR)
{
// Note that close failed, and continue with lower level clean-up.
- ret= FALSE;
+ ret= BSTREAM_ERROR;
}
#ifdef HAVE_COMPRESS
@@ -552,32 +538,53 @@ bool Output_stream::close()
if ((zerr= deflateEnd(&zstream)) != Z_OK)
m_log.report_error(ER_GET_ERRMSG, zerr, zstream.msg, "deflateEnd");
my_free(zbuf, MYF(0));
+ if (zerr != Z_OK)
+ ret= BSTREAM_ERROR;
}
#endif
- ret &= Stream::close();
+ ret = Stream::close();
+
+ if (ret)
+ m_log.report_error(log_level::ERROR, ER_BACKUP_CLOSE);
+
return ret;
}
-/**
- Rewind output stream so that it is positioned at its beginning and
- ready for writing new image.
-
- @retval TRUE operation succeeded
- @retval FALSE operation failed
-*/
-bool Output_stream::rewind()
+/*
+ Close stream and remove the underlying file if it was created when the
+ stream was opened.
+
+ @retval BSTREAM_OK success
+ @retval BSTREAM_ERROR failure
+*/
+int Output_stream::remove()
{
- if (bstream_close(this) != BSTREAM_OK)
- return FALSE;
-
- bool ret= Stream::rewind();
+ // First close the stream.
+ int ret1= close();
- if (!ret)
- return FALSE;
+ /*
+ If O_CREAT flag is set in m_flags then the underlying file was created
+ during openning of the stream (see Output_stream::open()) and needs to
+ be deleted. Otherwise there is nothing more to do.
+ */
+ if (!(m_flags & O_CREAT))
+ return ret1;
+
+ // Delete the underlying file.
+
+ int ret2= my_delete(m_path->c_ptr(), MYF(0));
+ DBUG_EXECUTE_IF("backup_remove_location_error", ret2= TRUE;);
+ /*
+ Ignore ENOENT error since it is ok if the file doesn't exist.
+ */
+ if (ret2 && my_errno != ENOENT)
+ m_log.report_error(log_level::ERROR, ER_CANT_DELETE_FILE,
+ m_path->c_ptr(), my_errno);
- return init();
+ return ret1 || ret2 ? BSTREAM_ERROR : BSTREAM_OK;
}
+
/**
Create file to be written to
@@ -620,6 +627,8 @@ int Input_stream::check_magic_and_versio
/**
Initialize backup stream after the underlying stream has been opened.
+
+ @return TRUE if initialization was succesful, FALSE otherwise.
*/
bool Input_stream::init()
{
@@ -712,23 +721,22 @@ int Input_stream::open()
}
/**
- Close backup stream
+ Close backup stream.
- If @c destroy is TRUE, the stream object is deleted.
-
- @retval TRUE Operation Succeeded
- @retval FALSE Operation Failed
+ @retval BSTREAM_OK success
+ @retval BSTREAM_ERROR failure
*/
-bool Input_stream::close()
+int Input_stream::close()
{
- bool ret= TRUE;
+ int ret= BSTREAM_OK;
+
if (m_fd < 0)
- return TRUE;
+ return BSTREAM_OK;
if (bstream_close(this) == BSTREAM_ERROR)
{
// Note that close failed, and continue with lower level clean-up.
- ret= FALSE;
+ ret= BSTREAM_ERROR;
}
#ifdef HAVE_COMPRESS
@@ -738,28 +746,21 @@ bool Input_stream::close()
if ((zerr= inflateEnd(&zstream)) != Z_OK)
m_log.report_error(ER_GET_ERRMSG, zerr, zstream.msg, "inflateEnd");
my_free(zbuf, (MYF(0)));
+ if (zerr)
+ ret = BSTREAM_ERROR;
}
#endif
- ret &= Stream::close();
- return ret;
-}
-/**
- Rewind input stream so that it can be read again.
+ if (Stream::close())
+ ret= BSTREAM_ERROR;
- @retval TRUE operation succeeded
- @retval FALSE operation failed
-*/
-bool Input_stream::rewind()
-{
- if (bstream_close(this) != BSTREAM_OK)
- return FALSE;
-
- bool ret= Stream::rewind();
+ if (ret)
+ m_log.report_error(log_level::ERROR, ER_BACKUP_CLOSE);
- return ret ? init() : FALSE;
+ return ret;
}
+
/// Move to next chunk in the stream.
int Input_stream::next_chunk()
{
=== modified file 'sql/backup/stream.h'
--- a/sql/backup/stream.h 2009-03-16 14:38:05 +0000
+++ b/sql/backup/stream.h 2009-08-11 11:09:31 +0000
@@ -83,9 +83,9 @@ class Stream: public fd_stream
public:
int open();
- virtual bool close();
-
- bool rewind();
+ virtual int close();
+ virtual int remove()
+ { return BSTREAM_ERROR; }
/// Check if stream is opened
bool is_open() const
@@ -125,8 +125,8 @@ public:
Output_stream(Logger&, ::String *, bool);
int open();
- bool close();
- bool rewind();
+ int close();
+ int remove();
protected:
@@ -147,8 +147,7 @@ public:
Input_stream(Logger&, ::String *);
int open();
- bool close();
- bool rewind();
+ int close();
int next_chunk();
Attachment: [text/bzr-bundle] bzr/rafal.somla@sun.com-20090811110931-piwv6pc448r1i70h.bundle
