On 10/26/26 05:13, Michael Paquier wrote:
On Sat, Oct 24, 2026 at 09:34:56PM +0000, David Steele wrote:
I'm not much a fan of clobbering the stat data given in input. It's
true that it should not matter in practice for the current state of
the code on HEAD, but such practice never gives me a warm feeling as
it makes harder to track the state of the operations across the stack
of function calls. So perhaps make a copy of the input value and
update the copy if we're dealing with a non-relation file for the
mew fstat() you are adding?
Sure thing. This does make the patch larger, but nothing scary looking
that I can see. Done in v2.
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.
Calling fstat() for everything or passing around a more consistent
picture of the stats data was my first feeling here, but yes, I'd side
with your argument that for non-relation files this is a waste as we
would send over the wire data that could be rebuilt from the WAL
anyway. The window is between the lstat() in sendDir() and the
OpenTransientFile() in sendFile(), which should be short in practice,
but I've also seen numerous systems where such short windows could
take seconds depending on what the CPU considers doing.
Yes, this gap is what concerns me. We assume these errors are rare since
there have been no field reports but in practice it may also be because
they would be almost impossible to reproduce.
At least
things are clearly designed so as we don't send unnecessary data over
the wire if we know that WAL will have it anyway.
Right.
You have a few more funky cases here, I think:
- If a file is truncated after the fstat(), we still send a
zero-filled file worth the initial size reported. pg_verifybackup
considers that as OK.
Yeah, I've never been too excited about this but it is a limitation of
the tar format that the file size needs to be known in advance of
sending bytes.
For relation files this is harmless since the zero-bytes will be
overwritten during WAL replay. This fix handles the case of replaced
files so their size should be stable during the copy.
- File grown in place after the fstat().
For relation files this is fine. We ignore the extra bytes and they are
reconstructed by WAL replay. Non-relation files should not grow since we
do the fstat() after open() and replaced files should never be modified.
None of these stand as far as we can rely on an atomic rename() with a
temporary file.
After this patch is applied that should be true.
You have looked at the core code and checked that we
do not have paths that use a different practice than the usual tmp
file+rename, and did not find anything fishy, right?
Yes, but I missed pg_twophase on the first pass. In this case the trick
was that the file *is* WAL-logged but then accessed before WAL replay
reconstructs it. It is possible to fix that, maybe, but it would be a
much larger change.
I have double
checked myself, just to be on the safe side, but things are paired
with WAL records (2PC files, logical decoding mappings, etc.).
My hope is that all the eyeballs have caught the only problematic case
(2pc) but we'll need to be careful when adding new files in the future.
If all the examples in the code are correct it should be easier to get
it right.
Regards,
-David
From 16aa25488432fbb59714930ff295ea34dbe17d4d Mon Sep 17 00:00:00 2001
From: David Steele <[email protected]>
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 <[email protected]>
Reviewed-by: Chao Li <[email protected]>
Reviewed-by: Michael Paquier <[email protected]>
Tested-by: Shihao Zhong <[email protected]>
Discussion:
https://postgr.es/m/cagrkxqs_on-plgcv6xjh8mvghdcpmfwtythw3r8pykug+hs...@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
