db/pgbackrest
1
0
Fork 0
mirror of https://github.com/pgbackrest/pgbackrest.git synced 2024-12-12 10:04:14 +02:00
Code Issues Releases Activity

Remove logic that tried to determine additional file system compression.

Browse Source 
In theory, the additional stat() call after a file has been copied to the repo can determine if additional compression has been applied by the file system. However, it has been a very long time since we tested this in practice. There are currently no unit tests that accurately test this feature since it requires a compressed file system like ZFS to work, which never seemed worth the extra cost.

It can also add a lot of time to backups if there are a large quantity of small files.

In addition, it stands as a blocker for combining files for small file support since it is no longer possible to get per-file sizes from the viewpoint of the file system. There are several ways this could be reworked but none of them are easy while at the same time maintaining current info functionality.

It doesn't seem worth keeping an untested feature that will only work in some special cases (if it still works) when it is blocking development.
This commit is contained in:
David Steele 2022-02-09 09:32:23 -06:00 committed by GitHub
parent 755bfc4d40
commit cb630ffe3b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 19 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
doc/xml/release.xml
View File

@ -16,6 +16,10 @@
<release-list>
<release date="XXXX-XX-XX" version="2.38dev" title="UNDER DEVELOPMENT">
<release-core-list>
<text>
<p><b>IMPORTANT NOTE</b>: Repository size reported by the <cmd>info</cmd> command is now entirely based on what <backrest/> has written to storage. Previously, in certain cases, <backrest/> could detect if additional compression was being applied by the storage but this is no longer supported.</p>
</text>
<release-feature-list>
<release-item>
<github-issue id="1430"/>
@ -55,6 +59,18 @@
<p>Remove support for <proper>PostgreSQL</proper> <id>8.3</id>/<id>8.4</id>.</p>
</release-item>
<release-item>
<github-pull-request id="1652"/>
<release-item-contributor-list>
<release-item-contributor id="david.steele"/>
<release-item-reviewer id="reid.thompson"/>
<release-item-reviewer id="stefan.fercot"/>
</release-item-contributor-list>
<p>Remove logic that tried to determine additional file system compression.</p>
</release-item>
</release-improvement-list>
<release-development-list>

2
src/build/help/help.xml
View File

@ -2246,7 +2246,7 @@
<p>The '<id>database size</id>' is the full uncompressed size of the database while '<id>database backup size</id>' is the amount of data in the database to actually back up (these will be the same for full backups).</p>
<p>The '<id>repo</id>' indicates in which repository this backup resides. The '<id>backup set size</id>' includes all the files from this backup and any referenced backups in the repository that are required to restore the database from this backup while '<id>backup size</id>' includes only the files in this backup (these will also be the same for full backups). Repository sizes reflect compressed file sizes if compression is enabled in <backrest/> or the filesystem.</p>
<p>The '<id>repo</id>' indicates in which repository this backup resides. The '<id>backup set size</id>' includes all the files from this backup and any referenced backups in the repository that are required to restore the database from this backup while '<id>backup size</id>' includes only the files in this backup (these will also be the same for full backups). Repository sizes reflect compressed file sizes if compression is enabled in <backrest/>.</p>
<p>The '<id>backup reference list</id>' contains the additional backups that are required to restore this backup.</p>

12
src/command/backup/file.c
View File

@ -261,18 +261,6 @@ backupFile(
else
result.backupCopyResult = backupCopyResultSkip;
}
// If the file was copied get the repo size only if the storage can store the files with a different size than what was
// written. This has to be checked after the file is at rest because filesystem compression may affect the actual repo size
// and this cannot be calculated in stream.
//
// If the file was checksummed then get the size in all cases since we don't already have it.
if (((result.backupCopyResult == backupCopyResultCopy || result.backupCopyResult == backupCopyResultReCopy) &&
storageFeature(storageRepo(), storageFeatureCompress)) ||
result.backupCopyResult == backupCopyResultChecksum)
{
result.repoSize = storageInfoP(storageRepo(), repoPathFile).size;
}
}
MEM_CONTEXT_TEMP_END();

2
src/storage/posix/storage.c
View File

@ -546,7 +546,7 @@ storagePosixRemove(THIS_VOID, const String *file, StorageInterfaceRemoveParam pa
/**********************************************************************************************************************************/
static const StorageInterface storageInterfacePosix =
{
.feature = 1 << storageFeaturePath | 1 << storageFeatureCompress,
.feature = 1 << storageFeaturePath,
.info = storagePosixInfo,
.infoList = storagePosixInfoList,

4
src/storage/storage.h
View File

@ -36,10 +36,6 @@ typedef enum
// Do paths need to be synced to ensure contents are durable? storeageFeaturePath must also be enabled.
storageFeaturePathSync,
// Is the storage able to do compression and therefore store the file more efficiently than what was written? If so, the size
// will need to checked after write to see if it is different.
storageFeatureCompress,
// Does the storage support hardlinks? Hardlinks allow the same file to be linked into multiple paths to save space.
storageFeatureHardLink,

25
test/src/module/command/backupTest.c
View File

@ -620,29 +620,6 @@ testRun(void)
// Create a pg file to backup
HRN_STORAGE_PUT_Z(storagePgWrite(), strZ(pgFile), "atestfile");
// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("copy file to repo success - no prior checksum, no compression, no pageChecksum, no delta, no hasReference");
// With the expected backupCopyResultCopy, unset the storageFeatureCompress bit for the storageRepo for code coverage
uint64_t feature = storageRepo()->pub.interface.feature;
((Storage *)storageRepo())->pub.interface.feature = feature & ((1 << storageFeatureCompress) ^ 0xFFFFFFFFFFFFFFFF);
TEST_ASSIGN(
result,
backupFile(
pgFile, false, 9999999, true, NULL, false, 0, pgFile, false, compressTypeNone, 1, backupLabel, false,
cipherTypeNone, NULL),
"pg file exists and shrunk, no repo file, no ignoreMissing, no pageChecksum, no delta, no hasReference");
((Storage *)storageRepo())->pub.interface.feature = feature;
TEST_RESULT_UINT(result.copySize, 9, "copy=pgFile size");
TEST_RESULT_UINT(result.repoSize, 9, "repo=pgFile size");
TEST_RESULT_UINT(result.backupCopyResult, backupCopyResultCopy, "copy file");
TEST_RESULT_STR_Z(result.copyChecksum, "9bc8ab2dda60ef4beed07d1e19ce0676d5edde67", "copy checksum matches");
TEST_RESULT_PTR(result.pageChecksumResult, NULL, "page checksum result is NULL");
TEST_STORAGE_EXISTS(storageRepo(), strZ(backupPathFile));
// Remove repo file
HRN_STORAGE_REMOVE(storageRepoWrite(), strZ(backupPathFile));
@ -818,7 +795,7 @@ testRun(void)
3, backupLabel, false, cipherTypeNone, NULL),
"pg file & repo exists, match, checksum, no ignoreMissing, compression, no pageChecksum, no delta, no hasReference");
TEST_RESULT_UINT(result.copySize, 9, "copy=pgFile size");
TEST_RESULT_UINT(result.repoSize, 29, "repo compress size");
TEST_RESULT_UINT(result.repoSize, 0, "repo size not calculated");
TEST_RESULT_UINT(result.backupCopyResult, backupCopyResultChecksum, "checksum file");
TEST_RESULT_STR_Z(result.copyChecksum, "9bc8ab2dda60ef4beed07d1e19ce0676d5edde67", "compressed repo file checksum matches");
TEST_STORAGE_EXISTS(

1
test/src/module/storage/azureTest.c
View File

@ -206,7 +206,6 @@ testRun(void)
TEST_RESULT_STR_Z(((StorageAzure *)storageDriver(storage))->pathPrefix, "/" TEST_CONTAINER, "check path prefix");
TEST_RESULT_UINT(((StorageAzure *)storageDriver(storage))->blockSize, STORAGE_AZURE_BLOCKSIZE_MIN, "check block size");
TEST_RESULT_BOOL(storageFeature(storage, storageFeaturePath), false, "check path feature");
TEST_RESULT_BOOL(storageFeature(storage, storageFeatureCompress), false, "check compress feature");
// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("storage with host but force host-style uri");

1
test/src/module/storage/gcsTest.c
View File

@ -226,7 +226,6 @@ testRun(void)
TEST_RESULT_UINT(((StorageGcs *)storageDriver(storage))->chunkSize, STORAGE_GCS_CHUNKSIZE_DEFAULT, "check chunk size");
TEST_RESULT_STR(((StorageGcs *)storageDriver(storage))->token, TEST_TOKEN_STR, "check token");
TEST_RESULT_BOOL(storageFeature(storage, storageFeaturePath), false, "check path feature");
TEST_RESULT_BOOL(storageFeature(storage, storageFeatureCompress), false, "check compress feature");
}
// *****************************************************************************************************************************

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

@ -109,7 +109,6 @@ testRun(void)
TEST_RESULT_PTR(storageDriver(storageTest), storageTest->pub.driver, "check driver");
TEST_RESULT_UINT(storageType(storageTest), storageTest->pub.type, "check type");
TEST_RESULT_BOOL(storageFeature(storageTest, storageFeaturePath), true, "check path feature");
TEST_RESULT_BOOL(storageFeature(storageTest, storageFeatureCompress), true, "check compress feature");
}
// *****************************************************************************************************************************
@ -1581,7 +1580,6 @@ testRun(void)
TEST_ASSIGN(storage, storageRepoGet(0, true), "get cifs repo storage");
TEST_RESULT_UINT(storageType(storage), STORAGE_CIFS_TYPE, "check storage type");
TEST_RESULT_BOOL(storageFeature(storage, storageFeaturePath), true, "check path feature");
TEST_RESULT_BOOL(storageFeature(storage, storageFeatureCompress), true, "check compress feature");
// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("write object path sync false");

1
test/src/module/storage/remoteTest.c
View File

@ -77,7 +77,6 @@ testRun(void)
{
TEST_RESULT_UINT(storageInterface(storageRepoWrite).feature, storageInterface(storageTest).feature, "check features");
TEST_RESULT_BOOL(storageFeature(storageRepoWrite, storageFeaturePath), true, "check path feature");
TEST_RESULT_BOOL(storageFeature(storageRepoWrite, storageFeatureCompress), true, "check compress feature");
TEST_RESULT_STR_Z(storagePathP(storageRepo, NULL), TEST_PATH "/repo128", "check repo path");
TEST_RESULT_STR_Z(storagePathP(storageRepoWrite, NULL), TEST_PATH "/repo128", "check repo write path");
TEST_RESULT_STR_Z(storagePathP(storagePgWrite, NULL), TEST_PATH "/pg256", "check pg write path");

2
test/src/module/storage/s3Test.c
View File

@ -411,7 +411,6 @@ testRun(void)
TEST_RESULT_STR(s3->path, path, "check path");
TEST_RESULT_BOOL(storageFeature(s3, storageFeaturePath), false, "check path feature");
TEST_RESULT_BOOL(storageFeature(s3, storageFeatureCompress), false, "check compress feature");
// -----------------------------------------------------------------------------------------------------------------
TEST_TITLE("coverage for noop functions");
@ -473,7 +472,6 @@ testRun(void)
TEST_RESULT_STR(s3->path, path, "check path");
TEST_RESULT_STR(driver->credRole, credRole, "check role");
TEST_RESULT_BOOL(storageFeature(s3, storageFeaturePath), false, "check path feature");
TEST_RESULT_BOOL(storageFeature(s3, storageFeatureCompress), false, "check compress feature");
// Set partSize to a small value for testing
driver->partSize = 16;