Re: Atomic rename feature for Windows. - Mailing list pgsql-hackers

Thank you,

In this variant:

1) renamed file win10.manifest to windows.manifest

2) renamed function pgrename_win10 to pgrename_windows_posix_semantics

3) Function pgrename returns result of pgrename_windows_posix_semantics 
function and not contiue run old version of function.

4) Added call GetLastError() after error MultiByteToWideChar fuction.

> +typedef struct _FILE_RENAME_INFO_VVS {
> +   union {
> +       BOOLEAN ReplaceIfExists;  // FileRenameInfo
> +       DWORD Flags;              // FileRenameInfoEx
> +   } DUMMYUNIONNAME;
> +   HANDLE RootDirectory;
> +   DWORD FileNameLength;
> +   WCHAR FileName[MAX_PATH];
> +} FILE_RENAME_INFO_VVS;
>
> Why can't we use a system header[2] for this?
I have a dynamic memory allocation version in the first patch.

     len = wcslen(to_w);
     rename_info = (FILE_RENAME_INFO*)malloc(sizeof(FILE_RENAME_INFO) + 
(len + 1) * sizeof(wchar_t));

     rename_info->ReplaceIfExists = TRUE;
     rename_info->Flags = FILE_RENAME_FLAG_POSIX_SEMANTICS | 
FILE_RENAME_FLAG_REPLACE_IF_EXISTS;

     rename_info->RootDirectory = NULL;
     rename_info->FileNameLength = len;
     memcpy(rename_info->FileName, to_w, (len + 1) * sizeof(wchar_t));

Is this code better? Maybe there is another correct method?

I checked the pgrename_windows_posix_semantics() function on Windows 7. 
It returns error 87: Parameter is incorrect. Hence, it is necessary to 
check the Windows version and call the old pgrename function for old 
Windows.


Victor Spirin
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company

07.09.2021 4:40, Thomas Munro пишет:
> On Tue, Sep 7, 2021 at 5:44 AM Victor Spirin <v.spirin@postgrespro.ru> wrote:
>> I have changed the way I add the manifest to projects. I used the
>> AdditionalManifestFiles option for a VS project.
> Hi Victor,
>
> Thanks for working on this!
>
> I wonder if POSIX-style rename is used automatically on recent
> Windows, based on the new clue that DeleteFile() has started
> defaulting to POSIX semantics[1] (maybe it would require ReplaceFile()
> instead of MoveFileEx(), but I have no idea.)  If so, one question is
> whether we'd still want to do this explicit POSIX rename dance, or
> whether we should just wait a bit longer for it to happen
> automatically on all relevant systems (plus tweak to use ReplaceFile()
> if necessary).  If not, we might want to do what you're proposing
> anyway, especially if ReplaceFile() is required, because its interface
> is weird (it only works if the other file exists?).  Hmm, that alone
> would be a good reason to go with your plan regardless, and of course
> it would be good to see this fixed everywhere ASAP.
>
> We still have to answer that question for pgunlink().  I was
> contemplating that over in that other thread, because unlink() ->
> EACCES is blocking something I'm working on.  I found a partial
> solution to that that works even on old and non-NTFS systems, and I
> was thinking that would be enough for now and we could just patiently
> wait until automatic POSIX semantics to arrives on all relevant
> systems as the real long term solution, so I didn't need to expend
> energy doing an intermediate explicit POSIX-mode wrapper like what
> you're proposing.  But then it seems strange to make a different
> choice about that for rename() and unlink().  So... do you think it
> would make sense to extend your patch to cover unlink() too?
>
> It would be great to have a tool in the tree that tests directory
> entry semantics, called something like src/bin/pg_test_dirmod, so that
> it becomes very clear when POSIX semantics are being used.  It could
> test various interesting unlink and rename scenarios through our
> wrappers (concurrent file handles, creating a new file with the name
> of the old one, unlinking the containing directory, ...).  It could
> run on the build farm animals, and we could even ask people to run it
> when they report problems, to try to lift the fog of bizarro Windows
> file system semantics.
>
> How exactly does the function fail on a file system that doesn't
> support the new POSIX semantics?  Assuming there is something like
> ENOSUPP to report "file system says no", do we want to keep trying
> every time, or remember that it doesn't work?  I guess the answer may
> vary per mount point, which makes it hard to track when you only have
> an fd...
>
> If it fails because of a sharing violation, it seems strange that we
> immediately fall back to the old code to do the traditional (and
> horrible) sleep/retry loop.  That means that in rare conditions we can
> still get the old behaviour that leads to problems, just because of a
> transient condition.  Hmm.  Would it make more sense to say: fall back
> to the traditional behaviour only for ENOSUPP (if there is such a
> thing), cope with transient sharing violations without giving up on
> POSIX semantics, and report all other failures immediately?
>
> I agree that the existing enum CHECKSUM_TYPE_NONE + friends should be
> renamed to something less collision-prone and more consistent with the
> name of the enum ("pg_checksum_type"), so I'd vote for adding a PG_
> prefix, in a separate patch.
>
> +    <Manifest>
> +      <AdditionalManifestFiles>src/port/win10.manifest</AdditionalManifestFiles>
> +    </Manifest>
>
> I have no opinion on how you're supposed to test for OS versions, but
> one trivial observation is that that file declares support for many
> Windows releases, and I guess pretty soon you'll need to add 11, and
> then we'll wonder why it says 10 in the file name.  Would it be better
> as "windows.manifest" or something?
>
> +pgrename_win10(const char *from, const char *to)
>
> Same thought on the name: this'll age badly.  What about something
> like pgrename_windows_posix_semantics?
>
> +typedef struct _FILE_RENAME_INFO_VVS {
> +   union {
> +       BOOLEAN ReplaceIfExists;  // FileRenameInfo
> +       DWORD Flags;              // FileRenameInfoEx
> +   } DUMMYUNIONNAME;
> +   HANDLE RootDirectory;
> +   DWORD FileNameLength;
> +   WCHAR FileName[MAX_PATH];
> +} FILE_RENAME_INFO_VVS;
>
> Why can't we use a system header[2] for this?
>
> +   if (MultiByteToWideChar(CP_ACP, 0, (LPCCH)from, -1,
> (LPWSTR)from_w, MAX_PATH) == 0) return -1;
> +   if (MultiByteToWideChar(CP_ACP, 0, (LPCCH)to, -1,
> (LPWSTR)rename_info.FileName, MAX_PATH) == 0) return -1;
>
> Don't these need _dosmaperr(GetLastError())?
>
> [1] https://www.postgresql.org/message-id/20210905214437.y25j42yigwnbdvtg%40alap3.anarazel.de
> [2] https://docs.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_rename_info
>
>