Hi Shihao,
On 10/25/26 16:36, shihao zhong wrote:
I reproduced all three cases on master with an
injection point, and v1 fixes them.
Thank you for testing!
I confirmed with injection points as well but was not sure it was worth
adding them to the patch since the fix is pretty straightforward.
I found one more file with the same problem. A checkpoint rewrites
pg_twophase files in place with O_TRUNC. If the backup copies one
right after the truncate, it gets an empty file.
Good catch. My audit noticed this truncate but failed to note that the
file would be accessed before WAL replay reconstructed it.
One small correction: a checkpoint writes each state file only once and
then flags the transaction as on disk. The window therefore appears to
be between creating the file and finishing the write rather than a
truncate of existing contents.
This is not limited to backups. It appears that if the checkpointer is
killed between the open and the write, or the OS crashes before the
fsync, then the file is left empty or partial and crash recovery fails
the same way.
A few notes on the patch:
- A stale .tmp file is never removed. restoreTwoPhaseData() will need to
handle this or they will build up.
- There's a double blank line before the rename.
- In the commit message, I think "truncates and rewrites" should be
something like "creates and writes".
> v1 cannot help here, since the open file really is empty.
Since v1 can't help here it should probably be a separate patch. Or, we
could include it as the first patch in a set with mine. At this point it
would be good to get some feedback from a committer to see how they want
to proceed.
Minor releases are soon but these bugs are so old that I'm not sure it
makes sense to rush.
Also, pg_verifybackup reports all of these broken backups as fine.
This is as expected. pg_verify can only work with the data it was
provided. If we fix the upstream code then pg_verify will be fine.
Regards,
-David
