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

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