Re: thinko in basic_archive.c

On Sat, Oct 15, 2022 at 02:10:26PM -0700, Nathan Bossart wrote:
> On Sat, Oct 15, 2022 at 10:19:05AM +0530, Bharath Rupireddy wrote:
>> Can you please help me understand how name collisions can happen with
>> temp file names including WAL file name, timestamp to millisecond
>> scale, and PID? Having the timestamp is enough to provide a non-unique
>> temp file name when PID wraparound occurs, right? Am I missing
>> something here?
>
> Outside of contrived cases involving multiple servers, inaccurate clocks,
> PID reuse, etc., it seems unlikely.

With a name based on a PID in a world where pid_max can be larger than
the default and a timestamp, I would say even more unlikely than what
you are implying with unlikely ;p

> I think the right way to do this would be to add handling for leftover
> files in the sigsetjmp() block and a shutdown callback (which just sets up
> a before_shmem_exit callback). While this should ensure those files are
> cleaned up after an ERROR or FATAL, crashes and unlink() failures could
> still leave files behind. We'd probably also need to avoid cleaning up the
> temp file if copy_file() fails because it already exists, as we won't know
> if it's actually ours. Overall, I suspect it'd be more trouble than it's
> worth.

Agreed. My opinion is that we should keep basic_archive as
minimalistic as we can: short still useful. It does not have to be
perfect, just to fit with what we want it to show, as a reference.

Anyway, the maths were wrong, so I have applied the patch of upthread,
with an extra pair of parenthesis, a comment where epoch is declared
to tell that it is in milliseconds, and a comment in basic_archive's
Makefile to mention the reason why we have NO_INSTALLCHECK.
--
Michael