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

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 пишет:
> 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
renamefile
 
>> + * 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