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

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