diff --git a/doc/xml/release.xml b/doc/xml/release.xml index 28863fa1a..b75c21a4b 100644 --- a/doc/xml/release.xml +++ b/doc/xml/release.xml @@ -14,6 +14,16 @@ + + + + + + +

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

+ + + diff --git a/lib/pgBackRest/Common/Io/Base.pm b/lib/pgBackRest/Common/Io/Base.pm index e278c9b73..b5f6475f2 100644 --- a/lib/pgBackRest/Common/Io/Base.pm +++ b/lib/pgBackRest/Common/Io/Base.pm @@ -105,11 +105,6 @@ sub resultSet $self->{rhResult}{$strModule} = $xResult; } -#################################################################################################################################### -# DESTROY - call close() -#################################################################################################################################### -sub DESTROY {shift->close()} - #################################################################################################################################### # Getters #################################################################################################################################### diff --git a/lib/pgBackRest/Common/Io/Handle.pm b/lib/pgBackRest/Common/Io/Handle.pm index b6ce367e8..8f70fee24 100644 --- a/lib/pgBackRest/Common/Io/Handle.pm +++ b/lib/pgBackRest/Common/Io/Handle.pm @@ -157,7 +157,7 @@ sub write } #################################################################################################################################### -# close/DESTROY - record read/write size +# close - record read/write size #################################################################################################################################### sub close { diff --git a/lib/pgBackRest/Storage/Posix/FileWrite.pm b/lib/pgBackRest/Storage/Posix/FileWrite.pm index 64a92ac77..79ddb52a0 100644 --- a/lib/pgBackRest/Storage/Posix/FileWrite.pm +++ b/lib/pgBackRest/Storage/Posix/FileWrite.pm @@ -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 #################################################################################################################################### diff --git a/src/perl/embed.auto.c b/src/perl/embed.auto.c index c146c9e86..7644b127b 100644 --- a/src/perl/embed.auto.c +++ b/src/perl/embed.auto.c @@ -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" diff --git a/src/storage/driver/posix/fileWrite.c b/src/storage/driver/posix/fileWrite.c index 1d42d0ac7..3bcaf5683 100644 --- a/src/storage/driver/posix/fileWrite.c +++ b/src/storage/driver/posix/fileWrite.c @@ -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); } diff --git a/test/lib/pgBackRestTest/Module/Common/CommonIoHandlePerlTest.pm b/test/lib/pgBackRestTest/Module/Common/CommonIoHandlePerlTest.pm index a61d92c41..17e1f89b7 100644 --- a/test/lib/pgBackRestTest/Module/Common/CommonIoHandlePerlTest.pm +++ b/test/lib/pgBackRestTest/Module/Common/CommonIoHandlePerlTest.pm @@ -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'); diff --git a/test/lib/pgBackRestTest/Module/Storage/StoragePosixPerlTest.pm b/test/lib/pgBackRestTest/Module/Storage/StoragePosixPerlTest.pm index a216f1e10..efc2600f8 100644 --- a/test/lib/pgBackRestTest/Module/Storage/StoragePosixPerlTest.pm +++ b/test/lib/pgBackRestTest/Module/Storage/StoragePosixPerlTest.pm @@ -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'); diff --git a/test/lib/pgBackRestTest/Module/Storage/StorageS3PerlTest.pm b/test/lib/pgBackRestTest/Module/Storage/StorageS3PerlTest.pm index 9d1c93671..25bf32013 100644 --- a/test/lib/pgBackRestTest/Module/Storage/StorageS3PerlTest.pm +++ b/test/lib/pgBackRestTest/Module/Storage/StorageS3PerlTest.pm @@ -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'); diff --git a/test/src/module/storage/posixTest.c b/test/src/module/storage/posixTest.c index 31cd2fe24..4893a0834 100644 --- a/test/src/module/storage/posixTest.c +++ b/test/src/module/storage/posixTest.c @@ -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");