Skip to content

Commit

Permalink
Fix spurious automatic delta backup on backup from standby.
Browse files Browse the repository at this point in the history 
When performing backup from standby the file sizes on the standby may not be equal to file sizes on the primary. This is because replication continues during the backup and by the time the file is copied from the standby it may have changed. Since we cap the size of all files copied from the standby this practically applies to truncation and in particular truncation of free space maps (at least, every case we have seen so far is an fsm). Free space maps are especially vulnerable since they are only partially replicated, which amplifies the difference between the primary and standby.

On an incremental backup it may look like the size has changed on the primary (because of the final size recorded by the standby in the prior backup) but the timestamp may not have changed on the primary and this will trigger a checksum delta for safety. While this has no impact on backup integrity, checksum delta incrementals can run much longer than regular incrementals and backup schedules may be impacted.

The solution is to preserve the original size in the manifest and use it to do the time/size check. In the case of backup from standby the original size will always be the size on the primary, which makes comparisons against subsequent file sizes on the primary consistent. Original size is only stored in the manifest when it differs from final size, so there should not be any noticeable manifest bloat.
  • Loading branch information
@dwsteele
dwsteele committed Jul 18, 2023
1 parent 4c27d74 commit 5ed6f8d
Show file tree
Hide file tree
Showing 8 changed files with 66 additions and 8 deletions.
19 changes: 19 additions & 0 deletions doc/xml/release.xml
View file
Expand Up @@ -46,6 +46,20 @@
<p>Fix block incremental file names in <cmd>verify</cmd> command.</p>
</release-item>

<release-item>
<github-issue id="2020"/>
<github-pull-request id="2124"/>

<release-item-contributor-list>
<release-item-ideator id="krmozejko"/>
<release-item-ideator id="don.seiler"/>
<release-item-contributor id="david.steele"/>
<release-item-reviewer id="stephen.frost"/>
</release-item-contributor-list>

<p>Fix spurious automatic delta backup on backup from standby.</p>
</release-item>

<release-item>
<github-issue id="2062"/>
<github-pull-request id="2070"/>
Expand Down Expand Up @@ -12775,6 +12789,11 @@
<contributor-id type="github">kaceymiri</contributor-id>
</contributor>

<contributor id="krmozejko">
<contributor-name-display>krmozejko</contributor-name-display>
<contributor-id type="github">krmozejko</contributor-id>
</contributor>

<contributor id="kyle.nevins">
<contributor-name-display>Kyle Nevins</contributor-name-display>
<contributor-id type="github">kyle-nevins</contributor-id>
Expand Down
2 changes: 2 additions & 0 deletions src/command/backup/backup.c
View file
Expand Up @@ -1209,6 +1209,7 @@ backupFilePut(
.user = basePath->user,
.group = basePath->group,
.size = strSize(content),
.sizeOriginal = strSize(content),
.sizeRepo = pckReadU64P(ioFilterGroupResultP(filterGroup, SIZE_FILTER_TYPE)),
.timestamp = timestamp,
.checksumSha1 = bufPtr(pckReadBinP(ioFilterGroupResultP(filterGroup, CRYPTO_HASH_FILTER_TYPE, .idx = 0))),
Expand Down Expand Up @@ -2403,6 +2404,7 @@ backupArchiveCheckCopy(const BackupData *const backupData, Manifest *const manif
.user = basePath->user,
.group = basePath->group,
.size = backupData->walSegmentSize,
.sizeOriginal = backupData->walSegmentSize,
.sizeRepo = pckReadU64P(ioFilterGroupResultP(filterGroup, SIZE_FILTER_TYPE)),
.timestamp = manifestData(manifest)->backupTimestampStop,
.checksumSha1 = bufPtr(bufNewDecode(encodingHex, strSubN(archiveFile, 25, 40))),
Expand Down
31 changes: 29 additions & 2 deletions src/info/manifest.c
View file
Expand Up @@ -108,6 +108,7 @@ typedef enum
manifestFilePackFlagChecksumPage,
manifestFilePackFlagChecksumPageError,
manifestFilePackFlagChecksumPageErrorList,
manifestFilePackFlagSizeOriginal,
manifestFilePackFlagMode,
manifestFilePackFlagUser,
manifestFilePackFlagUserNull,
Expand Down Expand Up @@ -166,6 +167,9 @@ manifestFilePack(const Manifest *const manifest, const ManifestFile *const file)
if (file->blockIncrSize != 0)
flag |= 1 << manifestFilePackFlagBlockIncr;

if (file->sizeOriginal != file->size)
flag |= 1 << manifestFilePackFlagSizeOriginal;

if (file->mode != manifest->fileModeDefault)
flag |= 1 << manifestFilePackFlagMode;

Expand All @@ -184,6 +188,10 @@ manifestFilePack(const Manifest *const manifest, const ManifestFile *const file)
// Size
cvtUInt64ToVarInt128(file->size, buffer, &bufferPos, sizeof(buffer));

// Original size
if (flag & (1 << manifestFilePackFlagSizeOriginal))
cvtUInt64ToVarInt128(file->sizeOriginal, buffer, &bufferPos, sizeof(buffer));

// Use the first timestamp that appears as the base for all other timestamps. Ideally we would like a timestamp as close to the
// middle as possible but it doesn't seem worth doing the calculation.
if (manifestPackBaseTime == -1)
Expand Down Expand Up @@ -308,6 +316,12 @@ manifestFileUnpack(const Manifest *const manifest, const ManifestFilePack *const
// Size
result.size = cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos, UINT_MAX);

// Original size
if (flag & (1 << manifestFilePackFlagSizeOriginal))
result.sizeOriginal = cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos, UINT_MAX);
else
result.sizeOriginal = result.size;

// Timestamp
result.timestamp =
manifestPackBaseTime - (time_t)cvtInt64FromZigZag(cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos, UINT_MAX));
Expand Down Expand Up @@ -1052,6 +1066,7 @@ manifestBuildInfo(
.user = info->user,
.group = info->group,
.size = info->size,
.sizeOriginal = info->size,
.sizeRepo = info->size,
.timestamp = info->timeModified,
};
Expand Down Expand Up @@ -1634,12 +1649,12 @@ manifestBuildIncr(Manifest *this, const Manifest *manifestPrior, BackupType type
}

// Check for size change with no timestamp change
if (file.size != filePrior.size && file.timestamp == filePrior.timestamp)
if (file.sizeOriginal != filePrior.sizeOriginal && file.timestamp == filePrior.timestamp)
{
LOG_WARN_FMT(
"file '%s' has same timestamp (%" PRId64 ") as prior but different size (prior %" PRIu64 ", current"
" %" PRIu64 "), enabling delta checksum",
strZ(manifestPathPg(file.name)), (int64_t)file.timestamp, filePrior.size, file.size);
strZ(manifestPathPg(file.name)), (int64_t)file.timestamp, filePrior.sizeOriginal, file.sizeOriginal);

this->pub.data.backupOptionDelta = BOOL_TRUE_VAR;
break;
Expand Down Expand Up @@ -1850,6 +1865,7 @@ manifestBuildComplete(
#define MANIFEST_KEY_PATH STRID5("path", 0x450300)
#define MANIFEST_KEY_REFERENCE STRID5("reference", 0x51b8b2298b20)
#define MANIFEST_KEY_SIZE STRID5("size", 0x2e9330)
#define MANIFEST_KEY_SIZE_ORIGINAL STRID5("szo", 0x3f530)
#define MANIFEST_KEY_SIZE_REPO STRID5("repo-size", 0x5d267b7c0b20)
#define MANIFEST_KEY_TABLESPACE_ID "tablespace-id"
#define MANIFEST_KEY_TABLESPACE_NAME "tablespace-name"
Expand Down Expand Up @@ -2053,6 +2069,13 @@ manifestLoadCallback(void *callbackData, const String *const section, const Stri
if (file.size == 0)
file.checksumSha1 = bufPtrConst(HASH_TYPE_SHA1_ZERO_BUF);

// If original is not present in the manifest file then it is the same as size (i.e. the file did not change size during
// copy) -- to save space the original size is only stored in the manifest file if it is different than size.
if (jsonReadKeyExpectStrId(json, MANIFEST_KEY_SIZE_ORIGINAL))
file.sizeOriginal = jsonReadUInt64(json);
else
file.sizeOriginal = file.size;

// Timestamp is required so error if it is not present
if (jsonReadKeyExpectStrId(json, MANIFEST_KEY_TIMESTAMP))
file.timestamp = (time_t)jsonReadInt64(json);
Expand Down Expand Up @@ -2766,6 +2789,10 @@ manifestSaveCallback(void *const callbackData, const String *const sectionNext,
jsonWriteUInt64(jsonWriteKeyStrId(json, MANIFEST_KEY_SIZE_REPO), file.sizeRepo);

jsonWriteUInt64(jsonWriteKeyStrId(json, MANIFEST_KEY_SIZE), file.size);

if (file.sizeOriginal != file.size)
jsonWriteUInt64(jsonWriteKeyStrId(json, MANIFEST_KEY_SIZE_ORIGINAL), file.sizeOriginal);

jsonWriteUInt64(jsonWriteKeyStrId(json, MANIFEST_KEY_TIMESTAMP), (uint64_t)file.timestamp);

if (!varEq(manifestOwnerVar(file.user), saveData->userDefault))
Expand Down
3 changes: 2 additions & 1 deletion src/info/manifest.h
View file
Expand Up @@ -157,7 +157,8 @@ typedef struct ManifestFile
size_t blockIncrSize; // Size of incremental blocks
size_t blockIncrChecksumSize; // Size of incremental block checksum
uint64_t blockIncrMapSize; // Block incremental map size
uint64_t size; // Original size
uint64_t size; // Final size (after copy)
uint64_t sizeOriginal; // Original size (from manifest build)
uint64_t sizeRepo; // Size in repo
time_t timestamp; // Original timestamp
} ManifestFile;
Expand Down
5 changes: 5 additions & 0 deletions test/src/common/harnessManifest.c
View file
Expand Up @@ -64,6 +64,7 @@ hrnManifestFileAdd(Manifest *const manifest, const HrnManifestFile hrnManifestFi
FUNCTION_HARNESS_PARAM(UINT64, hrnManifestFile.blockIncrSize);
FUNCTION_HARNESS_PARAM(UINT64, hrnManifestFile.blockIncrMapSize);
FUNCTION_HARNESS_PARAM(UINT64, hrnManifestFile.size);
FUNCTION_HARNESS_PARAM(UINT64, hrnManifestFile.sizeOriginal);
FUNCTION_HARNESS_PARAM(UINT64, hrnManifestFile.sizeRepo);
FUNCTION_HARNESS_PARAM(TIME, hrnManifestFile.timestamp);
FUNCTION_HARNESS_END();
Expand All @@ -87,10 +88,14 @@ hrnManifestFileAdd(Manifest *const manifest, const HrnManifestFile hrnManifestFi
.blockIncrChecksumSize = hrnManifestFile.blockIncrChecksumSize,
.blockIncrMapSize = hrnManifestFile.blockIncrMapSize,
.size = hrnManifestFile.size,
.sizeOriginal = hrnManifestFile.sizeOriginal,
.sizeRepo = hrnManifestFile.sizeRepo,
.timestamp = hrnManifestFile.timestamp,
};

if (manifestFile.sizeOriginal == 0)
manifestFile.sizeOriginal = manifestFile.size;

if (hrnManifestFile.mode == 0)
manifestFile.mode = 0600;
else
Expand Down
1 change: 1 addition & 0 deletions test/src/common/harnessManifest.h
View file
Expand Up @@ -46,6 +46,7 @@ typedef struct HrnManifestFile
size_t blockIncrChecksumSize;
uint64_t blockIncrMapSize;
uint64_t size;
uint64_t sizeOriginal;
uint64_t sizeRepo;
time_t timestamp;
} HrnManifestFile;
Expand Down
3 changes: 2 additions & 1 deletion test/src/module/command/backupTest.c
View file
Expand Up @@ -3148,7 +3148,8 @@ testRun(void)
",\"timestamp\":1571200002}\n"
"pg_data/base/1/1={\"size\":0,\"timestamp\":1571200000}\n"
"pg_data/base/1/2={\"checksum\":\"54ceb91256e8190e474aa752a6e0650a2df5ba37\",\"size\":2,\"timestamp\":1571200000}\n"
"pg_data/base/1/3={\"checksum\":\"3c01bdbb26f358bab27f267924aa2c9a03fcfdb8\",\"size\":3,\"timestamp\":1571200000}\n"
"pg_data/base/1/3={\"checksum\":\"3c01bdbb26f358bab27f267924aa2c9a03fcfdb8\",\"size\":3,\"szo\":4"
",\"timestamp\":1571200000}\n"
"pg_data/global/pg_control={\"size\":8192,\"timestamp\":1571200000}\n"
"pg_data/pg_xlog/0000000105DA69C000000000={\"size\":16777216,\"timestamp\":1571200002}\n"
"pg_data/postgresql.conf={\"checksum\":\"e3db315c260e79211b7b52587123b7aa060f30ab\",\"size\":11"
Expand Down
10 changes: 6 additions & 4 deletions test/src/module/info/manifestTest.c
View file
Expand Up @@ -1245,8 +1245,9 @@ testRun(void)
.group = "test", .user = "test");

HRN_MANIFEST_FILE_ADD(
manifestPrior, .name = MANIFEST_TARGET_PGDATA "/FILE2", .copy = true, .size = 4, .sizeRepo = 4, .timestamp = 1482182860,
.reference = "20190101-010101F_20190202-010101D", .checksumSha1 = "ddddddddddbbbbbbbbbbccccccccccaaaaaaaaaa");
manifestPrior, .name = MANIFEST_TARGET_PGDATA "/FILE2", .copy = true, .size = 6, .sizeOriginal = 4, .sizeRepo = 4,
.timestamp = 1482182860, .reference = "20190101-010101F_20190202-010101D",
.checksumSha1 = "ddddddddddbbbbbbbbbbccccccccccaaaaaaaaaa");

TEST_RESULT_VOID(
manifestBuildIncr(manifest, manifestPrior, backupTypeIncr, STRDEF("000000040000000400000004")),
Expand All @@ -1273,7 +1274,8 @@ testRun(void)
"\n"
"[target:file]\n"
"pg_data/FILE1={\"size\":6,\"timestamp\":1482182861}\n"
"pg_data/FILE2={\"size\":6,\"timestamp\":1482182860}\n"
"pg_data/FILE2={\"checksum\":\"ddddddddddbbbbbbbbbbccccccccccaaaaaaaaaa\""
",\"reference\":\"20190101-010101F_20190202-010101D\",\"repo-size\":4,\"size\":6,\"timestamp\":1482182860}\n"
TEST_MANIFEST_FILE_DEFAULT
"\n"
"[target:path]\n"
Expand Down Expand Up @@ -1576,7 +1578,7 @@ testRun(void)
",\"rck\":\"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\",\"reference\":\"20190818-084502F_20190819-084506D\"" \
",\"size\":4,\"timestamp\":1565282114}\n" \
"pg_data/base/16384/17000={\"bi\":4,\"bni\":1,\"checksum\":\"e0101dd8ffb910c9c202ca35b5f828bcb9697bed\"" \
",\"checksum-page\":false,\"checksum-page-error\":[1],\"repo-size\":4096,\"size\":8192" \
",\"checksum-page\":false,\"checksum-page-error\":[1],\"repo-size\":4096,\"size\":8192,\"szo\":16384" \
",\"timestamp\":1565282114}\n" \
"pg_data/base/16384/PG_VERSION={\"bni\":1,\"bno\":1,\"checksum\":\"184473f470864e067ee3a22e64b47b0a1c356f29\"" \
",\"group\":\"group2\",\"size\":4,\"timestamp\":1565282115,\"user\":false}\n" \
Expand Down

0 comments on commit 5ed6f8d

Please sign in to comment.