Date: Fri, 14 Oct 2022 11:33:10 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: thinko in basic_archive.c
Message-ID: <20221014183310.GB1678424@nathanxps13>
References: <20221014044106.GA1673343@nathanxps13>
 <CALj2ACWhqfi393dZ5Sdo=0j38yPFmwdSkCe_7SBi93x3s5vfJQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CALj2ACWhqfi393dZ5Sdo=0j38yPFmwdSkCe_7SBi93x3s5vfJQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20221014183310.GB1678424%40nathanxps13>
Precedence: bulk

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