Re: Atomic rename feature for Windows.

On Mon, Jul 05, 2021 at 04:53:06PM +0300, Victor Spirin wrote:
> This patch related to this post:
> https://www.postgresql.org/message-id/CAEepm%3D0FV-k%2B%3Dd9z08cW%3DZXoR1%3Dkw9wdpkP6WAuOrKJdz-8ujg%40mail.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