From 3ff88ffbb495fd669e9a361478e60192930ee0a1 Mon Sep 17 00:00:00 2001 From: David Steele Date: Tue, 25 Apr 2023 11:52:28 +0300 Subject: [PATCH] Avoid chown() on recovery files during restore. 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. --- doc/xml/release.xml | 21 +++++++++++ src/command/restore/restore.c | 50 +++++++++++++++------------ src/storage/posix/write.c | 41 +++++++++++++++++----- test/src/module/command/restoreTest.c | 15 ++++---- test/src/module/storage/posixTest.c | 14 ++++++++ 5 files changed, 102 insertions(+), 39 deletions(-) diff --git a/doc/xml/release.xml b/doc/xml/release.xml index 9d9e65098..3f8fd469b 100644 --- a/doc/xml/release.xml +++ b/doc/xml/release.xml @@ -18,6 +18,8 @@ + + @@ -27,6 +29,20 @@

Allow page header checks to be skipped.

+ + + + + + + + + + + +

Avoid chown() on recovery files during restore.

+
+ @@ -12637,6 +12653,11 @@ m-borger + + Marcelo Henrique Neppel + marceloneppel + + Marco Montagna mmontagna diff --git a/src/command/restore/restore.c b/src/command/restore/restore.c index bda49178f..668a2298b 100644 --- a/src/command/restore/restore.c +++ b/src/command/restore/restore.c @@ -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); diff --git a/src/storage/posix/write.c b/src/storage/posix/write.c index 5c35f82a8..4e993d006 100644 --- a/src/storage/posix/write.c +++ b/src/storage/posix/write.c @@ -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(); diff --git a/test/src/module/command/restoreTest.c b/test/src/module/command/restoreTest.c index 9b2c78bf8..5ae68cb94 100644 --- a/test/src/module/command/restoreTest.c +++ b/test/src/module/command/restoreTest.c @@ -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")), diff --git a/test/src/module/storage/posixTest.c b/test/src/module/storage/posixTest.c index 6c885bd2b..3c4323392 100644 --- a/test/src/module/storage/posixTest.c +++ b/test/src/module/storage/posixTest.c @@ -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(