From 16aa25488432fbb59714930ff295ea34dbe17d4d Mon Sep 17 00:00:00 2001 From: David Steele Date: Mon, 26 Oct 2026 10:14:42 +0000 Subject: Avoid truncated or zero-padded copies of replaced files in backups. Base backups stat each file while scanning the data directory and later send exactly the number of bytes reported by the stat. If the file is shorter when it is read then the copy is zero-padded. This is harmless for relation files since any data written after the stat will be restored from WAL. Some files that are not relation files are written to a temp file and then renamed into place, e.g. postgresql.auto.conf by ALTER SYSTEM and pg_logical/replorigin_checkpoint by every checkpoint. If the rename happens between the stat and the open then the backup contains the new file truncated or zero-padded to the size of the old file. WAL replay does not repair these files. A damaged replorigin_checkpoint fails its CRC check and the restored cluster will not start. A damaged postgresql.auto.conf either fails to parse or, if the truncation falls on a line boundary, silently loses settings. Some contrib modules also write files to the data directory this way. pg_prewarm rewrites autoprewarm.blocks every pg_prewarm.autoprewarm_interval. A truncated autoprewarm.blocks fails to load. The autoprewarm worker then exits and is not restarted so blocks are neither prewarmed nor dumped. pg_stash_advice rewrites pg_stash_advice.tsv every pg_stash_advice.persist_interval when the stashed advice has changed. A truncated pg_stash_advice.tsv may load a truncated advice string without error since the last line is not required to end with a newline. Two-phase state files had a related problem since they were written in place and could be copied while partially written. That was fixed separately in commit a5ccf20801c. For files that are not relation files, fstat() the opened file and use that stat for the tar header and the backup manifest. Since the rename is atomic the opened file is always complete. Relation files are not replaced this way and are still sent using the earlier stat. Calling fstat() for every file would be simpler, but using the earlier stat for relation files means that data added to relation files after the stat is not copied. WAL replay will restore that data anyway so copying it is wasteful. Back-patch to all supported branches. Before v17, sendFile() does not receive the relfilenumber so on those branches sendDir() should save the result of its existing parse_filename_for_nontemp_relation() call and pass it to sendFile(). That call is only made for database directories so relation files in global/ are not identified and use the stat of the opened file. The copy may then include data written between the earlier stat and the open, which is harmless since WAL replay will overwrite that data, just as it overwrites blocks modified while the file is being copied. Author: David Steele Reviewed-by: Chao Li Reviewed-by: Michael Paquier Tested-by: Shihao Zhong Discussion: https://postgr.es/m/CAGRkXqS_On-pLgcv6xjH8mvGhdcPmfWtythW3r8pYKUG+hsiuQ@mail.gmail.com Backpatch-through: 14 --- src/backend/backup/basebackup.c | 35 ++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c index d7c485dc002..f5c90db0ef4 100644 --- a/src/backend/backup/basebackup.c +++ b/src/backend/backup/basebackup.c @@ -93,7 +93,7 @@ static int64 sendDir(bbsink *sink, const char *path, int basepathlen, bool sizeo backup_manifest_info *manifest, Oid spcoid, IncrementalBackupInfo *ib); static bool sendFile(bbsink *sink, const char *readfilename, const char *tarfilename, - struct stat *statbuf, bool missing_ok, + const struct stat *statbuf, bool missing_ok, Oid dboid, Oid spcoid, RelFileNumber relfilenumber, unsigned segno, backup_manifest_info *manifest, @@ -1575,12 +1575,15 @@ sendDir(bbsink *sink, const char *path, int basepathlen, bool sizeonly, * If the whole file is to be sent, then incremental_blocks should be NULL, * and num_incremental_blocks can have any value, as it will be ignored. * + * For non-relation files, fstat() of the opened file is used instead of + * statbuf. + * * Returns true if the file was successfully sent, false if 'missing_ok', * and the file did not exist. */ static bool sendFile(bbsink *sink, const char *readfilename, const char *tarfilename, - struct stat *statbuf, bool missing_ok, Oid dboid, Oid spcoid, + const struct stat *statbuf, bool missing_ok, Oid dboid, Oid spcoid, RelFileNumber relfilenumber, unsigned segno, backup_manifest_info *manifest, unsigned num_incremental_blocks, BlockNumber *incremental_blocks, unsigned truncation_block_length) @@ -1593,6 +1596,7 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename, bool verify_checksum = false; pg_checksum_context checksum_ctx; int ibindex = 0; + struct stat filestat; if (pg_checksum_init(&checksum_ctx, manifest->checksum_type) < 0) elog(ERROR, "could not initialize checksum of file \"%s\"", @@ -1608,7 +1612,20 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename, errmsg("could not open file \"%s\": %m", readfilename))); } - _tarWriteHeader(sink, tarfilename, NULL, statbuf, false); + /* + * Non-relation files, e.g. postgresql.auto.conf, may be replaced by + * rename between the caller's stat and our open. fstat() the file we + * opened to avoid truncating or zero-padding its contents, which WAL + * replay cannot repair. Use a copy so the caller's stat is not modified. + */ + filestat = *statbuf; + + if (!RelFileNumberIsValid(relfilenumber) && fstat(fd, &filestat) != 0) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not stat file \"%s\": %m", readfilename))); + + _tarWriteHeader(sink, tarfilename, NULL, &filestat, false); /* * Checksums are verified in multiples of BLCKSZ, so the buffer length @@ -1697,13 +1714,13 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename, */ if (incremental_blocks == NULL) { - size_t remaining = statbuf->st_size - bytes_done; + size_t remaining = filestat.st_size - bytes_done; /* * If we've read the required number of bytes, then it's time to * stop. */ - if (bytes_done >= statbuf->st_size) + if (bytes_done >= filestat.st_size) break; /* @@ -1798,9 +1815,9 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename, } /* If the file was truncated while we were sending it, pad it with zeros */ - while (bytes_done < statbuf->st_size) + while (bytes_done < filestat.st_size) { - size_t remaining = statbuf->st_size - bytes_done; + size_t remaining = filestat.st_size - bytes_done; size_t nbytes = Min(sink->bbs_buffer_length, remaining); MemSet(sink->bbs_buffer, 0, nbytes); @@ -1835,8 +1852,8 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename, total_checksum_failures += checksum_failures; - AddFileToBackupManifest(manifest, spcoid, tarfilename, statbuf->st_size, - (pg_time_t) statbuf->st_mtime, &checksum_ctx); + AddFileToBackupManifest(manifest, spcoid, tarfilename, filestat.st_size, + (pg_time_t) filestat.st_mtime, &checksum_ctx); return true; } -- 2.34.1