Atomic rename feature for Windows.

On Mon, Jul 05, 2021 at 04:53:06PM +0300, Victor Spirin wrote:

This patch related to this post:
/messages/by-id/CAEepm=0FV-k+=d9z08cW=ZXoR1=kw9wdpkP6WAuOrKJdz-8ujg@mail.gmail.com

How does that cope with durable_rename_excl() where rename() is used
on Windows? The problems that 909b449 has somewhat "fixed" were
annoying for the users as it prevented WAL segment recycling, so we
need to be sure that this does not cause more harm.

+ /*
+ * CHECKSUM_TYPE_NONE defined in the winioctl.h when _WIN32_WINNT >= _WIN32_WINNT_WIN10
+ */
+#ifdef CHECKSUM_TYPE_NONE
+#undef CHECKSUM_TYPE_NONE
+#endif

Okay. Should this be renamed separately then to avoid conflicts?

- * get support for GetLocaleInfoEx() with locales. For everything else
+ * Studio 2015 the minimum requirement is Windows 10 (0x0A00) to get support for SetFileInformationByHandle.
+ * The minimum requirement is Windows Vista (0x0600) get support for GetLocaleInfoEx() with locales.
+ * For everything else
* the minimum version is Windows XP (0x0501).
*/
#if defined(_MSC_VER) && _MSC_VER >= 1900
-#define MIN_WINNT 0x0600
+#define MIN_WINNT 0x0A00
#else
#define MIN_WINNT 0x0501
#endif

This is a large bump for Studio >= 2015 I am afraid. That does not
seem acceptable, as it means losing support for GetLocaleInfoEx()
across older versions.

+#if defined(WIN32) && !defined(__CYGWIN__) && defined(_WIN32_WINNT_WIN10) && _WIN32_WINNT >= _WIN32_WINNT_WIN10
+
+#include <versionhelpers.h>
+
+/*
+ * win10_rename - uses SetFileInformationByHandle function with FILE_RENAME_FLAG_POSIX_SEMANTICS flag for atomic rename file
+ * working only on Windows 10 or later and  _WIN32_WINNT must be >= _WIN32_WINNT_WIN10
+ */
+static int win10_rename(wchar_t const* from, wchar_t const* to)

Having win10_rename(), a wrapper for pgrename_win10(), which is itself
an extra wrapper for pgrename(), is confusing. Could you reduce the
layers of functions here. At the end we just want an extra runtime
option for pgrename(). Note that pgrename_win10() could be static to
dirmod.c, and it seems to me that you just want a small function to do
the path conversion anyway. It would be better to avoid using
malloc() in those code paths as well, as the backend could finish by
calling that. We should be able to remove the malloc()s with local
variables large enough to hold those paths, no?

+		# manifest with ms_compatibility:supportedOS tags for using IsWindows10OrGreater() function
+		print $o "\n1 24 \"src/port/win10.manifest\"\n";
+
close($o);
close($i);
}
diff --git a/src/port/win10.manifest b/src/port/win10.manifest
new file mode 100644

It would be good to not require that. Those extra files make the
long-term maintenance harder.
--
Michael

Thanks!

In this version of the patch, calls to malloc have been removed.
Hopefully MAX_PATH is long enough for filenames.

How does that cope with durable_rename_excl() where rename() is used
on Windows?  The problems that 909b449 has somewhat "fixed" were
annoying for the users as it prevented WAL segment recycling, so we
need to be sure that this does not cause more harm.

I tested this patch to resolve the error message "could not rename
temporary statistics file "pg_stat_tmp/pgstat.tmp" to
"pg_stat_tmp/pgstat.stat": Permission denied".  (I have a patch option
to rename a temporary file for statistics only.)

+ /*
+ * CHECKSUM_TYPE_NONE defined in the winioctl.h when _WIN32_WINNT >= _WIN32_WINNT_WIN10
+ */
+#ifdef CHECKSUM_TYPE_NONE
+#undef CHECKSUM_TYPE_NONE
+#endif

Okay. Should this be renamed separately then to avoid conflicts?

Renaming CHECKSUM_TYPE_NONE in the  checksum_helper.h is the best way to go.

#if defined(_MSC_VER) && _MSC_VER >= 1900
-#define MIN_WINNT 0x0600
+#define MIN_WINNT 0x0A00
#else
#define MIN_WINNT 0x0501
#endif
This is a large bump for Studio >= 2015 I am afraid. That does not
seem acceptable, as it means losing support for GetLocaleInfoEx()
across older versions.

It seems that the MIN_WINNT value 0x0600 or 0x0A00 does not affect the
use of the GetLocaleInfoEx () function

+		# manifest with ms_compatibility:supportedOS tags for using IsWindows10OrGreater() function
+		print $o "\n1 24 \"src/port/win10.manifest\"\n";
+
close($o);
close($i);
}
diff --git a/src/port/win10.manifest b/src/port/win10.manifest
new file mode 100644

It would be good to not require that. Those extra files make the
long-term maintenance harder.

Function IsWindows10OrGreater() working properly if there is manifest
with <ms_compatibility:supportedOS
Id="{8e0f7a12-bfb3-4fe8-b9a5-48fd50a15a9a}" />

"Applications not manifested for Windows 10 return false, even if the
current operating system version is Windows 10."

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

06.07.2021 4:43, Michael Paquier пишет:

Hello.

I have changed the way I add the manifest to projects. I used the
AdditionalManifestFiles option for a VS project.

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

08.07.2021 1:32, Victor Spirin пишет:

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]/messages/by-id/20210905214437.y25j42yigwnbdvtg@alap3.anarazel.de (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]https://docs.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_rename_info 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]: /messages/by-id/20210905214437.y25j42yigwnbdvtg@alap3.anarazel.de
[2]: https://docs.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_rename_info

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 Thu, Jul 8, 2021 at 12:32 AM Victor Spirin <v.spirin@postgrespro.ru>
wrote:

#if defined(_MSC_VER) && _MSC_VER >= 1900
-#define MIN_WINNT 0x0600
+#define MIN_WINNT 0x0A00
#else
#define MIN_WINNT 0x0501
#endif
This is a large bump for Studio >= 2015 I am afraid. That does not
seem acceptable, as it means losing support for GetLocaleInfoEx()
across older versions.

It seems that the MIN_WINNT value 0x0600 or 0x0A00 does not affect the
use of the GetLocaleInfoEx () function

Anything below Windows Server 2012 (_WIN32_WINNT = 0x0602) is no longer

supported. A patch with a bump on MIN_WINNT might be due.

Regards,

Juan José Santamaría Flecha

On Tue, Sep 7, 2021 at 11:40 PM Victor Spirin <v.spirin@postgrespro.ru>
wrote:

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.

The FILE_RENAME_FLAGs are available starting from Windows 10 Release id

1607, NTDDI_WIN10_RS1. The check should be using something like
IsWindowsVersionOrGreater(10, 0, 1607). Or you could test this
using RtlGetVersion(), loading it from ntdll infrastructure coming from
stat() patch [1]/messages/by-id/CA+hUKG+oLqfBVJ_j3C03QgoshrX1KxYq0LB1vJV0OXPOcZZfhA@mail.gmail.com, which doesn't need a manifest.

[1]: /messages/by-id/CA+hUKG+oLqfBVJ_j3C03QgoshrX1KxYq0LB1vJV0OXPOcZZfhA@mail.gmail.com
/messages/by-id/CA+hUKG+oLqfBVJ_j3C03QgoshrX1KxYq0LB1vJV0OXPOcZZfhA@mail.gmail.com

Regards,

Juan José Santamaría Flecha

On Wed, Sep 8, 2021 at 9:40 AM Victor Spirin <v.spirin@postgrespro.ru> wrote:

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

Hmm, if we want to use the system header's struct definition, add some
space for a path at the end, and avoid heap allocation, perhaps we
could do something like:

struct {
FILE_RENAME_INFO fri;
WCHAR extra_space[MAX_PATH];
} x;

On Wed, Sep 8, 2021 at 10:13 PM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:

On Thu, Jul 8, 2021 at 12:32 AM Victor Spirin <v.spirin@postgrespro.ru> wrote:

#if defined(_MSC_VER) && _MSC_VER >= 1900
-#define MIN_WINNT 0x0600
+#define MIN_WINNT 0x0A00
#else
#define MIN_WINNT 0x0501
#endif
This is a large bump for Studio >= 2015 I am afraid. That does not
seem acceptable, as it means losing support for GetLocaleInfoEx()
across older versions.

It seems that the MIN_WINNT value 0x0600 or 0x0A00 does not affect the
use of the GetLocaleInfoEx () function

Anything below Windows Server 2012 (_WIN32_WINNT = 0x0602) is no longer supported. A patch with a bump on MIN_WINNT might be due.

+1

Thank you,

Fixed FILE_RENAME_INFO structure

I prepared 2 versions of the patch:

1) with manifest and IsWindows10OrGreater() function
2) without manifest and RtlGetVersion function from ntdll.dll

What's better?

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

23.09.2021 14:46, Thomas Munro пишет:

Thanks.

IsWindowsVersionOrGreater(10,0,1607) always returns false

Only IsWindowsVersionOrGreater(10,0,0) is a valid call. (There are no
service packs in Windows 10.)

I haven't found a way to determine the Windows 10 release ID.
The RtlGetVersion function returns dwBuildNumber = 19042 on my Windows.

I heard that Microsoft does not support older versions of Windows 10 and
requires a mandatory update.

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

23.09.2021 14:18, Juan José Santamaría Flecha пишет:

On Thu, Sep 30, 2021 at 11:00 PM Victor Spirin <v.spirin@postgrespro.ru>
wrote:

IsWindowsVersionOrGreater(10,0,1607) always returns false

Only IsWindowsVersionOrGreater(10,0,0) is a valid call. (There are no
service packs in Windows 10.)

I haven't found a way to determine the Windows 10 release ID.
The RtlGetVersion function returns dwBuildNumber = 19042 on my Windows.

I heard that Microsoft does not support older versions of Windows 10 and
requires a mandatory update.

You can translate the BuildNumber to the ReleaseId, for 1607 it will be 14393
[1]: https://en.wikipedia.org/wiki/Windows_10_version_history

We might find pretty much anything in the wild, the safer the check the
better.

[1]: https://en.wikipedia.org/wiki/Windows_10_version_history

Regards,

Juan José Santamaría Flecha

Thank you

Thank you
In this version of patch:

1. Made function isWindows1607OrGreater() without manifest

2. To open a directory using CreateFile, have to specify the
FILE_FLAG_BACKUP_SEMANTICS flag as part of dwFlagsAndAttributes. Checks
that file is a directory by the GetFileAttributes function.

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

01.10.2021 15:37, Juan José Santamaría Flecha пишет:

Hi

Added a small fix for calling the GetFileAttributes function

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

05.07.2021 16:53, Victor Spirin пишет:

Hi

Added the pgunlink_windows_posix_semantics function and modified the
pgunlink function

I used FILE_DISPOSITION_POSIX_SEMANTICS flag for unlink files on Windows
10 (1607) and above.

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

05.07.2021 16:53, Victor Spirin пишет:

Hi

The flags for calling the CreateFile function have been changed.

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

05.07.2021 16:53, Victor Spirin пишет:

On Tue, Jul 6, 2021 at 1:43 PM Michael Paquier <michael@paquier.xyz> wrote:

This is a large bump for Studio >= 2015 I am afraid. That does not
seem acceptable, as it means losing support for GetLocaleInfoEx()
across older versions.

Playing the devil's advocate here: why shouldn't we routinely drop
support for anything that'll be EOL'd when a given PostgreSQL major
release ships? The current policy seems somewhat extreme in the other
direction: our target OS baseline is a contemporary of RHEL 2 or 3 and
Linux 2.4.x, and our minimum compiler is a contemporary of GCC 3.x.

Something EOL'd over a year ago that has a bunch of features we've
really always wanted, like Unix domain sockets and Unix link
semantics, seems like a reasonable choice to me... hypothetical users
who refuse to upgrade or buy the extreme long life support options
just can't upgrade to PostgreSQL 15 until they upgrade their OS.
What's wrong with that? I don't think PostgreSQL 15 should support
FreeBSD 6, RHEL 4 or AT&T Unix Release 1 either.

On Fri, Dec 10, 2021 at 5:23 PM Thomas Munro <thomas.munro@gmail.com> wrote:

Playing the devil's advocate here: why shouldn't we routinely drop
support for anything that'll be EOL'd when a given PostgreSQL major
release ships? The current policy seems somewhat extreme in the other
direction: our target OS baseline is a contemporary of RHEL 2 or 3 and
Linux 2.4.x, and our minimum compiler is a contemporary of GCC 3.x.

Oops, I take those contemporaries back, I was looking at older
documentation... but still the general point stands, can't we be a
little more aggressive?

Thomas Munro <thomas.munro@gmail.com> writes:

Playing the devil's advocate here: why shouldn't we routinely drop
support for anything that'll be EOL'd when a given PostgreSQL major
release ships?

I don't like the word "routinely" here. Your next bit is a better
argument:

Something EOL'd over a year ago that has a bunch of features we've
really always wanted, like Unix domain sockets and Unix link
semantics, seems like a reasonable choice to me...

My general approach to platform compatibility is that when we
break compatibility with old versions of something, we should do so
because it will bring concrete benefits. If we can plausibly
drop support for Windows versions that don't have POSIX rename
semantics, I'm 100% for that. I'm not for dropping support for
some platform just because it's old.

regards, tom lane