Re: Postgresql13_beta1 (could not rename temporary statistics file) Windows 64bits

On Mon, Jun 15, 2020 at 11:49:33PM -0300, Ranier Vilela wrote:
> I can confirm that the problem is in pgrename (dirmod.c),
> something is not OK, with MoveFileEx, even with the
> (MOVEFILE_REPLACE_EXISTING) flag.
>
> Replacing MoveFileEx, with
> unlink (to);
> rename (from, to);
>
> #if defined (WIN32) &&! defined (__ CYGWIN__)
> unlink(to);
> while (rename (from, to)! = 0)
> #else
> while (rename (from, to) <0)
> #endif
>
> The log messages have disappeared.
>
> I suspect that if the target (to) file exists, MoveFileEx, it is failing to
> rename, even with the flag enabled.
>
> Windows have the native rename function (
> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/rename-wrename?view=vs-2019
> )
> However, it fails if the target (to) file exists.
>
> Question, is it acceptable delete the target file, if it exists, to
> guarantee the rename?

I don't think so - the idea behind writing $f.tmp and renaming it to $f is that
it's *atomic*.

I found an earlier thread addressing the same problem - we probably both
should've found this earlier.
https://commitfest.postgresql.org/27/2230/
https://www.postgresql.org/message-id/flat/CAPpHfds9trA6ipezK3BsuuOSQwEmESiqj8pkOxACFJpoLpcoNw%40mail.gmail.com#9b04576b717175e9dbf03cc991977d3f

That thread goes back to 2017, so I don't think this is a new problem in v13.
I'm not sure why you wouldn't also see the same behavior in v12.

There's a couple patches in that thread, and the latest patch was rejected.

Maybe you'd want to test them out and provide feedback.

BTW, the first patch does:

! if (filePresent)
! {
! if (ReplaceFile(to, from, NULL, REPLACEFILE_IGNORE_MERGE_ERRORS, 0, 0))
! break;
! }
! else
! {
! if (MoveFileEx(from, to, MOVEFILE_REPLACE_EXISTING))
! break;
! }

Since it's racy to first check if the file exists, I would've thought we should
instead do:

ret = ReplaceFile()
if ret == OK:
break
else if ret == FileDoesntExist:
if MoveFileEx():
break

Or, should we just try to create a file first, to allow ReplaceFile() to always
work ?

--
Justin