Re: thinko in basic_archive.c

On Fri, Oct 14, 2022 at 10:11 AM Nathan Bossart
<nathandbossart@gmail.com> wrote:
>
> I intended for the temporary file name generated by basic_archive.c to

I'm trying to understand this a bit:

    /*
     * Pick a sufficiently unique name for the temporary file so that a
     * collision is unlikely.  This helps avoid problems in case a temporary
     * file was left around after a crash or another server happens to be
     * archiving to the same directory.
     */

Given that temp file name includes WAL file name, epoch to
milliseconds scale and MyProcPid, can there be name collisions after a
server crash or even when multiple servers with different pids are
archiving/copying the same WAL file to the same directory?

What happens to the left-over temp files after a server crash? Will
they be lying around in the archive directory? I understand that we
can't remove such files because we can't distinguish left-over files
from a crash and the temp files that another server is in the process
of copying.

If the goal is to copy files atomically, why can't we name the temp
file 'wal_file_name.pid.temp', assuming no PID wraparound and get rid
of appending time? Since basic_archive is a test module illustrating
archive_library implementation, do we really need to worry about name
collisions?

> I've attached a small patch that fixes this so that the temporary file name
> includes the timestamp in milliseconds for when it was created.

The patch LGTM.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com