Re: thinko in basic_archive.c - Mailing list pgsql-hackers

 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in 
> On Mon, Oct 17, 2022 at 6:45 PM Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > On Fri, Oct 14, 2022 at 4:45 AM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > 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.
> >
> > Yeah, leaving a potentially unbounded number of files around after
> > system crashes seems pretty unfriendly. I'm not sure how to fix that,
> > exactly.

Unbounded number of sequential crash-restarts itself is a more serious
problem..

An archive module could clean up them at startup or at the first call
but that might be dangerous (since archive directory is I think
thought as external resource).

Honestly, I'm not sure about a reasonable scenario where simultaneous
archivings of a same file is acceptable, though. I feel that we should
not allow simultaneous archiving of the same segment by some kind of
interlocking. In other words, we might should serialize duplicate
archiving of asame file.

In another direction, the current code allows duplicate simultaneous
copying to temporary files with different names then the latest
renaming wins.  We reach the almost same result (on Linuxen (or
POSIX?))  by unlinking the existing tempfile first then create a new
one with the same name then continue. Even if the tempfile were left
alone after a crash, that file would be unlinked at the next trial of
archiving. But I'm not sure how this can be done on Windows..  In the
first place I'm not sure that the latest-unconditionally-wins policy
is appropriate or not, though.

> A simple server restart while the basic_archive module is copying
> to/from temp file would leave the file behind, see[1]. I think we can
> fix this by defining shutdown_cb for the basic_archive module, like
> the attached patch. While this helps in most of the crashes, but not
> all. However, this is better than what we have right now.

ShutdownWalRecovery() does the similar thing, but as you say this one
covers rather narrow cases than that since RestoreArchiveFile()
finally overwrites the left-alone files at the next call for that
file.

# The patch seems forgetting to clear the tmepfilepath *after* a
# successful renaming. And I don't see how the callback is called.

> [1] ubuntu:~/postgres/contrib/basic_archive/archive_directory$ ls
> 000000010000000000000001
> archtemp.000000010000000000000002.2493876.1666091933457
> archtemp.000000010000000000000002.2495316.1666091958680

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center