Re: thinko in basic_archive.c

On Fri, Oct 14, 2022 at 02:15:19PM +0530, Bharath Rupireddy wrote:
> 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?

While unlikely, I think it's theoretically possible. If there is a
collision, archiving should fail and retry later with a different temporary
file name.

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

The temporary files are not automatically removed after a crash. The
documentation for basic archive has a note about this [0].

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

Yeah, it's debatable how much we care about this for basic_archive. We
previously decided that we at least care a little [1], so that's why we
have such elaborate temporary file names. If anything, I hope that the
presence of this logic causes archive module authors to think about these
problems.

> The patch LGTM.

Thanks!

[0] https://www.postgresql.org/docs/devel/basic-archive.html#id-1.11.7.15.6
[1] https://postgr.es/m/CA%2BTgmoaSkSmo22SwJaV%2BycNPoGpxe0JV%3DTcTbh4ip8Cwjr0ULQ%40mail.gmail.com

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com