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 ?