db/pgbackrest
1
0
Fork 0
mirror of https://github.com/pgbackrest/pgbackrest.git synced 2025-05-29 22:48:16 +02:00
Code Issues Releases Activity

Fix possible truncated WAL segments when an error occurs mid-write.

Browse Source 
The file write object destructors called close() and finalized the file even if it was not completely written.  This was an issue in both the C and Perl code.

Rewrite the destructors to simply free resources (like file handles) rather than calling the close() method.  This leaves the temp file in place for filesystems that use temp files.

Add unit tests to prevent regression.

Reported by blogh.
This commit is contained in:
David Steele 2019-02-15 11:52:39 +02:00
parent 2cd204f380
commit d211c2b8b5
10 changed files with 87 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
doc/xml/release.xml
View File

@ -14,6 +14,16 @@
<release-list>
<release date="XXXX-XX-XX" version="2.11dev" title="UNDER DEVELOPMENT">
<release-core-list>
<release-bug-list>
<release-item>
<release-item-contributor-list>
<release-item-ideator id="benoit.lobréau"/>
</release-item-contributor-list>
<p>Fix possible truncated WAL segments when an error occurs mid-write.</p>
</release-item>
</release-bug-list>
<release-development-list>
<release-item>
<release-item-contributor-list>

5
lib/pgBackRest/Common/Io/Base.pm
View File

@ -105,11 +105,6 @@ sub resultSet
$self->{rhResult}{$strModule} = $xResult;
}
####################################################################################################################################
# DESTROY - call close()
####################################################################################################################################
sub DESTROY {shift->close()}
####################################################################################################################################
# Getters
####################################################################################################################################

2
lib/pgBackRest/Common/Io/Handle.pm
View File

@ -157,7 +157,7 @@ sub write
}
####################################################################################################################################
# close/DESTROY - record read/write size
# close - record read/write size
####################################################################################################################################
sub close
{

14
lib/pgBackRest/Storage/Posix/FileWrite.pm
View File

@ -185,6 +185,20 @@ sub close
return true;
}
####################################################################################################################################
# Close the handle if it is open (in case close() was never called)
####################################################################################################################################
sub DESTROY
{
my $self = shift;
if (defined($self->handle()))
{
CORE::close($self->handle());
undef($self->{fhFile});
}
}
####################################################################################################################################
# Getters
####################################################################################################################################

13
src/perl/embed.auto.c
View File

@ -6477,8 +6477,6 @@ static const EmbeddedModule embeddedModule[] =
"$self->{rhResult}{$strModule} = $xResult;\n"
"}\n"
"\n\n\n\n"
"sub DESTROY {shift->close()}\n"
"\n\n\n\n"
"sub className {blessed(shift)}\n"
"sub id {shift->{strId}}\n"
"\n"
@ -19808,6 +19806,17 @@ static const EmbeddedModule embeddedModule[] =
"return true;\n"
"}\n"
"\n\n\n\n"
"sub DESTROY\n"
"{\n"
"my $self = shift;\n"
"\n"
"if (defined($self->handle()))\n"
"{\n"
"CORE::close($self->handle());\n"
"undef($self->{fhFile});\n"
"}\n"
"}\n"
"\n\n\n\n"
"sub handle {shift->{fhFile}}\n"
"sub opened {shift->{bOpened}}\n"
"sub name {shift->{strName}}\n"

9
src/storage/driver/posix/fileWrite.c
View File

@ -352,9 +352,14 @@ storageDriverPosixFileWriteFree(StorageDriverPosixFileWrite *this)
if (this != NULL)
{
storageDriverPosixFileWriteClose(this);
memContextCallbackClear(this->memContext);
// Close the temp file. *Close() must be called explicitly in order for the file to be sycn'ed, renamed, etc. If *Free()
// is called first the assumption is that some kind of error occurred and we should only close the handle to free
// resources.
if (this->handle != -1)
storageDriverPosixFileClose(this->handle, this->name, true);
memContextFree(this->memContext);
}

1
test/lib/pgBackRestTest/Module/Common/CommonIoHandlePerlTest.pm
View File

@ -197,6 +197,7 @@ sub run
my $oIoHandle = $self->testResult(sub {new pgBackRest::Common::Io::Handle("'$strFile'", $fhRead)}, '[object]', 'open read');
$self->testResult(sub {$oIoHandle->read(\$tContent, $iFileLengthHalf)}, $iFileLengthHalf, ' read');
$self->testResult(sub {$oIoHandle->close()}, true, ' close');
$self->testResult(sub {$oIoHandle->close()}, true, ' close again');
$self->testResult(sub {$oIoHandle->result(COMMON_IO_HANDLE)}, $iFileLengthHalf, ' check result');

14
test/lib/pgBackRestTest/Module/Storage/StoragePosixPerlTest.pm
View File

@ -304,6 +304,20 @@ sub run
undef($oPosixIo);
# Test that a premature destroy (from error or otherwise) does not rename the file
#---------------------------------------------------------------------------------------------------------------------------
my $strFileAbort = $self->testPath() . '/file-abort.txt';
my $strFileAbortTmp = "${strFileAbort}.tmp";
$oPosixIo = $self->testResult(
sub {new pgBackRest::Storage::Posix::FileWrite($oPosix, $strFileAbort, {bAtomic => true})}, '[object]', 'open');
$oPosixIo->write(\$strFileContent);
undef($oPosixIo);
$self->testResult(sub {$oPosix->exists($strFileAbort)}, false, 'destination file does not exist');
$self->testResult(sub {$oPosix->exists($strFileAbortTmp)}, true, 'destination file tmp exists');
#---------------------------------------------------------------------------------------------------------------------------
$oPosixIo = $self->testResult(
sub {new pgBackRest::Storage::Posix::FileWrite($oPosix, $strFile, {lTimestamp => time()})}, '[object]', 'open');

11
test/lib/pgBackRestTest/Module/Storage/StorageS3PerlTest.pm
View File

@ -193,6 +193,17 @@ sub run
$self->testResult(sub {$oFileWrite->write(\$strFileContent)}, $iFileLength, ' write');
$oFileWrite->close();
$self->testResult(sub {$oS3->exists("/path/to/${strFile}" . '.@')}, true, 'destination file exists');
# Test that a premature destroy (from error or otherwise) does not rename the file
#---------------------------------------------------------------------------------------------------------------------------
$oFileWrite = $self->testResult(sub {$oS3->openWrite("/path/to/abort.file" . '.@')}, '[object]', 'open write');
$self->testResult(sub {$oFileWrite->write()}, 0, ' write undef');
$self->testResult(sub {$oFileWrite->write(\$strFileContent)}, $iFileLength, ' write');
undef($oFileWrite);
$self->testResult(sub {$oS3->exists("/path/to/abort.file")}, false, 'destination file does not exist');
#---------------------------------------------------------------------------------------------------------------------------
$oFileWrite = $self->testResult(sub {$oS3->openWrite("/path/to/${strFile}")}, '[object]', 'open write');

18
test/src/module/storage/posixTest.c
View File

@ -571,6 +571,21 @@ testRun(void)
TEST_RESULT_INT(storageInfoNP(storageTest, strPath(fileName)).mode, 0750, " check path mode");
TEST_RESULT_INT(storageInfoNP(storageTest, fileName).mode, 0640, " check file mode");
// Test that a premature free (from error or otherwise) does not rename the file
// -------------------------------------------------------------------------------------------------------------------------
fileName = strNewFmt("%s/sub1/testfile-abort", testPath());
String *fileNameTmp = strNewFmt("%s." STORAGE_FILE_TEMP_EXT, strPtr(fileName));
TEST_ASSIGN(file, storageNewWriteNP(storageTest, fileName), "new write file (defaults)");
TEST_RESULT_VOID(ioWriteOpen(storageFileWriteIo(file)), " open file");
TEST_RESULT_VOID(ioWrite(storageFileWriteIo(file), bufNewStr(strNew("TESTDATA"))), "write data");
TEST_RESULT_VOID(ioWriteFlush(storageFileWriteIo(file)), "flush data");
TEST_RESULT_VOID(ioWriteFree(storageFileWriteIo(file)), " free file");
TEST_RESULT_BOOL(storageExistsNP(storageTest, fileName), false, "destination file does not exist");
TEST_RESULT_BOOL(storageExistsNP(storageTest, fileNameTmp), true, "destination tmp file exists");
TEST_RESULT_INT(storageInfoNP(storageTest, fileNameTmp).size, 8, " check temp file size");
// -------------------------------------------------------------------------------------------------------------------------
fileName = strNewFmt("%s/sub2/testfile", testPath());
@ -578,6 +593,9 @@ testRun(void)
file, storageNewWriteP(storageTest, fileName, .modePath = 0700, .modeFile = 0600), "new write file (set mode)");
TEST_RESULT_VOID(ioWriteOpen(storageFileWriteIo(file)), " open file");
TEST_RESULT_VOID(ioWriteClose(storageFileWriteIo(file)), " close file");
TEST_RESULT_VOID(
storageDriverPosixFileWriteClose((StorageDriverPosixFileWrite *)storageFileWriteFileDriver(file)),
" close file again");
TEST_RESULT_INT(storageInfoNP(storageTest, strPath(fileName)).mode, 0700, " check path mode");
TEST_RESULT_INT(storageInfoNP(storageTest, fileName).mode, 0600, " check file mode");