Thread: Atomic rename feature for Windows.

Hi

I used the SetFileInformationByHandle function with the 
FILE_RENAME_FLAG_POSIX_SEMANTICS flag for the file rename function..

1) The _WIN32_WINNT variable needs to be increased to 0x0A00 (Windows 
10).  Fixed conflict with #undef CHECKSUM_TYPE_NONE

2) The SetFileInformationByHandle function works correctly only on 
Windows 10 and higher.

The app must have a manifest to check the Windows version using the 
IsWindows10OrGreater() function. I added a manifest to all postgres 
projects and disabled the GenerateManifest option on windows projects.

This patch related to this post: 
https://www.postgresql.org/message-id/CAEepm%3D0FV-k%2B%3Dd9z08cW%3DZXoR1%3Dkw9wdpkP6WAuOrKJdz-8ujg%40mail.gmail.com


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

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

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

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

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

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
>
>

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

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

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], which doesn't need a manifest.

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BoLqfBVJ_j3C03QgoshrX1KxYq0LB1vJV0OXPOcZZfhA%40mail.gmail.com#bfcc256e4eda369e369275f5b4e38185

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

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].

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

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
>
> I used the SetFileInformationByHandle function with the 
> FILE_RENAME_FLAG_POSIX_SEMANTICS flag for the file rename function..
>
> 1) The _WIN32_WINNT variable needs to be increased to 0x0A00 (Windows 
> 10).  Fixed conflict with #undef CHECKSUM_TYPE_NONE
>
> 2) The SetFileInformationByHandle function works correctly only on 
> Windows 10 and higher.
>
> The app must have a manifest to check the Windows version using the 
> IsWindows10OrGreater() function. I added a manifest to all postgres 
> projects and disabled the GenerateManifest option on windows projects.
>
> This patch related to this post: 
> https://www.postgresql.org/message-id/CAEepm%3D0FV-k%2B%3Dd9z08cW%3DZXoR1%3Dkw9wdpkP6WAuOrKJdz-8ujg%40mail.gmail.com
>
>

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
>
> I used the SetFileInformationByHandle function with the 
> FILE_RENAME_FLAG_POSIX_SEMANTICS flag for the file rename function..
>
> 1) The _WIN32_WINNT variable needs to be increased to 0x0A00 (Windows 
> 10).  Fixed conflict with #undef CHECKSUM_TYPE_NONE
>
> 2) The SetFileInformationByHandle function works correctly only on 
> Windows 10 and higher.
>
> The app must have a manifest to check the Windows version using the 
> IsWindows10OrGreater() function. I added a manifest to all postgres 
> projects and disabled the GenerateManifest option on windows projects.
>
> This patch related to this post: 
> https://www.postgresql.org/message-id/CAEepm%3D0FV-k%2B%3Dd9z08cW%3DZXoR1%3Dkw9wdpkP6WAuOrKJdz-8ujg%40mail.gmail.com
>
>

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 пишет:
> Hi
>
> I used the SetFileInformationByHandle function with the 
> FILE_RENAME_FLAG_POSIX_SEMANTICS flag for the file rename function..
>
> 1) The _WIN32_WINNT variable needs to be increased to 0x0A00 (Windows 
> 10).  Fixed conflict with #undef CHECKSUM_TYPE_NONE
>
> 2) The SetFileInformationByHandle function works correctly only on 
> Windows 10 and higher.
>
> The app must have a manifest to check the Windows version using the 
> IsWindows10OrGreater() function. I added a manifest to all postgres 
> projects and disabled the GenerateManifest option on windows projects.
>
> This patch related to this post: 
> https://www.postgresql.org/message-id/CAEepm%3D0FV-k%2B%3Dd9z08cW%3DZXoR1%3Dkw9wdpkP6WAuOrKJdz-8ujg%40mail.gmail.com
>
>

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

On Thu, Dec 09, 2021 at 11:33:17PM -0500, Tom Lane wrote:
> 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.

I'd agree with that.  Now, I would also say if we need something that
depends on a newer version of _WIN32_WINNT that proves to be trickier
or even not possible for older versions, there could be an argument
for dropping older versions, even in the back-branches, if the problem
to-be-fixed is bad enough.  In short history, we've never had to go
down to that AFAIK, though.
--
Michael

Hi

Updated patch: we use the posix semantic features in Windows build 17763 
and up.
We found an issue with this feature on Windows Server 2016 without 
updates (Windows 1607 Build 14393)

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

05.07.2021 16:53, Victor Spirin пишет:
> Hi
>
> I used the SetFileInformationByHandle function with the 
> FILE_RENAME_FLAG_POSIX_SEMANTICS flag for the file rename function..
>
> 1) The _WIN32_WINNT variable needs to be increased to 0x0A00 (Windows 
> 10).  Fixed conflict with #undef CHECKSUM_TYPE_NONE
>
> 2) The SetFileInformationByHandle function works correctly only on 
> Windows 10 and higher.
>
> The app must have a manifest to check the Windows version using the 
> IsWindows10OrGreater() function. I added a manifest to all postgres 
> projects and disabled the GenerateManifest option on windows projects.
>
> This patch related to this post: 
> https://www.postgresql.org/message-id/CAEepm%3D0FV-k%2B%3Dd9z08cW%3DZXoR1%3Dkw9wdpkP6WAuOrKJdz-8ujg%40mail.gmail.com
>
>

On Thu, 9 Dec 2021 at 23:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I'm not for dropping support for some platform just because it's old.

I guess I'll have to spin up the Vax again :)

On Fri, Apr 8, 2022 at 10:12 AM Greg Stark <stark@mit.edu> wrote:
> On Thu, 9 Dec 2021 at 23:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I'm not for dropping support for some platform just because it's old.
>
> I guess I'll have to spin up the Vax again :)

This is a pretty good summary of what's wrong with our current
deprecation policy. Like Tom, I kind of hate removing support for old
systems. But I've also come to realize that we often end up supporting
systems because there's one PostgreSQL developer who has access and
sets up a buildfarm member ... which tends to mean that we support all
the stuff that lots of people are using, plus a pretty random subset
of older systems that do funny things and most people can't access to
debug any problems that may occur. And that's kind of annoying.

(I don't have a specific proposal for what to do about it.)

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, 8 Apr 2022 at 11:30, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, Apr 8, 2022 at 10:12 AM Greg Stark <stark@mit.edu> wrote:
> > On Thu, 9 Dec 2021 at 23:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > I'm not for dropping support for some platform just because it's old.
> >
> > I guess I'll have to spin up the Vax again :)
>
> This is a pretty good summary of what's wrong with our current
> deprecation policy.

I didn't intend it that way but, ok.

> Like Tom, I kind of hate removing support for old
> systems. But I've also come to realize that we often end up supporting
> systems because there's one PostgreSQL developer who has access and
> sets up a buildfarm member ... which tends to mean that we support all
> the stuff that lots of people are using, plus a pretty random subset
> of older systems that do funny things and most people can't access to
> debug any problems that may occur. And that's kind of annoying.

Generally I think supporting older systems that do funny things is
helpful in avoiding problems that either 1) Can happen on newer
systems but rarely 2) Can happen on other systems that people are
using but we don't know about and aren't testing and 3) Can happen on
future systems or future compilers and we might not even find out
about.

But that's useful for some things and not for others. Like, it's
useful to be sure we don't have odd dependencies on timing quirks of
the specific machines that are currently common, or depend on gcc/llvm
compiler behaviour that isn't guaranteed. But less so for supporting
some quirky filesystem behaviour on Windows 8 that newer Windows
doesn't have and Unix guarantees not to have. (Or supporting non-IEEE
Vax FP now that we've decided we just don't any more).

-- 
greg

On Fri, Apr 8, 2022 at 11:45 AM Greg Stark <stark@mit.edu> wrote:
> But that's useful for some things and not for others. Like, it's
> useful to be sure we don't have odd dependencies on timing quirks of
> the specific machines that are currently common, or depend on gcc/llvm
> compiler behaviour that isn't guaranteed. But less so for supporting
> some quirky filesystem behaviour on Windows 8 that newer Windows
> doesn't have and Unix guarantees not to have. (Or supporting non-IEEE
> Vax FP now that we've decided we just don't any more).

Yeah, exactly.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, Apr 08, 2022 at 11:44:31AM -0400, Greg Stark wrote:
> Generally I think supporting older systems that do funny things is
> helpful in avoiding problems that either 1) Can happen on newer
> systems but rarely 2) Can happen on other systems that people are
> using but we don't know about and aren't testing and 3) Can happen on
> future systems or future compilers and we might not even find out
> about.

Agreed.  I think that things can be usually helpful.  Now, I am not
really convinced that there is a strong need in running a VAX if you
are worrying about timing issues so this is a matter of balance.  You
could get down to the same level of coverage with something as cheap
as a Raspberry PI or such.  There are also configure switches that
emulate rather non-common behaviors, like --disable-spinlocks or
--disable-atomics that I'd rather never see gone.
--
Michael

On Wed, Apr 13, 2022 at 3:04 AM Michael Paquier <michael@paquier.xyz> wrote:
> Agreed.  I think that things can be usually helpful.  Now, I am not
> really convinced that there is a strong need in running a VAX if you
> are worrying about timing issues so this is a matter of balance.  You
> could get down to the same level of coverage with something as cheap
> as a Raspberry PI or such.  There are also configure switches that
> emulate rather non-common behaviors, like --disable-spinlocks or
> --disable-atomics that I'd rather never see gone.

I don't really agree with you about any of this.

A Raspberry Pi running Linux is very similar to any other Linux
machine, except with less resources. A machine running a different
operating system is a totally different thing. I agree that there is
value in supporting other operating systems if those are things people
might actually use, because it's good for PostgreSQL to be portable,
but it isn't really useful for us to be portable to systems that
nobody runs any more. I am just old enough to have a bit of nostalgia
about the idea of surrendering support for VAX or a PDP-11 or an IRIX
workstation, but in practice efforts to port to those platforms are
efforts for the dedicated hobbyist rather than anything that will make
PostgreSQL a better piece of software.

Similarly for spinlocks and atomics. There have been in the past
systems that did not have these things, but it seems very unlikely
that there will be new systems in the future that don't have these
things. And continuing to support those configurations actually adds a
lot of complexity. The code is weirdly structured so that you can
emulate atomics with spinlocks, but spinlocks on most systems are
actually built using atomics, except when we emulate spinlocks with
semaphores. It's really kind of a mess, and we could clean things up
and make them more straightforward if we were willing to decide that
atomics and spinlocks are basic requirements for PostgreSQL to run.

Now I don't know if we should decide that today or at some point in
the future, but single-processor architectures are pretty dead.
Embedded devices like phones are shipping with multiple cores. Even a
single processor system is probably based on an underlying
architecture that is multiprocessor capable. Where, outside of a
museum, would you find a system that required --disable-spinlocks or
--disable-atomics?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Apr 13, 2022 at 3:04 AM Michael Paquier <michael@paquier.xyz> wrote:
>> Agreed.  I think that things can be usually helpful.  Now, I am not
>> really convinced that there is a strong need in running a VAX if you
>> are worrying about timing issues so this is a matter of balance.  You
>> could get down to the same level of coverage with something as cheap
>> as a Raspberry PI or such.  There are also configure switches that
>> emulate rather non-common behaviors, like --disable-spinlocks or
>> --disable-atomics that I'd rather never see gone.

> I don't really agree with you about any of this.

Meh.  I agree that it seems unlikely that anyone will come out with a
new processor design that lacks the ability to do spinlocks or atomics.
It's substantially more likely though that someone would want those
configure switches temporarily while in the process of porting
Postgres to a new processor, so that they don't have to make
absolutely everything work before they can test anything.

Independently of that, I think that our interest in weird old
processors is mostly about checking our assumptions about exactly
what processor-dependent facilities look like.  For example,
although I agree that spinlocks should be possible on everything
we care about supporting, I missed the stone tablet on which it is
graven that thou shalt use zero for the unlocked state of a spinlock.
The main reason I keep my old HPPA dinosaur alive is because it is
(I think) our only remaining architecture in which that isn't true,
and I think we need to keep ourselves honest about that sort of
detail.  Next decade's hot new processor design might do things
differently enough that it matters that we use SpinLockInit()
not memset-to-zero.  This is not academic either, as we've had
exactly such bugs in the past.

The situation for OSes is a bit different, because IMV we generally
prefer to restrict ourselves to POSIX-compliant system calls,
and to the extent we can do that all OSes look alike.  The reason
that Windows is such a grade-A pain in the rear is exactly that
their POSIX compliance sucks, and yet we committed to supporting
them anyway.  If some new OS that is not POSIX-compliant comes
down the pike, I think we're far more likely to decline to support
it than otherwise.

But to tie this back to the point of the thread --- anytime we
can reasonably start to rely on POSIX behavior in newer versions
of Windows, I'm for that.

            regards, tom lane

On Wed, Apr 13, 2022 at 10:19 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Meh.  I agree that it seems unlikely that anyone will come out with a
> new processor design that lacks the ability to do spinlocks or atomics.
> It's substantially more likely though that someone would want those
> configure switches temporarily while in the process of porting
> Postgres to a new processor, so that they don't have to make
> absolutely everything work before they can test anything.

It's possible. I don't think it's super-likely. If someone is
introducing a new architecture, they're probably going to make getting
the Linux kernel and gcc working on it a pretty high priority, and
they'll probably make the gcc intrinsics work, too. But you never
know. Humans are unpredictable like that.

> Independently of that, I think that our interest in weird old
> processors is mostly about checking our assumptions about exactly
> what processor-dependent facilities look like.  For example,
> although I agree that spinlocks should be possible on everything
> we care about supporting, I missed the stone tablet on which it is
> graven that thou shalt use zero for the unlocked state of a spinlock.
> The main reason I keep my old HPPA dinosaur alive is because it is
> (I think) our only remaining architecture in which that isn't true,
> and I think we need to keep ourselves honest about that sort of
> detail.  Next decade's hot new processor design might do things
> differently enough that it matters that we use SpinLockInit()
> not memset-to-zero.  This is not academic either, as we've had
> exactly such bugs in the past.

Here again, I think it's definitely possible that that could happen,
but I don't think it's super-likely. Nobody's really implementing
spinlocks as a primitive any more; they implement atomics, and you can
decide for yourself how to build spinlocks on top of that and what
values you want to use. And if you did decide to provide spinlocks but
not atomics for some reason, you'd probably use 0 and 1 rather than 17
and 42 just because otherwise a lot of software wouldn't work on your
brand new hardware, which as a hardware manufacturer is a thing you
really don't want. We can be as rigorous as we like about this sort of
thing, but I bet that in 2022 there is a huge amount of code out that
assumes memset(...., 0, ...) is good enough. And, like, nobody's going
to be that excited about building a machine where PostgreSQL works
because we've carefully avoided this assumption, but 5000 other
software packages that haven't been as careful all break.

> The situation for OSes is a bit different, because IMV we generally
> prefer to restrict ourselves to POSIX-compliant system calls,
> and to the extent we can do that all OSes look alike.  The reason
> that Windows is such a grade-A pain in the rear is exactly that
> their POSIX compliance sucks, and yet we committed to supporting
> them anyway.  If some new OS that is not POSIX-compliant comes
> down the pike, I think we're far more likely to decline to support
> it than otherwise.

Yeah.

> But to tie this back to the point of the thread --- anytime we
> can reasonably start to rely on POSIX behavior in newer versions
> of Windows, I'm for that.

Sure, makes sense.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2022-04-13 10:19:44 -0400, Tom Lane wrote:
> Meh.  I agree that it seems unlikely that anyone will come out with a
> new processor design that lacks the ability to do spinlocks or atomics.
> It's substantially more likely though that someone would want those
> configure switches temporarily while in the process of porting
> Postgres to a new processor, so that they don't have to make
> absolutely everything work before they can test anything.

I still think we ought to provide a compiler intrinsics spinlock
implementation for that...


> Independently of that, I think that our interest in weird old
> processors is mostly about checking our assumptions about exactly
> what processor-dependent facilities look like.  For example,
> although I agree that spinlocks should be possible on everything
> we care about supporting, I missed the stone tablet on which it is
> graven that thou shalt use zero for the unlocked state of a spinlock.
> The main reason I keep my old HPPA dinosaur alive is because it is
> (I think) our only remaining architecture in which that isn't true,
> and I think we need to keep ourselves honest about that sort of
> detail.

The other thing it currently has is the weird wide spinlock state where
we don't know which byte is going to be modified ... I don't think
that's likely to be needed again though.


> Next decade's hot new processor design might do things
> differently enough that it matters that we use SpinLockInit()
> not memset-to-zero.  This is not academic either, as we've had
> exactly such bugs in the past.

FWIW, I'l like to make spinlocks and atomics assert out if they've not
been initialized (which'd include preventing uninitialized use of
lwlocks). It's easy to accidentally zero out the state or start out
uninitialized. Right now nothing will complain on platforms created
after 1700 or using --disable-spinlocks --disable-atomics. That should
be caught well before running on the buildfarm...

Then the zero-state assumption wouldn't require continuing to support
HPPA.


> The situation for OSes is a bit different, because IMV we generally
> prefer to restrict ourselves to POSIX-compliant system calls,
> and to the extent we can do that all OSes look alike.  The reason
> that Windows is such a grade-A pain in the rear is exactly that
> their POSIX compliance sucks, and yet we committed to supporting
> them anyway.  If some new OS that is not POSIX-compliant comes
> down the pike, I think we're far more likely to decline to support
> it than otherwise.

Our windows support is not in a great state. Part of that is that we
just plaster random hacks over issues. Which often are only needed on
windows version that nobody has access to. As you say that's different
from most of the hackiness to support some random old unix platform,
which most of the time much more localized (with the exception of not
relying on threads in some places due to old platforms).


> But to tie this back to the point of the thread --- anytime we
> can reasonably start to rely on POSIX behavior in newer versions
> of Windows, I'm for that.

Yea. Same imo is true for msvc specific compiler oddities. If we can
simplify things by requiring a halfway modern msvc version, we shouldn't
hesitate.

I think it might be worth noting somewhere developer oriented that we're
ok with dropping support in HEAD for windows versions that aren't
"fully" supported anymore. Even if one can procure extended support for
a gazillion or two, they're not going to do that to run PG 19.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2022-04-13 10:19:44 -0400, Tom Lane wrote:
>> Next decade's hot new processor design might do things
>> differently enough that it matters that we use SpinLockInit()
>> not memset-to-zero.  This is not academic either, as we've had
>> exactly such bugs in the past.

> FWIW, I'l like to make spinlocks and atomics assert out if they've not
> been initialized (which'd include preventing uninitialized use of
> lwlocks). It's easy to accidentally zero out the state or start out
> uninitialized. Right now nothing will complain on platforms created
> after 1700 or using --disable-spinlocks --disable-atomics. That should
> be caught well before running on the buildfarm...

Yeah, even just doing that in --disable-spinlocks builds would be
enough for the purpose, and be much more accessible to Joe Developer.

> Then the zero-state assumption wouldn't require continuing to support
> HPPA.

I wouldn't mind retiring that machine once v11 is EOL.  (It's also one
of very few animals testing pre-C99 compilers, so not before then.)

            regards, tom lane

On Wed, Apr 13, 2022 at 11:03 AM Andres Freund <andres@anarazel.de> wrote:
> > Next decade's hot new processor design might do things
> > differently enough that it matters that we use SpinLockInit()
> > not memset-to-zero.  This is not academic either, as we've had
> > exactly such bugs in the past.
>
> FWIW, I'l like to make spinlocks and atomics assert out if they've not
> been initialized (which'd include preventing uninitialized use of
> lwlocks). It's easy to accidentally zero out the state or start out
> uninitialized. Right now nothing will complain on platforms created
> after 1700 or using --disable-spinlocks --disable-atomics. That should
> be caught well before running on the buildfarm...

I don't understand this bit about platforms created after 1700. Before
1700, they didn't even have computers.

Am I being really dense here?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On April 13, 2022 8:30:33 AM PDT, Robert Haas <robertmhaas@gmail.com> wrote:
>On Wed, Apr 13, 2022 at 11:03 AM Andres Freund <andres@anarazel.de> wrote:
>> > Next decade's hot new processor design might do things
>> > differently enough that it matters that we use SpinLockInit()
>> > not memset-to-zero.  This is not academic either, as we've had
>> > exactly such bugs in the past.
>>
>> FWIW, I'l like to make spinlocks and atomics assert out if they've not
>> been initialized (which'd include preventing uninitialized use of
>> lwlocks). It's easy to accidentally zero out the state or start out
>> uninitialized. Right now nothing will complain on platforms created
>> after 1700 or using --disable-spinlocks --disable-atomics. That should
>> be caught well before running on the buildfarm...
>
>I don't understand this bit about platforms created after 1700. Before
>1700, they didn't even have computers.
>
>Am I being really dense here?

It was a sarcastic reference to the age of pa-risc (the only platform detecting zeroed out spinlocks).

Andres

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

On Wed, Apr 6, 2022 at 10:40 AM Victor Spirin <v.spirin@postgrespro.ru> wrote:
> Updated patch: we use the posix semantic features in Windows build 17763
> and up.
> We found an issue with this feature on Windows Server 2016 without
> updates (Windows 1607 Build 14393)

Hi Victor,

I rebased and simplified this, and added a lot of tests to be able to
understand what it does.  I think all systems that didn't have this
are now EOL and we don't need to support them in PG16, but perhaps our
_WIN32_WINNT is not quite high enough (this requires Win10 RS1, which
itself was EOL'd in 2019); the main question I have now is what
happens when you run this on non-NTFS filesystems, and whether we want
to *require* this to work because the non-POSIX support will probably
finish up untested.  I posted all that over on a new thread where I am
tidying up lots of related stuff, and I didn't want to repost the
proposed testing framework in multiple threads...

https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BajSQ_8eu2AogTncOnZ5me2D-Cn66iN_-wZnRjLN%2Bicg%40mail.gmail.com

2022年10月19日(水) 6:06 Thomas Munro <thomas.munro@gmail.com>:
>
> On Wed, Apr 6, 2022 at 10:40 AM Victor Spirin <v.spirin@postgrespro.ru> wrote:
> > Updated patch: we use the posix semantic features in Windows build 17763
> > and up.
> > We found an issue with this feature on Windows Server 2016 without
> > updates (Windows 1607 Build 14393)
>
> Hi Victor,
>
> I rebased and simplified this, and added a lot of tests to be able to
> understand what it does.  I think all systems that didn't have this
> are now EOL and we don't need to support them in PG16, but perhaps our
> _WIN32_WINNT is not quite high enough (this requires Win10 RS1, which
> itself was EOL'd in 2019); the main question I have now is what
> happens when you run this on non-NTFS filesystems, and whether we want
> to *require* this to work because the non-POSIX support will probably
> finish up untested.  I posted all that over on a new thread where I am
> tidying up lots of related stuff, and I didn't want to repost the
> proposed testing framework in multiple threads...
>
> https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BajSQ_8eu2AogTncOnZ5me2D-Cn66iN_-wZnRjLN%2Bicg%40mail.gmail.com

Hi

As Thomas has incorporated this patch into another CommitFest entry
[1], we'll close
the entry for this thread [2]. If Victor or anyone else would like to
follow up, it would
probably be best to do that on the thread linked above.

[1] https://commitfest.postgresql.org/40/3951/
[2] https://commitfest.postgresql.org/40/3347/


Regards

Ian Barwick