Re: Avoid truncated or zero-padded copies of replaced files in backups.

On Sat, Oct 24, 2026 at 09:34:56PM +0000, David Steele wrote:
> 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

+ if (!RelFileNumberIsValid(relfilenumber) && fstat(fd, statbuf) != 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m", readfilename)));

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?

> 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. 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.

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.
- File grown in place after the fstat().

None of these stand as far as we can rely on an atomic rename() with a
temporary file. 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? 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.).
--
Michael