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

Avoid chown() on recovery files during restore.

Browse Source 
The chown() was already skipped on the files restored from the repository but the same logic was not applied to the generated recovery files, probably because chown'ing a few recovery files does not have performance implications. Use the same logic for recovery files to determined if they need to be chown'd.

Ultimately this behavior is pretty hard to test, so add a fail safe into the Posix driver that will skip chown if the permissions are already as required.
This commit is contained in:
David Steele 2023-04-25 11:52:28 +03:00 committed by GitHub
parent 750ab8e55c
commit 3ff88ffbb4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 102 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
doc/xml/release.xml
View File

@ -18,6 +18,8 @@
<release-core-list>
<release-improvement-list>
<release-item>
<github-pull-request id="2045"/>
<release-item-contributor-list>
<release-item-ideator id="david.christensen"/>
<release-item-contributor id="david.steele"/>
@ -27,6 +29,20 @@
<p>Allow page header checks to be skipped.</p>
</release-item>
<release-item>
<github-issue id="2036"/>
<github-pull-request id="2038"/>
<release-item-contributor-list>
<release-item-ideator id="marcelo.henrique.neppel"/>
<release-item-contributor id="david.steele"/>
<release-item-reviewer id="stefan.fercot"/>
<release-item-reviewer id="marcelo.henrique.neppel"/>
</release-item-contributor-list>
<p>Avoid <code>chown()</code> on recovery files during restore.</p>
</release-item>
<release-item>
<commit subject="Move error modules to common/error directory."/>
<commit subject="Improve retry error messages."/>
@ -12637,6 +12653,11 @@
<contributor-id type="github">m-borger</contributor-id>
</contributor>
<contributor id="marcelo.henrique.neppel">
<contributor-name-display>Marcelo Henrique Neppel</contributor-name-display>
<contributor-id type="github">marceloneppel</contributor-id>
</contributor>
<contributor id="marco.montagna">
<contributor-name-display>Marco Montagna</contributor-name-display>
<contributor-id type="github">mmontagna</contributor-id>

50
src/command/restore/restore.c
View File

@ -1731,10 +1731,13 @@ restoreRecoveryConf(const unsigned int pgVersion, const String *const restoreLab
// Helper to write recovery options into recovery.conf
static void
restoreRecoveryWriteConf(const Manifest *const manifest, const unsigned int pgVersion, const String *const restoreLabel)
restoreRecoveryWriteConf(
const Manifest *const manifest, const StorageInfo *const fileInfo, const unsigned int pgVersion,
const String *const restoreLabel)
{
FUNCTION_LOG_BEGIN(logLevelDebug);
FUNCTION_LOG_PARAM(MANIFEST, manifest);
FUNCTION_LOG_PARAM(STORAGE_INFO, fileInfo);
FUNCTION_LOG_PARAM(UINT, pgVersion);
FUNCTION_LOG_PARAM(STRING, restoreLabel);
FUNCTION_LOG_END();
@ -1746,15 +1749,11 @@ restoreRecoveryWriteConf(const Manifest *const manifest, const unsigned int pgVe
{
LOG_INFO_FMT("write %s", strZ(storagePathP(storagePg(), PG_FILE_RECOVERYCONF_STR)));
// Use the data directory to set permissions and ownership for recovery file
const ManifestPath *const dataPath = manifestPathFind(manifest, MANIFEST_TARGET_PGDATA_STR);
const mode_t recoveryFileMode = dataPath->mode & (S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
// Write recovery.conf
storagePutP(
storageNewWriteP(
storagePgWrite(), PG_FILE_RECOVERYCONF_STR, .noCreatePath = true, .modeFile = recoveryFileMode,
.noAtomic = true, .noSyncPath = true, .user = dataPath->user, .group = dataPath->group),
storagePgWrite(), PG_FILE_RECOVERYCONF_STR, .noCreatePath = true, .modeFile = fileInfo->mode, .noAtomic = true,
.noSyncPath = true, .user = fileInfo->user, .group = fileInfo->group),
BUFSTR(restoreRecoveryConf(pgVersion, restoreLabel)));
}
MEM_CONTEXT_TEMP_END();
@ -1765,10 +1764,13 @@ restoreRecoveryWriteConf(const Manifest *const manifest, const unsigned int pgVe
// Helper to write recovery options into postgresql.auto.conf
static void
restoreRecoveryWriteAutoConf(const Manifest *const manifest, const unsigned int pgVersion, const String *const restoreLabel)
restoreRecoveryWriteAutoConf(
const Manifest *const manifest, const StorageInfo *const fileInfo, const unsigned int pgVersion,
const String *const restoreLabel)
{
FUNCTION_LOG_BEGIN(logLevelDebug);
FUNCTION_LOG_PARAM(MANIFEST, manifest);
FUNCTION_LOG_PARAM(STORAGE_INFO, fileInfo);
FUNCTION_LOG_PARAM(UINT, pgVersion);
FUNCTION_LOG_PARAM(STRING, restoreLabel);
FUNCTION_LOG_END();
@ -1860,15 +1862,11 @@ restoreRecoveryWriteAutoConf(const Manifest *const manifest, const unsigned int
LOG_INFO_FMT(
"write %s%s", autoConf == NULL ? "" : "updated ", strZ(storagePathP(storagePg(), PG_FILE_POSTGRESQLAUTOCONF_STR)));
// Use the data directory to set permissions and ownership for recovery file
const StorageInfo dataPath = storageInfoP(storagePg(), NULL);
mode_t recoveryFileMode = dataPath.mode & (S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
// Write postgresql.auto.conf
storagePutP(
storageNewWriteP(
storagePgWrite(), PG_FILE_POSTGRESQLAUTOCONF_STR, .noCreatePath = true, .modeFile = recoveryFileMode,
.noAtomic = true, .noSyncPath = true, .user = dataPath.user, .group = dataPath.group),
storagePgWrite(), PG_FILE_POSTGRESQLAUTOCONF_STR, .noCreatePath = true, .modeFile = fileInfo->mode,
.noAtomic = true, .noSyncPath = true, .user = fileInfo->user, .group = fileInfo->group),
BUFSTR(content));
// The standby.signal file is required for standby mode
@ -1876,8 +1874,8 @@ restoreRecoveryWriteAutoConf(const Manifest *const manifest, const unsigned int
{
storagePutP(
storageNewWriteP(
storagePgWrite(), PG_FILE_STANDBYSIGNAL_STR, .noCreatePath = true, .modeFile = recoveryFileMode,
.noAtomic = true, .noSyncPath = true, .user = dataPath.user, .group = dataPath.group),
storagePgWrite(), PG_FILE_STANDBYSIGNAL_STR, .noCreatePath = true, .modeFile = fileInfo->mode,
.noAtomic = true, .noSyncPath = true, .user = fileInfo->user, .group = fileInfo->group),
NULL);
}
// Else the recovery.signal file is required for targeted recovery. Skip writing this file if the backup was offline and
@ -1886,8 +1884,8 @@ restoreRecoveryWriteAutoConf(const Manifest *const manifest, const unsigned int
{
storagePutP(
storageNewWriteP(
storagePgWrite(), PG_FILE_RECOVERYSIGNAL_STR, .noCreatePath = true, .modeFile = recoveryFileMode,
.noAtomic = true, .noSyncPath = true, .user = dataPath.user, .group = dataPath.group),
storagePgWrite(), PG_FILE_RECOVERYSIGNAL_STR, .noCreatePath = true, .modeFile = fileInfo->mode,
.noAtomic = true, .noSyncPath = true, .user = fileInfo->user, .group = fileInfo->group),
NULL);
}
}
@ -1897,10 +1895,11 @@ restoreRecoveryWriteAutoConf(const Manifest *const manifest, const unsigned int
}
static void
restoreRecoveryWrite(const Manifest *manifest)
restoreRecoveryWrite(const Manifest *const manifest, const StorageInfo *const fileInfo)
{
FUNCTION_LOG_BEGIN(logLevelDebug);
FUNCTION_LOG_PARAM(MANIFEST, manifest);
FUNCTION_LOG_PARAM(STORAGE_INFO, fileInfo);
FUNCTION_LOG_END();
// Get PostgreSQL version to write recovery for
@ -1935,9 +1934,9 @@ restoreRecoveryWrite(const Manifest *manifest)
// Write recovery file based on PostgreSQL version
if (pgVersion >= PG_VERSION_RECOVERY_GUC)
restoreRecoveryWriteAutoConf(manifest, pgVersion, restoreLabel);
restoreRecoveryWriteAutoConf(manifest, fileInfo, pgVersion, restoreLabel);
else
restoreRecoveryWriteConf(manifest, pgVersion, restoreLabel);
restoreRecoveryWriteConf(manifest, fileInfo, pgVersion, restoreLabel);
}
}
MEM_CONTEXT_TEMP_END();
@ -2542,8 +2541,13 @@ cmdRestore(void)
}
MEM_CONTEXT_TEMP_END();
// Write recovery settings
restoreRecoveryWrite(jobData.manifest);
// Write recovery settings. Use the data directory to set permissions and ownership for recovery files.
StorageInfo fileInfo = storageInfoP(storagePg(), NULL);
fileInfo.user = restoreManifestOwnerReplace(fileInfo.user, jobData.rootReplaceUser);
fileInfo.group = restoreManifestOwnerReplace(fileInfo.group, jobData.rootReplaceGroup);
fileInfo.mode = fileInfo.mode & (S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
restoreRecoveryWrite(jobData.manifest, &fileInfo);
// Remove backup.manifest
storageRemoveP(storagePgWrite(), BACKUP_MANIFEST_FILE_STR);

41
src/storage/posix/write.c
View File

@ -66,6 +66,18 @@ storageWritePosixFreeResource(THIS_VOID)
/***********************************************************************************************************************************
Open the file
***********************************************************************************************************************************/
static const char *
storageWritePosixOpenOwner(const String *const owner, const char *const defaultOwner)
{
return owner == NULL ? defaultOwner : strZ(owner);
}
static const char *
storageWritePosixOpenOwnerId(const String *const owner, const unsigned int ownerId)
{
return owner == NULL ? "" : zNewFmt("[%u]", ownerId);
}
static void
storageWritePosixOpen(THIS_VOID)
{
@ -108,19 +120,30 @@ storageWritePosixOpen(THIS_VOID)
// Update user/group owner
if (this->interface.user != NULL || this->interface.group != NULL)
{
const StorageInfo info = storageInterfaceInfoP(this->storage, this->nameTmp, storageInfoLevelDetail, .followLink = true);
ASSERT(info.exists);
uid_t updateUserId = userIdFromName(this->interface.user);
if (updateUserId == userId())
updateUserId = (uid_t)-1;
gid_t updateGroupId = groupIdFromName(this->interface.group);
if (updateGroupId == groupId())
updateGroupId = (gid_t)-1;
if (updateUserId == (uid_t)-1)
updateUserId = info.userId;
THROW_ON_SYS_ERROR_FMT(
chown(strZ(this->nameTmp), updateUserId, updateGroupId) == -1, FileOwnerError, "unable to set ownership for '%s'",
strZ(this->nameTmp));
if (updateGroupId == (gid_t)-1)
updateGroupId = info.groupId;
// Continue if one of the owners would be changed
if (updateUserId != info.userId || updateGroupId != info.groupId)
{
THROW_ON_SYS_ERROR_FMT(
chown(strZ(this->nameTmp), updateUserId, updateGroupId) == -1, FileOwnerError,
"unable to set ownership for '%s' to %s%s:%s%s from %s[%u]:%s[%u]", strZ(this->nameTmp),
storageWritePosixOpenOwner(this->interface.user, "[none]"),
storageWritePosixOpenOwnerId(this->interface.user, updateUserId),
storageWritePosixOpenOwner(this->interface.group, "[none]"),
storageWritePosixOpenOwnerId(this->interface.group, updateGroupId),
storageWritePosixOpenOwner(info.user, "[unknown]"), info.userId,
storageWritePosixOpenOwner(info.group, "[unknown]"), info.groupId);
}
}
FUNCTION_LOG_RETURN_VOID();

15
test/src/module/command/restoreTest.c
View File

@ -1856,6 +1856,7 @@ testRun(void)
if (testBegin("restoreRecoveryWrite*()"))
{
const String *pgPath = STRDEF(TEST_PATH "/pg");
const StorageInfo fileInfo = {.user = NULL, .group = NULL, .mode = 0600};
HRN_STORAGE_PATH_CREATE(storageTest, strZ(pgPath), .mode = 0700);
const String *restoreLabel = STRDEF("LABEL");
@ -1876,7 +1877,7 @@ testRun(void)
HRN_CFG_LOAD(cfgCmdRestore, argList);
TEST_ERROR(
restoreRecoveryWriteAutoConf(manifest, PG_VERSION_12, restoreLabel), OptionInvalidError,
restoreRecoveryWriteAutoConf(manifest, &fileInfo, PG_VERSION_12, restoreLabel), OptionInvalidError,
"'standby_mode' setting is not valid for PostgreSQL >= 12\n"
"HINT: use --type=standby instead of --recovery-option=standby_mode=on.");
@ -1892,7 +1893,7 @@ testRun(void)
hrnCfgArgRawZ(argList, cfgOptType, "none");
HRN_CFG_LOAD(cfgCmdRestore, argList);
restoreRecoveryWriteAutoConf(manifest, PG_VERSION_12, restoreLabel);
restoreRecoveryWriteAutoConf(manifest, &fileInfo, PG_VERSION_12, restoreLabel);
TEST_STORAGE_GET_EMPTY(storagePg(), PG_FILE_POSTGRESQLAUTOCONF, .comment = "check postgresql.auto.conf");
TEST_STORAGE_LIST(
@ -1915,7 +1916,7 @@ testRun(void)
"# DO NOT MODIFY\n"
"\t recovery_target_action='promote'\n\n");
restoreRecoveryWriteAutoConf(manifest, PG_VERSION_12, restoreLabel);
restoreRecoveryWriteAutoConf(manifest, &fileInfo, PG_VERSION_12, restoreLabel);
TEST_STORAGE_GET(
storagePg(), PG_FILE_POSTGRESQLAUTOCONF,
@ -1941,7 +1942,7 @@ testRun(void)
"# DO NOT MODIFY\n"
"\t recovery_target_action='promote'\n\n");
restoreRecoveryWriteAutoConf(manifest, PG_VERSION_12, restoreLabel);
restoreRecoveryWriteAutoConf(manifest, &fileInfo, PG_VERSION_12, restoreLabel);
TEST_STORAGE_GET(
storagePg(), PG_FILE_POSTGRESQLAUTOCONF,
@ -1976,7 +1977,7 @@ testRun(void)
hrnCfgArgRawZ(argList, cfgOptRecoveryOption, "restore-command=my_restore_command");
HRN_CFG_LOAD(cfgCmdRestore, argList);
restoreRecoveryWriteAutoConf(manifest, PG_VERSION_12, restoreLabel);
restoreRecoveryWriteAutoConf(manifest, &fileInfo, PG_VERSION_12, restoreLabel);
TEST_STORAGE_GET(
storagePg(), PG_FILE_POSTGRESQLAUTOCONF,
@ -2010,7 +2011,7 @@ testRun(void)
hrnCfgArgRawZ(argList, cfgOptType, "preserve");
HRN_CFG_LOAD(cfgCmdRestore, argList);
restoreRecoveryWrite(manifest);
restoreRecoveryWrite(manifest, &fileInfo);
TEST_STORAGE_GET(
storagePg(), PG_FILE_POSTGRESQLAUTOCONF, "# DO NOT MODIFY\n", .comment = "check postgresql.auto.conf");
@ -2033,7 +2034,7 @@ testRun(void)
hrnCfgArgRaw(argList, cfgOptPgPath, pgPath);
HRN_CFG_LOAD(cfgCmdRestore, argList);
restoreRecoveryWrite(manifest);
restoreRecoveryWrite(manifest, &fileInfo);
TEST_RESULT_BOOL(
bufEq(storageGetP(storageNewReadP(storagePg(), PG_FILE_POSTGRESQLAUTOCONF_STR)), BUFSTRDEF("# DO NOT MODIFY\n")),

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

@ -973,6 +973,12 @@ testRun(void)
fileName = STRDEF(TEST_PATH "/sub1/testfile-abort");
String *fileNameTmp = strNewFmt("%s." STORAGE_FILE_TEMP_EXT, strZ(fileName));
TEST_ASSIGN(file, storageNewWriteP(storageTest, fileName, .user = STRDEF("root")), "new write file (defaults)");
TEST_ERROR(
ioWriteOpen(storageWriteIo(file)), FileOwnerError,
"unable to set ownership for '" TEST_PATH "/sub1/testfile-abort.pgbackrest.tmp' to root[0]:[none] from " TEST_USER "["
TEST_USER_ID_Z "]:" TEST_GROUP "[" TEST_GROUP_ID_Z "]: [1] Operation not permitted");
TEST_ASSIGN(
file, storageNewWriteP(storageTest, fileName, .user = TEST_USER_STR), "new write file (defaults)");
TEST_RESULT_VOID(ioWriteOpen(storageWriteIo(file)), "open file");
@ -987,6 +993,14 @@ testRun(void)
// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("write file - set mode");
TEST_ASSIGN(
file, storageNewWriteP(storageTest, fileName, .user = TEST_USER_STR, .group = STRDEF("root")),
"new write file (defaults)");
TEST_ERROR(
ioWriteOpen(storageWriteIo(file)), FileOwnerError,
"unable to set ownership for '" TEST_PATH "/sub1/testfile-abort.pgbackrest.tmp' to " TEST_USER "[" TEST_USER_ID_Z
"]:root[0] from " TEST_USER "[" TEST_USER_ID_Z "]:" TEST_GROUP "[" TEST_GROUP_ID_Z "]: [1] Operation not permitted");
fileName = STRDEF(TEST_PATH "/sub2/testfile");
TEST_ASSIGN(