Re: [PATCH] Atomic pgrename on Windows

On Tue, Aug 06, 2019 at 09:03:08PM +0300, Alexander Korotkov wrote:
>
> ...
>
> Unfortunately, it seems that none of such strategies would fit all the
> cases.
>
> Basically, we have two option for renaming a file. * MoveFileEx() –
> safest possible option, less likely loose files, but takes a lock on
> target. * ReplaceFile() – "lockless" option, but may loose files on
> OS crash.
>
> Also we have two different cases of files renaming: 1) Renaming
> durable files. When target already exists, on OS crash we expect
> finding either old or new file in target filename. We don't expect to
> find nothing in target filename. 2) Renaming temporary files. In
> this case we don't much care on loosing files on OS crash. But
> locking appears to be an issue in some cases.
>
> So, in 1st case it doesn't seem like an option to try ReplaceFile()
> either in first or second place. In both ways it causes a risk to
> loose a target file, which seems unacceptable.
>
> In the 2nd case, trying MoveFileEx() then ReplaceFile() or vise versa
> might work. But error codes of these functions doesn't tell explicitly
> whether target file exists. So, I prefer checking it explicitly with
> GetFileAttributes().
>
> Attached patch implements idea described in [1]. It implements
> separate pgrename_temp() function, which is better for concurrent
> operation but less safe.
>

I don't have access to a Windows machine and my developer experience
with that platform is pretty much nil, but I think this patch makes
sense. It's not an ideal solution, but it's not clear such solution
exists, and an improvement is better than nothing.

I have two minor comments about rename_temp:

1) The name might seem to be hinting it's about files opened using
OpenTemporaryFile, but in practice it's about files that are not
critical. But maybe it's true.

2) I think the rename_temp comment should mention it can only be used in
cases when the renames happen in a single process (non-concurrently). If
we could call rename_temp() concurrently from two different processes,
it won't work as expected. Of course, we only call rename_temp from stat
collector and syslogger, where it obviously works.

Anyway, I'm really nitpicking here ...

Are there any objections to get the current patch committed as is, so
that it does not get pushed yet again to the next commitfest.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services