Обсуждение: Clean up some old cruft related to Windows

Hi all,

As discussed here, there is in the tree a couple of things related to
past versions of Windows:
https://www.postgresql.org/message-id/20191218021954.GE1836@paquier.xyz

So I have been looking at that more closely, and found more:
- MIN_WINNT can be removed from win32.h thanks to d9dd406 which has
added a requirement on C99 with Windows 7 as minimum platform
supported.  (The issue mentioned previously.)
- pipe_read_line(), used when finding another binary for a given
installation via find_other_exec() has some special handling related
to Windows 2000 and older versions.
- When trying to load getaddrinfo(), we try to load it from
wship6.ddl, which was something needed in Windows 2000, but newer
Windows versions include it in ws2_32.dll.
- A portion of the docs still refer to Windows 98.

Thoughts?
--
Michael

At Thu, 19 Dec 2019 11:15:26 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> Hi all,
> 
> As discussed here, there is in the tree a couple of things related to
> past versions of Windows:
> https://www.postgresql.org/message-id/201912180219SUSv254.GE1836@paquier.xyz
> 
> So I have been looking at that more closely, and found more:
> - MIN_WINNT can be removed from win32.h thanks to d9dd406 which has
> added a requirement on C99 with Windows 7 as minimum platform
> supported.  (The issue mentioned previously.)
> - pipe_read_line(), used when finding another binary for a given
> installation via find_other_exec() has some special handling related
> to Windows 2000 and older versions.
> - When trying to load getaddrinfo(), we try to load it from
> wship6.ddl, which was something needed in Windows 2000, but newer
> Windows versions include it in ws2_32.dll.
> - A portion of the docs still refer to Windows 98.
> 
> Thoughts?

I think MIN_WINNT is definitely emovable.

popen already has the plantform-dependent implement so I think it can
be removed irrelevantly for the C99 discussion.

I found some similar places by grep'ing for windows version names the
whole source tree.

- The comment for trapsig is mentioning win98/Me/NT/2000/XP.

- We don't need the (only) caller site of IsWindows7OrGreater()?

- The comment for AddUserToTokenDacl() is mentioning "XP/2K3,
  Vista/2008".

- InitializeLDAPConnection dynamically loads WLDAP32.DLL for Windows
  2000. It could either be statically loaded or could be left as it
  is, but the comment seems to need a change in either case.

- The comment for IsoLocaleName mentioning Vista and Visual Studio
  2012.

- install-windows.sgml is mentioning "XP and later" around line 117.

- installation.sgml is mentioning NT/2000/XP as platforms that don't
  support adduser/su, command.

- "of Windows 2000 or later" is found at installation.sgml:2467

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Dec 19, 2019 at 08:09:45PM +0100, Juan José Santamaría Flecha wrote:
> This is probably not an issue for the supported MSVC and their SDK, but
> current MinGW defaults to Windows 2003 [1]. So I would suggest a logic like:
>
> #define WINNTVER(ver) ((ver) >> 16)
> #define NTDDI_VERSION 0x06000100
> #define _WIN32_WINNT WINNTVER(NTDDI_VERSION)
>
> [1]
> https://github.com/mirror/mingw-w64/blob/master/mingw-w64-headers/include/sdkddkver.h

You're right, thanks for the pointer.  This is this part of the
header:
#define NTDDI_VERSION NTDDI_WS03

Thinking more about that, the changes in win32.h are giving me cold
feet.
--
Michael

On Thu, Dec 19, 2019 at 01:46:33PM +0900, Kyotaro Horiguchi wrote:
> I found some similar places by grep'ing for windows version names the
> whole source tree.
>
> - The comment for trapsig is mentioning win98/Me/NT/2000/XP.

Let's refresh the comment here, as per the following:
https://docs.microsoft.com/en-us/previous-versions/xdkz3x12(v=vs.140)

> - We don't need the (only) caller site of IsWindows7OrGreater()?

The compiled code can still run with Windows Server 2008.

> - The comment for AddUserToTokenDacl() is mentioning "XP/2K3,
>   Vista/2008".

Keeping some context is still good here IMO.

> - InitializeLDAPConnection dynamically loads WLDAP32.DLL for Windows
>   2000. It could either be statically loaded or could be left as it
>   is, but the comment seems to need a change in either case.

Looks safer to me to keep it.

> - The comment for IsoLocaleName mentioning Vista and Visual Studio
>   2012.

It is good to keep some history in this context.

> - install-windows.sgml is mentioning "XP and later" around line 117.

But this still applies to XP, even if compilation is supported from
Windows 7.

> - installation.sgml is mentioning NT/2000/XP as platforms that don't
>   support adduser/su, command.

No objections to simplify that a bit.

Attached is a simplified version.  It is smaller than the previous
one, but that's already a good cut.  I have also done some testing
with the service manager to check after pipe_read_line(), and that
works.

Thoughts?
--
Michael

Hello.

I understand that this is not for back-patching.

At Tue, 18 Feb 2020 16:44:59 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Thu, Dec 19, 2019 at 01:46:33PM +0900, Kyotaro Horiguchi wrote:
> > I found some similar places by grep'ing for windows version names the
> > whole source tree.
> > 
> > - The comment for trapsig is mentioning win98/Me/NT/2000/XP.
> 
> Let's refresh the comment here, as per the following:
> https://docs.microsoft.com/en-us/previous-versions/xdkz3x12(v=vs.140)

  * The Windows runtime docs at
  * http://msdn.microsoft.com/library/en-us/vclib/html/_crt_signal.asp
...
- *     Win32 operating systems generate a new thread to specifically handle
- *     that interrupt. This can cause a single-thread application such as UNIX,
- *     to become multithreaded, resulting in unexpected behavior.
+ *  SIGINT is not supported for any Win32 application. When a CTRL+C interrupt
+ *  occurs, Win32 operating systems generate a new thread to specifically
+ *  handle that interrupt. This can cause a single-thread application, such as
+ *  one in UNIX, to become multithreaded and cause unexpected behavior.
  *
  * I have no idea how to handle this. (Strange they call UNIX an application!)
  * So this will need some testing on Windows.

The unmodified section just above is griping that "Strange they call
UNIX an application". The expression "application such as UNIX" seems
corresponding to the gripe.  I tried to find the soruce of the phrase
but the above URL (.._crt_signal.asp) sent me "We're sorry, the page
you requested cannot be found.":(

Thank you for checking the belows.

> > - We don't need the (only) caller site of IsWindows7OrGreater()?
> 
> The compiled code can still run with Windows Server 2008. 

Do we let the new PG version for already-unsupported platforms?  If I
don't missing anything Windows Server 2008 is already
End-Of-Extended-Support (2020/1/14) along with Windows 7.

> > - The comment for AddUserToTokenDacl() is mentioning "XP/2K3,
> >   Vista/2008".
> 
> Keeping some context is still good here IMO.

I'm fine with that.

> > - InitializeLDAPConnection dynamically loads WLDAP32.DLL for Windows
> >   2000. It could either be statically loaded or could be left as it
> >   is, but the comment seems to need a change in either case.
> 
> Looks safer to me to keep it.

If it is still possible that the file is missing on Windows 8/ Server
2012 or later, the comment should be updatd accordingly.

> > - The comment for IsoLocaleName mentioning Vista and Visual Studio
> >   2012.
> 
> It is good to keep some history in this context.

Agreed.

> > - install-windows.sgml is mentioning "XP and later" around line 117.
> 
> But this still applies to XP, even if compilation is supported from
> Windows 7.

Hmm. "/xp" can be the reason to preserve it.

By the way that pharse is considering Windows environment and perhaps
cmd.exe. So the folloinwg description:

https://www.postgresql.org/docs/current/install-windows-full.html
> In recent SDK versions you can change the targeted CPU architecture,
> build type, and target OS by using the setenv command, e.g. setenv
> /x86 /release /xp to target Windows XP or later with a 32-bit
> release build. See /? for other options to setenv. All commands
> should be run from the src\tools\msvc directory.

AFAICS we cannot use "setenv command" on cmd.exe, or no such command
found in the msvc directory.

> > - installation.sgml is mentioning NT/2000/XP as platforms that don't
> >   support adduser/su, command.
> 
> No objections to simplify that a bit.

Sorry for the ambiguity. I meant the following part

installation.sgml
>  <para>
>   <productname>PostgreSQL</productname> can be expected to work on these operating
>   systems: Linux (all recent distributions), Windows (Win2000 SP4 and later),
>   FreeBSD, OpenBSD, NetBSD, macOS, AIX, HP/UX, and Solaris.
>   Other Unix-like systems may also work but are not currently
>   being tested.  In most cases, all CPU architectures supported by

(The coming version of) PostgreSQL doesn't support Win2000 SP4.

> Attached is a simplified version.  It is smaller than the previous
> one, but that's already a good cut.  I have also done some testing
> with the service manager to check after pipe_read_line(), and that
> works.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Feb 18, 2020 at 12:05:42PM +0100, Juan José Santamaría Flecha wrote:
> Maybe this needs a specific thread, as it is not quite cruft but something
> that will require maintenance.

Makes sense.  I have discarded that part for now.
--
Michael

On Tue, Feb 18, 2020 at 12:26:06PM +0100, Juan José Santamaría Flecha wrote:
> I cannot point when SetEnv.bat was exactly dropped, probably Windows 7 SDK
> was the place where it was included [1], so that needs to be updated.
>
> Using VS2019 and VS2017 this would be done using VsDevCmd.bat [2], while
> VS2015 and VS2013 use VSVARS32.bat.

Would you like to write a patch for that part?
--
Michael

On Tue, Feb 18, 2020 at 05:54:43PM +0900, Kyotaro Horiguchi wrote:
> I understand that this is not for back-patching.

Cleanups don't go to back-branches.

> At Tue, 18 Feb 2020 16:44:59 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> The unmodified section just above is griping that "Strange they call
> UNIX an application". The expression "application such as UNIX" seems
> corresponding to the gripe.  I tried to find the soruce of the phrase
> but the above URL (.._crt_signal.asp) sent me "We're sorry, the page
> you requested cannot be found.":(

Yes, we should use that instead:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/signal

> Do we let the new PG version for already-unsupported platforms?  If I
> don't missing anything Windows Server 2008 is already
> End-Of-Extended-Support (2020/1/14) along with Windows 7.

Windows is known for keeping things backward compatible, so I don't
see any reason to not allow Postgres to run on those versions.
Outdated of course, still they could be used at runtime even if they
cannot compile the code.

> By the way that pharse is considering Windows environment and perhaps
> cmd.exe. So the folloinwg description:
>
> https://www.postgresql.org/docs/current/install-windows-full.html

Let's tackle that as a separate patch as this is MSVC-dependent.

>>  <para>
>>   <productname>PostgreSQL</productname> can be expected to work on these operating
>>   systems: Linux (all recent distributions), Windows (Win2000 SP4 and later),
>>   FreeBSD, OpenBSD, NetBSD, macOS, AIX, HP/UX, and Solaris.
>>   Other Unix-like systems may also work but are not currently
>>   being tested.  In most cases, all CPU architectures supported by
>
> (The coming version of) PostgreSQL doesn't support Win2000 SP4.

Right, per the change for src/common/exec.c.  I am wondering though if
we don't have more portability issues if we try to run Postgres on
something older than XP as there has been many changes in the last
couple of years, and we have no more buildfarm members that old.
Anyway, that's not worth the cost.  For now I have applied to the tree
the smaller version as that's still a good cut.
--
Michael

On Wed, Feb 19, 2020 at 07:02:55PM +0100, Juan José Santamaría Flecha wrote:
> Please find a patched for so. I have tried to make it more version
> neutral.

+  You can change certain build options, such as the targeted CPU
+  architecture, build type, and the selection of the SDK by using either
+  <command>VSVARS32.bat</command> or <command>VsDevCmd.bat</command> depending
+  on your <productname>Visual Studio</productname> release. All commands
+  should be run from the <filename>src\tools\msvc</filename> directory.

Both commands have different ways of doing things, and don't really
shine with their documentation, so it could save time to the reader to
add more explicit details of how to switch to the 32-bit mode, like
with "VsDevCmd -arch=x86".  And I am not actually sure which
environment variable to touch when using VSVARS32.bat or
VSVARSALL.bat with MSVC <= 2017.
--
Michael

On Thu, Feb 20, 2020 at 12:39:56PM +0100, Juan José Santamaría Flecha wrote:
> Actually, you can still use the vcvars% scripts to configure architecture,
> platform_type and winsdk_version with current VS [1].

We still support the build down to MSVC 2013, so I think that it is
good to mention the options available for 2013 and 2015 as well, as
noted here:

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-options/how-to-set-environment-variables-for-the-visual-studio-command-line
"Visual Studio 2015 and earlier versions used VSVARS32.bat, not
VsDevCmd.bat for the same purpose."

> Please find attached a new version that addresses these issues.
>
> [1]
> https://docs.microsoft.com/en-us/cpp/build/building-on-the-command-line?view=vs-2019

Thanks, applied after tweaking the text a bit.  I have applied that
down to 12 where we support MSVC from 2013.
--
Michael