From ffc679fa4f4261cf9050571bb486575b2e4995f3 Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Tue, 31 Jan 2023 20:01:49 +1300 Subject: [PATCH v4 1/2] Acquire ControlFileLock in base backups. The control file could previously be overwritten while a base backup was reading it. On some file systems with weak interlocking, including ext4 and ntfs, the concurrent read could see a mixture of old and new contents. The resulting copy would be corrupted, and the new cluster would not be able to start up. Other files are read without interlocking, but those will be corrected by replaying the log. Exclude this possibility by acquiring ControlFileLock. Copy into a temporary buffer first because we don't want to hold the lock while in sendFile() code that might sleep if throttling is configured. This requires a minor adjustment to sendFileWithContent() to support binary data. This does not address the same possibility when using using external file system copy tools (as described in the documentation under "Making a Base Backup Using the Low Level API"). Back-patch to all supported releases. Reviewed-by: Anton A. Melnikov (earlier version) Discussion: https://postgr.es/m/20221123014224.xisi44byq3cf5psi%40awork3.anarazel.de --- src/backend/backup/basebackup.c | 36 ++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c index 45be21131c..f03496665f 100644 --- a/src/backend/backup/basebackup.c +++ b/src/backend/backup/basebackup.c @@ -24,6 +24,7 @@ #include "backup/basebackup_target.h" #include "commands/defrem.h" #include "common/compression.h" +#include "common/controldata_utils.h" #include "common/file_perm.h" #include "lib/stringinfo.h" #include "miscadmin.h" @@ -39,6 +40,7 @@ #include "storage/dsm_impl.h" #include "storage/fd.h" #include "storage/ipc.h" +#include "storage/lwlock.h" #include "storage/reinit.h" #include "utils/builtins.h" #include "utils/guc.h" @@ -84,7 +86,7 @@ static bool sendFile(bbsink *sink, const char *readfilename, const char *tarfile struct stat *statbuf, bool missing_ok, Oid dboid, backup_manifest_info *manifest, const char *spcoid); static void sendFileWithContent(bbsink *sink, const char *filename, - const char *content, + const char *content, int len, backup_manifest_info *manifest); static int64 _tarWriteHeader(bbsink *sink, const char *filename, const char *linktarget, struct stat *statbuf, @@ -315,7 +317,8 @@ perform_base_backup(basebackup_options *opt, bbsink *sink) if (ti->path == NULL) { - struct stat statbuf; + ControlFileData *control_file; + bool crc_ok; bool sendtblspclinks = true; char *backup_label; @@ -324,14 +327,14 @@ perform_base_backup(basebackup_options *opt, bbsink *sink) /* In the main tar, include the backup_label first... */ backup_label = build_backup_content(backup_state, false); sendFileWithContent(sink, BACKUP_LABEL_FILE, - backup_label, &manifest); + backup_label, -1, &manifest); pfree(backup_label); /* Then the tablespace_map file, if required... */ if (opt->sendtblspcmapfile) { sendFileWithContent(sink, TABLESPACE_MAP, - tablespace_map->data, &manifest); + tablespace_map->data, -1, &manifest); sendtblspclinks = false; } @@ -340,13 +343,13 @@ perform_base_backup(basebackup_options *opt, bbsink *sink) sendtblspclinks, &manifest, NULL); /* ... and pg_control after everything else. */ - if (lstat(XLOG_CONTROL_FILE, &statbuf) != 0) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not stat file \"%s\": %m", - XLOG_CONTROL_FILE))); - sendFile(sink, XLOG_CONTROL_FILE, XLOG_CONTROL_FILE, &statbuf, - false, InvalidOid, &manifest, NULL); + LWLockAcquire(ControlFileLock, LW_SHARED); + control_file = get_controlfile(DataDir, &crc_ok); + LWLockRelease(ControlFileLock); + sendFileWithContent(sink, XLOG_CONTROL_FILE, + (char *) control_file, sizeof(*control_file), + &manifest); + pfree(control_file); } else { @@ -591,7 +594,7 @@ perform_base_backup(basebackup_options *opt, bbsink *sink) * complete segment. */ StatusFilePath(pathbuf, walFileName, ".done"); - sendFileWithContent(sink, pathbuf, "", &manifest); + sendFileWithContent(sink, pathbuf, "", -1, &manifest); } /* @@ -619,7 +622,7 @@ perform_base_backup(basebackup_options *opt, bbsink *sink) /* unconditionally mark file as archived */ StatusFilePath(pathbuf, fname, ".done"); - sendFileWithContent(sink, pathbuf, "", &manifest); + sendFileWithContent(sink, pathbuf, "", -1, &manifest); } /* Properly terminate the tar file. */ @@ -1030,18 +1033,19 @@ SendBaseBackup(BaseBackupCmd *cmd) */ static void sendFileWithContent(bbsink *sink, const char *filename, const char *content, + int len, backup_manifest_info *manifest) { struct stat statbuf; - int bytes_done = 0, - len; + int bytes_done = 0; pg_checksum_context checksum_ctx; if (pg_checksum_init(&checksum_ctx, manifest->checksum_type) < 0) elog(ERROR, "could not initialize checksum of file \"%s\"", filename); - len = strlen(content); + if (len < 0) + len = strlen(content); /* * Construct a stat struct for the backup_label file we're injecting in -- 2.41.0