VS 2015 support in src/tools/msvc

On 03/03/16 15:02, Michael Paquier wrote:

Hi all,

Microsoft provides a set of VMs that one can use for testing and
Windows 10 is in the set:
https://dev.windows.com/en-us/microsoft-edge/tools/vms/windows/
I have grabbed one and installed the community version of Visual
Studio 2015 so I think that I am going to be able to compile Postgres
with VS2015 with a bit of black magic.

My goal is double:
1) As far as things have been discussed, VS2015 is making difficult
the compilation of Postgres, particularly for locales. So I'd like to
see what are the problems behind that and see if we can patch it
properly. This would facilitate the integration of cmake as well for
Windows.

The locale problem is that:
a) the MS crt headers are broken for this particular part due to
unfinished refactoring, even their msvcrt sources don't compile with
them, if you read them it's obvious they forgot to put one variable in
the struct
b) we are using somewhat legacy API there that's internally implemented
as hacks over the new API (string parsing and generation and stuff is
happening there)
c) the non-legacy API works only on Vista+ and does not support the old
locale names (which is why the legacy api that is written on top of this
has to do the string parsing and generation)

To me the least bad solution for MSVC 2015 and codepage detection seemed
to use the new API when possible (GetLocaleInfoEx and friends) and fall
back to our old string parsing that we did pre-VC2013 when it's not,
since afaics it works correctly on all the locale names that are not
supported by the new API. It means that MSVC 2015 builds would be Vista+
but I honestly don't see that as big issue given Microsoft's own policy
about old Windows versions.

2) src/tools/msvc stuff has support only up to VS2013. I think that it
would be nice to bump that a bit and get something for 9.5~.

Yeah, we'll also have to do something about perl 5.22 compatibility
there as psed is not included in the core distribution anymore from that
version and we use it to generate one header file IIRC.

So, would there be interest for a patch on the perl scripts in
src/tools/msvc or are they considered a lost cause? Having a look at
the failures could be done with the cmake work, but it seems a bit
premature to me to look at that for the moment, and having support for
VS2015 with 9.5 (support for new versions of VS won a backpatch the
last couple of years) would be a good thing I think.

+1, and I can help (at least review and testing if nothing else).

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

Petr Jelinek <petr@2ndquadrant.com> writes:

On 03/03/16 15:02, Michael Paquier wrote:

So, would there be interest for a patch on the perl scripts in
src/tools/msvc or are they considered a lost cause? Having a look at
the failures could be done with the cmake work, but it seems a bit
premature to me to look at that for the moment, and having support for
VS2015 with 9.5 (support for new versions of VS won a backpatch the
last couple of years) would be a good thing I think.

+1, and I can help (at least review and testing if nothing else).

Yeah. At this point, the earliest the cmake work could land is 9.7,
and TBH I do not think we are committed to landing it at all. So it'd
behoove us to fix at least HEAD, and probably older branches too,
to work with up-to-date MSVC.

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

Tom Lane wrote:

Yeah. At this point, the earliest the cmake work could land is 9.7,
and TBH I do not think we are committed to landing it at all. So it'd
behoove us to fix at least HEAD, and probably older branches too,
to work with up-to-date MSVC.

In that case we should probably add the configure check for IPC::Run
that was proposed in another thread.

--
�lvaro Herrera http://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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Tom Lane wrote:

Yeah. At this point, the earliest the cmake work could land is 9.7,
and TBH I do not think we are committed to landing it at all. So it'd
behoove us to fix at least HEAD, and probably older branches too,
to work with up-to-date MSVC.

In that case we should probably add the configure check for IPC::Run
that was proposed in another thread.

Yeah, agreed. I opposed it at the time because I thought we might be
considering cmake conversion for 9.6. But now that that's missed the
final 9.6 commitfest, we'd better operate on the assumption that it's
not landing anytime soon.

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

On 03/03/2016 09:02 AM, Michael Paquier wrote:

Hi all,

Microsoft provides a set of VMs that one can use for testing and
Windows 10 is in the set:
https://dev.windows.com/en-us/microsoft-edge/tools/vms/windows/
I have grabbed one and installed the community version of Visual
Studio 2015 so I think that I am going to be able to compile Postgres
with VS2015 with a bit of black magic.

My goal is double:
1) As far as things have been discussed, VS2015 is making difficult
the compilation of Postgres, particularly for locales. So I'd like to
see what are the problems behind that and see if we can patch it
properly. This would facilitate the integration of cmake as well for
Windows.
2) src/tools/msvc stuff has support only up to VS2013. I think that it
would be nice to bump that a bit and get something for 9.5~.

So, would there be interest for a patch on the perl scripts in
src/tools/msvc or are they considered a lost cause? Having a look at
the failures could be done with the cmake work, but it seems a bit
premature to me to look at that for the moment, and having support for
VS2015 with 9.5 (support for new versions of VS won a backpatch the
last couple of years) would be a good thing I think.

I am not holding my breath on cmake. Until we have something pretty
solid on that front I'm going to assume it's not happening. If we're
going to support VS2015 (and I think we should) then it should be
supported for all live branches if possible. I'm assuming the changes
would be pretty localized, at least in src/tools/msvc, and adding a new
compile shouldn't break anything with existing compilers.

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

On Thu, Mar 3, 2016 at 6:18 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 03/03/2016 09:02 AM, Michael Paquier wrote:

Hi all,

Microsoft provides a set of VMs that one can use for testing and
Windows 10 is in the set:
https://dev.windows.com/en-us/microsoft-edge/tools/vms/windows/
I have grabbed one and installed the community version of Visual
Studio 2015 so I think that I am going to be able to compile Postgres
with VS2015 with a bit of black magic.

My goal is double:
1) As far as things have been discussed, VS2015 is making difficult
the compilation of Postgres, particularly for locales. So I'd like to
see what are the problems behind that and see if we can patch it
properly. This would facilitate the integration of cmake as well for
Windows.
2) src/tools/msvc stuff has support only up to VS2013. I think that it
would be nice to bump that a bit and get something for 9.5~.

So, would there be interest for a patch on the perl scripts in
src/tools/msvc or are they considered a lost cause? Having a look at
the failures could be done with the cmake work, but it seems a bit
premature to me to look at that for the moment, and having support for
VS2015 with 9.5 (support for new versions of VS won a backpatch the
last couple of years) would be a good thing I think.

I am not holding my breath on cmake. Until we have something pretty solid
on that front I'm going to assume it's not happening. If we're going to
support VS2015 (and I think we should) then it should be supported for all
live branches if possible. I'm assuming the changes would be pretty
localized, at least in src/tools/msvc, and adding a new compile shouldn't
break anything with existing compilers.

+1.

Definitely do it for HEAD.

Then if it gets backpatched is going to depend on the locality I think. If
it's just the build system then it should be no problem, but I thought
Michael also suggested some API changes. If that's so, then it is going to
depend on how invasive those are. But that part should be done for HEAD
regardless, so it's definitely worth the effort to figure out exactly what
it involves.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On 03/03/16 18:41, Magnus Hagander wrote:

On Thu, Mar 3, 2016 at 6:18 PM, Andrew Dunstan <andrew@dunslane.net
<mailto:andrew@dunslane.net>> wrote:

On 03/03/2016 09:02 AM, Michael Paquier wrote:

Microsoft provides a set of VMs that one can use for testing and
Windows 10 is in the set:
https://dev.windows.com/en-us/microsoft-edge/tools/vms/windows/
I have grabbed one and installed the community version of Visual
Studio 2015 so I think that I am going to be able to compile
Postgres
with VS2015 with a bit of black magic.

So, would there be interest for a patch on the perl scripts in
src/tools/msvc or are they considered a lost cause? Having a look at
the failures could be done with the cmake work, but it seems a bit
premature to me to look at that for the moment, and having
support for
VS2015 with 9.5 (support for new versions of VS won a backpatch the
last couple of years) would be a good thing I think.

I am not holding my breath on cmake. Until we have something pretty
solid on that front I'm going to assume it's not happening. If we're
going to support VS2015 (and I think we should) then it should be
supported for all live branches if possible. I'm assuming the
changes would be pretty localized, at least in src/tools/msvc, and
adding a new compile shouldn't break anything with existing compilers.

+1.

Definitely do it for HEAD.

Then if it gets backpatched is going to depend on the locality I think.
If it's just the build system then it should be no problem, but I
thought Michael also suggested some API changes. If that's so, then it
is going to depend on how invasive those are. But that part should be
done for HEAD regardless, so it's definitely worth the effort to figure
out exactly what it involves.

Well the source code does not compile on MSVC2015, the perl changes
needed are really tiny, there is some code that needs changes to work
with 2015, particularly in the locale code-page detection area so it's
definitely not just build system. But I think it should be fairly
localized and fenced by ifdef anyway.

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

On Fri, Mar 4, 2016 at 7:21 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:

Well the source code does not compile on MSVC2015, the perl changes needed
are really tiny, there is some code that needs changes to work with 2015,
particularly in the locale code-page detection area so it's definitely not
just build system. But I think it should be fairly localized and fenced by
ifdef anyway.

Yeah, that's my first impression as well. We should not need any APIs
changes and the changes would be limited to if extra blocks with
_MSC_VER, if that would occur then I definitely agree that patching
only HEAD is the way to go. I'll look at that today and the next
couple of days, let's see what I can get out of it...
--
Michael

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

On Fri, Mar 4, 2016 at 9:36 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Yeah, that's my first impression as well. We should not need any APIs
changes and the changes would be limited to if extra blocks with
_MSC_VER, if that would occur then I definitely agree that patching
only HEAD is the way to go. I'll look at that today and the next
couple of days, let's see what I can get out of it...

So, I have finally been able to set up my environment correctly, and I
am at a stage where I can compile Postgres with VS 2015 thanks to the
patch attached that extends src/tools/msvc to do so. Unsurprisingly,
the first failures detected are related to locales :)

I still need to dig into that in more details. For the time being the
patch attached is useful IMO to plug in VS 2015 with the existing
infrastructure. So if anybody has a Windows environment, feel free to
play with it and dig into those problems. I'll update this thread once
I have a more advanced status.
Regards,
--
Michael

On Fri, Mar 4, 2016 at 3:54 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

I still need to dig into that in more details. For the time being the
patch attached is useful IMO to plug in VS 2015 with the existing
infrastructure. So if anybody has a Windows environment, feel free to
play with it and dig into those problems. I'll update this thread once
I have a more advanced status.

OK, attached are a set of patches that allowed me to compile Postgres
using VS2015, in more details:
- 0001, as mentioned by Petr upthread, psed is removed from the core
distribution of Perl in 5.22, so when installing ActivePerl it is not
possible to create probes.h, and the code compilation would fail. I
bumped into that so here is a patch. What I am proposing here is to
replace psed by sed, sed being available in MSYS like bison and flex,
so when building using MSVC the environment to set up is normally
already good to go even with this additional dependency. Now, it is
important to mention that probes.h is not part of a source tarball. I
think that we would want probes.h to be part of a source tarball so as
it would be possible to compile the code on Windows using MSVC without
having to install MSYS. I haven't done that in this patch, thoughts on
the matter are welcome.
- 0002, which adds support for VS2015 in src/tools/scripts
- 0003, to address a compilation failure that I bumped into when
compiling ecpg. In src/port, TIMEZONE_GLOBAL and TZNAME_GLOBAL refer
to respectively timezone and tzname, however for win32, those should
be _timezone and _tzname. See here:
https://msdn.microsoft.com/en-us/library/htb3tdkc.aspx
- 0004, which is to address the problem of the missing lc_codepage
from locale.h in src/port/. I have been pondering about the use of
more fancy routines like GetLocaleInfoEx as mentioned by Petr
upthread. However, I think that we had better avoid any kind of
complication and just fall back to the old code path should _MSC_VER

= 1900. We could always reuse lc_codepage if it gets reintroduced in

a future version of VS.

This set of patches is clearly a work-in-progress, but I am at the
point where feedback is welcome, and the code can compile. The issue
wit psed is something I think could be backpatched a bit more than the
VS2015 core patches, support for perl 5.22 happening on all the
supported branches.
Note that I did not bump into the stdbool issues. Note as well that VS
has complained about a couple of warnings. I am attaching them in the
file named VS2015_warnings.txt. Those are quite interesting as well.
--
Michael

Michael Paquier wrote:

- 0001, as mentioned by Petr upthread, psed is removed from the core
distribution of Perl in 5.22, so when installing ActivePerl it is not
possible to create probes.h, and the code compilation would fail. I
bumped into that so here is a patch. What I am proposing here is to
replace psed by sed, sed being available in MSYS like bison and flex,
so when building using MSVC the environment to set up is normally
already good to go even with this additional dependency. Now, it is
important to mention that probes.h is not part of a source tarball. I
think that we would want probes.h to be part of a source tarball so as
it would be possible to compile the code on Windows using MSVC without
having to install MSYS. I haven't done that in this patch, thoughts on
the matter are welcome.

I think the path of least resistance is to change the sed script into a
Perl script. Should be fairly simple ...

--
�lvaro Herrera http://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

On Sat, Mar 5, 2016 at 12:08 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Michael Paquier wrote:

- 0001, as mentioned by Petr upthread, psed is removed from the core
distribution of Perl in 5.22, so when installing ActivePerl it is not
possible to create probes.h, and the code compilation would fail. I
bumped into that so here is a patch. What I am proposing here is to
replace psed by sed, sed being available in MSYS like bison and flex,
so when building using MSVC the environment to set up is normally
already good to go even with this additional dependency. Now, it is
important to mention that probes.h is not part of a source tarball. I
think that we would want probes.h to be part of a source tarball so as
it would be possible to compile the code on Windows using MSVC without
having to install MSYS. I haven't done that in this patch, thoughts on
the matter are welcome.

I think the path of least resistance is to change the sed script into a
Perl script. Should be fairly simple ...

Yes that's possible as well. It would be better to use the same
process for *nix platforms 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

On 4 March 2016 at 22:23, Michael Paquier <michael.paquier@gmail.com> wrote:

On Fri, Mar 4, 2016 at 3:54 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

I still need to dig into that in more details. For the time being the
patch attached is useful IMO to plug in VS 2015 with the existing
infrastructure. So if anybody has a Windows environment, feel free to
play with it and dig into those problems. I'll update this thread once
I have a more advanced status.

OK, attached are a set of patches that allowed me to compile Postgres
using VS2015, in more details:
- 0001, as mentioned by Petr upthread, psed is removed from the core
distribution of Perl in 5.22, so when installing ActivePerl it is not
possible to create probes.h, and the code compilation would fail. I
bumped into that so here is a patch. What I am proposing here is to
replace psed by sed, sed being available in MSYS like bison and flex,
so when building using MSVC the environment to set up is normally
already good to go even with this additional dependency.

The assumption here is that we're using msys to provide bison and flex
(probably via msysgit), so adding sed isn't any more intrusive.

I think that's reasonable, but wanted to spell it out since right now msys
isn't actually a dependency of the MSVC builds, just a convenient way to
get some of the dependencies. This still adds a new dependency, but it's
one most people will have anyway. If they're using bison/flex from gnuwin32
or whatever instead they can get sed there too. So +1, sensible.

Now, it is
important to mention that probes.h is not part of a source tarball. I
think that we would want probes.h to be part of a source tarball so as
it would be possible to compile the code on Windows using MSVC without
having to install MSYS. I haven't done that in this patch, thoughts on
the matter are welcome.

That's consistent with how we include the generated scanner and lexer files
etc in the source tarball, so +1.

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

On 04/03/16 15:23, Michael Paquier wrote:

OK, attached are a set of patches that allowed me to compile Postgres
using VS2015, in more details:
- 0001, as mentioned by Petr upthread, psed is removed from the core
distribution of Perl in 5.22, so when installing ActivePerl it is not
possible to create probes.h, and the code compilation would fail. I
bumped into that so here is a patch. What I am proposing here is to
replace psed by sed, sed being available in MSYS like bison and flex,
so when building using MSVC the environment to set up is normally
already good to go even with this additional dependency. Now, it is
important to mention that probes.h is not part of a source tarball. I
think that we would want probes.h to be part of a source tarball so as
it would be possible to compile the code on Windows using MSVC without
having to install MSYS. I haven't done that in this patch, thoughts on
the matter are welcome.

I vote for just using sed considering we need flex and bison anyway.

- 0003, to address a compilation failure that I bumped into when
compiling ecpg. In src/port, TIMEZONE_GLOBAL and TZNAME_GLOBAL refer
to respectively timezone and tzname, however for win32, those should
be _timezone and _tzname. See here:
https://msdn.microsoft.com/en-us/library/htb3tdkc.aspx

Yep I hit that one as well, looks good.

- 0004, which is to address the problem of the missing lc_codepage
from locale.h in src/port/. I have been pondering about the use of
more fancy routines like GetLocaleInfoEx as mentioned by Petr
upthread. However, I think that we had better avoid any kind of
complication and just fall back to the old code path should _MSC_VER

= 1900. We could always reuse lc_codepage if it gets reintroduced in

a future version of VS.

Well I am worried that this way we might break existing installs which
means we can't backpatch this. The problem here is that the fallback
code does not support the <language>-<REGION> format which Microsoft
documents everywhere as recommended locale format. The good news is that
our own initdb won't auto-generate those when no locale was specified as
it uses the setlocale() which returns the legacy (and not recommended)
locale names and our fallback code can handle those. But manually set
locales can be a problem.

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

On Sat, Mar 5, 2016 at 1:41 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:

On 04/03/16 15:23, Michael Paquier wrote:

OK, attached are a set of patches that allowed me to compile Postgres
using VS2015, in more details:
- 0001, as mentioned by Petr upthread, psed is removed from the core
distribution of Perl in 5.22, so when installing ActivePerl it is not
possible to create probes.h, and the code compilation would fail. I
bumped into that so here is a patch. What I am proposing here is to
replace psed by sed, sed being available in MSYS like bison and flex,
so when building using MSVC the environment to set up is normally
already good to go even with this additional dependency. Now, it is
important to mention that probes.h is not part of a source tarball. I
think that we would want probes.h to be part of a source tarball so as
it would be possible to compile the code on Windows using MSVC without
having to install MSYS. I haven't done that in this patch, thoughts on
the matter are welcome.

I vote for just using sed considering we need flex and bison anyway.

OK cool, we could go with something else than sed to generate probes.h
but that seems sensible considering that this should definitely be
back-patched. Not sure what the others think about adding a new file
in the source tarball by default though.

- 0003, to address a compilation failure that I bumped into when
compiling ecpg. In src/port, TIMEZONE_GLOBAL and TZNAME_GLOBAL refer
to respectively timezone and tzname, however for win32, those should
be _timezone and _tzname. See here:
https://msdn.microsoft.com/en-us/library/htb3tdkc.aspx

Yep I hit that one as well, looks good.

MinGW would react to that correctly I think. If I look at
mingw/include/timezone.h, both timezone and _timezone are defined. I
would think that this is intentional to declare both there.

- 0004, which is to address the problem of the missing lc_codepage
from locale.h in src/port/. I have been pondering about the use of
more fancy routines like GetLocaleInfoEx as mentioned by Petr
upthread. However, I think that we had better avoid any kind of
complication and just fall back to the old code path should _MSC_VER

= 1900. We could always reuse lc_codepage if it gets reintroduced in

a future version of VS.

Well I am worried that this way we might break existing installs which means
we can't backpatch this. The problem here is that the fallback code does not
support the <language>-<REGION> format which Microsoft documents everywhere
as recommended locale format. The good news is that our own initdb won't
auto-generate those when no locale was specified as it uses the setlocale()
which returns the legacy (and not recommended) locale names and our fallback
code can handle those. But manually set locales can be a problem.

I am open to more fancy solutions if it is possible to get reliably
the codepage in a different way, but I am not sure this is worth the
complication. The pre-VS2012 code has been able to live with that.
--
Michael

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

Michael Paquier <michael.paquier@gmail.com> writes:

On Sat, Mar 5, 2016 at 1:41 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:

I vote for just using sed considering we need flex and bison anyway.

OK cool, we could go with something else than sed to generate probes.h
but that seems sensible considering that this should definitely be
back-patched. Not sure what the others think about adding a new file
in the source tarball by default though.

AFAIK, sed flex and bison originate from three separate source projects;
there is no reason to suppose that the presence of flex and bison on a
particular system guarantee the presence of sed. I thought the proposal
to get rid of the psed dependence in favor of some more perl code was
pretty sane.

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

On 03/05/2016 12:46 AM, Tom Lane wrote:

Michael Paquier <michael.paquier@gmail.com> writes:

On Sat, Mar 5, 2016 at 1:41 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:

I vote for just using sed considering we need flex and bison anyway.

OK cool, we could go with something else than sed to generate probes.h
but that seems sensible considering that this should definitely be
back-patched. Not sure what the others think about adding a new file
in the source tarball by default though.

AFAIK, sed flex and bison originate from three separate source projects;
there is no reason to suppose that the presence of flex and bison on a
particular system guarantee the presence of sed. I thought the proposal
to get rid of the psed dependence in favor of some more perl code was
pretty sane.

Here is a translation into perl of the sed script, courtesy of the s2p
incarnation of psed:
<https://gist.github.com/adunstan/d61b1261a4b91496bdc6&gt; The sed script
appears to have been stable for a long time, so I don't think we need to
be too concerned about possibly maintaining two versions.

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

On Sat, Mar 5, 2016 at 11:34 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

Here is a translation into perl of the sed script, courtesy of the s2p
incarnation of psed: <https://gist.github.com/adunstan/d61b1261a4b91496bdc6&gt;
The sed script appears to have been stable for a long time, so I don't think
we need to be too concerned about possibly maintaining two versions.

That's 95% of the work already done, nice! If I finish wrapping up a
patch for this issue at least would you backpatch? It would be saner
to get rid of this dependency everywhere I think regarding compilation
with perl 5.22.
--
Michael

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

On 03/05/2016 01:31 PM, Michael Paquier wrote:

On Sat, Mar 5, 2016 at 11:34 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

Here is a translation into perl of the sed script, courtesy of the s2p
incarnation of psed: <https://gist.github.com/adunstan/d61b1261a4b91496bdc6&gt;
The sed script appears to have been stable for a long time, so I don't think
we need to be too concerned about possibly maintaining two versions.

That's 95% of the work already done, nice! If I finish wrapping up a
patch for this issue at least would you backpatch? It would be saner
to get rid of this dependency everywhere I think regarding compilation
with perl 5.22.

Sure.

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

On Sun, Mar 6, 2016 at 5:55 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 03/05/2016 01:31 PM, Michael Paquier wrote:

On Sat, Mar 5, 2016 at 11:34 PM, Andrew Dunstan <andrew@dunslane.net>
wrote:

Here is a translation into perl of the sed script, courtesy of the s2p
incarnation of psed:
<https://gist.github.com/adunstan/d61b1261a4b91496bdc6&gt;
The sed script appears to have been stable for a long time, so I don't
think
we need to be too concerned about possibly maintaining two versions.

That's 95% of the work already done, nice! If I finish wrapping up a
patch for this issue at least would you backpatch? It would be saner
to get rid of this dependency everywhere I think regarding compilation
with perl 5.22.

Sure.

OK, so after some re-lecture of the script and perltidy-ing I finish
with the attached. How does that look?
--
Michael

On Mon, Mar 7, 2016 at 10:40 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Sun, Mar 6, 2016 at 5:55 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 03/05/2016 01:31 PM, Michael Paquier wrote:

On Sat, Mar 5, 2016 at 11:34 PM, Andrew Dunstan <andrew@dunslane.net>
wrote:

Here is a translation into perl of the sed script, courtesy of the s2p
incarnation of psed:
<https://gist.github.com/adunstan/d61b1261a4b91496bdc6&gt;
The sed script appears to have been stable for a long time, so I don't
think
we need to be too concerned about possibly maintaining two versions.

That's 95% of the work already done, nice! If I finish wrapping up a
patch for this issue at least would you backpatch? It would be saner
to get rid of this dependency everywhere I think regarding compilation
with perl 5.22.

Sure.

OK, so after some re-lecture of the script and perltidy-ing I finish
with the attached. How does that look?

Attached is a new set for the support of MS 2015 + the psed issue,
please use those ones for testing:
- 0001 is the replacement of psed by a dedicated perl script to
generate probes.h
- 0002 Fix of the declaration of TIMEZONE_GLOBAL and TZNAME_GLOBAL for WIN32
- 0003 addresses the issue with lc_codepage missing from _locale_t.
- 0004 adds support for MS2015 in src/tools/scripts/
Regards,
--
Michael

On 03/08/2016 08:32 AM, Michael Paquier wrote:

On Mon, Mar 7, 2016 at 10:40 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Sun, Mar 6, 2016 at 5:55 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 03/05/2016 01:31 PM, Michael Paquier wrote:

On Sat, Mar 5, 2016 at 11:34 PM, Andrew Dunstan <andrew@dunslane.net>
wrote:

Here is a translation into perl of the sed script, courtesy of the s2p
incarnation of psed:
<https://gist.github.com/adunstan/d61b1261a4b91496bdc6&gt;
The sed script appears to have been stable for a long time, so I don't
think
we need to be too concerned about possibly maintaining two versions.

That's 95% of the work already done, nice! If I finish wrapping up a
patch for this issue at least would you backpatch? It would be saner
to get rid of this dependency everywhere I think regarding compilation
with perl 5.22.

Sure.

OK, so after some re-lecture of the script and perltidy-ing I finish
with the attached. How does that look?

Attached is a new set for the support of MS 2015 + the psed issue,
please use those ones for testing:
- 0001 is the replacement of psed by a dedicated perl script to
generate probes.h
- 0002 Fix of the declaration of TIMEZONE_GLOBAL and TZNAME_GLOBAL for WIN32
- 0003 addresses the issue with lc_codepage missing from _locale_t.
- 0004 adds support for MS2015 in src/tools/scripts/

Thanks for doing this work.

Do we already have a hard dependency on perl for all non-Windows builds?
I know it's been discussed but I don't recall. If so, back to what version?

The comment in the codepage patch is a bit unclear. Can you reword it a bit?

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

Andrew Dunstan <andrew@dunslane.net> writes:

Do we already have a hard dependency on perl for all non-Windows builds?
I know it's been discussed but I don't recall. If so, back to what version?

I think the policy is we require perl for building from a git pull,
but not for building from a tarball. Thus, any files that perl is used
to produce have to be shipped in tarballs.

However, there definitely *is* a hard requirement on perl for Windows
builds, even from tarballs, and I thought this patch was only about
the Windows build?

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

On 03/08/2016 10:43 AM, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Do we already have a hard dependency on perl for all non-Windows builds?
I know it's been discussed but I don't recall. If so, back to what version?

I think the policy is we require perl for building from a git pull,
but not for building from a tarball. Thus, any files that perl is used
to produce have to be shipped in tarballs.

However, there definitely *is* a hard requirement on perl for Windows
builds, even from tarballs, and I thought this patch was only about
the Windows build?

Michael's patch proposes to replace the use of sed to generate probes.h
with the perl equivalent everywhere. That has the advantage that we keep
to one script to generate probes.h, but it does impose a perl dependency.

We could get around that by shipping probes.h in tarballs, which seems
like a perfectly reasonable thing to do. If we don't like that we can
fall back to using the perl script just for MSVC builds.

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

Andrew Dunstan <andrew@dunslane.net> writes:

On 03/08/2016 10:43 AM, Tom Lane wrote:

However, there definitely *is* a hard requirement on perl for Windows
builds, even from tarballs, and I thought this patch was only about
the Windows build?

Michael's patch proposes to replace the use of sed to generate probes.h
with the perl equivalent everywhere. That has the advantage that we keep
to one script to generate probes.h, but it does impose a perl dependency.

Meh.

We could get around that by shipping probes.h in tarballs, which seems
like a perfectly reasonable thing to do.

Well, it'd be messier than you think, because you could not just ship
a dummy probes.h, or make would think it was up to date and not replace
it even in a dtrace-using build. You'd have to ship it as something
like "probes.dummy.h" and cp it into place at build time.

On the whole, I'd rather that this patch left the non-Windows side alone.

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

On 03/08/2016 11:17 AM, Tom Lane wrote:

On the whole, I'd rather that this patch left the non-Windows side alone.

OK, that's why I raised the issue. We'll do it that way.

As I noted upthread, the sed script has been very stable so the overhead
of having to maintain two scripts is pretty minimal.

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

On Wed, Mar 9, 2016 at 1:07 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

Michael's patch proposes to replace the use of sed to generate probes.h with
the perl equivalent everywhere. That has the advantage that we keep to one
script to generate probes.h, but it does impose a perl dependency.

Good point. It did not occur to me that this would bring a hard
dependency for non-Windows builds. Let's keep both scripts then. The
attached is changed to do so.
--
Michael

On Tue, Mar 08, 2016 at 10:32:28PM +0900, Michael Paquier wrote:

Subject: [PATCH 3/4] Fix use of locales for VS 2015

lc_codepage is a flag missing from locale.h, causing this code path
introduced in VS 2012 to fail. Perhaps there is a reason for this field
to have been clobbered, but let's fall back to the pre-VS-2012 code
parsing directly LC_TYPE to get the codepage wanted.
---
src/port/chklocale.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/port/chklocale.c b/src/port/chklocale.c
index a551fdc..a7d88fb 100644
--- a/src/port/chklocale.c
+++ b/src/port/chklocale.c
@@ -203,7 +203,16 @@ win32_langinfo(const char *ctype)
{
char	   *r = NULL;

-#if (_MSC_VER >= 1700)
+	/*
+	 * lc_codepage is correctly declared in Visual Studio 2012 and 2013.
+	 * However in VS 2015 this flag is missing from locale.h, visibly this
+	 * is an error of refactoring from Microsoft that is at the origin of
+	 * this missing field, causing a compilation failure in this code path.
+	 * Hence, it is more reliable to fall back to other code path grabbing

No, the other path can't handle a "locale name" like "initdb --locale=th-TH".
That makes the other code path inadequate for VS2012 and later. See the
IsoLocaleName() header comment.

+	 * the codepage from the ctype name itself. If VS gets back this field
+	 * in the future, we may want to relax the use of _create_locale here.
+	 */
+#if (_MSC_VER >= 1700) && (_MSC_VER <= 1800)
_locale_t	loct = NULL;

loct = _create_locale(LC_CTYPE, ctype);

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

Good point. It did not occur to me that this would bring a hard
dependency for non-Windows builds. Let's keep both scripts then. The
attached is changed to do so.

Hello.
What about putenv problem? We can't write:
#define putenv(x) pgwin32_putenv(x)
because in new CRT putenv have different signature.

Thanks.
--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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

On 09/03/16 12:09, Yury Zhuravlev wrote:

Good point. It did not occur to me that this would bring a hard
dependency for non-Windows builds. Let's keep both scripts then. The
attached is changed to do so.

Hello.
What about putenv problem? We can't write:
#define putenv(x) pgwin32_putenv(x)
because in new CRT putenv have different signature.

Hmm, I don't see any problem there.

We should however add the msvc 2015 module to rtmodules in the
pgwin32_putenv so that we can run just with that runtime.

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

On 09/03/16 05:31, Noah Misch wrote:

On Tue, Mar 08, 2016 at 10:32:28PM +0900, Michael Paquier wrote:

Subject: [PATCH 3/4] Fix use of locales for VS 2015

lc_codepage is a flag missing from locale.h, causing this code path
introduced in VS 2012 to fail. Perhaps there is a reason for this field
to have been clobbered, but let's fall back to the pre-VS-2012 code
parsing directly LC_TYPE to get the codepage wanted.
---
src/port/chklocale.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/port/chklocale.c b/src/port/chklocale.c
index a551fdc..a7d88fb 100644
--- a/src/port/chklocale.c
+++ b/src/port/chklocale.c
@@ -203,7 +203,16 @@ win32_langinfo(const char *ctype)
{
char	   *r = NULL;

-#if (_MSC_VER >= 1700)
+	/*
+	 * lc_codepage is correctly declared in Visual Studio 2012 and 2013.
+	 * However in VS 2015 this flag is missing from locale.h, visibly this
+	 * is an error of refactoring from Microsoft that is at the origin of
+	 * this missing field, causing a compilation failure in this code path.
+	 * Hence, it is more reliable to fall back to other code path grabbing

No, the other path can't handle a "locale name" like "initdb --locale=th-TH".
That makes the other code path inadequate for VS2012 and later. See the
IsoLocaleName() header comment.

Agreed, that's what I was saying as well.

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
<Language>_<Country>.<CodePage> 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).

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

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

On 03/08/2016 07:40 PM, Michael Paquier wrote:

On Wed, Mar 9, 2016 at 1:07 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

Michael's patch proposes to replace the use of sed to generate probes.h with
the perl equivalent everywhere. That has the advantage that we keep to one
script to generate probes.h, but it does impose a perl dependency.

Good point. It did not occur to me that this would bring a hard
dependency for non-Windows builds. Let's keep both scripts then. The
attached is changed to do so.

Actually, it wasn't, but I fixed it and committed it.

Still to do: the non-perl pieces.

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

On Sun, Mar 20, 2016 at 8:06 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 03/08/2016 07:40 PM, Michael Paquier wrote:

On Wed, Mar 9, 2016 at 1:07 AM, Andrew Dunstan <andrew@dunslane.net>
wrote:

Michael's patch proposes to replace the use of sed to generate probes.h
with
the perl equivalent everywhere. That has the advantage that we keep to
one
script to generate probes.h, but it does impose a perl dependency.

Good point. It did not occur to me that this would bring a hard
dependency for non-Windows builds. Let's keep both scripts then. The
attached is changed to do so.

Actually, it wasn't, but I fixed it and committed it.

Ah, indeed, thanks. It looks like I have been bitten by an incorrect
rebase/cherry-pick.

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

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

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

On Sun, Mar 20, 2016 at 8:06 AM, Andrew Dunstan <andrew@dunslane.net> 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

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

On Sun, Mar 20, 2016 at 8:06 AM, Andrew Dunstan <andrew@dunslane.net> 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

On Wed, Mar 9, 2016 at 11:10 PM, Petr Jelinek <petr@2ndquadrant.com> 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
<Language>_<Country>.<CodePage> 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

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

On Wed, Mar 9, 2016 at 11:10 PM, Petr Jelinek <petr@2ndquadrant.com> 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
<Language>_<Country>.<CodePage> 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

On Mon, Mar 21, 2016 at 3:43 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

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

On Sun, Mar 20, 2016 at 8:06 AM, Andrew Dunstan <andrew@dunslane.net>
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:
/messages/by-id/CAB7nPqTDiXxS8CdL8mOiVh6qFQ-1qV9mKN0AyjzYBvzv6WC0dA@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

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

On Mon, Mar 21, 2016 at 3:43 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

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

On Sun, Mar 20, 2016 at 8:06 AM, Andrew Dunstan <andrew@dunslane.net>
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:
/messages/by-id/CAB7nPqTDiXxS8CdL8mOiVh6qFQ-1qV9mKN0AyjzYBvzv6WC0dA@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

On Wed, Mar 23, 2016 at 8:45 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:

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

On Mon, Mar 21, 2016 at 3:43 AM, Andrew Dunstan <andrew@dunslane.net>
wrote:

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

On Sun, Mar 20, 2016 at 8:06 AM, Andrew Dunstan <andrew@dunslane.net>
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:

/messages/by-id/CAB7nPqTDiXxS8CdL8mOiVh6qFQ-1qV9mKN0AyjzYBvzv6WC0dA@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

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:

/messages/by-id/CAB7nPqTDiXxS8CdL8mOiVh6qFQ-1qV9mKN0AyjzYBvzv6WC0dA@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

On Thu, Mar 24, 2016 at 5:18 AM, Petr Jelinek <petr@2ndquadrant.com> 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

On Wed, Mar 23, 2016 at 3:17 AM, Michael Paquier
<michael.paquier@gmail.com> 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

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

On Wed, Mar 23, 2016 at 3:17 AM, Michael Paquier
<michael.paquier@gmail.com> 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

On Thu, Mar 24, 2016 at 1:07 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:

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

On Wed, Mar 23, 2016 at 3:17 AM, Michael Paquier
<michael.paquier@gmail.com> 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

On Fri, Mar 25, 2016 at 9:09 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Mar 24, 2016 at 1:07 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:

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

On Wed, Mar 23, 2016 at 3:17 AM, Michael Paquier
<michael.paquier@gmail.com> 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

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

On Fri, Mar 25, 2016 at 9:09 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Mar 24, 2016 at 1:07 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:

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

On Wed, Mar 23, 2016 at 3:17 AM, Michael Paquier
<michael.paquier@gmail.com> 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

On Fri, Mar 25, 2016 at 8:31 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Fri, Mar 25, 2016 at 9:09 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Mar 24, 2016 at 1:07 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:

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

On Wed, Mar 23, 2016 at 3:17 AM, Michael Paquier
<michael.paquier@gmail.com> 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

On Fri, Mar 25, 2016 at 9:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Mar 25, 2016 at 8:31 AM, Michael Paquier
<michael.paquier@gmail.com> 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

On Fri, Mar 25, 2016 at 9:48 PM, Andrew Dunstan <andrew@dunslane.net> 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

On Fri, Mar 25, 2016 at 9:48 PM, Andrew Dunstan <andrew@dunslane.net> 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

On Sun, Mar 27, 2016 at 10:35 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Fri, Mar 25, 2016 at 9:48 PM, Andrew Dunstan <andrew@dunslane.net> 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

On Tue, Mar 29, 2016 at 10:06 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Sun, Mar 27, 2016 at 10:35 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Fri, Mar 25, 2016 at 9:48 PM, Andrew Dunstan <andrew@dunslane.net> 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

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

On Sun, Mar 27, 2016 at 10:35 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Fri, Mar 25, 2016 at 9:48 PM, Andrew Dunstan <andrew@dunslane.net> 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

On Tue, Mar 29, 2016 at 11:31 AM, Petr Jelinek <petr@2ndquadrant.com> 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

On Wed, Mar 30, 2016 at 12:45 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Mar 29, 2016 at 11:31 AM, Petr Jelinek <petr@2ndquadrant.com> 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

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

On Wed, Mar 30, 2016 at 12:45 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Mar 29, 2016 at 11:31 AM, Petr Jelinek <petr@2ndquadrant.com> 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

On Tue, Mar 29, 2016 at 9:29 PM, Andrew Dunstan <andrew@dunslane.net> 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

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

On Tue, Mar 29, 2016 at 9:29 PM, Andrew Dunstan <andrew@dunslane.net> 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

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 <andrew@dunslane.net>
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

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 Herrera http://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

On Thu, Apr 7, 2016 at 6:11 AM, Petr Jelinek <petr@2ndquadrant.com> 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

On Thu, Apr 7, 2016 at 7:44 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Thu, Apr 7, 2016 at 6:11 AM, Petr Jelinek <petr@2ndquadrant.com> 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

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

On Thu, Apr 7, 2016 at 7:44 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Thu, Apr 7, 2016 at 6:11 AM, Petr Jelinek <petr@2ndquadrant.com> 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

* Petr Jelinek wrote:

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

On Thu, Apr 7, 2016 at 7:44 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Thu, Apr 7, 2016 at 6:11 AM, Petr Jelinek <petr@2ndquadrant.com>
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 <PlatformToolset>
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

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 <andrew@dunslane.net>
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

On Fri, Apr 8, 2016 at 9:29 AM, Andrew Dunstan <andrew@dunslane.net> 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

* Michael Paquier wrote:

On Fri, Apr 8, 2016 at 9:29 AM, Andrew Dunstan <andrew@dunslane.net>
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 <winnls.h> 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

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

* Michael Paquier wrote:

On Fri, Apr 8, 2016 at 9:29 AM, Andrew Dunstan <andrew@dunslane.net>
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 <winnls.h> 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

Andrew Dunstan <andrew@dunslane.net> 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

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

Andrew Dunstan <andrew@dunslane.net> 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

On Fri, Apr 8, 2016 at 10:05 PM, Andrew Dunstan <andrew@dunslane.net> 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

On Fri, Apr 8, 2016 at 4:12 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Fri, Apr 8, 2016 at 10:05 PM, Andrew Dunstan <andrew@dunslane.net>
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

* Michael Paquier wrote:

On Fri, Apr 8, 2016 at 10:05 PM, Andrew Dunstan <andrew@dunslane.net> 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

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

* Michael Paquier wrote:

On Fri, Apr 8, 2016 at 10:05 PM, Andrew Dunstan <andrew@dunslane.net>
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

* 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

On Sat, Apr 9, 2016 at 1:46 AM, Christian Ullrich <chris@chrullrich.net> 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

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

On Sat, Apr 9, 2016 at 1:46 AM, Christian Ullrich <chris@chrullrich.net> 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

On Sat, Apr 9, 2016 at 7:41 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Sat, Apr 9, 2016 at 1:46 AM, Christian Ullrich <chris@chrullrich.net> 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

Michael Paquier wrote:

On Sat, Apr 9, 2016 at 7:41 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Sat, Apr 9, 2016 at 1:46 AM, Christian Ullrich <chris@chrullrich.net> 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

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

Michael Paquier wrote:

On Sat, Apr 9, 2016 at 7:41 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Sat, Apr 9, 2016 at 1:46 AM, Christian Ullrich <chris@chrullrich.net> 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

* 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

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

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

On Mon, Apr 11, 2016 at 6:03 AM, Petr Jelinek <petr@2ndquadrant.com> 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

On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
<michael.paquier@gmail.com> 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

Michael Paquier wrote:

On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
<michael.paquier@gmail.com> 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 Herrera http://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

On Tue, Apr 19, 2016 at 2:42 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Michael Paquier wrote:

On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
<michael.paquier@gmail.com> 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

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

On Tue, Apr 19, 2016 at 2:42 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Michael Paquier wrote:

On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
<michael.paquier@gmail.com> 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

* Alvaro Herrera wrote:

Michael Paquier wrote:

On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
<michael.paquier@gmail.com> 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

On Wed, Apr 20, 2016 at 1:48 AM, Christian Ullrich <chris@chrullrich.net> 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

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
<michael.paquier@gmail.com> 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

* Michael Paquier wrote:

On Wed, Apr 20, 2016 at 1:48 AM, Christian Ullrich <chris@chrullrich.net> 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

On Wed, Apr 20, 2016 at 1:39 PM, Noah Misch <noah@leadboat.com> 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
<michael.paquier@gmail.com> 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

On Wed, Apr 20, 2016 at 02:03:16PM +0900, Michael Paquier wrote:

On Wed, Apr 20, 2016 at 1:39 PM, Noah Misch <noah@leadboat.com> 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

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

On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
<michael.paquier@gmail.com> 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

* From: Andrew Dunstan [mailto:andrew@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 <whatever.h>
#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

Andrew Dunstan <andrew@dunslane.net> 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

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

Andrew Dunstan <andrew@dunslane.net> 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

On Fri, Apr 22, 2016 at 4:56 AM, Christian Ullrich <chris@chrullrich.net> wrote:

* From: Andrew Dunstan [mailto:andrew@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 <whatever.h>
#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

On Fri, Apr 22, 2016 at 4:39 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

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

On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
<michael.paquier@gmail.com> 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

On Fri, Apr 22, 2016 at 1:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew@dunslane.net> 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

Michael Paquier <michael.paquier@gmail.com> writes:

On Fri, Apr 22, 2016 at 1:30 PM, Tom Lane <tgl@sss.pgh.pa.us> 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

On Fri, Apr 22, 2016 at 10:49 PM, Tom Lane <tgl@sss.pgh.pa.us> 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

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
<https://social.msdn.microsoft.com/Forums/sqlserver/en-US/ea671596-9e8d-4e80-bb1f-8c9bfb1738dc/2012-and-2015-solutions?forum=vcgeneral&gt;
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

Michael Paquier <michael.paquier@gmail.com> writes:

On Fri, Apr 22, 2016 at 10:49 PM, Tom Lane <tgl@sss.pgh.pa.us> 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

* 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
<https://social.msdn.microsoft.com/Forums/sqlserver/en-US/ea671596-9e8d-4e80-bb1f-8c9bfb1738dc/2012-and-2015-solutions?forum=vcgeneral&gt;
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:

-
<https://social.msdn.microsoft.com/Forums/sqlserver/en-US/2d08da92-f16a-4887-935a-e3159104ff2c/&gt;
- "converted to VS2015 in anticipation of the release"

-
<https://connect.microsoft.com/VisualStudio/feedback/details/1909707/getting-empty-folder-in-project-root-after-opening-sln-file-dtar-08e86330-4835-4b5c-9e5a-61f37ae1a077-dtar&gt;
- "but I changed the the contents of the .SLN to Microsoft Visual Studio
Solution File, Format Version 14.00"

-
<https://fossies.org/linux/ACE/examples/Logger/Acceptor-server/Acceptor_server_vc14.sln&gt;
- "# 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

* 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
<https://social.msdn.microsoft.com/Forums/sqlserver/en-US/ea671596-9e8d-4e80-bb1f-8c9bfb1738dc/2012-and-2015-solutions?forum=vcgeneral&gt;

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

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

* 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

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

Andrew Dunstan <andrew@dunslane.net> 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

I wrote:

Andrew Dunstan <andrew@dunslane.net> 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

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

I wrote:

Andrew Dunstan <andrew@dunslane.net> 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

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
<https://social.msdn.microsoft.com/Forums/sqlserver/en-US/ea671596-9e8d-4e80-bb1f-8c9bfb1738dc/2012-and-2015-solutions?forum=vcgeneral&gt;

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

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

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

I wrote:

Andrew Dunstan <andrew@dunslane.net> 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

Andrew Dunstan <andrew@dunslane.net> 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 <dbghelp.h>
+#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

Andrew Dunstan <andrew@dunslane.net> 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

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

Andrew Dunstan <andrew@dunslane.net> 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 <dbghelp.h>
+#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

Andrew Dunstan <andrew@dunslane.net> 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

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

Andrew Dunstan <andrew@dunslane.net> 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

Andrew Dunstan <andrew@dunslane.net> 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

* Andrew Dunstan wrote:

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

Andrew Dunstan <andrew@dunslane.net> 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

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 <andrew@dunslane.net> 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

* 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

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

+  <productname>Visual Studio 2008</productname> and above. Compilation
+  is supported down to <productname>Windows XP</productname> and
+  <productname>Windows Server 2003</> when building with
+  <productname>Visual Studio 2005</> to
+  <productname>Visual Studio 2013</productname>. Building with
+  <productname>Visual Studio 2015</productname> is supported down to
+  <productname>Windows Vista</> and <productname>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_t	bgchild_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:

<https://msdn.microsoft.com/en-us/library/windows/desktop/dd373815(v=vs.85).aspx&gt;:
'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

On Mon, Apr 25, 2016 at 4:29 AM, Christian Ullrich <chris@chrullrich.net> 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:

<https://msdn.microsoft.com/en-us/library/windows/desktop/dd373815(v=vs.85).aspx&gt;:
'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 <dbghelp.h>
+#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

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

On Mon, Apr 25, 2016 at 4:29 AM, Christian Ullrich <chris@chrullrich.net> 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:

<https://msdn.microsoft.com/en-us/library/windows/desktop/dd373815(v=vs.85).aspx&gt;:
'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 <dbghelp.h>
+#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

Andrew Dunstan <andrew@dunslane.net> 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

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

Andrew Dunstan <andrew@dunslane.net> 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

On Fri, Apr 29, 2016 at 9:13 PM, Andrew Dunstan <andrew@dunslane.net> 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