Hi! I'd like to resume the discussion on this subject. Sorry for so long delay.
On Sat, Jan 20, 2018 at 6:13 PM Magnus Hagander <mag...@hagander.net> wrote: > On Tue, Nov 28, 2017 at 2:47 AM, Michael Paquier <michael.paqu...@gmail.com> > wrote: >> >> On Mon, Nov 27, 2017 at 3:28 PM, Alexander Korotkov >> <a.korot...@postgrespro.ru> wrote: >> > Attached patch atomic-pgrename-windows-1.patch fixes this problem. It >> > appears to be possible to atomically replace file on Windows – >> > ReplaceFile() >> > does that. ReplaceFiles() requires target file to exist, this is why we >> > still need to call MoveFileEx() when it doesn't exist. >> >> Do you think that it could be safer to unlink the target file first >> with pgunlink()? This way you make sure that the target file is >> removed and not locked. This change makes me worrying about the >> introduction of more race conditions. > > Unlinking it first seems dangerous, as pointed out by Andres. > > What about first trying ReplaceFile() and then if it fails with "target > doesn't exist", then call MoveFileEx(). > > Or the other way around -- try MoveFileEx() first since that seems to work > most of the time today (if it mostly broke we'd be in trouble already), and > if it fails with a sharing violation, try ReplaceFile()? And perhaps end up > doing it something similar to what we do with shared memory which is just to > loop over it and try each a couple of time, before giving up and failing? 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. Links 1. https://www.postgresql.org/message-id/CAPpHfds9trA6ipezK3BsuuOSQwEmESiqj8pkOxACFJpoLpcoNw%40mail.gmail.com ------ Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
pgrename_temp-1.patch
Description: Binary data
