Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Sat, Apr 30, 2016 at 12:22 AM, Petr Jelinek  wrote:
> After bit of fighting with the system the "caecilian" reported first
> successful build to the buildfarm.

Thanks! The fight was there as well.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Petr Jelinek]

On 29/04/16 16:02, Michael Paquier wrote:

On Fri, Apr 29, 2016 at 9:13 PM, Andrew Dunstan  wrote:

Yeah, I noticed the ugliness, should have fixed it. Applied your fix and
committed.


Thanks for the commit!



+1

After bit of fighting with the system the "caecilian" reported first 
successful build to the buildfarm.


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Fri, Apr 29, 2016 at 9:13 PM, Andrew Dunstan  wrote:
> Yeah, I noticed the ugliness, should have fixed it. Applied your fix and
> committed.

Thanks for the commit!

Nitpick comment:
+ * Also  for VS2015, add a define that stops compiler complaints about
Two spaces instead of one between "Also" and "for" :)
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/29/2016 12:39 AM, Tom Lane wrote:

Andrew Dunstan  writes:

latest patch attached. I have also cleaned up the docs some, and removed
references to the now redundant msysGit.

Please don't do stuff like this:

@@ -232,6 +265,10 @@ win32_langinfo(const char *ctype)
if (r != NULL)
sprintf(r, "CP%s", codepage);
}
+
+#if (_MSC_VER >= 1900)
+   }
+#endif
  #endif
  
  	return r;


I'm not very sure what pgindent will do with conditionally-included
indentation, but it's unlikely to be pleasing.

In this particular case, you could probably fix it by making the
other end of that be

+   if (GetLocaleInfoEx(wctype,
+   LOCALE_IDEFAULTANSICODEPAGE | LOCALE_RETURN_NUMBER,
+   (LPWSTR) &cp, sizeof(cp) / sizeof(WCHAR)) > 0)
+   {
+   r = malloc(16); /* excess */
+   if (r != NULL)
+   sprintf(r, "CP%u", cp);
+   }
+   else
+#endif
+   {
+

and omitting the #if/#endif around the trailing }.






Yeah, I noticed the ugliness, should have fixed it. Applied your fix and 
committed.


cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Tom Lane]

Andrew Dunstan  writes:
> latest patch attached. I have also cleaned up the docs some, and removed 
> references to the now redundant msysGit.

Please don't do stuff like this:

@@ -232,6 +265,10 @@ win32_langinfo(const char *ctype)
if (r != NULL)
sprintf(r, "CP%s", codepage);
}
+
+#if (_MSC_VER >= 1900)
+   }
+#endif
 #endif
 
return r;

I'm not very sure what pgindent will do with conditionally-included
indentation, but it's unlikely to be pleasing.

In this particular case, you could probably fix it by making the
other end of that be 

+   if (GetLocaleInfoEx(wctype,
+   LOCALE_IDEFAULTANSICODEPAGE | LOCALE_RETURN_NUMBER,
+   (LPWSTR) &cp, sizeof(cp) / sizeof(WCHAR)) > 0)
+   {
+   r = malloc(16); /* excess */
+   if (r != NULL)
+   sprintf(r, "CP%u", cp);
+   }
+   else
+#endif
+   {
+

and omitting the #if/#endif around the trailing }.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/25/2016 03:11 AM, Michael Paquier wrote:

On Mon, Apr 25, 2016 at 4:29 AM, Christian Ullrich  wrote:

Andrew wrote:

OK, here's my final version of the patch, which I will apply in 24 hours

or so unless there is an objection.

Thanks Andrew for the updated patch!


This one doesn't matter, but just for perfection's sake:

+#if (_MSC_VER >= 1900)
+   uint32  cp;
+   WCHAR   wctype[80];
+
+   memset(wctype, 0, 80 * sizeof(WCHAR));
+   MultiByteToWideChar(CP_ACP, 0, ctype, -1, wctype, 80);

The maximum length is documented as 85 characters, also:

:
'Note   Your application must use the constant [LOCALE_NAME_MAX_LENGTH] for
the maximum locale name length, instead of hard-coding the value "85".'

Just an addition on top of the comments of Christian..

+#pragma warning(push)
+#pragma warning(disable : 4091)
  #include 
+#pragma warning(pop)
It seems to me that we had better protect those pragmas with _MSC_VER

= 1900. The crash dump facility could be used by MinGW as well, no?




I think we're being overcautious here.  But to keep people happy I have 
protected it with "#ifdef _MSC_VER".


latest patch attached. I have also cleaned up the docs some, and removed 
references to the now redundant msysGit.


cheers

andrew


diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml
index 74265fc..4e92cc9 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -19,10 +19,10 @@
  
   There are several different ways of building PostgreSQL on
   Windows. The simplest way to build with
-  Microsoft tools is to install Visual Studio Express 2013
+  Microsoft tools is to install Visual Studio Express 2015
   for Windows Desktop and use the included
   compiler. It is also possible to build with the full
-  Microsoft Visual C++ 2005 to 2013.
+  Microsoft Visual C++ 2005 to 2015.
   In some cases that requires the installation of the
   Windows SDK in addition to the compiler.
  
@@ -77,19 +77,26 @@
   Visual Studio Express or some versions of the
   Microsoft Windows SDK. If you do not already have a
   Visual Studio environment set up, the easiest
-  ways are to use the compilers from Visual Studio Express 2013
+  ways are to use the compilers from Visual Studio Express 2015
   for Windows Desktop or those in the Windows SDK
   7.1, which are both free downloads from Microsoft.
  
 
  
-  PostgreSQL is known to support compilation using the compilers shipped with
+  Both 32-bit and 64-bit builds are possible with the Microsoft Compiler suite.
+  32-bit PostgreSQL buils are possible with
   Visual Studio 2005 to
-  Visual Studio 2013 (including Express editions),
+  Visual Studio 2015 (including Express editions),
   as well as standalone Windows SDK releases 6.0 to 7.1.
-  64-bit PostgreSQL builds are only supported with
+  64-bit PostgreSQL builds are supported with
   Microsoft Windows SDK version 6.0a to 7.1 or
-  Visual Studio 2008 and above.
+  Visual Studio 2008 and above. Compilation
+  is supported down to Windows XP and
+  Windows Server 2003 when building with
+  Visual Studio 2005 to
+  Visual Studio 2013. Building with
+  Visual Studio 2015 is supported down to
+  Windows Vista and Windows Server 2008.
  
 
  
@@ -210,9 +217,7 @@ $ENV{PATH}=$ENV{PATH} . ';c:\some\where\bison\bin';
   Both Bison and Flex
   are included in the msys tool suite, available
   from http://www.mingw.org/wiki/MSYS";> as part of the
-  MinGW compiler suite. You can also get
-  msys as part of
-  msysGit from http://git-scm.com/";>.
+  MinGW compiler suite.
  
 
  
@@ -221,9 +226,7 @@ $ENV{PATH}=$ENV{PATH} . ';c:\some\where\bison\bin';
   PATH environment variable in buildenv.pl unless
   they are already in PATH. In the case of MinGW, the directory is the
   \msys\1.0\bin subdirectory of your MinGW
-  installation directory. For msysGit, it's the bin
-  directory in your Git install directory. Do not add the MinGW compiler
-  tools themselves to PATH.
+  installation directory.
  
 
  
diff --git a/src/backend/port/win32/crashdump.c b/src/backend/port/win32/crashdump.c
index ec7c325..92e23cb 100644
--- a/src/backend/port/win32/crashdump.c
+++ b/src/backend/port/win32/crashdump.c
@@ -41,7 +41,21 @@
 #define WIN32_LEAN_AND_MEAN
 #include 
 #include 
+/*
+ * Some versions of the MS SDK contain "typedef enum { ... } ;" which the MS
+ * compiler quite sanely complains about. Well done, Microsoft.
+ * This pragma disables the warning just while we include the header.
+ * The pragma is known to work with all (as at the time of writing) supported
+ * versions of MSVC.
+ */
+#ifdef _MSC_VER
+#pragma warning(push)
+#pragma warning(disable : 4091)
+#endif
 #include 
+#ifdef _MSC_VER
+#pragma warning(pop)
+#endif
 
 /*
  * Much of the following code is based on CodeProject and MSDN examples,
dif

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Mon, Apr 25, 2016 at 4:29 AM, Christian Ullrich  wrote:
> Andrew wrote:
>> OK, here's my final version of the patch, which I will apply in 24 hours
> or so unless there is an objection.

Thanks Andrew for the updated patch!

> This one doesn't matter, but just for perfection's sake:
>
> +#if (_MSC_VER >= 1900)
> +   uint32  cp;
> +   WCHAR   wctype[80];
> +
> +   memset(wctype, 0, 80 * sizeof(WCHAR));
> +   MultiByteToWideChar(CP_ACP, 0, ctype, -1, wctype, 80);
>
> The maximum length is documented as 85 characters, also:
>
> :
> 'Note   Your application must use the constant [LOCALE_NAME_MAX_LENGTH] for
> the maximum locale name length, instead of hard-coding the value "85".'

Just an addition on top of the comments of Christian..

+#pragma warning(push)
+#pragma warning(disable : 4091)
 #include 
+#pragma warning(pop)
It seems to me that we had better protect those pragmas with _MSC_VER
>= 1900. The crash dump facility could be used by MinGW as well, no?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Andrew Dunstan wrote:


OK, here's my final version of the patch, which I will apply in 24 hours
or so unless there is an objection.


+  Visual Studio 2008 and above. Compilation
+  is supported down to Windows XP and
+  Windows Server 2003 when building with
+  Visual Studio 2005 to
+  Visual Studio 2013. Building with
+  Visual Studio 2015 is supported down to
+  Windows Vista and Windows Server
   2008.

This paragraph contradicts itself; it first says 2008 is the minimum 
version, right after that it supports 2005, too. The last version of
Visual Studio that will install on XP, 2003, Vista, and 2008 is VS 2010 
anyway, anything released after that requires at least 7/2008R2.


I would actually just say "is supported with Visual Studio 2005 [or 
possibly 2008] and higher" instead of listing versions. I don't think we 
need to recapitulate the system requirements of individual VS releases 
and anyone trying to install 2012+ on Vista will quickly find out it 
doesn't work. It would be different if we actually excluded particular 
VS versions or VS/OS combinations that are supported by VS itself, but 
we don't.



+   /*
+* get a pointer sized version of bgchild to avoid
warnings about
+* casting to a different size on WIN64.
+*/
+   intptr_tbgchild_handle = bgchild;

If you're going to copy it anyway, why not just use a HANDLE rather than 
cast it again later? It's only used in two places, cast to HANDLE in 
both, and the special case of -1 does not matter in either.



+ * Leave a higher value in place. When building with at least Visual
+ * Studio 2015 the minimum requirement is Windows Vista (0x0600) to
+ * get support for GetLocaleInfoEx() with locales. For everything else
+ * the minumum version os Windows XP (0x0501).

s/os/is/ in the last line.


This one doesn't matter, but just for perfection's sake:

+#if (_MSC_VER >= 1900)
+   uint32  cp;
+   WCHAR   wctype[80];
+
+   memset(wctype, 0, 80 * sizeof(WCHAR));
+   MultiByteToWideChar(CP_ACP, 0, ctype, -1, wctype, 80);

The maximum length is documented as 85 characters, also:

: 
'Note   Your application must use the constant [LOCALE_NAME_MAX_LENGTH] 
for the maximum locale name length, instead of hard-coding the value "85".'


--
Christian



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Andrew Dunstan wrote:


On 04/24/2016 03:16 PM, Christian Ullrich wrote:



* Andrew Dunstan wrote:



msvcr120.dll seems to be the highest numbered one on my system, and we
already cover that. If you like we can add to the comments in that file.


There won't be a higher one, with VS 2015, CRT became a system
component, namely, UCRT. However, ucrtbase.dll does export putenv, so
it might need to be added.



OK. Does that mean we won't have to add anything more for future VS
releases?


Microsoft is swearing up one side and down the other that they will keep 
binary compatibility forever, and ever, hallelujah, hallelujah, and 
hence the name of the DLL is not expected to change again.


OTOH, the next manager of the development tools division might have 
different ideas. You never know.


--
Christian




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/24/2016 03:16 PM, Christian Ullrich wrote:

* Andrew Dunstan wrote:


On 04/24/2016 12:14 PM, Tom Lane wrote:



Andrew Dunstan  writes:
OK, here's my final version of the patch, which I will apply in 24 
hours

or so unless there is an objection.



BTW, in view of 9f633b404, shouldn't there be a similar addition to
win32env.c in this patch?



msvcr120.dll seems to be the highest numbered one on my system, and we
already cover that. If you like we can add to the comments in that file.


There won't be a higher one, with VS 2015, CRT became a system 
component, namely, UCRT. However, ucrtbase.dll does export putenv, so 
it might need to be added.





OK. Does that mean we won't have to add anything more for future VS 
releases?


cheers

andrew



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Andrew Dunstan wrote:


On 04/24/2016 12:14 PM, Tom Lane wrote:



Andrew Dunstan  writes:

OK, here's my final version of the patch, which I will apply in 24 hours
or so unless there is an objection.



BTW, in view of 9f633b404, shouldn't there be a similar addition to
win32env.c in this patch?



msvcr120.dll seems to be the highest numbered one on my system, and we
already cover that. If you like we can add to the comments in that file.


There won't be a higher one, with VS 2015, CRT became a system 
component, namely, UCRT. However, ucrtbase.dll does export putenv, so it 
might need to be added.


--
Christian




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Tom Lane]

Andrew Dunstan  writes:
> On 04/24/2016 12:14 PM, Tom Lane wrote:
>> BTW, in view of 9f633b404, shouldn't there be a similar addition to
>> win32env.c in this patch?

> msvcr120.dll seems to be the highest numbered one on my system, and we 
> already cover that. If you like we can add to the comments in that file.

Yeah, something like s/Visual Studio 2013/Visual Studio 2013 and 2105/
seems like it'd be appropriate.  The main thing I'm concerned about is
that this patch touch that list somehow, so that if someone uses it as
a reference for what to consider when adding support for VS N+1, they
know to check there.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/24/2016 12:14 PM, Tom Lane wrote:

Andrew Dunstan  writes:

OK, here's my final version of the patch, which I will apply in 24 hours
or so unless there is an objection.

BTW, in view of 9f633b404, shouldn't there be a similar addition to
win32env.c in this patch?





msvcr120.dll seems to be the highest numbered one on my system, and we 
already cover that. If you like we can add to the comments in that file.


cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Tom Lane]

Andrew Dunstan  writes:
> On 04/24/2016 11:58 AM, Tom Lane wrote:
>> Hmm ... does this pragma work on *every* compiler we're going to use
>> on Windows?

> According to my research it works on all the MSVC versions we support. I 
> didn't research the others (i.e. gcc), but we already use the pragma in 
> float.c without ill effect. Isn't the way #pragma works that compilers 
> that don't understand the particular pragma are just supposed to ignore it?

Well, the ones in float.c are guarded by "#if (_MSC_VER >= 1800)" and
will therefore not get compiled by any non-MSVC compiler.  I don't see
any entirely-unprotected #pragma uses in our tree, indicating that
we've not depended on any such assumption up to now.

Given that the whole file is platform-specific, it may well be fine;
I'm just voicing some concern.  I suppose the buildfarm will soon
tell us if it's not fine, though.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/24/2016 11:58 AM, Tom Lane wrote:

Andrew Dunstan  writes:

OK, here's my final version of the patch, which I will apply in 24 hours
or so unless there is an objection.
+#pragma warning(push)
+#pragma warning(disable : 4091)
  #include 
+#pragma warning(pop)

Hmm ... does this pragma work on *every* compiler we're going to use
on Windows?  I'm afraid that trying to suppress a warning in VS2015
is going to result in outright failure with other compilers.





According to my research it works on all the MSVC versions we support. I 
didn't research the others (i.e. gcc), but we already use the pragma in 
float.c without ill effect. Isn't the way #pragma works that compilers 
that don't understand the particular pragma are just supposed to ignore it?


cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Tom Lane]

Andrew Dunstan  writes:
> OK, here's my final version of the patch, which I will apply in 24 hours 
> or so unless there is an objection.

BTW, in view of 9f633b404, shouldn't there be a similar addition to
win32env.c in this patch?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Tom Lane]

Andrew Dunstan  writes:
> OK, here's my final version of the patch, which I will apply in 24 hours 
> or so unless there is an objection.

> +#pragma warning(push)
> +#pragma warning(disable : 4091)
>  #include 
> +#pragma warning(pop)

Hmm ... does this pragma work on *every* compiler we're going to use
on Windows?  I'm afraid that trying to suppress a warning in VS2015
is going to result in outright failure with other compilers.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/23/2016 11:25 PM, Andrew Dunstan wrote:



On 04/23/2016 06:33 PM, Tom Lane wrote:

I wrote:

Andrew Dunstan  writes:

On 04/23/2016 05:30 PM, Christian Ullrich wrote:

In this case, I would prefer this:

#ifdef WIN32_ONLY_COMPILER
-typedef int pid_t;
+typedef intptr_t pid_t;
#endif

That's a change that will have a pretty wide effect. Everything up to
now has been pretty low risk, but this worries me rather more. Maybe
it's safe, but I'd like to hear others' comments.

Yeah, it makes me a bit nervous too.

One other thought: even if this is safe for HEAD, I think we could
*not* back-patch it into 9.5, because it would amount to an ABI
break on Windows anywhere that pid_t is used in globally visible
structs or function signatures.  (Maybe there are no such places,
but I doubt it.)  So we'd need to go with the messy-cast solution
for 9.5.




It's not that messy. I'm inclined just to make minimal changed to 
pg_basebackup.c and be done with it. I don't think a compiler warning 
is worth doing more for.






OK, here's my final version of the patch, which I will apply in 24 hours 
or so unless there is an objection.


cheers

andrew



VS2015-support.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Petr Jelinek]

On 23/04/16 08:08, Christian Ullrich wrote:

* Christian Ullrich wrote:


* Andrew Dunstan wrote:


On 04/22/2016 02:46 AM, Michael Paquier wrote:



Progress report:

1. My VS 2015 installations (I now have several) all generate
solution file
with:
Microsoft Visual Studio Solution File, Format Version 12.00
so I propose to set that as the solution file version.


I am wondering why it happens this way for you..


It's not just me. See the reply at



and notice that in both cases the Solution file format is version 12.00.


Try as I may, I cannot get my VS 2015 to write a version 14 .sln file.
It's always "12.00". If I change it to 14 by hand, it still opens and
appears to work fine.

I tried to find a real-world version 14 solution to see if I can spot a
difference between it and the version 12 files, but there appears to be
very little out there, and what there is, looks like it was either
autogenerated or edited manually. Examples:


OK, so searching on GitHub yielded a few more results, but I could not
identify a single one that was clearly not created or edited by anything
other than Visual Studio itself.



I did some checking too, and yes it seems having version 12 .sln is fine 
or maybe even desirable. We mainly need to make sure that the tools 
version is 14 and platform toolset is v140 for MSVC15.


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/23/2016 06:33 PM, Tom Lane wrote:

I wrote:

Andrew Dunstan  writes:

On 04/23/2016 05:30 PM, Christian Ullrich wrote:

In this case, I would prefer this:

#ifdef WIN32_ONLY_COMPILER
-typedef int pid_t;
+typedef intptr_t pid_t;
#endif

That's a change that will have a pretty wide effect. Everything up to
now has been pretty low risk, but this worries me rather more. Maybe
it's safe, but I'd like to hear others' comments.

Yeah, it makes me a bit nervous too.

One other thought: even if this is safe for HEAD, I think we could
*not* back-patch it into 9.5, because it would amount to an ABI
break on Windows anywhere that pid_t is used in globally visible
structs or function signatures.  (Maybe there are no such places,
but I doubt it.)  So we'd need to go with the messy-cast solution
for 9.5.




It's not that messy. I'm inclined just to make minimal changed to 
pg_basebackup.c and be done with it. I don't think a compiler warning is 
worth doing more for.


cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Tom Lane]

I wrote:
> Andrew Dunstan  writes:
>> On 04/23/2016 05:30 PM, Christian Ullrich wrote:
>>> In this case, I would prefer this:
>>> 
>>> #ifdef WIN32_ONLY_COMPILER
>>> -typedef int pid_t;
>>> +typedef intptr_t pid_t;
>>> #endif

>> That's a change that will have a pretty wide effect. Everything up to 
>> now has been pretty low risk, but this worries me rather more. Maybe 
>> it's safe, but I'd like to hear others' comments.

> Yeah, it makes me a bit nervous too.

One other thought: even if this is safe for HEAD, I think we could
*not* back-patch it into 9.5, because it would amount to an ABI
break on Windows anywhere that pid_t is used in globally visible
structs or function signatures.  (Maybe there are no such places,
but I doubt it.)  So we'd need to go with the messy-cast solution
for 9.5.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Tom Lane]

Andrew Dunstan  writes:
> On 04/23/2016 05:30 PM, Christian Ullrich wrote:
>> In this case, I would prefer this:
>>
>> #ifdef WIN32_ONLY_COMPILER
>> -typedef int pid_t;
>> +typedef intptr_t pid_t;
>> #endif

> That's a change that will have a pretty wide effect. Everything up to 
> now has been pretty low risk, but this worries me rather more. Maybe 
> it's safe, but I'd like to hear others' comments.

Yeah, it makes me a bit nervous too.  We know that most of the code is
agnostic as to the width of pid_t, because it works on Unixen where
pid_t is 64bit.  But I'm less sure about whether the Windows-specific
parts are equally flexible.

On the other hand, wouldn't you expect to get compiler warnings for
any code that does get affected?  The main hazard would be incorrect
printf format flags (cf 994f11257 for a recent example), and I'd
certainly expect any C compiler worth its salt to whine about
inconsistencies there.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/23/2016 05:30 PM, Christian Ullrich wrote:

* Andrew Dunstan wrote:


On 04/22/2016 01:21 AM, Michael Paquier wrote:



5. It also complains about us casting a pid_t to a HANDLE in
pg_basebackup.c. Not sure what to do about that.

The thing that's being cast is not a PID, but a HANDLE to a process.
pid_t is a typedef for int (in port/win32.h), therefore is always 32
bits, while HANDLE is actually void*. However, Microsoft guarantees
that kernel32 HANDLEs (this includes those to threads and processes)
fit into 32 bits on AMD64.



Yes, when casting things this way I think that a comment would be fine
in the code. We could do that as separate patches actually.


We are already casting the pid_t to HANDLE and still getting a warning.
Apparently we need to do something on win64 like

(HANDLE) ((int64) bgchild)


Ah, OK, it warns about a cast to a larger type because the value might 
get sign extended. Not unreasonable.


In this case, I would prefer this:

diff --git a/src/include/port/win32.h b/src/include/port/win32.h
index ba8cf9d..b4086f1 100644
--- a/src/include/port/win32.h
+++ b/src/include/port/win32.h
@@ -256,7 +256,7 @@ typedef int gid_t;
 typedef long key_t;

 #ifdef WIN32_ONLY_COMPILER
-typedef int pid_t;
+typedef intptr_t pid_t;
 #endif

 /*

With this change, pg_basebackup -X stream works the same when built 
for 64 and 32 bits.







That's a change that will have a pretty wide effect. Everything up to 
now has been pretty low risk, but this worries me rather more. Maybe 
it's safe, but I'd like to hear others' comments.


cheers

andrew



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Andrew Dunstan wrote:


On 04/22/2016 01:21 AM, Michael Paquier wrote:



5. It also complains about us casting a pid_t to a HANDLE in
pg_basebackup.c. Not sure what to do about that.

The thing that's being cast is not a PID, but a HANDLE to a process.
pid_t is a typedef for int (in port/win32.h), therefore is always 32
bits, while HANDLE is actually void*. However, Microsoft guarantees
that kernel32 HANDLEs (this includes those to threads and processes)
fit into 32 bits on AMD64.



Yes, when casting things this way I think that a comment would be fine
in the code. We could do that as separate patches actually.


We are already casting the pid_t to HANDLE and still getting a warning.
Apparently we need to do something on win64 like

(HANDLE) ((int64) bgchild)


Ah, OK, it warns about a cast to a larger type because the value might 
get sign extended. Not unreasonable.


In this case, I would prefer this:

diff --git a/src/include/port/win32.h b/src/include/port/win32.h
index ba8cf9d..b4086f1 100644
--- a/src/include/port/win32.h
+++ b/src/include/port/win32.h
@@ -256,7 +256,7 @@ typedef int gid_t;
 typedef long key_t;

 #ifdef WIN32_ONLY_COMPILER
-typedef int pid_t;
+typedef intptr_t pid_t;
 #endif

 /*

With this change, pg_basebackup -X stream works the same when built for 
64 and 32 bits.


--
Christian




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/22/2016 01:21 AM, Michael Paquier wrote:

5. It also complains about us casting a pid_t to a HANDLE in
pg_basebackup.c. Not sure what to do about that.

The thing that's being cast is not a PID, but a HANDLE to a process. pid_t is a 
typedef for int (in port/win32.h), therefore is always 32 bits, while HANDLE is 
actually void*. However, Microsoft guarantees that kernel32 HANDLEs (this 
includes those to threads and processes) fit into 32 bits on AMD64.

Source: 
https://msdn.microsoft.com/en-us/library/windows/desktop/ee872017(v=vs.85).aspx,
 third bullet point.

So we can simply silence the warning by casting explicitly.

Yes, when casting things this way I think that a comment would be fine
in the code. We could do that as separate patches actually.



We are already casting the pid_t to HANDLE and still getting a warning. 
Apparently we need to do something on win64 like


(HANDLE) ((int64) bgchild)


cheers

andrew



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Christian Ullrich wrote:


* Andrew Dunstan wrote:


On 04/22/2016 02:46 AM, Michael Paquier wrote:



Progress report:

1. My VS 2015 installations (I now have several) all generate
solution file
with:
Microsoft Visual Studio Solution File, Format Version 12.00
so I propose to set that as the solution file version.


I am wondering why it happens this way for you..


It's not just me. See the reply at


and notice that in both cases the Solution file format is version 12.00.


Try as I may, I cannot get my VS 2015 to write a version 14 .sln file.
It's always "12.00". If I change it to 14 by hand, it still opens and
appears to work fine.

I tried to find a real-world version 14 solution to see if I can spot a
difference between it and the version 12 files, but there appears to be
very little out there, and what there is, looks like it was either
autogenerated or edited manually. Examples:


OK, so searching on GitHub yielded a few more results, but I could not 
identify a single one that was clearly not created or edited by anything 
other than Visual Studio itself.


--
Christian




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Andrew Dunstan wrote:


On 04/22/2016 02:46 AM, Michael Paquier wrote:



Progress report:

1. My VS 2015 installations (I now have several) all generate
solution file
with:
Microsoft Visual Studio Solution File, Format Version 12.00
so I propose to set that as the solution file version.

>>

I am wondering why it happens this way for you..


It's not just me. See the reply at

and notice that in both cases the Solution file format is version 12.00.


Try as I may, I cannot get my VS 2015 to write a version 14 .sln file. 
It's always "12.00". If I change it to 14 by hand, it still opens and 
appears to work fine.


I tried to find a real-world version 14 solution to see if I can spot a 
difference between it and the version 12 files, but there appears to be 
very little out there, and what there is, looks like it was either 
autogenerated or edited manually. Examples:


- 
 
- "converted to VS2015 in anticipation of the release"


- 
 
- "but I changed the the contents of the .SLN to Microsoft Visual Studio 
Solution File, Format Version 14.00"


- 
 
- "# This file was generated by MPC."


Google returns not quite two pages of results for "Microsoft Visual 
Studio Solution File, Format Version 14.00"; it *has* to be nonstandard.


My best guess at this point is that the solution format really did not 
change between 2013 and 2015. Microsoft may just have added support for 
.sln version 14 for compatibility with people who generate solutions by 
other means and who are under the impression the .sln version has to be 
in step with the VS version, when in fact the VisualStudioVersion line 
in a version 12 .sln file controls which version of VS will open any 
given file by default, and the MinimumVisualStudioVersion line indicates 
which VS version supports the project types in the solution.


If my guess is wrong, and anyone knows a way to make VS 2015 write a 
version 14 .sln file, please tell me.


--
Christian




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Tom Lane]

Michael Paquier  writes:
> On Fri, Apr 22, 2016 at 10:49 PM, Tom Lane  wrote:
>> How far back are we thinking of supporting VS2015, anyway?  I can check
>> and push this as a separate patch.

> My guess is 9.5: HEAD + last stable branch. That's what has been done
> when support for VS2012 or VS2013 has been added.

OK.  Pushed with some adjustments --- mainly, I undid the conversion
of the non-bool-returning functions, since those aren't broken (yet)
and I think it's still valuable to test V0 as much as we can here.

Also, I static-ified some functions that weren't SQL-visible, and
thereby discovered that some of those were actually dead code ...

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/22/2016 02:46 AM, Michael Paquier wrote:



Progress report:

1. My VS 2015 installations (I now have several) all generate solution file
with:
Microsoft Visual Studio Solution File, Format Version 12.00
so I propose to set that as the solution file version.

I am wondering why it happens this way for you..



It's not just me. See the reply at 
 
and notice that in both cases the Solution file format is version 12.00.


cheers

andrew




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Fri, Apr 22, 2016 at 10:49 PM, Tom Lane  wrote:
> How far back are we thinking of supporting VS2015, anyway?  I can check
> and push this as a separate patch.

My guess is 9.5: HEAD + last stable branch. That's what has been done
when support for VS2012 or VS2013 has been added.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Tom Lane]

Michael Paquier  writes:
> On Fri, Apr 22, 2016 at 1:30 PM, Tom Lane  wrote:
>> If we assume that oldstyle functions returning integer are still okay,
>> which the success of the regression test case involving oldstyle_length()
>> seems to prove, then indeed seg's bool-returning functions are the only
>> hazard.

> Your assumption is right. With the patch attached for contrib/seg/
> that converts all those functions to use the V1 declaration, I am able
> to make the tests pass. As the internal shape of the functions is not
> changed and that there are no functional changes, I guess that it
> would be fine to backpatch down to where VS2015 is intended to be
> supported. Is anybody here foreseeing any problems for back-branches
> if there is such a change?

It should be fine, since converting a function to V1 makes no difference
at the SQL level --- we don't need an extension script modification.

How far back are we thinking of supporting VS2015, anyway?  I can check
and push this as a separate patch.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Fri, Apr 22, 2016 at 1:30 PM, Tom Lane  wrote:
> Andrew Dunstan  writes:
>> On 04/21/2016 05:15 PM, Tom Lane wrote:
>>> Do the other contrib modules all pass?  I can't recall if seg was the
>>> only one we'd left like this.
>
>> Only seg fails.
>
> As a crosscheck, I put some code into fmgr_c_validator() to log a message
> when creating a V0 function with a pass-by-val return type.  (Pass-by-ref
> is no problem, according to my hypothesis, since it necessarily means
> the C function returns a pointer.)  I get these hits in core + contrib
> regression tests:
>
> [...]
>
> If we assume that oldstyle functions returning integer are still okay,
> which the success of the regression test case involving oldstyle_length()
> seems to prove, then indeed seg's bool-returning functions are the only
> hazard.
>
> Note though that this test fails to cover any contrib modules that
> lack regression tests, since they wouldn't have gotten loaded by
> "make installcheck".

Your assumption is right. With the patch attached for contrib/seg/
that converts all those functions to use the V1 declaration, I am able
to make the tests pass. As the internal shape of the functions is not
changed and that there are no functional changes, I guess that it
would be fine to backpatch down to where VS2015 is intended to be
supported. Is anybody here foreseeing any problems for back-branches
if there is such a change?
-- 
Michael
diff --git a/contrib/seg/seg.c b/contrib/seg/seg.c
index c6c082b..c3651d4 100644
--- a/contrib/seg/seg.c
+++ b/contrib/seg/seg.c
@@ -47,52 +47,60 @@ PG_FUNCTION_INFO_V1(seg_center);
 /*
 ** GiST support methods
 */
-bool gseg_consistent(GISTENTRY *entry,
-SEG *query,
-StrategyNumber strategy,
-Oid subtype,
-bool *recheck);
-GISTENTRY  *gseg_compress(GISTENTRY *entry);
-GISTENTRY  *gseg_decompress(GISTENTRY *entry);
-float	   *gseg_penalty(GISTENTRY *origentry, GISTENTRY *newentry, float *result);
-GIST_SPLITVEC *gseg_picksplit(GistEntryVector *entryvec, GIST_SPLITVEC *v);
+PG_FUNCTION_INFO_V1(gseg_consistent);
+PG_FUNCTION_INFO_V1(gseg_compress);
+PG_FUNCTION_INFO_V1(gseg_decompress);
+PG_FUNCTION_INFO_V1(gseg_penalty);
+PG_FUNCTION_INFO_V1(gseg_picksplit);
+PG_FUNCTION_INFO_V1(gseg_same);
+PG_FUNCTION_INFO_V1(gseg_union);
+
 bool		gseg_leaf_consistent(SEG *key, SEG *query, StrategyNumber strategy);
 bool		gseg_internal_consistent(SEG *key, SEG *query, StrategyNumber strategy);
-SEG		   *gseg_union(GistEntryVector *entryvec, int *sizep);
 SEG		   *gseg_binary_union(SEG *r1, SEG *r2, int *sizep);
-bool	   *gseg_same(SEG *b1, SEG *b2, bool *result);
 
 
 /*
 ** R-tree support functions
 */
-bool		seg_same(SEG *a, SEG *b);
+PG_FUNCTION_INFO_V1(seg_same);
+static inline bool seg_same_internal(SEG *a, SEG *b);
 bool		seg_contains_int(SEG *a, int *b);
 bool		seg_contains_float4(SEG *a, float4 *b);
 bool		seg_contains_float8(SEG *a, float8 *b);
-bool		seg_contains(SEG *a, SEG *b);
-bool		seg_contained(SEG *a, SEG *b);
-bool		seg_overlap(SEG *a, SEG *b);
-bool		seg_left(SEG *a, SEG *b);
-bool		seg_over_left(SEG *a, SEG *b);
-bool		seg_right(SEG *a, SEG *b);
-bool		seg_over_right(SEG *a, SEG *b);
-SEG		   *seg_union(SEG *a, SEG *b);
-SEG		   *seg_inter(SEG *a, SEG *b);
+PG_FUNCTION_INFO_V1(seg_contains);
+static inline bool seg_contains_internal(SEG *a, SEG *b);
+PG_FUNCTION_INFO_V1(seg_contained);
+static inline bool seg_contained_internal(SEG *a, SEG *b);
+PG_FUNCTION_INFO_V1(seg_overlap);
+static inline bool seg_overlap_internal(SEG *a, SEG *b);
+PG_FUNCTION_INFO_V1(seg_left);
+static inline bool seg_left_internal(SEG *a, SEG *b);
+PG_FUNCTION_INFO_V1(seg_over_left);
+static inline bool seg_over_left_internal(SEG *a, SEG *b);
+PG_FUNCTION_INFO_V1(seg_right);
+static inline bool seg_right_internal(SEG *a, SEG *b);
+PG_FUNCTION_INFO_V1(seg_over_right);
+static inline bool seg_over_right_internal(SEG *a, SEG *b);
+PG_FUNCTION_INFO_V1(seg_union);
+static inline SEG *seg_union_internal(SEG *a, SEG *b);
+PG_FUNCTION_INFO_V1(seg_inter);
 void		rt_seg_size(SEG *a, float *sz);
 
 /*
 ** Various operators
 */
-int32		seg_cmp(SEG *a, SEG *b);
-bool		seg_lt(SEG *a, SEG *b);
-bool		seg_le(SEG *a, SEG *b);
-bool		seg_gt(SEG *a, SEG *b);
-bool		seg_ge(SEG *a, SEG *b);
-bool		seg_different(SEG *a, SEG *b);
+PG_FUNCTION_INFO_V1(seg_cmp);
+static inline int32 seg_cmp_internal(SEG *a, SEG *b);
+PG_FUNCTION_INFO_V1(seg_lt);
+PG_FUNCTION_INFO_V1(seg_le);
+PG_FUNCTION_INFO_V1(seg_gt);
+PG_FUNCTION_INFO_V1(seg_ge);
+PG_FUNCTION_INFO_V1(seg_different);
+
 
 /*
-** Auxiliary funxtions
+** Auxiliary functions
 */
 static int	restore(char *s, float val, int n);
 
@@ -193,13 +201,15 @@ seg_upper(PG_FUNCTION_ARGS)
 ** the predicate x op query == FALSE, where op is the oper
 ** corresponding to strategy in the pg_amop table.
 */
-bool
-gseg_consistent(GISTENTRY *entry,
-SEG *query,
-StrategyNumber strategy,
-Oid subtype,
-bool *recheck)
+Datum
+gseg_consistent(PG_FUNCTION_ARGS)
 {
+	GISTENTRY  *entry = (GISTENTRY *)

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Fri, Apr 22, 2016 at 4:39 AM, Andrew Dunstan  wrote:
> On 04/11/2016 03:47 AM, Michael Paquier wrote:
>> On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
>>  wrote:
>>>
>>> Now, I have produced two patches:
>>> - 0001-Support-for-VS-2015-locale-hack.patch, which makes use of
>>> __crt_locale_data_public in ucrt/corecrt.h. This is definitely an ugly
>>> hack, though I am coming to think that this may be the approach that
>>> would us the less harm, and that's closer to what is done for VS 2012
>>> and 2013.
>>> - 0001-Support-for-VS-2015-getlocaleinfoex.patch, which make use of
>>> GetLocaleInfoEx, this requires us to lower a bit the support grid for
>>> Windows, basically that's throwing support for XP if compilation is
>>> done with VS 2015.
>>> Based on my tests, both are working with short and long local names,
>>> testing via initdb --locale.
>>
>> The first patch is actually not what I wanted to send. Here are the
>> correct ones...
>
> I think I prefer the less hacky solution.

OK, at least we go in one direction.

> Progress report:
>
> 1. My VS 2015 installations (I now have several) all generate solution file
> with:
>Microsoft Visual Studio Solution File, Format Version 12.00
> so I propose to set that as the solution file version.

I am wondering why it happens this way for you..

> 2. The logic in win32.h is a bit convoluted. I'm going to simplify it.

OK. Should I wait for a patch to look at then?

> 3. I'm going to define _WINSOCK_DEPRECATED_NO_WARNINGS to silence some
> compiler bleatings about the way we use the Winsock API

Agreed here. I saw those warnings as well.

> 6. most importantly, on my Release/X64 build, I am getting a regression
> failure on contrib/seg. regression diffs attached. Until that's fixed this
> isn't going anywhere.

Hm. I am now looking at that, and I can reproduce the failure... I
will send a patch soon.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Fri, Apr 22, 2016 at 4:56 AM, Christian Ullrich  wrote:
> * From: Andrew Dunstan [mailto:and...@dunslane.net]
>
>> 4. The compiler complains about one of Microsoft's own header files -
>> essentially it dislikes the=is construct:
>>
>> typedef enum { ... };
>>
>> It would be nice to make it shut up about it.
>
> I doubt that's possible; the declaration *is* wrong after all. We could turn 
> off the warning:
>
> #pragma warning(push)
> #pragma warning(disable : 1234, or whatever the number is)
> #include 
> #pragma warning(pop)

Well, yes.. Even if that's not pretty that would not be the first one
caused by a VS header bug (float.c)..

>> 5. It also complains about us casting a pid_t to a HANDLE in
>> pg_basebackup.c. Not sure what to do about that.
>
> The thing that's being cast is not a PID, but a HANDLE to a process. pid_t is 
> a typedef for int (in port/win32.h), therefore is always 32 bits, while 
> HANDLE is actually void*. However, Microsoft guarantees that kernel32 HANDLEs 
> (this includes those to threads and processes) fit into 32 bits on AMD64.
>
> Source: 
> https://msdn.microsoft.com/en-us/library/windows/desktop/ee872017(v=vs.85).aspx,
>  third bullet point.
>
> So we can simply silence the warning by casting explicitly.

Yes, when casting things this way I think that a comment would be fine
in the code. We could do that as separate patches actually.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Tom Lane]

Andrew Dunstan  writes:
> On 04/21/2016 05:15 PM, Tom Lane wrote:
>> Do the other contrib modules all pass?  I can't recall if seg was the
>> only one we'd left like this.

> Only seg fails.

As a crosscheck, I put some code into fmgr_c_validator() to log a message
when creating a V0 function with a pass-by-val return type.  (Pass-by-ref
is no problem, according to my hypothesis, since it necessarily means
the C function returns a pointer.)  I get these hits in core + contrib
regression tests:

core:
LOG:  version-0 function widget_in returns type widget
LOG:  version-0 function oldstyle_length returns type integer

contrib:
LOG:  version-0 function seg_over_left returns type boolean
LOG:  version-0 function seg_over_right returns type boolean
LOG:  version-0 function seg_left returns type boolean
LOG:  version-0 function seg_right returns type boolean
LOG:  version-0 function seg_lt returns type boolean
LOG:  version-0 function seg_le returns type boolean
LOG:  version-0 function seg_gt returns type boolean
LOG:  version-0 function seg_ge returns type boolean
LOG:  version-0 function seg_contains returns type boolean
LOG:  version-0 function seg_contained returns type boolean
LOG:  version-0 function seg_overlap returns type boolean
LOG:  version-0 function seg_same returns type boolean
LOG:  version-0 function seg_different returns type boolean
LOG:  version-0 function seg_cmp returns type integer
LOG:  version-0 function gseg_consistent returns type boolean
LOG:  version-0 function gseg_compress returns type internal
LOG:  version-0 function gseg_decompress returns type internal
LOG:  version-0 function gseg_penalty returns type internal
LOG:  version-0 function gseg_picksplit returns type internal
LOG:  version-0 function gseg_same returns type internal

The widget_in gripe is a false positive, caused by the fact that we don't
know the properties of type widget when widget_in is declared.  We also
need not worry about the functions that return "internal", since that's
defined to be pointer-sized.

If we assume that oldstyle functions returning integer are still okay,
which the success of the regression test case involving oldstyle_length()
seems to prove, then indeed seg's bool-returning functions are the only
hazard.

Note though that this test fails to cover any contrib modules that
lack regression tests, since they wouldn't have gotten loaded by
"make installcheck".

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/21/2016 05:15 PM, Tom Lane wrote:


IIRC, we had intentionally left contrib/seg using mostly V0 calling
convention as a canary to let us know when that broke.  It's a bit
astonishing that it lasted this long, maybe.  But it looks like we're
going to have to convert at least the bool-returning functions to V1
if we want it to work on VS 2015.

Do the other contrib modules all pass?  I can't recall if seg was the
only one we'd left like this.





Only seg fails.

cheers

andrew



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Tom Lane]

Andrew Dunstan  writes:
> 5. most importantly, on my Release/X64 build, I am getting a regression 
> failure on contrib/seg. regression diffs attached. Until that's fixed 
> this isn't going anywhere.

I'll bet a nickel that this means MSVC has broken the ABI rules that
allowed old-style (version 0) C functions to limp along without changes.
seg_same() and the other comparison functions that are misbehaving are
declared at the C level to return "bool".  I think what's happening is
they are now setting only one byte in the return register, or perhaps
only the low-order word, leaving the high-order bits as trash.  But
fmgr_oldstyle is coded as though V0 functions return "char *", and it's
going to be putting that whole pointer value into the result Datum, and
if the value isn't entirely zero then DatumGetBool will consider it
"true".  So this theory predicts a lot of intended "false" results will
read as "true", which is what your regression diffs show.

IIRC, we had intentionally left contrib/seg using mostly V0 calling
convention as a canary to let us know when that broke.  It's a bit
astonishing that it lasted this long, maybe.  But it looks like we're
going to have to convert at least the bool-returning functions to V1
if we want it to work on VS 2015.

Do the other contrib modules all pass?  I can't recall if seg was the
only one we'd left like this.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* From: Andrew Dunstan [mailto:and...@dunslane.net]

> 4. The compiler complains about one of Microsoft's own header files -
> essentially it dislikes the=is construct:
> 
> typedef enum { ... };
> 
> It would be nice to make it shut up about it.

I doubt that's possible; the declaration *is* wrong after all. We could turn 
off the warning:

#pragma warning(push)
#pragma warning(disable : 1234, or whatever the number is)
#include 
#pragma warning(pop)

> 5. It also complains about us casting a pid_t to a HANDLE in
> pg_basebackup.c. Not sure what to do about that.

The thing that's being cast is not a PID, but a HANDLE to a process. pid_t is a 
typedef for int (in port/win32.h), therefore is always 32 bits, while HANDLE is 
actually void*. However, Microsoft guarantees that kernel32 HANDLEs (this 
includes those to threads and processes) fit into 32 bits on AMD64.

Source: 
https://msdn.microsoft.com/en-us/library/windows/desktop/ee872017(v=vs.85).aspx,
 third bullet point.

So we can simply silence the warning by casting explicitly.

-- 
Christian




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/11/2016 03:47 AM, Michael Paquier wrote:

On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
 wrote:

Now, I have produced two patches:
- 0001-Support-for-VS-2015-locale-hack.patch, which makes use of
__crt_locale_data_public in ucrt/corecrt.h. This is definitely an ugly
hack, though I am coming to think that this may be the approach that
would us the less harm, and that's closer to what is done for VS 2012
and 2013.
- 0001-Support-for-VS-2015-getlocaleinfoex.patch, which make use of
GetLocaleInfoEx, this requires us to lower a bit the support grid for
Windows, basically that's throwing support for XP if compilation is
done with VS 2015.
Based on my tests, both are working with short and long local names,
testing via initdb --locale.

The first patch is actually not what I wanted to send. Here are the
correct ones...





I think I prefer the less hacky solution.

Progress report:

1. My VS 2015 installations (I now have several) all generate solution 
file with:


   Microsoft Visual Studio Solution File, Format Version 12.00

so I propose to set that as the solution file version.

2. The logic in win32.h is a bit convoluted. I'm going to simplify it.

3. I'm going to define _WINSOCK_DEPRECATED_NO_WARNINGS to silence some 
compiler bleatings about the way we use the Winsock API


4. The compiler complains about one of Microsoft's own header files - 
essentially it dislikes the=is construct:


   typedef enum { ... };

It would be nice to make it shut up about it.

5. It also complains about us casting a pid_t to a HANDLE in 
pg_basebackup.c. Not sure what to do about that.


5. most importantly, on my Release/X64 build, I am getting a regression 
failure on contrib/seg. regression diffs attached. Until that's fixed 
this isn't going anywhere.


cheers

andrew


*** C:/prog/postgresql/contrib/seg/expected/seg.out Wed Apr  6 12:18:10 2016
--- C:/prog/postgresql/contrib/seg/results/seg.out  Thu Apr 21 11:34:26 2016
***
*** 444,450 
  SELECT '24 .. 33.20'::seg = '24 .. 33.21'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '24 .. 33.20'::seg != '24 .. 33.20'::seg AS bool;
--- 444,450 
  SELECT '24 .. 33.20'::seg = '24 .. 33.21'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '24 .. 33.20'::seg != '24 .. 33.20'::seg AS bool;
***
*** 556,562 
  SELECT '1'::seg &< '0'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '1'::seg &< '1'::seg AS bool;
--- 556,562 
  SELECT '1'::seg &< '0'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '1'::seg &< '1'::seg AS bool;
***
*** 574,580 
  SELECT '0 .. 1'::seg &< '0'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '0 .. 1'::seg &< '1'::seg AS bool;
--- 574,580 
  SELECT '0 .. 1'::seg &< '0'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '0 .. 1'::seg &< '1'::seg AS bool;
***
*** 592,598 
  SELECT '0 .. 1'::seg &< '0 .. 0.5'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '0 .. 1'::seg &< '0 .. 1'::seg AS bool;
--- 592,598 
  SELECT '0 .. 1'::seg &< '0 .. 0.5'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '0 .. 1'::seg &< '0 .. 1'::seg AS bool;
***
*** 624,630 
  SELECT '0'::seg &> '1'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '1'::seg &> '1'::seg AS bool;
--- 624,630 
  SELECT '0'::seg &> '1'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '1'::seg &> '1'::seg AS bool;
***
*** 692,704 
  SELECT '1'::seg << '0'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '1'::seg << '1'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '1'::seg << '2'::seg AS bool;
--- 692,704 
  SELECT '1'::seg << '0'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '1'::seg << '1'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '1'::seg << '2'::seg AS bool;
***
*** 710,722 
  SELECT '0 .. 1'::seg << '0'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '0 .. 1'::seg << '1'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '0 .. 1'::seg << '2'::seg AS bool;
--- 710,722 
  SELECT '0 .. 1'::seg << '0'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '0 .. 1'::seg << '1'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '0 .. 1'::seg << '2'::seg AS bool;
***
*** 728,752 
  SELECT '0 .. 1'::seg << '0 .. 0.5'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '0 .. 1'::seg << '0 .. 1'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '0 .. 1'::seg << '0 .. 2'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '0 .. 1'::seg << '1 .. 2'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '0 .. 1'::seg << '2 .. 3'::seg AS bool;
--- 728,752 
  SELECT '0 .. 1'::seg << '0 .. 0.5'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '0 .. 1'::seg << '0 .. 1'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELE

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Noah Misch]

On Wed, Apr 20, 2016 at 02:03:16PM +0900, Michael Paquier wrote:
> On Wed, Apr 20, 2016 at 1:39 PM, Noah Misch  wrote:
> > On Tue, Apr 19, 2016 at 02:42:24AM -0300, Alvaro Herrera wrote:
> >> This thread seems to have stalled.  I thought we were going to consider
> >> these patches for 9.6.
> >
> > Committers have given this thread's patches a generous level of 
> > consideration.
> > At this point, if $you wouldn't back-patch them to at least 9.5, they don't
> > belong in 9.6.  However, a back-patch to 9.3 does seem fair, assuming the
> > final patch looks anything like the current proposals.
> 
> Er, the change is rather located and fully controlled by _MSC_VER >=
> 1900, so this represents no risk for existing compilations on Windows,
> don't you agree?

Yes.  That is why I said a back-patch to 9.3 seems fair.

> >> Should we simply push them to see what the
> >> buildfarm thinks?
> >
> > No.  The thread has been getting suitable test reports for a few weeks now.
> > If it were not, better to make the enhancement wait as long as necessary 
> > than
> > to use the buildfarm that way.  Buildfarm results wouldn't even be 
> > pertinent;
> > they would merely tell us whether the patch broke non-VS 2015 compilers.
> 
> Well, they could push them, the results won't really matter and
> existing machines won't be impacted, as no buildfarm machine is using
> _MSC_VER >= 1900 as of now. Petr has one ready though as mentioned
> upthread.

Here you've presented two additional good reasons to not "simply push them to
see what the buildfarm thinks."


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Wed, Apr 20, 2016 at 1:39 PM, Noah Misch  wrote:
> On Tue, Apr 19, 2016 at 02:42:24AM -0300, Alvaro Herrera wrote:
>> Michael Paquier wrote:
>> > On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
>> >  wrote:
>> > > Now, I have produced two patches:
>> > > - 0001-Support-for-VS-2015-locale-hack.patch, which makes use of
>> > > __crt_locale_data_public in ucrt/corecrt.h. This is definitely an ugly
>> > > hack, though I am coming to think that this may be the approach that
>> > > would us the less harm, and that's closer to what is done for VS 2012
>> > > and 2013.
>> > > - 0001-Support-for-VS-2015-getlocaleinfoex.patch, which make use of
>> > > GetLocaleInfoEx, this requires us to lower a bit the support grid for
>> > > Windows, basically that's throwing support for XP if compilation is
>> > > done with VS 2015.
>> > > Based on my tests, both are working with short and long local names,
>> > > testing via initdb --locale.
>> >
>> > The first patch is actually not what I wanted to send. Here are the
>> > correct ones...
>>
>> This thread seems to have stalled.  I thought we were going to consider
>> these patches for 9.6.
>
> Committers have given this thread's patches a generous level of consideration.
> At this point, if $you wouldn't back-patch them to at least 9.5, they don't
> belong in 9.6.  However, a back-patch to 9.3 does seem fair, assuming the
> final patch looks anything like the current proposals.

Er, the change is rather located and fully controlled by _MSC_VER >=
1900, so this represents no risk for existing compilations on Windows,
don't you agree? With HEAD, code compilation would just fail btw
knowing how locales are broken, so it would just not work. That said,
support for new VS versions have been backpatched to the last stable
version at least, now that would be 9.5. There is indeed no written
policy stating what to do precisely in this case, but it seems to me
that there is no point in being overly protective as well..

>> Should we simply push them to see what the
>> buildfarm thinks?
>
> No.  The thread has been getting suitable test reports for a few weeks now.
> If it were not, better to make the enhancement wait as long as necessary than
> to use the buildfarm that way.  Buildfarm results wouldn't even be pertinent;
> they would merely tell us whether the patch broke non-VS 2015 compilers.

Well, they could push them, the results won't really matter and
existing machines won't be impacted, as no buildfarm machine is using
_MSC_VER >= 1900 as of now. Petr has one ready though as mentioned
upthread.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Michael Paquier wrote:


On Wed, Apr 20, 2016 at 1:48 AM, Christian Ullrich  wrote:

Both patches behave exactly the same in this test. Of the 102 remaining
locales, I get an unexpected codepage for just four:

- kk: Expected 1251, used 1252
- kk-KZ: Expected 1251, used 1252
- sr: Expected 1251, used 1250
- uk: Expected 1251, used 1252

I suspect that "sr" is a bug in MSDN: 1250 is Eastern European (Latin), 1251
is Cyrillic, and "sr" alone is listed as Latin. So either the script or the
codepage are likely wrong.


Does VS2013 or older behave the same way for those locales? The patch
using __crt_locale_data_public on VS2015 should work more or less
similarly to VS2012~2013.


Identical results for unmodified master built with 2013.

--
Christian




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Noah Misch]

On Tue, Apr 19, 2016 at 02:42:24AM -0300, Alvaro Herrera wrote:
> Michael Paquier wrote:
> > On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
> >  wrote:
> > > Now, I have produced two patches:
> > > - 0001-Support-for-VS-2015-locale-hack.patch, which makes use of
> > > __crt_locale_data_public in ucrt/corecrt.h. This is definitely an ugly
> > > hack, though I am coming to think that this may be the approach that
> > > would us the less harm, and that's closer to what is done for VS 2012
> > > and 2013.
> > > - 0001-Support-for-VS-2015-getlocaleinfoex.patch, which make use of
> > > GetLocaleInfoEx, this requires us to lower a bit the support grid for
> > > Windows, basically that's throwing support for XP if compilation is
> > > done with VS 2015.
> > > Based on my tests, both are working with short and long local names,
> > > testing via initdb --locale.
> > 
> > The first patch is actually not what I wanted to send. Here are the
> > correct ones...
> 
> This thread seems to have stalled.  I thought we were going to consider
> these patches for 9.6.

Committers have given this thread's patches a generous level of consideration.
At this point, if $you wouldn't back-patch them to at least 9.5, they don't
belong in 9.6.  However, a back-patch to 9.3 does seem fair, assuming the
final patch looks anything like the current proposals.

> Should we simply push them to see what the
> buildfarm thinks?

No.  The thread has been getting suitable test reports for a few weeks now.
If it were not, better to make the enhancement wait as long as necessary than
to use the buildfarm that way.  Buildfarm results wouldn't even be pertinent;
they would merely tell us whether the patch broke non-VS 2015 compilers.

nm


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Wed, Apr 20, 2016 at 1:48 AM, Christian Ullrich  wrote:
> Both patches behave exactly the same in this test. Of the 102 remaining
> locales, I get an unexpected codepage for just four:
>
> - kk: Expected 1251, used 1252
> - kk-KZ: Expected 1251, used 1252
> - sr: Expected 1251, used 1250
> - uk: Expected 1251, used 1252
>
> I suspect that "sr" is a bug in MSDN: 1250 is Eastern European (Latin), 1251
> is Cyrillic, and "sr" alone is listed as Latin. So either the script or the
> codepage are likely wrong.

Does VS2013 or older behave the same way for those locales? The patch
using __crt_locale_data_public on VS2015 should work more or less
similarly to VS2012~2013.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Alvaro Herrera wrote:


Michael Paquier wrote:

On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
 wrote:

Now, I have produced two patches:
- 0001-Support-for-VS-2015-locale-hack.patch, which makes use of
__crt_locale_data_public in ucrt/corecrt.h. This is definitely an ugly
hack, though I am coming to think that this may be the approach that
would us the less harm, and that's closer to what is done for VS 2012
and 2013.
- 0001-Support-for-VS-2015-getlocaleinfoex.patch, which make use of
GetLocaleInfoEx, this requires us to lower a bit the support grid for
Windows, basically that's throwing support for XP if compilation is
done with VS 2015.
Based on my tests, both are working with short and long local names,
testing via initdb --locale.


The first patch is actually not what I wanted to send. Here are the
correct ones...


This thread seems to have stalled.  I thought we were going to consider
these patches for 9.6.  Should we simply push them to see what the
buildfarm thinks?  Has anyone other than Michael actually tested it in
VS2015?


I built them both, and for lack of a better idea, ran initdb with all 
(short) locales in the Vista list 
(https://msdn.microsoft.com/en-us/goglobal/bb896001.aspx, second column) 
whose ANSI codepage is not either 0 or 1252. The former probably means 
"no such thing", the latter is my own default which I excluded to detect 
cases where it falls back to that.


Both patches behave exactly the same in this test. Of the 102 remaining 
locales, I get an unexpected codepage for just four:


- kk: Expected 1251, used 1252
- kk-KZ: Expected 1251, used 1252
- sr: Expected 1251, used 1250
- uk: Expected 1251, used 1252

I suspect that "sr" is a bug in MSDN: 1250 is Eastern European (Latin), 
1251 is Cyrillic, and "sr" alone is listed as Latin. So either the 
script or the codepage are likely wrong.


--
Christian



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/19/2016 01:47 AM, Michael Paquier wrote:

On Tue, Apr 19, 2016 at 2:42 PM, Alvaro Herrera
 wrote:

Michael Paquier wrote:

On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
 wrote:

Now, I have produced two patches:
- 0001-Support-for-VS-2015-locale-hack.patch, which makes use of
__crt_locale_data_public in ucrt/corecrt.h. This is definitely an ugly
hack, though I am coming to think that this may be the approach that
would us the less harm, and that's closer to what is done for VS 2012
and 2013.
- 0001-Support-for-VS-2015-getlocaleinfoex.patch, which make use of
GetLocaleInfoEx, this requires us to lower a bit the support grid for
Windows, basically that's throwing support for XP if compilation is
done with VS 2015.
Based on my tests, both are working with short and long local names,
testing via initdb --locale.

The first patch is actually not what I wanted to send. Here are the
correct ones...

This thread seems to have stalled.  I thought we were going to consider
these patches for 9.6.  Should we simply push them to see what the
buildfarm thinks?

We need to choose first which one we'd like to have, it would be great
to get some input from Andrew or even Magnus here who I thought would
get the last word.



trying to make some more time to spend on this. I hope to in the next 
day or two.


cheers

andrew




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Tue, Apr 19, 2016 at 2:42 PM, Alvaro Herrera
 wrote:
> Michael Paquier wrote:
>> On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
>>  wrote:
>> > Now, I have produced two patches:
>> > - 0001-Support-for-VS-2015-locale-hack.patch, which makes use of
>> > __crt_locale_data_public in ucrt/corecrt.h. This is definitely an ugly
>> > hack, though I am coming to think that this may be the approach that
>> > would us the less harm, and that's closer to what is done for VS 2012
>> > and 2013.
>> > - 0001-Support-for-VS-2015-getlocaleinfoex.patch, which make use of
>> > GetLocaleInfoEx, this requires us to lower a bit the support grid for
>> > Windows, basically that's throwing support for XP if compilation is
>> > done with VS 2015.
>> > Based on my tests, both are working with short and long local names,
>> > testing via initdb --locale.
>>
>> The first patch is actually not what I wanted to send. Here are the
>> correct ones...
>
> This thread seems to have stalled.  I thought we were going to consider
> these patches for 9.6.  Should we simply push them to see what the
> buildfarm thinks?

We need to choose first which one we'd like to have, it would be great
to get some input from Andrew or even Magnus here who I thought would
get the last word.

> Has anyone other than Michael actually tested it in VS2015?

It doesn't seem to be the case...
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Alvaro Herrera]

Michael Paquier wrote:
> On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
>  wrote:
> > Now, I have produced two patches:
> > - 0001-Support-for-VS-2015-locale-hack.patch, which makes use of
> > __crt_locale_data_public in ucrt/corecrt.h. This is definitely an ugly
> > hack, though I am coming to think that this may be the approach that
> > would us the less harm, and that's closer to what is done for VS 2012
> > and 2013.
> > - 0001-Support-for-VS-2015-getlocaleinfoex.patch, which make use of
> > GetLocaleInfoEx, this requires us to lower a bit the support grid for
> > Windows, basically that's throwing support for XP if compilation is
> > done with VS 2015.
> > Based on my tests, both are working with short and long local names,
> > testing via initdb --locale.
> 
> The first patch is actually not what I wanted to send. Here are the
> correct ones...

This thread seems to have stalled.  I thought we were going to consider
these patches for 9.6.  Should we simply push them to see what the
buildfarm thinks?  Has anyone other than Michael actually tested it in
VS2015?

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
 wrote:
> Now, I have produced two patches:
> - 0001-Support-for-VS-2015-locale-hack.patch, which makes use of
> __crt_locale_data_public in ucrt/corecrt.h. This is definitely an ugly
> hack, though I am coming to think that this may be the approach that
> would us the less harm, and that's closer to what is done for VS 2012
> and 2013.
> - 0001-Support-for-VS-2015-getlocaleinfoex.patch, which make use of
> GetLocaleInfoEx, this requires us to lower a bit the support grid for
> Windows, basically that's throwing support for XP if compilation is
> done with VS 2015.
> Based on my tests, both are working with short and long local names,
> testing via initdb --locale.

The first patch is actually not what I wanted to send. Here are the
correct ones...
-- 
Michael


0001-Support-for-VS-2015-getlocaleinfoex.patch
Description: binary/octet-stream


0001-Support-for-VS-2015-locale-hack.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Mon, Apr 11, 2016 at 6:03 AM, Petr Jelinek  wrote:
> On 10/04/16 20:47, Christian Ullrich wrote:
>>> don't think we need to be too precious about saving a byte or two
>>> here. Can one or other of you please prepare a replacement patch based
>>> in this discussion?

So, I am finally back on the battlefield.

>> Sorry, I don't think I'm up to that (at least not for another week or
>> so). I have to read up on the issue first; right now I'm not sure what
>> exactly the problem is.
>>
>> What I can say is that the existing patch needs more work, because
>> GetLocaleInfoEx() expects a locale name ("de-DE") as its first argument,
>> but the patched win32_langinfo() is giving it a locale identifier
>> ("German_Germany.1252"). At least it does for me.
>
>
> That really depends on what you set in config/commandline. The current code
> supports both (that's why there is the else fallback to old code which
> handles the "German_Germany.1252" format).

Yes, that's the whole point of falling back to the old code path
should the call to GetLocaleInfoEx() fail.

>> As for missing code page information in the _locale_t type, ISTM it
>> isn't hidden any better in UCRT than it was before:
>>
>> int main()
>> {
>>  /* Set locale with nonstandard code page */
>>  _locale_t loc = _create_locale(LC_ALL, "German_Germany.850");
>>
>>  __crt_locale_data_public* pub =
>> (__crt_locale_data_public*)(loc->locinfo);
>>  printf("CP: %d\n", pub->_locale_lc_codepage);// "CP: 850"
>>  return 0;
>> }
>>
>
> This is basically same as the approach of fixing _locale_t that was also
> considered upthread but nobody was too happy with it.

I am one of them to be honest... Now if I look at that with one step
backward at this problem this requires just a ugly hack of a couple of
lines, and this does not require to bump __WIN32_WINNT... Neither does
it need an extra code hunk to handle the short locale name parsing
with GetLocaleInfoEx. We could even just handle _MSC_VER as an
exception, so as it is easy to check with future versions of VS if we
still have lc_codepage going missing:
+#if (_MSC_VER == 1900)
+   __crt_locale_data_public *pub =
+   (__crt_locale_data_public *) loct->locinfo;
+   lc_codepage = pub->_locale_lc_codepage;
+#else
+   lc_codepage = loct->locinfo->lc_codepage;
+#endif

Another thing that I am wondering is that this would allow us to parse
even locale names that are not short, type ja-JP and not with the
COUNTRY_LANG.CODEPAGE format, though based on what I read on the docs
those do not exist.. But the world of Windows is full of surprises.

> I guess the worry is
> that given that the header file is obviously unfinished in the area where
> this is defined and also the fact that this looks like something that's not
> meant to be used outside of that header makes people worry that it might
> change between point releases of the SDK.

Yep.

Now, I have produced two patches:
- 0001-Support-for-VS-2015-locale-hack.patch, which makes use of
__crt_locale_data_public in ucrt/corecrt.h. This is definitely an ugly
hack, though I am coming to think that this may be the approach that
would us the less harm, and that's closer to what is done for VS 2012
and 2013.
- 0001-Support-for-VS-2015-getlocaleinfoex.patch, which make use of
GetLocaleInfoEx, this requires us to lower a bit the support grid for
Windows, basically that's throwing support for XP if compilation is
done with VS 2015.
Based on my tests, both are working with short and long local names,
testing via initdb --locale.
-- 
Michael


0001-Support-for-VS-2015-getlocaleinfoex.patch
Description: binary/octet-stream


0001-Support-for-VS-2015-locale-hack.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Petr Jelinek]

On 10/04/16 20:47, Christian Ullrich wrote:

* Andrew Dunstan:


On 04/09/2016 08:43 AM, Christian Ullrich wrote:



Michael Paquier wrote:



I don't think that's good to use malloc in those code paths, and I


Oh?

+#if (_MSC_VER >= 1900)
+uint32cp;
+
+if (GetLocaleInfoEx(ctype,
+LOCALE_IDEFAULTANSICODEPAGE | LOCALE_RETURN_NUMBER,
+(LPWSTR) &cp, sizeof(cp) / sizeof(WCHAR)) > 0)
+{
+r = malloc(16);/* excess */
+if (r != NULL)
+sprintf(r, "CP%u", cp);
+}
+else
+{
+#endif



But r is return value so it has to be allocated. The intermediate 
variables are function local.



don't think we need to be too precious about saving a byte or two
here. Can one or other of you please prepare a replacement patch based
in this discussion?


Sorry, I don't think I'm up to that (at least not for another week or
so). I have to read up on the issue first; right now I'm not sure what
exactly the problem is.

What I can say is that the existing patch needs more work, because
GetLocaleInfoEx() expects a locale name ("de-DE") as its first argument,
but the patched win32_langinfo() is giving it a locale identifier
("German_Germany.1252"). At least it does for me.


That really depends on what you set in config/commandline. The current 
code supports both (that's why there is the else fallback to old code 
which handles the "German_Germany.1252" format).



As for missing code page information in the _locale_t type, ISTM it
isn't hidden any better in UCRT than it was before:

int main()
{
 /* Set locale with nonstandard code page */
 _locale_t loc = _create_locale(LC_ALL, "German_Germany.850");

 __crt_locale_data_public* pub =
(__crt_locale_data_public*)(loc->locinfo);
 printf("CP: %d\n", pub->_locale_lc_codepage);// "CP: 850"
 return 0;
}



This is basically same as the approach of fixing _locale_t that was also 
considered upthread but nobody was too happy with it. I guess the worry 
is that given that the header file is obviously unfinished in the area 
where this is defined and also the fact that this looks like something 
that's not meant to be used outside of that header makes people worry 
that it might change between point releases of the SDK.


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Andrew Dunstan:


On 04/09/2016 08:43 AM, Christian Ullrich wrote:



Michael Paquier wrote:



I don't think that's good to use malloc in those code paths, and I


Oh?

+#if (_MSC_VER >= 1900)
+   uint32  cp;
+
+   if (GetLocaleInfoEx(ctype,
+   LOCALE_IDEFAULTANSICODEPAGE | LOCALE_RETURN_NUMBER,
+   (LPWSTR) &cp, sizeof(cp) / sizeof(WCHAR)) > 0)
+   {
+   r = malloc(16); /* excess */
+   if (r != NULL)
+   sprintf(r, "CP%u", cp);
+   }
+   else
+   {
+#endif


I don't think we need to be too precious about saving a byte or two
here. Can one or other of you please prepare a replacement patch based
in this discussion?


Sorry, I don't think I'm up to that (at least not for another week or 
so). I have to read up on the issue first; right now I'm not sure what 
exactly the problem is.


What I can say is that the existing patch needs more work, because 
GetLocaleInfoEx() expects a locale name ("de-DE") as its first argument, 
but the patched win32_langinfo() is giving it a locale identifier 
("German_Germany.1252"). At least it does for me.


I have not gone through the code sufficiently closely to find out where 
the argument to win32_langinfo() originates, but I will when I can.


As for missing code page information in the _locale_t type, ISTM it 
isn't hidden any better in UCRT than it was before:


int main()
{
/* Set locale with nonstandard code page */
_locale_t loc = _create_locale(LC_ALL, "German_Germany.850");

__crt_locale_data_public* pub = 
(__crt_locale_data_public*)(loc->locinfo);

printf("CP: %d\n", pub->_locale_lc_codepage);  // "CP: 850"
return 0;
}

--
Christian



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/09/2016 08:43 AM, Christian Ullrich wrote:

Michael Paquier wrote:

On Sat, Apr 9, 2016 at 7:41 AM, Michael Paquier
 wrote:

On Sat, Apr 9, 2016 at 1:46 AM, Christian Ullrich  wrote:

* Andrew Dunstan wrote:

On 04/08/2016 11:02 AM, Christian Ullrich wrote:
  src/port/chklocale.c(233): warning C4133: 'function': incompatible
  types - from 'const char *' to 'LPCWSTR' [...\postgres.vcxproj]

Do you have a fix for the LPCWSTR parameter issue?

As long as the locale short name cannot contain characters outside of ASCII,
and I don't see how it could, just the typical measure-allocate-convert
dance, add error handling to taste:

int res = MultiByteToWideChar(CP_ACP, 0, ctype, -1, NULL, 0);
WCHAR *wctype = malloc(res * sizeof(WCHAR));
memset(wctype, 0, res * sizeof(WCHAR));
res = MultiByteToWideChar(CP_ACP, 0, ctype, -1, wctype, wctypelen);

I don't think that's good to use malloc in those code paths, and I
think that we cannot use palloc as well for a buffer passed directly
into this function, so it looks that we had better use a fix-sized
buffer and allocate the output into that. What would be a a correct
estimation of the maximum size we should allow? 80 (similar to
pg_locale.c)?

I think it should not take more than 16 characters, but if you want to make 
sure the code can deal with long names as well, MSDN gives an upper limit of 85 
for those somewhere.




I don't think we need to be too precious about saving a byte or two 
here. Can one or other of you please prepare a replacement patch based 
in this discussion?


cheers

andrew



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

> Michael Paquier wrote:
> 
> On Sat, Apr 9, 2016 at 7:41 AM, Michael Paquier
>  wrote:
>> On Sat, Apr 9, 2016 at 1:46 AM, Christian Ullrich  
>> wrote:
>>> * Andrew Dunstan wrote:
> On 04/08/2016 11:02 AM, Christian Ullrich wrote:
>  src/port/chklocale.c(233): warning C4133: 'function': incompatible
>  types - from 'const char *' to 'LPCWSTR' [...\postgres.vcxproj]
>>> 
 Do you have a fix for the LPCWSTR parameter issue?
>>> 
>>> As long as the locale short name cannot contain characters outside of ASCII,
>>> and I don't see how it could, just the typical measure-allocate-convert
>>> dance, add error handling to taste:
>>> 
>>> int res = MultiByteToWideChar(CP_ACP, 0, ctype, -1, NULL, 0);
>>> WCHAR *wctype = malloc(res * sizeof(WCHAR));
>>> memset(wctype, 0, res * sizeof(WCHAR));
>>> res = MultiByteToWideChar(CP_ACP, 0, ctype, -1, wctype, wctypelen);
> 
> I don't think that's good to use malloc in those code paths, and I
> think that we cannot use palloc as well for a buffer passed directly
> into this function, so it looks that we had better use a fix-sized
> buffer and allocate the output into that. What would be a a correct
> estimation of the maximum size we should allow? 80 (similar to
> pg_locale.c)?

I think it should not take more than 16 characters, but if you want to make 
sure the code can deal with long names as well, MSDN gives an upper limit of 85 
for those somewhere. 

-- 
Christian

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Sat, Apr 9, 2016 at 7:41 AM, Michael Paquier
 wrote:
> On Sat, Apr 9, 2016 at 1:46 AM, Christian Ullrich  
> wrote:
>> * Andrew Dunstan wrote:
>>> On 04/08/2016 11:02 AM, Christian Ullrich wrote:
   src/port/chklocale.c(233): warning C4133: 'function': incompatible
   types - from 'const char *' to 'LPCWSTR' [...\postgres.vcxproj]
>>
>>> Do you have a fix for the LPCWSTR parameter issue?
>>
>> As long as the locale short name cannot contain characters outside of ASCII,
>> and I don't see how it could, just the typical measure-allocate-convert
>> dance, add error handling to taste:
>>
>> int res = MultiByteToWideChar(CP_ACP, 0, ctype, -1, NULL, 0);
>> WCHAR *wctype = malloc(res * sizeof(WCHAR));
>> memset(wctype, 0, res * sizeof(WCHAR));
>> res = MultiByteToWideChar(CP_ACP, 0, ctype, -1, wctype, wctypelen);

I don't think that's good to use malloc in those code paths, and I
think that we cannot use palloc as well for a buffer passed directly
into this function, so it looks that we had better use a fix-sized
buffer and allocate the output into that. What would be a a correct
estimation of the maximum size we should allow? 80 (similar to
pg_locale.c)?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Petr Jelinek]

On 09/04/16 00:41, Michael Paquier wrote:

On Sat, Apr 9, 2016 at 1:46 AM, Christian Ullrich  wrote:

* Andrew Dunstan wrote:


On 04/08/2016 11:02 AM, Christian Ullrich wrote:




   src/port/chklocale.c(233): warning C4133: 'function': incompatible
   types - from 'const char *' to 'LPCWSTR' [...\postgres.vcxproj]




Do you have a fix for the LPCWSTR parameter issue?



As long as the locale short name cannot contain characters outside of ASCII,
and I don't see how it could, just the typical measure-allocate-convert
dance, add error handling to taste:

int res = MultiByteToWideChar(CP_ACP, 0, ctype, -1, NULL, 0);
WCHAR *wctype = malloc(res * sizeof(WCHAR));
memset(wctype, 0, res * sizeof(WCHAR));
res = MultiByteToWideChar(CP_ACP, 0, ctype, -1, wctype, wctypelen);

If it is somehow guaranteed that ctype is only the most basic short name
("xx-YY") with no code pages or anything, it becomes much simpler, of
course, and I would just use a loop.

If the locale name can contain characters above 0x7f, we'd have to know the
code page of the string we use to get the code page.


Could somebody give a try instead of me? I could take a look on it,
but just in 12 hours or so, aka after the deadline if that matters for
this patch.



I won't be able to get back to it (well mainly to windows environment) 
till Thursday due to travel, sorry.


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Sat, Apr 9, 2016 at 1:46 AM, Christian Ullrich  wrote:
> * Andrew Dunstan wrote:
>
>> On 04/08/2016 11:02 AM, Christian Ullrich wrote:
>
>
>>>   src/port/chklocale.c(233): warning C4133: 'function': incompatible
>>>   types - from 'const char *' to 'LPCWSTR' [...\postgres.vcxproj]
>
>
>> Do you have a fix for the LPCWSTR parameter issue?
>
>
> As long as the locale short name cannot contain characters outside of ASCII,
> and I don't see how it could, just the typical measure-allocate-convert
> dance, add error handling to taste:
>
> int res = MultiByteToWideChar(CP_ACP, 0, ctype, -1, NULL, 0);
> WCHAR *wctype = malloc(res * sizeof(WCHAR));
> memset(wctype, 0, res * sizeof(WCHAR));
> res = MultiByteToWideChar(CP_ACP, 0, ctype, -1, wctype, wctypelen);
>
> If it is somehow guaranteed that ctype is only the most basic short name
> ("xx-YY") with no code pages or anything, it becomes much simpler, of
> course, and I would just use a loop.
>
> If the locale name can contain characters above 0x7f, we'd have to know the
> code page of the string we use to get the code page.

Could somebody give a try instead of me? I could take a look on it,
but just in 12 hours or so, aka after the deadline if that matters for
this patch.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Andrew Dunstan wrote:


On 04/08/2016 11:02 AM, Christian Ullrich wrote:



  src/port/chklocale.c(233): warning C4133: 'function': incompatible
  types - from 'const char *' to 'LPCWSTR' [...\postgres.vcxproj]



Do you have a fix for the LPCWSTR parameter issue?


As long as the locale short name cannot contain characters outside of 
ASCII, and I don't see how it could, just the typical 
measure-allocate-convert dance, add error handling to taste:


int res = MultiByteToWideChar(CP_ACP, 0, ctype, -1, NULL, 0);
WCHAR *wctype = malloc(res * sizeof(WCHAR));
memset(wctype, 0, res * sizeof(WCHAR));
res = MultiByteToWideChar(CP_ACP, 0, ctype, -1, wctype, wctypelen);

If it is somehow guaranteed that ctype is only the most basic short name 
("xx-YY") with no code pages or anything, it becomes much simpler, of 
course, and I would just use a loop.


If the locale name can contain characters above 0x7f, we'd have to know 
the code page of the string we use to get the code page.


--
Christian



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/08/2016 11:02 AM, Christian Ullrich wrote:

* Michael Paquier wrote:

On Fri, Apr 8, 2016 at 10:05 PM, Andrew Dunstan  
wrote:

¥> On 04/08/2016 07:15 AM, Christian Ullrich wrote:


Michael, none of your patches change this, so how does it ever 
build on

your system?


Luck. I am getting a warning but the code is able to somewhat compile:
   src/port/chklocale.c(230): warning C4013: 'GetLocaleInfoEx'
undefined; assuming extern returning int
[C:\Users\IEUser\git\postgres\libpgport.vcxproj]


Weird. This assumed declaration is __cdecl, the actual function is 
__stdcall, and I think this should be guaranteed to crash.



But that's clearly incorrect to get that. As you are saying, what we
actually just need to do is bumping _WIN32_WINNT to 0x0600 when
compiling with VS2015, meaning that the minimum build requirement for
Postgres with VS2015 would be Windows Vista, and it would not be
possible to compile it on XP or Windows server 2k3. As XP is already
out of support, I think that this is an acceptable tradeoff, and it
would still be possible to build Postgres on XP with older versions of
Visual. Thoughts?


I think you confuse two things here, let's call them "build 
environment" and "build platform". The build environment is whichever 
Windows SDK (among other things) is installed; if it is a version for 
Vista or later, that just means it has the declaration in the first 
place, and has the import in kernel32.lib. The build platform is the 
OS the compiler is run on; as long as you find a compiler that 
understands the headers from your chosen SDK version, you can run it 
on Windows 95 if both of you want.


Changing _WIN32_WINNT also affects, indirectly, on which platforms the 
resulting binaries can run. Assume a macro that has an alternative 
definition, conditioned on _WIN32_WINNT >= _WIN32_WINNT_VISTA, that 
uses a function added in Vista. A binary built using this declaration, 
no matter where and when, will not run on anything older.



Anyway, attached are updated patches. This makes the warning go away
from my side, so I guess that it should allow Andrew to compile the
code.


Which brings us to the next problem:

  src/port/chklocale.c(233): warning C4133: 'function': incompatible
  types - from 'const char *' to 'LPCWSTR' [...\postgres.vcxproj]

I have no idea why I get this warning; I would have expected something 
more like this:


  localetest.cpp(26): error C2664: 'int
  GetLocaleInfoEx(LPCWSTR,LCTYPE,LPWSTR,int)': cannot convert
  argument 1 from 'const char *' to 'LPCWSTR'

Apparently the warning is triggered by type mismatches in pointer 
arithmetic, although I can't see any here. Anyway, it concerns the 
first argument in this call to GetLocaleInfoEx(), which here is a 
const char*.


According to the documentation and the prototype, however, it should 
be an LPCWSTR, because this function is Unicode-only (has no A/W 
variants). Unless LOCALE_IDEFAULTANSICODEPAGE also changes the 
interpretation of this first argument to a single-byte string, which 
is not mentioned anywhere in the documentation and makes no sense to 
begin with, I don't think this has ever worked either.


I just tested it, and, of course, if I pass '(LPCWSTR)"de-DE"' (narrow 
string cast to LPCWSTR), the call fails with ERROR_INVALID_PARAMETER. 
With a wide string, I get the correct code page for the locale.



Also, while you're at it, could you improve the comments a bit? I have 
not yet tried following the code to see which locale formats it uses 
where ("German_Germany", "de-DE", etc.), but GetLocaleInfoEx() takes 
the short form and there is a comment about the long form right below 
that call once patched (in the old code that gets turned into an else 
branch).






OK, well, we're making progress. I can confirm that using _WIN32_WINNT = 
0x0600 fixes my problems - I can build and run the regression tests. I'm 
inclined to define _WINSOCK_DEPRECATED_NO_WARNINGS to silence a few 
compiler bleatings.


Do you have a fix for the LPCWSTR parameter issue?

cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Michael Paquier wrote:


On Fri, Apr 8, 2016 at 10:05 PM, Andrew Dunstan  wrote:
¥> On 04/08/2016 07:15 AM, Christian Ullrich wrote:



Michael, none of your patches change this, so how does it ever build on
your system?


Luck. I am getting a warning but the code is able to somewhat compile:
   src/port/chklocale.c(230): warning C4013: 'GetLocaleInfoEx'
undefined; assuming extern returning int
[C:\Users\IEUser\git\postgres\libpgport.vcxproj]


Weird. This assumed declaration is __cdecl, the actual function is 
__stdcall, and I think this should be guaranteed to crash.



But that's clearly incorrect to get that. As you are saying, what we
actually just need to do is bumping _WIN32_WINNT to 0x0600 when
compiling with VS2015, meaning that the minimum build requirement for
Postgres with VS2015 would be Windows Vista, and it would not be
possible to compile it on XP or Windows server 2k3. As XP is already
out of support, I think that this is an acceptable tradeoff, and it
would still be possible to build Postgres on XP with older versions of
Visual. Thoughts?


I think you confuse two things here, let's call them "build environment" 
and "build platform". The build environment is whichever Windows SDK 
(among other things) is installed; if it is a version for Vista or 
later, that just means it has the declaration in the first place, and 
has the import in kernel32.lib. The build platform is the OS the 
compiler is run on; as long as you find a compiler that understands the 
headers from your chosen SDK version, you can run it on Windows 95 if 
both of you want.


Changing _WIN32_WINNT also affects, indirectly, on which platforms the 
resulting binaries can run. Assume a macro that has an alternative 
definition, conditioned on _WIN32_WINNT >= _WIN32_WINNT_VISTA, that uses 
a function added in Vista. A binary built using this declaration, no 
matter where and when, will not run on anything older.



Anyway, attached are updated patches. This makes the warning go away
from my side, so I guess that it should allow Andrew to compile the
code.


Which brings us to the next problem:

  src/port/chklocale.c(233): warning C4133: 'function': incompatible
  types - from 'const char *' to 'LPCWSTR' [...\postgres.vcxproj]

I have no idea why I get this warning; I would have expected something 
more like this:


  localetest.cpp(26): error C2664: 'int
  GetLocaleInfoEx(LPCWSTR,LCTYPE,LPWSTR,int)': cannot convert
  argument 1 from 'const char *' to 'LPCWSTR'

Apparently the warning is triggered by type mismatches in pointer 
arithmetic, although I can't see any here. Anyway, it concerns the first 
argument in this call to GetLocaleInfoEx(), which here is a const char*.


According to the documentation and the prototype, however, it should be 
an LPCWSTR, because this function is Unicode-only (has no A/W variants). 
Unless LOCALE_IDEFAULTANSICODEPAGE also changes the interpretation of 
this first argument to a single-byte string, which is not mentioned 
anywhere in the documentation and makes no sense to begin with, I don't 
think this has ever worked either.


I just tested it, and, of course, if I pass '(LPCWSTR)"de-DE"' (narrow 
string cast to LPCWSTR), the call fails with ERROR_INVALID_PARAMETER. 
With a wide string, I get the correct code page for the locale.



Also, while you're at it, could you improve the comments a bit? I have 
not yet tried following the code to see which locale formats it uses 
where ("German_Germany", "de-DE", etc.), but GetLocaleInfoEx() takes the 
short form and there is a comment about the long form right below that 
call once patched (in the old code that gets turned into an else branch).


--
Christian



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Magnus Hagander]

On Fri, Apr 8, 2016 at 4:12 PM, Michael Paquier 
wrote:

> On Fri, Apr 8, 2016 at 10:05 PM, Andrew Dunstan 
> wrote:
> ¥> On 04/08/2016 07:15 AM, Christian Ullrich wrote:
> >> GetLocaleInfoEx() is covered by #if (WINVER >= 0x0600), we define
> >> _WIN32_WINNT as 0x0501 (src/include/port/win32.h) and WINVER inherits
> that
> >> value.
>
> Yes, I had a look at winnls.h and that's true.
>
> >> Michael, none of your patches change this, so how does it ever build on
> >> your system?
>
> Luck. I am getting a warning but the code is able to somewhat compile:
>   src/port/chklocale.c(230): warning C4013: 'GetLocaleInfoEx'
> undefined; assuming extern returning int
> [C:\Users\IEUser\git\postgres\libpgport.vcxproj]
> But that's clearly incorrect to get that. As you are saying, what we
> actually just need to do is bumping _WIN32_WINNT to 0x0600 when
> compiling with VS2015, meaning that the minimum build requirement for
> Postgres with VS2015 would be Windows Vista, and it would not be
> possible to compile it on XP or Windows server 2k3. As XP is already
> out of support, I think that this is an acceptable tradeoff, and it
> would still be possible to build Postgres on XP with older versions of
> Visual. Thoughts?
>

I think that's easily acceptable for the build system, as long as we
document it clearly.

I think it would probalby be a bad idea to use those binaries in our binary
installers though, but that's a different discussion really.  We have other
compiler flags that we don't recommend for that :)


//Magnus

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Fri, Apr 8, 2016 at 10:05 PM, Andrew Dunstan  wrote:
¥> On 04/08/2016 07:15 AM, Christian Ullrich wrote:
>> GetLocaleInfoEx() is covered by #if (WINVER >= 0x0600), we define
>> _WIN32_WINNT as 0x0501 (src/include/port/win32.h) and WINVER inherits that
>> value.

Yes, I had a look at winnls.h and that's true.

>> Michael, none of your patches change this, so how does it ever build on
>> your system?

Luck. I am getting a warning but the code is able to somewhat compile:
  src/port/chklocale.c(230): warning C4013: 'GetLocaleInfoEx'
undefined; assuming extern returning int
[C:\Users\IEUser\git\postgres\libpgport.vcxproj]
But that's clearly incorrect to get that. As you are saying, what we
actually just need to do is bumping _WIN32_WINNT to 0x0600 when
compiling with VS2015, meaning that the minimum build requirement for
Postgres with VS2015 would be Windows Vista, and it would not be
possible to compile it on XP or Windows server 2k3. As XP is already
out of support, I think that this is an acceptable tradeoff, and it
would still be possible to build Postgres on XP with older versions of
Visual. Thoughts?

> OK, at this stage it appears to me that if today is the deadline for getting
> this in for 9.6 then it's going to be missed.

Really? I thought that new VS support were done on HEAD and the last
stable version, aka 9.5 here. This was what was done previously for
VS2012 and VS2013.

> And no, Michael's suggested inclusion didn't help, probably for the reasons
> Christian suggests.

Yes.

List of _WIN32_WINNT is here:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745%28v=vs.85%29.aspx
And here is the compatibility grid of VS2015 with existing Windows OSes:
https://www.visualstudio.com/en-us/products/visual-studio-2015-compatibility-vs.aspx

Anyway, attached are updated patches. This makes the warning go away
from my side, so I guess that it should allow Andrew to compile the
code.
-- 
Michael


0001-Add-support-for-VS-2015-in-MSVC-scripts.patch
Description: invalid/octet-stream


0002-Fix-code-page-calculation-for-Visual-Studio-2015.patch
Description: invalid/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/08/2016 09:52 AM, Tom Lane wrote:

Andrew Dunstan  writes:

OK, at this stage it appears to me that if today is the deadline for
getting this in for 9.6 then it's going to be missed.

It would be unfortunate to go another year without support for that
toolchain.  My suggestion is that you continue to work away on the
problem, and when you have something that passes your testing, you
present it to -hackers and we can decide then whether it's too invasive
for a post-feature-freeze patch.





OK. Michael just told me in IM that he thinks he has a fix, so we'll see.

cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Tom Lane]

Andrew Dunstan  writes:
> OK, at this stage it appears to me that if today is the deadline for 
> getting this in for 9.6 then it's going to be missed.

It would be unfortunate to go another year without support for that
toolchain.  My suggestion is that you continue to work away on the
problem, and when you have something that passes your testing, you
present it to -hackers and we can decide then whether it's too invasive
for a post-feature-freeze patch.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/08/2016 07:15 AM, Christian Ullrich wrote:

* Michael Paquier wrote:

> On Fri, Apr 8, 2016 at 9:29 AM, Andrew Dunstan  
> wrote:


Not out of the woods yet. Attached is what I got from VS2015 on a 
fresh W10

VM, with Michael's patch 0002 and 0004 applied.


Interesting, I have no idea what we are doing differently, and seeing
those errors it seems to me that Petr and I are actually getting it
wrong for some reason, because GetLocaleInfoEx should need a direct
declaration of windows.h. And similarly to some of the other files in
src/port I think that we should have it. Do you still get failures if
you add the following thing at the top of chklocale.c?

#if defined(WIN32) && (_MSC_VER >= 1900)
#include "windows.h"
#endif

Looking at the docs
(https://msdn.microsoft.com/en-us/library/windows/desktop/dd318103%28v=vs.85%29.aspx), 


winnls.h would be enough, but I'd rather be consistent with the
approach taken by the other files.


GetLocaleInfoEx() is covered by #if (WINVER >= 0x0600), we define 
_WIN32_WINNT as 0x0501 (src/include/port/win32.h) and WINVER inherits 
that value.


Michael, none of your patches change this, so how does it ever build 
on your system?


MSDN also says this about the function: "Header: winnls.h (include 
windows.h)"; so there's probably something else needed to make it work 
that is not in  alone.



OK, at this stage it appears to me that if today is the deadline for 
getting this in for 9.6 then it's going to be missed.


And no, Michael's suggested inclusion didn't help, probably for the 
reasons Christian suggests.


cheers

andrew




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Michael Paquier wrote:

> On Fri, Apr 8, 2016 at 9:29 AM, Andrew Dunstan  
> wrote:



Not out of the woods yet. Attached is what I got from VS2015 on a fresh W10
VM, with Michael's patch 0002 and 0004 applied.


Interesting, I have no idea what we are doing differently, and seeing
those errors it seems to me that Petr and I are actually getting it
wrong for some reason, because GetLocaleInfoEx should need a direct
declaration of windows.h. And similarly to some of the other files in
src/port I think that we should have it. Do you still get failures if
you add the following thing at the top of chklocale.c?

#if defined(WIN32) && (_MSC_VER >= 1900)
#include "windows.h"
#endif

Looking at the docs
(https://msdn.microsoft.com/en-us/library/windows/desktop/dd318103%28v=vs.85%29.aspx),
winnls.h would be enough, but I'd rather be consistent with the
approach taken by the other files.


GetLocaleInfoEx() is covered by #if (WINVER >= 0x0600), we define 
_WIN32_WINNT as 0x0501 (src/include/port/win32.h) and WINVER inherits 
that value.


Michael, none of your patches change this, so how does it ever build on 
your system?


MSDN also says this about the function: "Header: winnls.h (include 
windows.h)"; so there's probably something else needed to make it work 
that is not in  alone.


--
Christian



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Fri, Apr 8, 2016 at 9:29 AM, Andrew Dunstan  wrote:
> Not out of the woods yet. Attached is what I got from VS2015 on a fresh W10
> VM, with Michael's patch 0002 and 0004 applied.

Interesting, I have no idea what we are doing differently, and seeing
those errors it seems to me that Petr and I are actually getting it
wrong for some reason, because GetLocaleInfoEx should need a direct
declaration of windows.h. And similarly to some of the other files in
src/port I think that we should have it. Do you still get failures if
you add the following thing at the top of chklocale.c?

#if defined(WIN32) && (_MSC_VER >= 1900)
#include "windows.h"
#endif

Looking at the docs
(https://msdn.microsoft.com/en-us/library/windows/desktop/dd318103%28v=vs.85%29.aspx),
winnls.h would be enough, but I'd rather be consistent with the
approach taken by the other files.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/06/2016 04:50 PM, Andrew Dunstan wrote:



On 03/29/2016 09:38 PM, Robert Haas wrote:
On Tue, Mar 29, 2016 at 9:29 PM, Andrew Dunstan  
wrote:
I am currently travelling, but my intention is to deal with the 
remaining

patches when I'm back home this weekend, unless someone beats me to it.

Cool.



Progress report:


I have spent way too much time on this and don't have it working yet. 
I'm setting up a sacrificial VM from scratch in a last ditch attempt 
to get it working.


Things to note so far:

 * VS2015 appears to create version 12 solution files, not version 14,
   and the tools complained about version 14.
 * Windows git (the successor to msysGit) apparently no longer ships
   with bison and flex. So if you need those (i.e. to built from git,
   not tarball) you're probably going to need to install the MsysDTK
   even if you're not using its compiler.






Not out of the woods yet. Attached is what I got from VS2015 on a fresh 
W10 VM, with Michael's patch 0002 and 0004 applied.



cheers

andrew
"C:\prog\postgresql\pgsql.sln" (default target) (1) ->
"C:\prog\postgresql\zic.vcxproj" (default target) (2) ->
"C:\prog\postgresql\libpgport.vcxproj" (default target) (4) ->
(ClCompile target) ->
  src/port/chklocale.c(230): warning C4013: 'GetLocaleInfoEx' undefined; 
assuming extern returning int [C:\prog\postgre
sql\libpgport.vcxproj]
  src/port/thread.c(134): warning C4996: 'gethostbyname': Use getaddrinfo() or 
GetAddrInfoW() instead or define _WINSOC
K_DEPRECATED_NO_WARNINGS to disable deprecated API warnings 
[C:\prog\postgresql\libpgport.vcxproj]


"C:\prog\postgresql\pgsql.sln" (default target) (1) ->
"C:\prog\postgresql\ascii_and_mic.vcxproj" (default target) (5) ->
"C:\prog\postgresql\postgres.vcxproj" (default target) (6) ->
  src/backend/libpq/ip.c(476): warning C4996: 'WSASocketA': Use WSASocketW() 
instead or define _WINSOCK_DEPRECATED_NO_W
ARNINGS to disable deprecated API warnings [C:\prog\postgresql\postgres.vcxproj]
  C:\Program Files (x86)\Windows Kits\8.1\Include\um\dbghelp.h(1544): warning 
C4091: 'typedef ': ignored on left of ''
when no variable is declared [C:\prog\postgresql\postgres.vcxproj]
  C:\Program Files (x86)\Windows Kits\8.1\Include\um\dbghelp.h(3190): warning 
C4091: 'typedef ': ignored on left of ''
when no variable is declared [C:\prog\postgresql\postgres.vcxproj]
  src/backend/port/win32/socket.c(247): warning C4996: 'WSASocketA': Use 
WSASocketW() instead or define _WINSOCK_DEPREC
ATED_NO_WARNINGS to disable deprecated API warnings 
[C:\prog\postgresql\postgres.vcxproj]
  src/backend/postmaster/postmaster.c(5892): warning C4996: 
'WSADuplicateSocketA': Use WSADuplicateSocketW() instead or
 define _WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings 
[C:\prog\postgresql\postgres.vcxproj]
  src/backend/postmaster/postmaster.c(5919): warning C4996: 'WSASocketA': Use 
WSASocketW() instead or define _WINSOCK_D
EPRECATED_NO_WARNINGS to disable deprecated API warnings 
[C:\prog\postgresql\postgres.vcxproj]
  src/port/chklocale.c(230): warning C4013: 'GetLocaleInfoEx' undefined; 
assuming extern returning int [C:\prog\postgre
sql\postgres.vcxproj]
  src/port/getaddrinfo.c(199): warning C4996: 'gethostbyname': Use 
getaddrinfo() or GetAddrInfoW() instead or define _W
INSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings 
[C:\prog\postgresql\postgres.vcxproj]
  src/port/thread.c(134): warning C4996: 'gethostbyname': Use getaddrinfo() or 
GetAddrInfoW() instead or define _WINSOC
K_DEPRECATED_NO_WARNINGS to disable deprecated API warnings 
[C:\prog\postgresql\postgres.vcxproj]


"C:\prog\postgresql\pgsql.sln" (default target) (1) ->
"C:\prog\postgresql\isolationtester.vcxproj" (default target) (34) ->
"C:\prog\postgresql\libpq.vcxproj" (default target) (35) ->
  src/backend/libpq/ip.c(476): warning C4996: 'WSASocketA': Use WSASocketW() 
instead or define _WINSOCK_DEPRECATED_NO_W
ARNINGS to disable deprecated API warnings [C:\prog\postgresql\libpq.vcxproj]
  src/port/chklocale.c(230): warning C4013: 'GetLocaleInfoEx' undefined; 
assuming extern returning int [C:\prog\postgre
sql\libpq.vcxproj]
  src/port/thread.c(134): warning C4996: 'gethostbyname': Use getaddrinfo() or 
GetAddrInfoW() instead or define _WINSOC
K_DEPRECATED_NO_WARNINGS to disable deprecated API warnings 
[C:\prog\postgresql\libpq.vcxproj]


"C:\prog\postgresql\pgsql.sln" (default target) (1) ->
"C:\prog\postgresql\ascii_and_mic.vcxproj" (default target) (5) ->
"C:\prog\postgresql\postgres.vcxproj" (default target) (6) ->
(Link target) ->
  chklocale.obj : error LNK2019: unresolved external symbol _GetLocaleInfoEx 
referenced in function _win32_langinfo [C:
\prog\postgresql\postgres.vcxproj]
  .\Release\postgres\postgres.exe : fatal error LNK1120: 1 unresolved externals 
[C:\prog\postgresql\postgres.vcxproj]


"C:\prog\postgresql\pgsql.sln" (default target) (1) ->
"C:\prog\postgresql\isolationtester.vcxproj" (default target) (34) ->
"C:\prog\postgresql\libpq.vcxproj" (default 

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Petr Jelinek wrote:


On 07/04/16 00:50, Michael Paquier wrote:

On Thu, Apr 7, 2016 at 7:44 AM, Michael Paquier
 wrote:

On Thu, Apr 7, 2016 at 6:11 AM, Petr Jelinek 
wrote:

On 06/04/16 22:50, Andrew Dunstan wrote:



   * VS2015 appears to create version 12 solution files, not
 version 14, and the tools complained about version 14.


The "14" is the toolset version, i.e. the Visual Studio 2015 C/C++ 
compiler; this number appears in the .vcxproj files. The "12" is the 
file format version of the solution (.sln) files.


There are quite a few version numbers involved. Creating and building a 
C project in VS 2015 involves a solution file of version 12 referencing
a project file whose only version number is the 2003 in the XML 
namespace URL, feeding it to MSBuild version 14, which then invokes the 
compiler (in version 19). And then there is the  
element, where the 14 is repeated as "v140", at least, I think it is the 
same number.


--
Christian



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Thu, Apr 7, 2016 at 7:44 AM, Michael Paquier
 wrote:
> On Thu, Apr 7, 2016 at 6:11 AM, Petr Jelinek  wrote:
>> On 06/04/16 22:50, Andrew Dunstan wrote:
>>> I have spent way too much time on this and don't have it working yet.
>>> I'm setting up a sacrificial VM from scratch in a last ditch attempt to
>>> get it working.
>>>
>>> Things to note so far:
>>>
>>>   * VS2015 appears to create version 12 solution files, not version 14,
>>> and the tools complained about version 14.
>>>   * Windows git (the successor to msysGit) apparently no longer ships
>>> with bison and flex. So if you need those (i.e. to built from git,
>>> not tarball) you're probably going to need to install the MsysDTK
>>> even if you're not using its compiler.
>>
>> It's fun to set it up yes. I do have the machine with buildfarm client ready
>> still (although now also traveling so slightly complicated to get to it) but
>> I didn't activate it yet as I don't want it to just report failures forever.
>
> Petr, did you actually try the patches I sent? I did my tests using
> the community version of Visual Studio 2015 and a full install of it.
> If I am the only able to make those working, well we surely have a
> problem captain.

By the way, if I look at the vcxproj files generated in my case, those
are correctly with Tools=14.0 or PlatformToolSet=v140.. Perhaps not
using Win10 differs in the way things are generated.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Petr Jelinek]

On 07/04/16 00:50, Michael Paquier wrote:

On Thu, Apr 7, 2016 at 7:44 AM, Michael Paquier
 wrote:

On Thu, Apr 7, 2016 at 6:11 AM, Petr Jelinek  wrote:

On 06/04/16 22:50, Andrew Dunstan wrote:

I have spent way too much time on this and don't have it working yet.
I'm setting up a sacrificial VM from scratch in a last ditch attempt to
get it working.

Things to note so far:

   * VS2015 appears to create version 12 solution files, not version 14,
 and the tools complained about version 14.
   * Windows git (the successor to msysGit) apparently no longer ships
 with bison and flex. So if you need those (i.e. to built from git,
 not tarball) you're probably going to need to install the MsysDTK
 even if you're not using its compiler.


It's fun to set it up yes. I do have the machine with buildfarm client ready
still (although now also traveling so slightly complicated to get to it) but
I didn't activate it yet as I don't want it to just report failures forever.


Petr, did you actually try the patches I sent? I did my tests using
the community version of Visual Studio 2015 and a full install of it.
If I am the only able to make those working, well we surely have a
problem captain.


By the way, if I look at the vcxproj files generated in my case, those
are correctly with Tools=14.0 or PlatformToolSet=v140.. Perhaps not
using Win10 differs in the way things are generated.



I have community edition on win10 as well, worked fine there yes, I used 
just the command-line tools from it.


VS2015 creates version 12 solution only before the patches are applied 
for me, once they are applied it works fine.


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Thu, Apr 7, 2016 at 6:11 AM, Petr Jelinek  wrote:
> On 06/04/16 22:50, Andrew Dunstan wrote:
>> I have spent way too much time on this and don't have it working yet.
>> I'm setting up a sacrificial VM from scratch in a last ditch attempt to
>> get it working.
>>
>> Things to note so far:
>>
>>   * VS2015 appears to create version 12 solution files, not version 14,
>> and the tools complained about version 14.
>>   * Windows git (the successor to msysGit) apparently no longer ships
>> with bison and flex. So if you need those (i.e. to built from git,
>> not tarball) you're probably going to need to install the MsysDTK
>> even if you're not using its compiler.
>
> It's fun to set it up yes. I do have the machine with buildfarm client ready
> still (although now also traveling so slightly complicated to get to it) but
> I didn't activate it yet as I don't want it to just report failures forever.

Petr, did you actually try the patches I sent? I did my tests using
the community version of Visual Studio 2015 and a full install of it.
If I am the only able to make those working, well we surely have a
problem captain.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Alvaro Herrera]

Petr Jelinek wrote:

> It's fun to set it up yes. I do have the machine with buildfarm client ready
> still (although now also traveling so slightly complicated to get to it) but
> I didn't activate it yet as I don't want it to just report failures forever.

Maybe you should just activate it regardless, and we'll see it go from
red to green once things are good.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Petr Jelinek]

On 06/04/16 22:50, Andrew Dunstan wrote:



On 03/29/2016 09:38 PM, Robert Haas wrote:

On Tue, Mar 29, 2016 at 9:29 PM, Andrew Dunstan 
wrote:

I am currently travelling, but my intention is to deal with the
remaining
patches when I'm back home this weekend, unless someone beats me to it.

Cool.



Progress report:


I have spent way too much time on this and don't have it working yet.
I'm setting up a sacrificial VM from scratch in a last ditch attempt to
get it working.

Things to note so far:

  * VS2015 appears to create version 12 solution files, not version 14,
and the tools complained about version 14.
  * Windows git (the successor to msysGit) apparently no longer ships
with bison and flex. So if you need those (i.e. to built from git,
not tarball) you're probably going to need to install the MsysDTK
even if you're not using its compiler.



It's fun to set it up yes. I do have the machine with buildfarm client 
ready still (although now also traveling so slightly complicated to get 
to it) but I didn't activate it yet as I don't want it to just report 
failures forever.


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 03/29/2016 09:38 PM, Robert Haas wrote:

On Tue, Mar 29, 2016 at 9:29 PM, Andrew Dunstan  wrote:

I am currently travelling, but my intention is to deal with the remaining
patches when I'm back home this weekend, unless someone beats me to it.

Cool.



Progress report:


I have spent way too much time on this and don't have it working yet. 
I'm setting up a sacrificial VM from scratch in a last ditch attempt to 
get it working.


Things to note so far:

 * VS2015 appears to create version 12 solution files, not version 14,
   and the tools complained about version 14.
 * Windows git (the successor to msysGit) apparently no longer ships
   with bison and flex. So if you need those (i.e. to built from git,
   not tarball) you're probably going to need to install the MsysDTK
   even if you're not using its compiler.

cheers

andrew



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Robert Haas]

On Tue, Mar 29, 2016 at 9:29 PM, Andrew Dunstan  wrote:
> I am currently travelling, but my intention is to deal with the remaining
> patches when I'm back home this weekend, unless someone beats me to it.

Cool.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 03/29/2016 08:48 PM, Michael Paquier wrote:

On Wed, Mar 30, 2016 at 12:45 AM, Robert Haas  wrote:

On Tue, Mar 29, 2016 at 11:31 AM, Petr Jelinek  wrote:

I have machine ready, waiting for animal name and secret.

Great!

Nice. Thanks.


It will obviously
fail until we push the 0002 and 0004 though.

I think it would be a shame if we shipped 9.6 without MSVC 2015
support, but it'd be nice if Andrew or Magnus could do the actual
committing, 'cuz I really don't want to be responsible for breaking
the Windows build.

I'm fine to back you up as needed. That's not a committer guarantee,
still better than nothing I guess. And I make a specialty of looking
at VS-related bugs lately :)



I am currently travelling, but my intention is to deal with the 
remaining patches when I'm back home this weekend, unless someone beats 
me to it.


cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Wed, Mar 30, 2016 at 12:45 AM, Robert Haas  wrote:
> On Tue, Mar 29, 2016 at 11:31 AM, Petr Jelinek  wrote:
>> I have machine ready, waiting for animal name and secret.
>
> Great!

Nice. Thanks.

>> It will obviously
>> fail until we push the 0002 and 0004 though.
>
> I think it would be a shame if we shipped 9.6 without MSVC 2015
> support, but it'd be nice if Andrew or Magnus could do the actual
> committing, 'cuz I really don't want to be responsible for breaking
> the Windows build.

I'm fine to back you up as needed. That's not a committer guarantee,
still better than nothing I guess. And I make a specialty of looking
at VS-related bugs lately :)
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Robert Haas]

On Tue, Mar 29, 2016 at 11:31 AM, Petr Jelinek  wrote:
> I have machine ready, waiting for animal name and secret.

Great!

> It will obviously
> fail until we push the 0002 and 0004 though.

I think it would be a shame if we shipped 9.6 without MSVC 2015
support, but it'd be nice if Andrew or Magnus could do the actual
committing, 'cuz I really don't want to be responsible for breaking
the Windows build.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Petr Jelinek]

On 29/03/16 03:06, Robert Haas wrote:

On Sun, Mar 27, 2016 at 10:35 PM, Michael Paquier
 wrote:

On Fri, Mar 25, 2016 at 9:48 PM, Andrew Dunstan  wrote:

OK, sounds good.


Just a side-note. Andres has pushed the fix for the GinIs* macros as
af4472bc, making patch 0003 from the last series useless now.


I'm not prepared to get very involved in this patch series since I am
not a Windows guy, but I went ahead and pushed 0001 anyway.  I'm not
sure that we want to commit 0002 without a BF machine.  Can anybody
help with that?



I have machine ready, waiting for animal name and secret. It will 
obviously fail until we push the 0002 and 0004 though.


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Tue, Mar 29, 2016 at 10:06 AM, Robert Haas  wrote:
> On Sun, Mar 27, 2016 at 10:35 PM, Michael Paquier
>  wrote:
>> On Fri, Mar 25, 2016 at 9:48 PM, Andrew Dunstan  wrote:
>>> OK, sounds good.
>>
>> Just a side-note. Andres has pushed the fix for the GinIs* macros as
>> af4472bc, making patch 0003 from the last series useless now.
>
> I'm not prepared to get very involved in this patch series since I am
> not a Windows guy, but I went ahead and pushed 0001 anyway.  I'm not
> sure that we want to commit 0002 without a BF machine.  Can anybody
> help with that?

I'll try to get a machine up and running, I guess Win7 with VS2015 at
this stage.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Robert Haas]

On Sun, Mar 27, 2016 at 10:35 PM, Michael Paquier
 wrote:
> On Fri, Mar 25, 2016 at 9:48 PM, Andrew Dunstan  wrote:
>> OK, sounds good.
>
> Just a side-note. Andres has pushed the fix for the GinIs* macros as
> af4472bc, making patch 0003 from the last series useless now.

I'm not prepared to get very involved in this patch series since I am
not a Windows guy, but I went ahead and pushed 0001 anyway.  I'm not
sure that we want to commit 0002 without a BF machine.  Can anybody
help with that?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Fri, Mar 25, 2016 at 9:48 PM, Andrew Dunstan  wrote:
> OK, sounds good.

Just a side-note. Andres has pushed the fix for the GinIs* macros as
af4472bc, making patch 0003 from the last series useless now.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Fri, Mar 25, 2016 at 9:48 PM, Andrew Dunstan  wrote:
> OK, sounds good. I don't have a spare machine on which to install VS2015,
> nor time to set one up, so I'm going to have to trust the two of you
> (Michael and Petr) that this works.

With Virtual Box, you could set up an environment for development on
WinX. They expire after 90 days but setting up the environment is a
matter of 1~2 hours, then by taking a snapshot of the VM you can
rollback to the basic setup easily. That's what I am doing.

> Will either of you be setting up a buildfarm animal with VS2015?

We are definitely going to need one... I have a machine at home that
could be used with it, an intel NUC of a couple of years back that's
still waiting to be useful with 16GB of RAM in it, but I have no
license at hand, be it either Win7, Win8 or Win10.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Fri, Mar 25, 2016 at 9:55 PM, Robert Haas  wrote:
> On Fri, Mar 25, 2016 at 8:31 AM, Michael Paquier
>  wrote:
> The relationship between doc/src/sgml/install-windows.sgml, the
> section of doc/src/sgml/installation.sgml entitled "MinGW/Native
> Windows", and src/tools/msvc/README is not altogether clear to me.

This README still mentions that:
"Microsoft Visual Studio 2005 - 2011. This builds the whole backend,
not just [...]"
The bump to 2013 has missed an update here, now I guess that we had
better just change it to 2015 and move on.
--
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Robert Haas]

On Fri, Mar 25, 2016 at 8:31 AM, Michael Paquier
 wrote:
> On Fri, Mar 25, 2016 at 9:09 PM, Robert Haas  wrote:
>> On Thu, Mar 24, 2016 at 1:07 PM, Petr Jelinek  wrote:
>>> On 24/03/16 17:28, Robert Haas wrote:
 On Wed, Mar 23, 2016 at 3:17 AM, Michael Paquier
  wrote:
>
> - 0001 fixes the global declarations of TIMEZONE_GLOBAL and
> TZNAME_GLOBAL to be WIN32-compliant. I got bitten by that in the ECPG
> compilation.

 So this isn't going to break other Windows builds?  I mean, if we've
 got the names for those symbols wrong, how is this working right now?

>>>
>>> We didn't older versions just defined the other variants as well, but the
>>> _timezone and _tzname have been around since at least VS2003.
>>
>> I am unable to parse this sentence.  Sorry.
>
> Petr means that both _timezone and _tzname are objects defined in
> Visual Studio since more or less its 2003 release
> (https://msdn.microsoft.com/en-us/library/htb3tdkc%28v=vs.71%29.aspx).
> The oldest version on the buildfarm is Visual Studio 2005, and I agree
> with him that there is no need to worry about older versions than
> VS2003. The issue is that VS2015 does *not* define timezone and tzname
> (please note the prefix underscore missing in those variable names),
> causing compilation failures. That's why I am suggesting such a change
> in this patch: this will allow the code to compile on VS2015, and
> that's compatible with VS2003~.

OK, that makes sense.  The documentation says: "There are several
different ways of building PostgreSQL on Windows. The simplest way to
build with Microsoft tools is to install Visual Studio Express 2013
for Windows Desktop and use the included compiler. It is also possible
to build with the full Microsoft Visual C++ 2005 to 2013. In some
cases that requires the installation of the Windows SDK in addition to
the compiler."  So the fact that pre-2003 is out is, I think, covered
by that.

The relationship between doc/src/sgml/install-windows.sgml, the
section of doc/src/sgml/installation.sgml entitled "MinGW/Native
Windows", and src/tools/msvc/README is not altogether clear to me.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 03/25/2016 08:31 AM, Michael Paquier wrote:

On Fri, Mar 25, 2016 at 9:09 PM, Robert Haas  wrote:

On Thu, Mar 24, 2016 at 1:07 PM, Petr Jelinek  wrote:

On 24/03/16 17:28, Robert Haas wrote:

On Wed, Mar 23, 2016 at 3:17 AM, Michael Paquier
 wrote:

- 0001 fixes the global declarations of TIMEZONE_GLOBAL and
TZNAME_GLOBAL to be WIN32-compliant. I got bitten by that in the ECPG
compilation.

So this isn't going to break other Windows builds?  I mean, if we've
got the names for those symbols wrong, how is this working right now?


We didn't older versions just defined the other variants as well, but the
_timezone and _tzname have been around since at least VS2003.

I am unable to parse this sentence.  Sorry.

Petr means that both _timezone and _tzname are objects defined in
Visual Studio since more or less its 2003 release
(https://msdn.microsoft.com/en-us/library/htb3tdkc%28v=vs.71%29.aspx).
The oldest version on the buildfarm is Visual Studio 2005, and I agree
with him that there is no need to worry about older versions than
VS2003. The issue is that VS2015 does *not* define timezone and tzname
(please note the prefix underscore missing in those variable names),
causing compilation failures. That's why I am suggesting such a change
in this patch: this will allow the code to compile on VS2015, and
that's compatible with VS2003~.



OK, sounds good. I don't have a spare machine on which to install 
VS2015, nor time to set one up, so I'm going to have to trust the two of 
you (Michael and Petr) that this works. Will either of you be setting up 
a buildfarm animal with VS2015?


cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Fri, Mar 25, 2016 at 9:09 PM, Robert Haas  wrote:
> On Thu, Mar 24, 2016 at 1:07 PM, Petr Jelinek  wrote:
>> On 24/03/16 17:28, Robert Haas wrote:
>>> On Wed, Mar 23, 2016 at 3:17 AM, Michael Paquier
>>>  wrote:

 - 0001 fixes the global declarations of TIMEZONE_GLOBAL and
 TZNAME_GLOBAL to be WIN32-compliant. I got bitten by that in the ECPG
 compilation.
>>>
>>> So this isn't going to break other Windows builds?  I mean, if we've
>>> got the names for those symbols wrong, how is this working right now?
>>>
>>
>> We didn't older versions just defined the other variants as well, but the
>> _timezone and _tzname have been around since at least VS2003.
>
> I am unable to parse this sentence.  Sorry.

Petr means that both _timezone and _tzname are objects defined in
Visual Studio since more or less its 2003 release
(https://msdn.microsoft.com/en-us/library/htb3tdkc%28v=vs.71%29.aspx).
The oldest version on the buildfarm is Visual Studio 2005, and I agree
with him that there is no need to worry about older versions than
VS2003. The issue is that VS2015 does *not* define timezone and tzname
(please note the prefix underscore missing in those variable names),
causing compilation failures. That's why I am suggesting such a change
in this patch: this will allow the code to compile on VS2015, and
that's compatible with VS2003~.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Robert Haas]

On Thu, Mar 24, 2016 at 1:07 PM, Petr Jelinek  wrote:
> On 24/03/16 17:28, Robert Haas wrote:
>> On Wed, Mar 23, 2016 at 3:17 AM, Michael Paquier
>>  wrote:
>>>
>>> - 0001 fixes the global declarations of TIMEZONE_GLOBAL and
>>> TZNAME_GLOBAL to be WIN32-compliant. I got bitten by that in the ECPG
>>> compilation.
>>
>> So this isn't going to break other Windows builds?  I mean, if we've
>> got the names for those symbols wrong, how is this working right now?
>>
>
> We didn't older versions just defined the other variants as well, but the
> _timezone and _tzname have been around since at least VS2003.

I am unable to parse this sentence.  Sorry.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Petr Jelinek]

On 24/03/16 17:28, Robert Haas wrote:

On Wed, Mar 23, 2016 at 3:17 AM, Michael Paquier
 wrote:

- 0001 fixes the global declarations of TIMEZONE_GLOBAL and
TZNAME_GLOBAL to be WIN32-compliant. I got bitten by that in the ECPG
compilation.


So this isn't going to break other Windows builds?  I mean, if we've
got the names for those symbols wrong, how is this working right now?



We didn't older versions just defined the other variants as well, but 
the _timezone and _tzname have been around since at least VS2003.


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Robert Haas]

On Wed, Mar 23, 2016 at 3:17 AM, Michael Paquier
 wrote:
> - 0001 fixes the global declarations of TIMEZONE_GLOBAL and
> TZNAME_GLOBAL to be WIN32-compliant. I got bitten by that in the ECPG
> compilation.

So this isn't going to break other Windows builds?  I mean, if we've
got the names for those symbols wrong, how is this working right now?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Thu, Mar 24, 2016 at 5:18 AM, Petr Jelinek  wrote:
> Hmm I see you are right, I missed the last couple emails. Ok I'll mark it
> ready for committer - it does work fine on my vs2015 machine and I am happy
> with the code too.

Thanks, let's see what others have to say.

> Well, as happy as I can be given the locale mess.

We are responsible for that, that's the frustrating part :)
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Petr Jelinek]

On 23/03/16 13:05, Michael Paquier wrote:


OK, the please send an updated set of patches for what remains.



Here you go:
- 0001 fixes the global declarations of TIMEZONE_GLOBAL and
TZNAME_GLOBAL to be WIN32-compliant. I got bitten by that in the ECPG
compilation.
- 0002, support of VS2015 in the scripts of src/tools/msvc
- 0003, this is necessary in order to run the regression tests,
details are here:

http://www.postgresql.org/message-id/cab7npqtdixxs8cdl8moivh6qfq-1qv9mkn0ayjzybvzv6wc...@mail.gmail.com


So that's basically what Andres did? Interesting that we now actually really
need it. I was in favor of doing those changes in any case.


Yes, that's what Andres did. I am just attaching it here to allow
Andrew to test this patch set appropriately.


- 0004 is the fix for locales. This is actually Petr's work upthread,
I have updated comments in the code though and did a bit more
polishing. This still looks like the cleanest solution we have. Other
solutions are mentioned upthread: redeclaration of struct _locale_t in
backend code is one.


Thanks for polishing this.

I think this is ready to be committed, but the 0003 might be somewhat
controversial based on the original thread.


I thought that the end consensus regarding 0003 was to apply it, but
we could as well wait for the final verdict (committer push) on the
other thread. And well, if this is not applied, some of the gin tests
will complain about "right sibling of GIN page is of different type" a
couple of times.



Hmm I see you are right, I missed the last couple emails. Ok I'll mark 
it ready for committer - it does work fine on my vs2015 machine and I am 
happy with the code too. Well, as happy as I can be given the locale mess.


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Wed, Mar 23, 2016 at 8:45 PM, Petr Jelinek  wrote:
> On 23/03/16 08:17, Michael Paquier wrote:
>>
>> On Mon, Mar 21, 2016 at 3:43 AM, Andrew Dunstan 
>> wrote:
>>>
>>>
>>>
>>> On 03/20/2016 12:02 AM, Michael Paquier wrote:


 On Sun, Mar 20, 2016 at 8:06 AM, Andrew Dunstan 
 wrote:
>
>
>
> Still to do: the non-perl pieces.


 The patch to address locales is the sensitive part. The patch from
 Petr is taking the correct approach though I think that we had better
 simplify it a bit and if possible we had better not rely on the else
 block it introduces.
>>>
>>>
>>>
>>>
>>> OK, the please send an updated set of patches for what remains.
>>
>>
>> Here you go:
>> - 0001 fixes the global declarations of TIMEZONE_GLOBAL and
>> TZNAME_GLOBAL to be WIN32-compliant. I got bitten by that in the ECPG
>> compilation.
>> - 0002, support of VS2015 in the scripts of src/tools/msvc
>> - 0003, this is necessary in order to run the regression tests,
>> details are here:
>>
>> http://www.postgresql.org/message-id/cab7npqtdixxs8cdl8moivh6qfq-1qv9mkn0ayjzybvzv6wc...@mail.gmail.com
>
> So that's basically what Andres did? Interesting that we now actually really
> need it. I was in favor of doing those changes in any case.

Yes, that's what Andres did. I am just attaching it here to allow
Andrew to test this patch set appropriately.

>> - 0004 is the fix for locales. This is actually Petr's work upthread,
>> I have updated comments in the code though and did a bit more
>> polishing. This still looks like the cleanest solution we have. Other
>> solutions are mentioned upthread: redeclaration of struct _locale_t in
>> backend code is one.
>
> Thanks for polishing this.
>
> I think this is ready to be committed, but the 0003 might be somewhat
> controversial based on the original thread.

I thought that the end consensus regarding 0003 was to apply it, but
we could as well wait for the final verdict (committer push) on the
other thread. And well, if this is not applied, some of the gin tests
will complain about "right sibling of GIN page is of different type" a
couple of times.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Petr Jelinek]

On 23/03/16 08:17, Michael Paquier wrote:

On Mon, Mar 21, 2016 at 3:43 AM, Andrew Dunstan  wrote:



On 03/20/2016 12:02 AM, Michael Paquier wrote:


On Sun, Mar 20, 2016 at 8:06 AM, Andrew Dunstan 
wrote:



Still to do: the non-perl pieces.


The patch to address locales is the sensitive part. The patch from
Petr is taking the correct approach though I think that we had better
simplify it a bit and if possible we had better not rely on the else
block it introduces.




OK, the please send an updated set of patches for what remains.


Here you go:
- 0001 fixes the global declarations of TIMEZONE_GLOBAL and
TZNAME_GLOBAL to be WIN32-compliant. I got bitten by that in the ECPG
compilation.
- 0002, support of VS2015 in the scripts of src/tools/msvc
- 0003, this is necessary in order to run the regression tests,
details are here:
http://www.postgresql.org/message-id/cab7npqtdixxs8cdl8moivh6qfq-1qv9mkn0ayjzybvzv6wc...@mail.gmail.com


So that's basically what Andres did? Interesting that we now actually 
really need it. I was in favor of doing those changes in any case.



- 0004 is the fix for locales. This is actually Petr's work upthread,
I have updated comments in the code though and did a bit more
polishing. This still looks like the cleanest solution we have. Other
solutions are mentioned upthread: redeclaration of struct _locale_t in
backend code is one.



Thanks for polishing this.

I think this is ready to be committed, but the 0003 might be somewhat 
controversial based on the original thread.


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Mon, Mar 21, 2016 at 3:43 AM, Andrew Dunstan  wrote:
>
>
> On 03/20/2016 12:02 AM, Michael Paquier wrote:
>>
>> On Sun, Mar 20, 2016 at 8:06 AM, Andrew Dunstan 
>> wrote:
>>>
>>>
>>> Still to do: the non-perl pieces.
>>
>> The patch to address locales is the sensitive part. The patch from
>> Petr is taking the correct approach though I think that we had better
>> simplify it a bit and if possible we had better not rely on the else
>> block it introduces.
>
>
>
> OK, the please send an updated set of patches for what remains.

Here you go:
- 0001 fixes the global declarations of TIMEZONE_GLOBAL and
TZNAME_GLOBAL to be WIN32-compliant. I got bitten by that in the ECPG
compilation.
- 0002, support of VS2015 in the scripts of src/tools/msvc
- 0003, this is necessary in order to run the regression tests,
details are here:
http://www.postgresql.org/message-id/cab7npqtdixxs8cdl8moivh6qfq-1qv9mkn0ayjzybvzv6wc...@mail.gmail.com
- 0004 is the fix for locales. This is actually Petr's work upthread,
I have updated comments in the code though and did a bit more
polishing. This still looks like the cleanest solution we have. Other
solutions are mentioned upthread: redeclaration of struct _locale_t in
backend code is one.
-- 
Michael


0001-Fix-declaration-of-TIMEZONE_GLOBAL-and-TZNAME_GLOBAL.patch
Description: binary/octet-stream


0002-Add-support-for-VS-2015-in-MSVC-scripts.patch
Description: binary/octet-stream


0003-Fix-non-compliant-boolean-declarations-with-stdbool.patch
Description: binary/octet-stream


0004-Fix-code-page-calculation-for-Visual-Studio-2015.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Petr Jelinek]

On 22/03/16 14:40, Michael Paquier wrote:

On Wed, Mar 9, 2016 at 11:10 PM, Petr Jelinek  wrote:

Something like attached is simplest way this would work correctly (note that
I didn't really test it and it's missing comments). Note that we are falling
back to the old parsing in case the GetLocaleInfoEx didn't work, that's
important because GetLocaleInfoEx does not support the
_. format but supports all the rest of them.
The problem with this is the binaries would need to be compiled with target
of vista/windows server 2008+. That would be of course only the case with
builds made with VS2015, so I am not sure if we care all that much
(especially given the fact that older windows are not supported by MS
anyway).


Ah, OK. Of course.


I don't currently know of any way of doing this in VS2015 that would work
with older versions of windows with the exception of having our own
definition of the locale_t struct so that the VS2013 code would still
work...


There is no actual way to be sure if this is an intentional change or
not, if we see it back in the next version of VS, why not... I would
like to think like you that this is actually a merge mistake from
Redmond-side, but I think we had better be careful here, this could
lead to undetected errors in the future.



Sure, I am looking at it from the perspective that they didn't even 
deprecate the function and changes to the struct it returns would break 
binary compatibility for existing apps.


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Wed, Mar 9, 2016 at 11:10 PM, Petr Jelinek  wrote:
> Something like attached is simplest way this would work correctly (note that
> I didn't really test it and it's missing comments). Note that we are falling
> back to the old parsing in case the GetLocaleInfoEx didn't work, that's
> important because GetLocaleInfoEx does not support the
> _. format but supports all the rest of them.
> The problem with this is the binaries would need to be compiled with target
> of vista/windows server 2008+. That would be of course only the case with
> builds made with VS2015, so I am not sure if we care all that much
> (especially given the fact that older windows are not supported by MS
> anyway).

Ah, OK. Of course.

> I don't currently know of any way of doing this in VS2015 that would work
> with older versions of windows with the exception of having our own
> definition of the locale_t struct so that the VS2013 code would still
> work...

There is no actual way to be sure if this is an intentional change or
not, if we see it back in the next version of VS, why not... I would
like to think like you that this is actually a merge mistake from
Redmond-side, but I think we had better be careful here, this could
lead to undetected errors in the future.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Petr Jelinek]

On 20/03/16 05:02, Michael Paquier wrote:

On Sun, Mar 20, 2016 at 8:06 AM, Andrew Dunstan  wrote:


Still to do: the non-perl pieces.


The patch to address locales is the sensitive part. The patch from
Petr is taking the correct approach though I think that we had better
simplify it a bit and if possible we had better not rely on the else
block it introduces.



That would be ideal, not particularly sure that both are possible at the 
same time. We can definitely remove the else block but it involves 
enumerating system locales which makes the patch orders of magnitude 
bigger and uglier. Do you have any better approaches in you mind?


As a side note, I have to say the current locale API in Windows is one 
huge mess and if it was on me I'd just redefine the locale_t struct 
correctly as that is simplest solution and the APIs involved are not 
deprecated or anything, it's just that the public headers are botched in 
current version and apparently nobody important is using them to force 
MS to fix them.


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 03/20/2016 12:02 AM, Michael Paquier wrote:

On Sun, Mar 20, 2016 at 8:06 AM, Andrew Dunstan  wrote:


Still to do: the non-perl pieces.

The patch to address locales is the sensitive part. The patch from
Petr is taking the correct approach though I think that we had better
simplify it a bit and if possible we had better not rely on the else
block it introduces.



OK, the please send an updated set of patches for what remains.

cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers