Re: [HACKERS] Build failure on thrips

[Christian Ullrich]

* Michael Meskes wrote:

>> It's not just thrips.  Of the Windows machines that have reported in
>> since
>> that commit, only currawong and baiji passed.  Although lorikeet,
>> woodlouse, bowerbird, and brolga are showing green, this proves
>> nothing
>> because they're all configured to skip the ecpg check (... I wonder
>> why).
> 
> I would assume it works on woodlouse as the patch has been submitted by
> woodlouse's owner.

woodlouse does not do ecpg-check, and Tom is right anyway. I need to get a 
large sign nailed to my forehead that says "Postgres is C, and declarations 
must come first."

I'm not sure, and not able to find out right this moment, why thrips' compiler 
does not accept this code now, because I tested it on thrips (and woodlouse, 
IIRC) before I sent it in. 

-- 
Christian

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

Re: [HACKERS] ECPG: WHENEVER statement with DO CONTINUE action

[Christian Ullrich]

* Michael Meskes wrote:


The v3 patch looks good to me. I've changed this patch status to Ready
for Committer.


Thank you all, committed.


The buildfarm says that sorting is frequently done case-sensitively:

*** 1,2 
- josh 1.00 10.00
  Ram 00.00 21.00
--- 1,2 
  Ram 00.00 21.00
+ josh 1.00 10.00

--
Christian


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

Re: [HACKERS] visual studio 2017 build support

[Christian Ullrich]

* On 2017-06-21 02:06, Haribabu Kommi wrote:


Thanks for the review. Here I attached an updated patch with README update.


Hello,

the most recent update to VS 2017, version 15.3, now identifies as 
"14.11" rather than "14.10" in the output of nmake /?. Simply adding 
this value to the two places that check for 14.10 in your patch appears 
to work for me.


In a newly created project, PlatformToolset is still "v141". 
ToolsVersion is "15.0" whereas your patch uses "14.1".


ISTM that the ToolsVersion has been like this in all versions of VS 
2017; in my collection of .vcxproj files the auto-generated PostgreSQL 
projects are the only ones using "14.1".


The build is successful with either value.

I think there was some discussion on this topic, but I cannot find it in 
the archives. If there was, I certainly don't want to reopen any 
discussions.


All the best,

--
Christian


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

Re: [HACKERS] Buildfarm failures on woodlouse (in ecpg-check)

[Christian Ullrich]

* I wrote:


If there is interest in fixing this issue that is apparently limited
to VS 2013, I will try and produce a proper patch.


Patch.

Perhaps surprisingly, the bug is in the failing test cases themselves, 
not ecpg. The CRT has two modes for its locale implementation: When 
called in a "global" mode thread, setlocale() affects all global mode 
threads; when called from a per-thread mode thread, it affects that 
thread only. [1]


In the threaded test cases, many global mode threads call setlocale() 
simultaneously, getting in each other's way. The fix is to switch the 
worker threads to per-thread mode first.


Without this patch, I see crashes in approximately 25 percent of runs 
(four crashes in 16 cycles of vcregress ecpgcheck, ~10 in 50 runs of 
thread.exe alone). With the patch, I have no crashes in 100 ecpgcheck runs.


On the other hand, this affects only the buildfarm VM I mentioned 
earlier. I have another VM where it does not ever crash even without the 
patch -- such are the joys of multithreading. Both of them should have 
plenty of cores, both physical and virtual.



There are some alternatives to fixing it this way, but I think this is 
the best approach:


- Selecting per-thread mode in ecpglib takes the choice away from the
  developer who might want shared locale.

- Adding locking around setlocale() is difficult because ecpglib already
  uses a wrapper around the CRT function, provided by libpgport.

  These test cases are the only consumers of the wrapper that have any
  concept of multithreading. Supporting it in libpgport for the sole
  benefit of threaded ECPG applications on Windows does not seem to
  be a good idea, and re-wrapping the wrapper in ecpglib is not only
  beyond my abilities to write, but is also going to be unmaintainable.

- Adding locking around every setlocale() call in ecpglib is just ugly.


While working on this, I also noticed that there seem to be two separate 
partial implementations of a pthread synchronization emulation for 
Win32. One is in ecpglib, uses mutexes and provides 
PTHREAD_MUTEX_INITIALIZER and pthread_once(), the other has the header 
in src/port and the implementation in libpq, uses critical sections and 
does not cover either feature.


Should the two be merged at some point?


[1] https://msdn.microsoft.com/en-us/library/ms235302(v=vs.120).aspx

--
Christian
From 5dee698f4cef684a320ced59b19cd4fea61319fb Mon Sep 17 00:00:00 2001
From: Christian Ullrich 
Date: Wed, 14 Jun 2017 22:18:18 +0200
Subject: [PATCH] Make setlocale() aware of multithreading to avoid crash.

---
 src/interfaces/ecpg/test/expected/thread-alloc.c   | 39 +++--
 .../ecpg/test/expected/thread-descriptor.c | 19 +++---
 src/interfaces/ecpg/test/expected/thread-prep.c| 67 --
 src/interfaces/ecpg/test/expected/thread-thread.c  | 60 ++-
 .../ecpg/test/expected/thread-thread_implicit.c| 60 ++-
 src/interfaces/ecpg/test/thread/alloc.pgc  |  5 ++
 src/interfaces/ecpg/test/thread/descriptor.pgc |  5 ++
 src/interfaces/ecpg/test/thread/prep.pgc   |  5 ++
 src/interfaces/ecpg/test/thread/thread.pgc |  6 ++
 .../ecpg/test/thread/thread_implicit.pgc   |  6 ++
 10 files changed, 163 insertions(+), 109 deletions(-)

diff --git a/src/interfaces/ecpg/test/expected/thread-alloc.c 
b/src/interfaces/ecpg/test/expected/thread-alloc.c
index 49f1cd1..1312580 100644
--- a/src/interfaces/ecpg/test/expected/thread-alloc.c
+++ b/src/interfaces/ecpg/test/expected/thread-alloc.c
@@ -22,6 +22,7 @@ main(void)
 #define WIN32_LEAN_AND_MEAN
 #include 
 #include 
+#include 
 #else
 #include 
 #endif
@@ -99,7 +100,7 @@ struct sqlca_t *ECPGget_sqlca(void);
 
 #endif
 
-#line 24 "alloc.pgc"
+#line 25 "alloc.pgc"
 
 
 #line 1 "regression.h"
@@ -109,14 +110,14 @@ struct sqlca_t *ECPGget_sqlca(void);
 
 
 
-#line 25 "alloc.pgc"
+#line 26 "alloc.pgc"
 
 
 /* exec sql whenever sqlerror  sqlprint ; */
-#line 27 "alloc.pgc"
+#line 28 "alloc.pgc"
 
 /* exec sql whenever not found  sqlprint ; */
-#line 28 "alloc.pgc"
+#line 29 "alloc.pgc"
 
 
 #ifdef WIN32
@@ -127,59 +128,63 @@ static void* fn(void* arg)
 {
int i;
 
+#ifdef WIN32
+   _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
+#endif
+
/* exec sql begin declare section */
  
 
   

-#line 39 "alloc.pgc"
+#line 44 "alloc.pgc"
  int value ;
  
-#line 40 "alloc.pgc"
+#line 45 "alloc.pgc"
  char name [ 100 ] ;
  
-#line 41 "alloc.pgc"
+#line 46 "alloc.pgc"
  char ** r = NULL ;
 /* exec sql end declare section */
-#line 42 "alloc.pgc"
+#line 47 "alloc.pgc"
 
 
value = (long)arg;
sprintf(name, "Connection: %d", value);
 
{ ECPGconnect(__LINE__, 0, "ecpg1_regression" , NULL

Re: [HACKERS] Buildfarm failures on woodlouse (in ecpg-check)

[Christian Ullrich]

* Andrew Dunstan wrote:


On 06/11/2017 11:33 AM, Christian Ullrich wrote:



To build correctly, it requires defining _WIN32_WINNT to be 0x600 or
above (and using an SDK that knows about InitOnceExecuteOnce()).



We already define _WIN32_WINNT to be 0x0600 on all appropriate platforms
(Vista/2008 and above), so I think you could probably just check for
that value.


Not quite; the definition depends on the build toolset, not the build 
platform. When building with VS 2015 and above, _WIN32_WINNT is set to 
0x600 (Vista/2008), while with older compilers, the value is 0x501 
(XP/2003). This is also due to locale issues, but of a different kind, 
and is apparently coincidental.


The build platform does not figure into the target platform, which is 
clearly a good idea for binary distribution reasons.


--
Christian



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

[HACKERS] Buildfarm failures on woodlouse (in ecpg-check)

[Christian Ullrich]

Hello,

my buildfarm animal woodlouse (Visual Studio 2013 on Windows 7) stopped 
working correctly some months ago. After Tom kindly prodded me into 
fixing it, I noticed that I had configured it to skip the ecpg-check 
step because one of the tests in the "thread" section (not always the 
same) nearly always failed.


I had set it up in circa 2015, so I reenabled the step to see whether 
anything had changed since, and it started failing again.


Through some trial and error, and with the help of Microsoft's 
Application Verifier tool, I found what I think is the cause: It looks 
like the VS 2013 CRT's setlocale() function is not entirely thread-safe; 
the heap debugging options make it crash when manipulating per-thread 
locale state, and according to the comments surrounding that spot in the 
CRT source, the developers were aware the code is fragile.


I crudely hacked a critical section around the setlocale() call in 
pgwin32_setlocale(). After this change, the test does not crash, while 
without it, it crashes every time.


If there is interest in fixing this issue that is apparently limited to 
VS 2013, I will try and produce a proper patch. I notice, however, that 
there is a pthread compatibility layer available that I have no 
experience with at all, and I assume using it is preferred over straight 
Win32?


My hack is attached; please feel free to tell me I'm on the wrong track.
To build correctly, it requires defining _WIN32_WINNT to be 0x600 or 
above (and using an SDK that knows about InitOnceExecuteOnce()).


--
Christian
diff --git a/src/port/win32setlocale.c b/src/port/win32setlocale.c
index cbf109836b..278d836b4d 100644
--- a/src/port/win32setlocale.c
+++ b/src/port/win32setlocale.c
@@ -164,19 +164,33 @@ map_locale(const struct locale_map * map, const char 
*locale)
return locale;
 }
 
+static CRITICAL_SECTION setlocale_cs;
+static INIT_ONCE init_once;
+static BOOL CALLBACK init_setlocale_cs(PINIT_ONCE pInitOnce, PVOID pParam, 
PVOID pCtx)
+{
+   InitializeCriticalSection((PCRITICAL_SECTION)pParam);
+   return TRUE;
+}
+
 char *
 pgwin32_setlocale(int category, const char *locale)
 {
const char *argument;
char   *result;
 
+   if (InitOnceExecuteOnce(&init_once, init_setlocale_cs, 
(PVOID)&setlocale_cs, NULL) == 0) {
+   abort();
+   }
+
if (locale == NULL)
argument = NULL;
else
argument = map_locale(locale_map_argument, locale);
 
/* Call the real setlocale() function */
+   EnterCriticalSection(&setlocale_cs);
result = setlocale(category, argument);
+   LeaveCriticalSection(&setlocale_cs);
 
/*
 * setlocale() is specified to return a "char *" that the caller is

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

Re: [HACKERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* From: Michael Paquier

> With 0005 I am seeing a compilation failure: you need to have the
> declarations in the _MSC_VER block at the beginning of the routine. It

Sorry, too used to C++.

> would be nice to mention in a code comment that this what Noah has
> mentioned upthread: if a CRT loads while pgwin32_putenv() is
> executing, the newly-loaded CRT will always have the latest value.

I fixed the existing comment by removing the last sentence that is in the wrong 
place now, but I don't see the point in suddenly starting to explain why it is 
done this way and not the other.

> Based on that 0006 will need a rebase, nothing huge though.

Done, new revisions attached.

-- 
Christian



0005-Do-the-process-environment-update-first.patch
Description: 0005-Do-the-process-environment-update-first.patch


0006-Fix-the-unload-race-as-best-we-can.patch
Description: 0006-Fix-the-unload-race-as-best-we-can.patch

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

Re: [HACKERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* Michael Paquier wrote:

> On Wed, Nov 16, 2016 at 12:45 PM, Christian Ullrich
>  wrote:
>> I also did a debug build with 1+2+4 that came in at 84 μs/iteration.
>
> Moved to next CF. Christian, perhaps this patch should have an extra
> round of reviews?

It is significantly different from the last version, so yes, of course.

--
Christian



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

Re: [HACKERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* Noah Misch wrote:

> On Tue, Apr 26, 2016 at 07:39:29PM +0200, Christian Ullrich
> wrote:

> > * Christian Ullrich wrote:

> Patch 1 looks good, except that it should be three patches:
> 
> - cosmetic parts: change whitespace and s/0/NULL/
> - remove CloseHandle() call
> - probe for debug CRT modules, not just release CRT modules

Attached as 0001, 0002, 0003, in that order.

0004 is what used to be 0002, it disables caching of "DLL not 
loaded" results.

> I recommend also moving the SetEnvironmentVariable() call before
> the putenv calls.  That way, if a CRT loads while pgwin32_putenv()
> is executing, the newly-loaded CRT will always have the latest
> value.

Agreed, attached as 0005.

0006 was previously known as 0004, removing all caching.

> > Even with patch 0004, there is still a race condition between
> > the main thread and a theoretical additional thread created by
> > some other component that unloads some CRT between
> > GetProcAddress() and the _putenv() call, but that is hardly
> > fixable.
> 
> I think you can fix it by abandoning GetModuleHandle() in favor
> of GetModuleHandleEx() + GetProcessAddress() + FreeLibrary(). 

Attached as 0007.

> > There is another possible fix, ugly as sin, if we really need
> > to keep the whole environment update machinery *and* cannot do 
> > the full loop each time. Patch 0003 pins each CRT when we see 
> > it for the first time.

This is now 0008.

Patch topology: master --- 1 .. 5 --- 6 --- 7
  \
   `- 8

> I prefer the simplicity of abandoning the cache (patch 4), if it
> performs decently.  Would you compare the performance of patch 1,
> patches 1+2+3, and patches 1+2+4?  This should measure the right 
> thing (after substituting @libdir@ for your environment):

Tested with release builds; Core i7-6700K (quad/HT; 4000 MHz).
I did three runs each, and they were always within 0.5 % of each
other's run time.

- patch 1 (now 1-3): 24 μs/iteration (24 s for 1e6)
- patch 1+2+3 (now 1-5 + 8): 29 μs/iteration
- patch 1+2+4 (now 1-7): 30 μs/iteration

I also did a debug build with 1+2+4 that came in at 84 μs/iteration.

--
Christian

... now how do I get all the dangling debris out of the repo ...


0008-getmodulehandleex-pin.patch
Description: 0008-getmodulehandleex-pin.patch


0001-whitespace.patch
Description: 0001-whitespace.patch


0002-closehandle.patch
Description: 0002-closehandle.patch


0003-debug-crt.patch
Description: 0003-debug-crt.patch


0004 no-caching-notfound.patch
Description: 0004 no-caching-notfound.patch


0005-reorder-update.patch
Description: 0005-reorder-update.patch


0006-no-caching-at-all.patch
Description: 0006-no-caching-at-all.patch


0007-getmodulehandleex-correctness.patch
Description: 0007-getmodulehandleex-correctness.patch

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

Re: [HACKERS] Parallel build with MSVC

[Christian Ullrich]

* Noah Misch wrote:


Committed.


Much apologizings for coming in late again, but I just realized it would 
be better if the user-controlled flags came after all predefined options 
the user might want to override. Right now that is only /verbosity in 
both build and clean operations.


Patch attached, also reordering the ecpg-clean command line in clean.bat 
to match the others that have the project file first.


--
Christian


>From 09a5f3945e2b87b56ca3525a56db970e9ecf8ffd Mon Sep 17 00:00:00 2001
From: Christian Ullrich 
Date: Thu, 8 Sep 2016 08:34:45 +0200
Subject: [PATCH] Let MSBFLAGS be used to override predefined options.

---
 src/tools/msvc/build.pl  | 4 ++--
 src/tools/msvc/clean.bat | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/tools/msvc/build.pl b/src/tools/msvc/build.pl
index 5273977..a5469cd 100644
--- a/src/tools/msvc/build.pl
+++ b/src/tools/msvc/build.pl
@@ -54,7 +54,7 @@ elsif (uc($ARGV[0]) ne "RELEASE")
 if ($buildwhat and $vcver >= 10.00)
 {
system(
-   "msbuild $buildwhat.vcxproj $msbflags /verbosity:normal 
/p:Configuration=$bconf"
+   "msbuild $buildwhat.vcxproj /verbosity:normal $msbflags 
/p:Configuration=$bconf"
);
 }
 elsif ($buildwhat)
@@ -63,7 +63,7 @@ elsif ($buildwhat)
 }
 else
 {
-   system("msbuild pgsql.sln $msbflags /verbosity:normal 
/p:Configuration=$bconf");
+   system("msbuild pgsql.sln /verbosity:normal $msbflags 
/p:Configuration=$bconf");
 }
 
 # report status
diff --git a/src/tools/msvc/clean.bat b/src/tools/msvc/clean.bat
index e21e37f..b972ddf 100755
--- a/src/tools/msvc/clean.bat
+++ b/src/tools/msvc/clean.bat
@@ -107,6 +107,6 @@ REM for /r %%f in (*.sql) do if exist %%f.in del %%f
 cd %D%
 
 REM Clean up ecpg regression test files
-msbuild %MSBFLAGS% /NoLogo ecpg_regression.proj /t:clean /v:q
+msbuild ecpg_regression.proj /NoLogo /v:q %MSBFLAGS% /t:clean 
 
 goto :eof
-- 
2.10.0.windows.1


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

Re: [HACKERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* Michael Paquier wrote:


On Tue, Sep 6, 2016 at 5:36 PM, Christian Ullrich  wrote:

* Michael Paquier wrote:



In order to avoid any problems with the load and unload windows, my
bet goes for 0001, 0002 and 0003, with the last two patches merged
together, 0001 being only a set of independent fixes. That's ugly, but
that looks the safest course of actions. I have rebased/rewritten the
patches as attached. Thoughts?


In lieu of convincing you to drop the entire thing, yes, that looks about
right, except for the BOOL thing.


Yes, right. Thanks. Patch 0001 is definitely something that should be
applied and backpatched, the CloseHandle() call is buggy. Now 0002 and
0003 are improving things, but there have been no reports regarding
problems in this area, so this could just be applied to master I
guess. Christian, does that sound right?


Yes.


By the way, how is it decided that a DLL gets unloaded in a process if
it is not pinned? Is that something the OS decides?


Reference counting in LoadLibrary() and FreeLibrary(), among other 
places. For example, GetModuleHandleEx() (but not the non-Ex) will by 
default increment the counter.


--
Christian


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

Re: [HACKERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* Michael Paquier wrote:


On Thu, Sep 1, 2016 at 4:03 PM, Christian Ullrich  wrote:



My conclusion from April stands:


The fact that master looks like it does means that there have not been
many (or any) complaints about missing cross-module environment
variables. If nobody ever needs to see a variable set elsewhere, we
have a very simple solution: Why don't we simply throw out the whole
#ifdef _MSC_VER block?


Looking at the commit logs and 741e4ad7 that does not sound like a good idea.


Well, I still maintain that if it doesn't work and has never worked, 
getting rid of it is better than making it work six years after the 
fact. OTOH, there may have been cases where it did actually work, 
perhaps those gnuwin32 libraries that were mentioned in the comment 
before it was changed in that commit above, if they were loaded before 
the first call to the function.


OTTH, wouldn't it be funny if fixing it actually broke something that 
worked accidentally because it *didn't* get the updated environment? I 
think that is at least as likely as suddenly getting excited reports 
that something now works that hasn't before.



It is better to avoid !!. See for example 1a7a436 that avoided
problems with VS2015 as far as I recall.


Agreed, thanks for noticing. This change creates a warning, however, 
because GetModuleHandleEx() returns BOOL, not HMODULE. Updated 0003 
attached, simplified over my original one.



In order to avoid any problems with the load and unload windows, my
bet goes for 0001, 0002 and 0003, with the last two patches merged
together, 0001 being only a set of independent fixes. That's ugly, but
that looks the safest course of actions. I have rebased/rewritten the
patches as attached. Thoughts?


In lieu of convincing you to drop the entire thing, yes, that looks 
about right, except for the BOOL thing.


--
Christian

>From dfbe7384b309c20d7733b0d18e6819302a483d95 Mon Sep 17 00:00:00 2001
From: Christian Ullrich 
Date: Tue, 6 Sep 2016 10:02:06 +0200
Subject: [PATCH] Pin any DLL as soon as seen when looking for _putenv()

---
 src/port/win32env.c | 54 ++---
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/src/port/win32env.c b/src/port/win32env.c
index 3f56ba8..7417b60 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -43,36 +43,37 @@ pgwin32_putenv(const char *envval)
char   *modulename;
HMODULE hmodule;
PUTENVPROC  putenvFunc;
+   BOOLpinned;
}   rtmodules[] =
{
/* Visual Studio 6.0 / mingw */
-   {"msvcrt",  NULL,   NULL},
-   {"msvcrtd", NULL,   NULL},
+   {"msvcrt",  NULL,   NULL,   FALSE},
+   {"msvcrtd", NULL,   NULL,   FALSE},
/* Visual Studio 2002 */
-   {"msvcr70", NULL,   NULL},
-   {"msvcr70d",NULL,   NULL},
+   {"msvcr70", NULL,   NULL,   FALSE},
+   {"msvcr70d",NULL,   NULL,   FALSE},
/* Visual Studio 2003 */
-   {"msvcr71", NULL,   NULL},
-   {"msvcr71d",NULL,   NULL},
+   {"msvcr71", NULL,   NULL,   FALSE},
+   {"msvcr71d",NULL,   NULL,   FALSE},
/* Visual Studio 2005 */
-   {"msvcr80", NULL,   NULL},
-   {"msvcr80d",NULL,   NULL},
+   {"msvcr80", NULL,   NULL,   FALSE},
+   {"msvcr80d",NULL,   NULL,   FALSE},
/* Visual Studio 2008 */
-   {"msvcr90", NULL,   NULL},
-   {"msvcr90d",NULL,   NULL},
+   {"msvcr90", NULL,   NULL,   FALSE},
+   {"msvcr90d",NULL,   NULL,   FALSE},
/* Visual Studio 2010 */
-   {"msvcr100",NULL,   NULL},
-   {"msvcr100d",   NULL,   NULL},
+   {"msvcr100",NULL,   NULL,   FALSE},
+   {"msvcr100d",   NULL,   NULL,   FALSE},
/* Visual Studio 2012 */
-   {"msvcr110",NULL,   NULL},
-   {"msvcr110d",   NULL,   NULL},
+   {"msvcr110",NULL,   NULL,   FALSE},
+   {"msvcr110d",   NULL,   NULL,   FALSE},
/* Visual Studio 2013 */
-   {"msvcr120",NULL,   NULL},
-   {"msvcr120d",   NULL,   NULL},
+   {"msvcr120",NULL,   NULL,   FALSE},
+   

Re: [HACKERS] Parallel build with MSVC

[Christian Ullrich]

* Michael Paquier wrote:


On Mon, Sep 5, 2016 at 9:18 AM, Noah Misch  wrote:



Every vcbuild and msbuild invocation ought to recognize this variable, so
please update the two places involving ecpg_regression.proj.  Apart from that,
the patch looks good.


Good catch. I did not notice those during my lookups of those patches.
Not my intention to bump into Christian's feet, but here are updated
patches.


My feet feel fine. Thanks for updating.

--
Christian



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

Re: [HACKERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* Michael Paquier wrote:

> On Wed, Apr 27, 2016 at 2:39 AM, Christian Ullrich 
 wrote:


>> * Christian Ullrich wrote:

> And actually, by looking at those patches, isn't it a dangerous
> practice to be able to load multiple versions of the same DLL routines
> in the same workspace? I have personally very bad souvenirs with that,

No, it is exactly what the version-specific CRTs are meant to allow. 
Each module uses the CRT version it needs, and they don't share any 
state, so absent bugs, they cannot conflict.


Of the processes currently running on my system, 25 have more than one 
CRT loaded (one has three, the others two).


> and particularly loading multiple versions of msvcr into the same
> workspace can cause unwanted crashes and corruptions of processes. In
> short I mean this thing: https://en.wikipedia.org/wiki/DLL_Hell.

That was about DLLs existing in different versions with the same file 
name, and installers replacing them with their own, leading to problems 
with other applications that expected to load their preferred version. 
This does not apply to the multiple-CRT situation, because all minor 
versions of a given CRT are supposed to be ABI compatible.


> So, shouldn't we first make the DLL list a little bit more severe
> depending on the value of _MSC_VER? I would mean something like that:
> #ifdef _MSC_VER >= 1900
> {"ucrtbase",NULL,   NULL},
> #elif _MSC_VER >= 1800
> {"msvcr120",NULL,   NULL},
> #elif etc, etc.
> [...]
> #endif
>
> This would require modules to be built using the same msvc version as
> the core server, but that's better than just plain crash if loaded
> DLLs corrupt the stack. Am I missing something?

Yes: This turns (this part of) pgwin32_putenv() into a great big NOP. We 
call putenv() anyway on the very last line of the function, so if we 
require common-CRT builds, that call alone (together with the 
SetEnvironmentVariable() just above) is sufficient.


That said, introducing this requirement would be a very significant 
change. I'm not sure how many independently maintained compiled 
extensions there are, but this would mean that their developers would 
have to have the matching VS versions for every PG distribution they 
want to support. Even if that's just EDB, it still is a lot of effort.


My conclusion from April stands:

> The fact that master looks like it does means that there have not been
> many (or any) complaints about missing cross-module environment
> variables. If nobody ever needs to see a variable set elsewhere, we
> have a very simple solution: Why don't we simply throw out the whole
> #ifdef _MSC_VER block?

--
Christian



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

Re: [HACKERS] 10.0

[Christian Ullrich]

* Tom Lane wrote:


So I think we should solve these problems at a stroke, and save ourselves
lots of breath in the future, by getting rid of the whole "major major"
idea and going over to a two-part version numbering scheme.  To be
specific:

* This year's major release will be 9.6.0, with minor updates 9.6.1,
9.6.2, etc.  It's too late to do otherwise for this release cycle.

* Next year's major release will be 10.0, with minor updates 10.1,
10.2, etc.

* The year after, 11.0.  Etc cetera.


When threaded Postgres is introduced in 2021, the version goes from 13 
to 14. Not quite worthy of that momentous occasion.


Losing a third of the version number means losing the ability to 
differentiate between significant-but-still-incremental improvements and 
(never-again)-stop-the-presses-this-is-great improvements; it's all just 
another major version. (Unless you'd want to jump from 13 to 20, 
effectively making the first digit special.)


Finally, from the, er, visual point of view, 10.1 and 10.2 look good and 
modern, but what about 10.13, 10.17, 10.22?


* Elsewhere, Tom Lane wrote:

> There's also the problem that the current numbering scheme confuses
> people who aren't familiar with the project.  How many times have
> you seen people say "I'm using Postgres 8" or "Postgres 9" when asked
> what version they're on?

Knowing only the first component would be very nearly as useless with 
two-part version numbers as it is today. The conversation will continue 
just the same:


- "No, what is the exact version?"
- ...
- "There have been several updates for that version since. Can you
  please update to the latest one and see if the problem persists?"

In short, -1 from me, but I don't matter much ...


PS: Three of the five 9.x micro versions are prime right now. Any
chance an extra 9.1 release could be arranged?

--
Christian



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

[HACKERS] Patch for German translation

[Christian Ullrich]

Hello,

here is a patch for the German translation that removes (all) five 
instances of *the* most annoying mistake ever.


By the way, is there any documentation anywhere about how and where to 
send translation patches? I'm just guessing here.


--
Christian

diff --git a/de/postgres.po b/de/postgres.po
index 6dd941d..f20cdc5 100644
--- a/de/postgres.po
+++ b/de/postgres.po
@@ -21038,7 +21038,7 @@ msgstr "zu viele Verbindungen von Rolle „%s“"
 #: utils/init/miscinit.c:618
 #, c-format
 msgid "permission denied to set session authorization"
-msgstr "keine Berechtigung, um Sitzungsauthorisierung zu setzen"
+msgstr "keine Berechtigung, um Sitzungsautorisierung zu setzen"
 
 #: utils/init/miscinit.c:701
 #, c-format
@@ -21174,7 +21174,7 @@ msgstr "Bibliothek „%s“ geladen"
 #: utils/init/postinit.c:252
 #, c-format
 msgid "replication connection authorized: user=%s SSL enabled (protocol=%s, 
cipher=%s, compression=%s)"
-msgstr "Replikationsverbindung authorisiert: Benutzer=%s SSL an (Protokoll=%s, 
Verschlüsselungsmethode=%s, Komprimierung=%s)"
+msgstr "Replikationsverbindung autorisiert: Benutzer=%s SSL an (Protokoll=%s, 
Verschlüsselungsmethode=%s, Komprimierung=%s)"
 
 #: utils/init/postinit.c:254 utils/init/postinit.c:268
 msgid "off"
@@ -21187,17 +21187,17 @@ msgstr "an"
 #: utils/init/postinit.c:258
 #, c-format
 msgid "replication connection authorized: user=%s"
-msgstr "Replikationsverbindung authorisiert: Benutzer=%s"
+msgstr "Replikationsverbindung autorisiert: Benutzer=%s"
 
 #: utils/init/postinit.c:266
 #, c-format
 msgid "connection authorized: user=%s database=%s SSL enabled (protocol=%s, 
cipher=%s, compression=%s)"
-msgstr "Verbindung authorisiert: Benutzer=%s Datenbank=%s SSL an 
(Protokoll=%s, Verschlüsselungsmethode=%s, Komprimierung=%s)"
+msgstr "Verbindung autorisiert: Benutzer=%s Datenbank=%s SSL an (Protokoll=%s, 
Verschlüsselungsmethode=%s, Komprimierung=%s)"
 
 #: utils/init/postinit.c:272
 #, c-format
 msgid "connection authorized: user=%s database=%s"
-msgstr "Verbindung authorisiert: Benutzer=%s Datenbank=%s"
+msgstr "Verbindung autorisiert: Benutzer=%s Datenbank=%s"
 
 #: utils/init/postinit.c:304
 #, c-format

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

Re: [HACKERS] Initial release notes created for 9.6

[Christian Ullrich]

* Tom Lane wrote:


Christian Ullrich  writes:



I suggest writing "use the Kerberos realm name for authentication
instead of the NetBIOS name" either in place of the existing description
or together with it.


OK, how about this:

   
Add new SSPI authentication parameters compat_realm
and upn_username, to control whether NetBIOS or Kerberos
realm names and user names are used during SSPI authentication
(Christian Ullrich)
   


Perfect, except for the implied idea of a "NetBIOS realm name", see 
below. I can live with that in release notes, though.



BTW, I went to read the descriptions of those parameters again, and this
one seems a bit confusing:

 
  compat_realm
  
   
If set to 1, the domain's SAM-compatible name (also known as the
NetBIOS name) is used for the include_realm
option. This is the default. If set to 0, the true realm name from
the Kerberos user principal name is used.
   
   
Do not enable this option unless your server runs under a domain
account (this includes virtual service accounts on a domain member
system) and all clients authenticating through SSPI are also using
domain accounts, or authentication will fail.
   
  
 

To my mind, an option that's set to 1 is "enabled".  Should the second
para read "Do not disable ..."?  Or maybe we should reverse the sense
of the flag, so that the default state can be 0 == disabled?


Well spotted, thanks. It should be "disable" instead.

This is left from when the sense of the option _was_ the other way 
around (it was called "real_realm" then). I reversed and renamed it 
after Magnus reviewed the patch and was -- correctly -- opposed to the name.


If the default state should be off, we're back to inventing a useful new 
name. Magnus suggested "sspi_netbios_realm", which could be shortened to 
just "netbios_realm", but I don't like to have both "NetBIOS" and 
"realm" in the name because nobody else calls a domain's NetBIOS name a 
"realm". (For the release notes, on the other hand, there is no need to 
split this hair quite so thin.)


Unless you _really_ want the default (that is, backwards compatible) 
behavior with the option off, I would rather keep it the way it is.


--
Christian



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

Re: [HACKERS] Initial release notes created for 9.6

[Christian Ullrich]

* Tom Lane wrote:


I've pushed a first cut at release notes for 9.6.  There's a good deal
of work to do yet:


+
+   
+Add new SSPI authentication parameters compat_realm
+and upn_usename, to make it possible to make SSPI
+work more like GSSAPI (Christian Ullrich)
+   

It is upn_username, not usename. Typo in the commit message.

"Make SSPI work more like GSSAPI" reads like it changed authentication 
behavior in some fundamental way, and as if SSPI did not work like 
GSSAPI without it. The difference in behavior of include_realm between 
GSSAPI and SSPI is not caused by SSPI, but is an implementation detail 
on our end.


I suggest writing "use the Kerberos realm name for authentication 
instead of the NetBIOS name" either in place of the existing description 
or together with it.


Thanks,

--
Christian





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

Re: [HACKERS] pgsql: Support building with Visual Studio 2015

[Christian Ullrich]

* Andrew Dunstan wrote:


Support building with Visual Studio 2015
http://git.postgresql.org/pg/commitdiff/da52474f3d3cbdf38d8a6677a4ebedaf402ade3a


diff --git a/src/port/win32env.c b/src/port/win32env.c
index 7e4ff62..d6b0ebe 100644 (file)
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -70,6 +70,9 @@ pgwin32_putenv(const char *envval)
"msvcr120", 0, NULL
},  /* Visual Studio 2013 */
{
+   "urctbase", 0, NULL
+   },  /* Visual Studio 2015 and later */
+   {
NULL, 0, NULL
}
};

s/urctbase/ucrtbase/

Sorry, I missed that this morning.

--
Christian




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

Re: [HACKERS] Parallel build with MSVC

[Christian Ullrich]

* Michael Paquier wrote:


On Wed, Apr 27, 2016 at 5:15 PM, Christian Ullrich  wrote:

OK then, hopefully last round attached.


Thanks. Those are fine in my view. It is hard to tell if a committer
is going to have a look at that soon, so I think that it would be
wiser to add that to the next CF so as those patches don't fall into
oblivion.


Done.

--
Christian



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

Re: [HACKERS] Parallel build with MSVC

[Christian Ullrich]

* From: Michael Paquier [mailto:michael.paqu...@gmail.com]

> On Wed, Apr 27, 2016 at 4:06 PM, Christian Ullrich 
> wrote:

> > * From: Michael Paquier [mailto:michael.paqu...@gmail.com]

> >> vcbuild also supports /m. Wouldn't it make sense to have a environment
> >> variable flag for it as well? vcbuild has been replaced by msbuild in
> >> VS2010 but I would think that in back-branches this would have value
> >> for users still compiling with VS2008 or older, and those are still
> >> supported things.
> >
> > Good point, but I have no installation of 2008 around, so I cannot test
> > it. Perhaps there is someone around who could do that (just add the
> > $msbflags as the first argument to vcbuild)?
> 
> I forgot that I have actually one! Bumping my Win7 VM CPU from 1 to 2,
> and using VS2008 command prompt with vcbuild /m I am not seeing
> issues. Moving symbols.out would be the main issue, but I am not
> noticing problems related to that.

OK then, hopefully last round attached.

-- 
Christian



0001-Support-passing-arbitrary-arguments-to-MSBuild-VCBui.patch
Description: 0001-Support-passing-arbitrary-arguments-to-MSBuild-VCBui.patch


0002-Make-gendef.pl-work-in-MSVC-parallel-build.patch
Description: 0002-Make-gendef.pl-work-in-MSVC-parallel-build.patch

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

Re: [HACKERS] Parallel build with MSVC

[Christian Ullrich]

* From: Michael Paquier [mailto:michael.paqu...@gmail.com]

> On Tue, Apr 26, 2016 at 10:09 PM, Christian Ullrich
>  wrote:

>  
> -build DEBUG
> +build -c DEBUG
>  
> To build just a single project, for example psql, run the commands:
>  
>  build psql
> -build DEBUG psql
> +build -c DEBUG psql
>  
>
> Why is that? Your patch just has a look at argv[0] to see if that's a
> debug or release build.

Sorry, forgot to fix that. I originally used Getopt in build.pl, then realized 
maintaining compatibility was more important.

Thanks for noticing; new patches attached; the second one is unmodified.

> +use File::Spec::Functions qw(splitpath catpath);
> This is present since at least perl 5.8, so that's not a blocker.
> 
> vcbuild also supports /m. Wouldn't it make sense to have a environment
> variable flag for it as well? vcbuild has been replaced by msbuild in
> VS2010 but I would think that in back-branches this would have value
> for users still compiling with VS2008 or older, and those are still
> supported things.

Good point, but I have no installation of 2008 around, so I cannot test it. 
Perhaps there is someone around who could do that (just add the $msbflags as 
the first argument to vcbuild)?

-- 
Christian



0001-Support-passing-arbitrary-arguments-to-MSBuild.patch
Description: 0001-Support-passing-arbitrary-arguments-to-MSBuild.patch


0002-Support-parallel-build-with-MSBuild.patch
Description: 0002-Support-parallel-build-with-MSBuild.patch

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

Re: [HACKERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* Christian Ullrich wrote:


wrong even without considering the debug/release split. If we load a
compiled extension built with a CRT we have not seen yet, _after_ the
first call to pgwin32_putenv(), that module's CRT's view of its
environment will be frozen because we will never attempt to update it.


Four patches attached:

master --- 0001 --- 0002 --- 0003
 \
  `- 0004

0001 is just some various fixes to set the stage.

0002 fixes this "load race" by not remembering a negative result 
anymore. However, ...



If it can happen that a CRT DLL is unloaded before the process exits,
and we cached the module handle while it was loaded, and later
pgwin32_putenv() is called, that won't end well for the process. This
might be a bit far-fetched; I have to see if I can actually make it happen.


... this *can* and does happen, so fixing the load race alone is not 
enough. 0004 fixes the unload race as well, by dropping the entire DLL 
handle/_putenv pointer cache from the function and going through the 
list of DLLs each time.


I tested this with a project 
(<https://bitbucket.org/chrullrich/pgputenv-demo>) that contains two DLLs:


- The first one is built with VS 2013 (debug), as is the server. It
  does not matter what it is built with, except it must not be the same
  as the second DLL. It exports a single function callable from SQL.

- The second one is built with VS 2015 (debug), and again, the exact
  CRT is not important as long as it is not the same as the server
  or the first DLL.

The function does this:

1. It loads the second DLL. This brings in ucrtbased.dll as well.
2. It calls putenv().
3. It unloads the second DLL. This also causes ucrtbased.dll to be
   unloaded because it is not needed anymore.
4. It calls putenv() again.

   - With current master, this works, because pgwin32_putenv(),
 after the first call somewhere early during backend startup,
 never looks for ucrtbased again (including in step 2).

   - With patch 0002 applied, it crashes because pgwin32_putenv(),
 having detected ucrtbased.dll and cached its HMODULE during
 the call in step 2 above, reuses these values after the DLL
 is long gone.

   - With patch 0004 applied as well, it works again because no
 caching is done anymore.

Even with patch 0004, there is still a race condition between the main 
thread and a theoretical additional thread created by some other 
component that unloads some CRT between GetProcAddress() and the 
_putenv() call, but that is hardly fixable.


The fact that master looks like it does means that there have not been 
many (or any) complaints about missing cross-module environment 
variables. If nobody ever needs to see a variable set elsewhere, we have 
a very simple solution: Why don't we simply throw out the whole #ifdef 
_MSC_VER block?


There is another possible fix, ugly as sin, if we really need to keep 
the whole environment update machinery *and* cannot do the full loop 
each time. Patch 0003 pins each CRT when we see it for the first time. 
GET_MODULE_HANDLE_EX_FLAG_PIN is documented as "The module stays loaded 
until the process is terminated, no matter how many times FreeLibrary is 
called", so the unload race cannot occur anymore.


--
Christian

From d74e778d8abbfea244bacbeb352cadc737032e85 Mon Sep 17 00:00:00 2001
From: Christian Ullrich 
Date: Tue, 26 Apr 2016 15:26:14 +0200
Subject: [PATCH 1/3] Cleanups in pgwin32_putenv().

- Added UCRT and all debug CRTs
- Condensed the array to one line per item (it was getting long)
- Test HMODULEs for NULL instead of zero
- Removed unnecessary CloseHandle() call
---
 src/port/win32env.c | 49 -
 1 file changed, 20 insertions(+), 29 deletions(-)

diff --git a/src/port/win32env.c b/src/port/win32env.c
index 7e4ff62..fd6762e 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -45,33 +45,25 @@ pgwin32_putenv(const char *envval)
PUTENVPROC  putenvFunc;
}   rtmodules[] =
{
-   {
-   "msvcrt", 0, NULL
-   },  /* Visual 
Studio 6.0 / mingw */
-   {
-   "msvcr70", 0, NULL
-   },  /* Visual 
Studio 2002 */
-   {
-   "msvcr71", 0, NULL
-   },  /* Visual 
Studio 2003 */
-   {
-   "msvcr80", 0, NULL
-   },  /* Visual 
Studio 2005 */
-   {
-   "msvcr90", 0, NULL
-   },  /* Visual 
Studio 2008 */
-   {
-   "msvc

[HACKERS] Parallel build with MSVC

[Christian Ullrich]

On my system, "build DEBUG" takes ~2.75 minutes. When I tell MSBuild to 
build in parallel by passing it the /m flag, that goes down to 1.5 minutes.


The first patch passes the value of the MSBFLAGS environment variable to 
msbuild's command line.


The output of parallel and sequential builds has identical file count 
and size after installing; all tests pass.


Even without /m, MSBuild will already run enough compilers to keep all 
CPUs busy whenever it can do that with only a single project's files. 
With /m, it will also process _projects_ in parallel.


One additional fix required is to put gendef.pl's "symbols.out" file 
into the directory it is working on, rather than the source tree root, 
to avoid collisions that cause the parallel build to fail otherwise.


--
Christian
From 4b455e1ac38f1441f1faf695da4be8c5eb478970 Mon Sep 17 00:00:00 2001
From: Christian Ullrich 
Date: Tue, 26 Apr 2016 14:52:27 +0200
Subject: [PATCH 1/2] Support passing arbitrary arguments to MSBuild.

This is particularly useful to pass /m, to perform a parallel build.
---
 doc/src/sgml/install-windows.sgml | 11 +--
 src/tools/msvc/build.pl   |  5 +++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/install-windows.sgml 
b/doc/src/sgml/install-windows.sgml
index 74265fc..2b9cf1a 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -137,6 +137,13 @@ $config->{python} = 'c:\python26';
 $ENV{PATH}=$ENV{PATH} . ';c:\some\where\bison\bin';
 
  
+ 
+  To pass additional command line arguments to msbuild (Visual Studio 2010
+  and later):
+
+$ENV{MSBFLAGS}="/m";
+
+ 
 
  
   Requirements
@@ -361,12 +368,12 @@ $ENV{PATH}=$ENV{PATH} . ';c:\some\where\bison\bin';
 
To build all of PostgreSQL in debug configuration, run the command:
 
-build DEBUG
+build -c DEBUG
 
To build just a single project, for example psql, run the commands:
 
 build psql
-build DEBUG psql
+build -c DEBUG psql
 
To change the default build configuration to debug, put the following
in the buildenv.pl file:
diff --git a/src/tools/msvc/build.pl b/src/tools/msvc/build.pl
index c4e4dc7..85109a2 100644
--- a/src/tools/msvc/build.pl
+++ b/src/tools/msvc/build.pl
@@ -38,6 +38,7 @@ my $vcver = Mkvcbuild::mkvcbuild($config);
 # check what sort of build we are doing
 
 my $bconf = $ENV{CONFIG} || "Release";
+my $msbflags  = $ENV{MSBFLAGS} || "";
 my $buildwhat = $ARGV[1] || "";
 if (uc($ARGV[0]) eq 'DEBUG')
 {
@@ -53,7 +54,7 @@ elsif (uc($ARGV[0]) ne "RELEASE")
 if ($buildwhat and $vcver >= 10.00)
 {
system(
-"msbuild $buildwhat.vcxproj /verbosity:normal /p:Configuration=$bconf");
+"msbuild $buildwhat.vcxproj $msbflags /verbosity:normal 
/p:Configuration=$bconf");
 }
 elsif ($buildwhat)
 {
@@ -61,7 +62,7 @@ elsif ($buildwhat)
 }
 else
 {
-   system("msbuild pgsql.sln /verbosity:normal /p:Configuration=$bconf");
+   system("msbuild pgsql.sln $msbflags /verbosity:normal 
/p:Configuration=$bconf");
 }
 
 # report status
-- 
2.8.1.windows.1

From 51cccd968b5ba98da4d971d9c69b59ec85cfa513 Mon Sep 17 00:00:00 2001
From: Christian Ullrich 
Date: Tue, 26 Apr 2016 14:52:53 +0200
Subject: [PATCH 2/2] Support parallel build with MSBuild.

gendef.pl now puts the temporary output file into the target directory instead 
of the source tree root, to avoid collisions.
---
 src/tools/msvc/gendef.pl | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/tools/msvc/gendef.pl b/src/tools/msvc/gendef.pl
index 8ccaab3..4bd05fa 100644
--- a/src/tools/msvc/gendef.pl
+++ b/src/tools/msvc/gendef.pl
@@ -4,6 +4,7 @@ use warnings;
 use strict;
 use 5.8.0;
 use List::Util qw(max);
+use File::Spec::Functions qw(splitpath catpath);
 
 #
 # Script that generates a .DEF file for all objects in a directory
@@ -14,9 +15,11 @@ use List::Util qw(max);
 sub dumpsyms
 {
my ($objfile, $symfile) = @_;
-   system("dumpbin /symbols /out:symbols.out $_ >NUL")
+   my ($symvol, $symdirs, $symbase) = splitpath($symfile);
+   my $tmpfile = catpath($symvol, $symdirs, "symbols.out");
+   system("dumpbin /symbols /out:$tmpfile $_ >NUL")
  && die "Could not call dumpbin";
-   rename("symbols.out", $symfile);
+   rename($tmpfile, $symfile);
 }
 
 # Given a symbol file path, loops over its contents
-- 
2.8.1.windows.1


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

Re: [HACKERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* Christian Ullrich wrote:


* Andrew Dunstan wrote:



What if both are present? Is a release build prevented from loading a
debug dll and vice versa?


Debug and release are simply two separate CRTs. If your process contains
a module that needs the one, and another that needs the other, you will
have both loaded at once.


pgwin32_putenv() is the gift that keeps giving.


According to its comment, it is not required that all modules exchanging 
calls with postgres.exe have to be built with the same CRT version (100, 
110, 120, etc.). Is it?


If not, the logic that remembers negative results from GetModuleHandle() 
(i.e. gives up forever on each possible CRT once it does not find it) is 
wrong even without considering the debug/release split. If we load a 
compiled extension built with a CRT we have not seen yet, _after_ the 
first call to pgwin32_putenv(), that module's CRT's view of its 
environment will be frozen because we will never attempt to update it.


If that code is in there because it has some noticeable performance 
advantage, the negative results could probably be reset in SQL LOAD, 
rather than just not remembering them anymore.



This comment is also incomplete then:

/*
 * Module loaded, but we did not find the function last time.
 * We're not going to find it this time either...
 */

This else branch is also taken if the module handle is set to 
INVALID_HANDLE_VALUE because the module was not found in a previous call.



If it can happen that a CRT DLL is unloaded before the process exits, 
and we cached the module handle while it was loaded, and later 
pgwin32_putenv() is called, that won't end well for the process. This 
might be a bit far-fetched; I have to see if I can actually make it happen.


One situation I can think of where this could occur is if an extension 
loaded with LOAD creates a COM in-proc server from a DLL built with yet 
another CRT, and when that object is released, either FreeLibrary() 
(transitively) or CoFreeUnusedLibraries() (directly) boots that CRT (if 
they do; it's possible that a CRT, once loaded, stays loaded.)



Finally: A nonzero handle returned from GetModuleHandle() is not 
something that needs to be CloseHandle()d. It is not actually a handle, 
but a pointer to the base (load) address of the module, although the 
documentation for GetModuleHandle() is careful not to admit that.


The value it is compared against to see whether we have seen the module 
before should be NULL, not 0.



It's getting a bit late for me today, but I will do the necessary 
experimentation and try to come up with a POC patch to fix whatever of 
the above I can actually prove to be real. Should anyone know for sure 
that I'm completely off track on something, better yet, everything, 
please let me know.


I should finish thinking before posting, then I would not have to reply 
to myself so often.


--
Christian



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

Re: [HACKERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* Andrew Dunstan wrote:


On 04/25/2016 09:27 AM, Christian Ullrich wrote:

* Magnus Hagander wrote:


On Sun, Apr 24, 2016 at 9:56 PM, Christian Ullrich
 wrote:



Just noticed something. This DLL detection by name has never worked in
debug builds where the DLL names end in "d". Is that important?



That's an interesting point.  I guess our release builds are never with
debugging info - but could it make the buildfarm "wrong"?



What if both are present? Is a release build prevented from loading a
debug dll and vice versa?


Debug and release are simply two separate CRTs. If your process contains 
a module that needs the one, and another that needs the other, you will 
have both loaded at once.


I had hoped they might share state, but they don't, at least as far as 
putenv()/getenv() are concerned.



Alternatively, can we detect at compile time if we are a debug build and
if so add the suffix?


No; same reason as above.

--
Christian



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

Re: [HACKERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* Magnus Hagander wrote:


On Sun, Apr 24, 2016 at 9:56 PM, Christian Ullrich 
wrote:


* Magnus Hagander wrote:

Add putenv support for msvcrt from Visual Studio 2013
http://git.postgresql.org/pg/commitdiff/9f633b404cb3be6139f8dfdea00538489ffef9ab



Just noticed something. This DLL detection by name has never worked in
debug builds where the DLL names end in "d". Is that important?



That's an interesting point.  I guess our release builds are never with
debugging info - but could it make the buildfarm "wrong"?

Fixing it should probably be as easy as trying each dll with the specified
name and also with a "d" as a suffix?


I think so, yes.

Personally, I would have expected that at least the debug/release DLLs 
of a single CRT version would somehow share their environment, but I 
tried it and they don't.


--
Christian





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

Re: [HACKERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* Magnus Hagander wrote:


Add putenv support for msvcrt from Visual Studio 2013

This was missed when VS 2013 support was added.

Michael Paquier

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/9f633b404cb3be6139f8dfdea00538489ffef9ab


Just noticed something. This DLL detection by name has never worked in 
debug builds where the DLL names end in "d". Is that important?


--
Christian



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

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

[Christian Ullrich]

* Andrew Dunstan wrote:


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


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

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


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



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

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



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

s/os/is/ in the last line.


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

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

The maximum length is documented as 85 characters, also:

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


--
Christian



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

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

[Christian Ullrich]

* Andrew Dunstan wrote:


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



* Andrew Dunstan wrote:



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


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



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


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


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


--
Christian




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

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

[Christian Ullrich]

* Andrew Dunstan wrote:


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



Andrew Dunstan  writes:

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



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



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


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


--
Christian




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

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

[Christian Ullrich]

* Andrew Dunstan wrote:


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



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

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



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


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

(HANDLE) ((int64) bgchild)


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


In this case, I would prefer this:

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

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

 /*

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


--
Christian




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

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

[Christian Ullrich]

* Christian Ullrich wrote:


* Andrew Dunstan wrote:


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



Progress report:

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


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


It's not just me. See the reply at
<https://social.msdn.microsoft.com/Forums/sqlserver/en-US/ea671596-9e8d-4e80-bb1f-8c9bfb1738dc/2012-and-2015-solutions?forum=vcgeneral>

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


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

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


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


--
Christian




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

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

[Christian Ullrich]

* Andrew Dunstan wrote:


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



Progress report:

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

>>

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


It's not just me. See the reply at

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


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


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


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


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


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


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


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


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


--
Christian




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

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

[Christian Ullrich]

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

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

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

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

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

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

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

So we can simply silence the warning by casting explicitly.

-- 
Christian




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

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

[Christian Ullrich]

* Michael Paquier wrote:


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

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

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

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


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


Identical results for unmodified master built with 2013.

--
Christian




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

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

[Christian Ullrich]

* Alvaro Herrera wrote:


Michael Paquier wrote:

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

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


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


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


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


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


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

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


--
Christian



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

Re: [HACKERS] Preprocessor condition fix

[Christian Ullrich]

* From: Tom Lane [mailto:t...@sss.pgh.pa.us]

> Christian Ullrich  writes:

> > * Tom Lane wrote:
> >> Hm, my grep found another one ...
> 
> > Oh, sorry. I saw that one, but thought it was intentional because _WIN64
> > is defined automatically anyway.
> 
> Oh?  Then we should not need that one (the /D switch in win32.mak) at all.
> Should we just remove it?

We have both confirmed several times that nothing depends on it. I think it can 
go.

-- 
Christian



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

Re: [HACKERS] Preprocessor condition fix

[Christian Ullrich]

* Tom Lane wrote:


Christian Ullrich  writes:



According to git grep, this is the only place where WIN64 is used
without the leading underscore.


Hm, my grep found another one ...


Oh, sorry. I saw that one, but thought it was intentional because _WIN64 
is defined automatically anyway.


--
Christian




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

[HACKERS] Preprocessor condition fix

[Christian Ullrich]

Here is a one-line patch to fix a wrong preprocessor condition in 
pg_regress, found because the VS 2015 compiler warns on the cast in the 
32-bit branch where apparently earlier versions did not.


According to git grep, this is the only place where WIN64 is used 
without the leading underscore.


--
Christian
From 9bc9e8ed79747f7bf3e727c9f64f4a088de589fb Mon Sep 17 00:00:00 2001
From: Christian Ullrich 
Date: Mon, 11 Apr 2016 15:47:20 +0200
Subject: [PATCH] Fixed preprocessor condition (WIN64 -> _WIN64).

---
 src/test/regress/pg_regress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 1674445..2f6f56d 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -2386,7 +2386,7 @@ regression_main(int argc, char *argv[], init_function 
ifunc, test_function tfunc
 
postmaster_running = true;
 
-#ifdef WIN64
+#ifdef _WIN64
 /* need a series of two casts to convert HANDLE without compiler warning */
 #define ULONGPID(x) (unsigned long) (unsigned long long) (x)
 #else
-- 
2.7.0.windows.1


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

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

[Christian Ullrich]

* Andrew Dunstan:


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



Michael Paquier wrote:



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


Oh?

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


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


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


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


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


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


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

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

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

--
Christian



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

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

[Christian Ullrich]

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

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

-- 
Christian

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

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

[Christian Ullrich]

* Andrew Dunstan wrote:


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



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



Do you have a fix for the LPCWSTR parameter issue?


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


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

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


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


--
Christian



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

Re: [HACKERS] Lower msvc build verbosity level

[Christian Ullrich]

* Tom Lane wrote:


+several.  Grepping for compiler warnings, for example, is really painful
right now on any MSVC critter.  I've resorted to grepping for "warning C",
which skips the noise messages, but I'm never sure if I'm missing
something.


You miss all diagnostics from other tools than the compiler, for one thing.

There is a simple solution to that, however. MSBuild and VCBuild repeat 
all warnings and errors produced during the build at the bottom of the 
log. I just checked on mastodon (VS 2005, the oldest), and it does that 
already.


This depends on the whole build being done using a single solution file 
that contains all the individual projects, but there is no reason to 
assume we will start building individual projects instead, I assume.


--
Christian



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

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

[Christian Ullrich]

* Michael Paquier wrote:


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



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


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


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



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


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


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



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


Which brings us to the next problem:

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

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


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

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


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


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



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


--
Christian



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

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

[Christian Ullrich]

* Michael Paquier wrote:

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



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


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

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

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


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


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


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


--
Christian



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

[HACKERS] Lower msvc build verbosity level

[Christian Ullrich]

Hello,

could we perhaps lower the verbosity level of the msvc build (in 
src/tools/msvc/build.pl) from "detailed" to "normal"? In my experiment, 
this reduces the size of the build log by 96.4 percent (from 12.5 MiB to 
438 KiB), or if the log is not redirected, it shortens the build time by 
45 percent, from just under 6 minutes to 3.


The lines that would not be logged anymore are mostly "Task [or Target] 
so-and-so skipped, due to false condition". All the compiler, linker, 
etc. command lines are still there.


Also, with the "detailed" logs, loading the buildfarm status page for a 
failure in the Make stage is really painful.


--
Christian
diff --git a/src/tools/msvc/build.pl b/src/tools/msvc/build.pl
new file mode 100644
index e107d41..c4e4dc7
*** a/src/tools/msvc/build.pl
--- b/src/tools/msvc/build.pl
*** elsif (uc($ARGV[0]) ne "RELEASE")
*** 53,59 
  if ($buildwhat and $vcver >= 10.00)
  {
system(
! "msbuild $buildwhat.vcxproj /verbosity:detailed /p:Configuration=$bconf");
  }
  elsif ($buildwhat)
  {
--- 53,59 
  if ($buildwhat and $vcver >= 10.00)
  {
system(
! "msbuild $buildwhat.vcxproj /verbosity:normal /p:Configuration=$bconf");
  }
  elsif ($buildwhat)
  {
*** elsif ($buildwhat)
*** 61,67 
  }
  else
  {
!   system("msbuild pgsql.sln /verbosity:detailed /p:Configuration=$bconf");
  }
  
  # report status
--- 61,67 
  }
  else
  {
!   system("msbuild pgsql.sln /verbosity:normal /p:Configuration=$bconf");
  }
  
  # report status

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

Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Christian Ullrich]

* Magnus Hagander wrote:


On Tue, Mar 29, 2016 at 5:09 PM, David Steele  wrote:



It seems like this patch should be set "ready for committer".  Can one of
the reviewers do that if appropriate?



I'll pick it up to do that as well as committing it.


Magnus, do you intend to commit the patch before the feature freeze?

--
Christian




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

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

[Christian Ullrich]

* Petr Jelinek wrote:


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

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

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

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



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


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


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


--
Christian



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

Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Christian Ullrich]

* Magnus Hagander wrote:


On Tue, Mar 29, 2016 at 5:09 PM, David Steele  wrote:



It seems like this patch should be set "ready for committer".  Can one of
the reviewers do that if appropriate?


I'll pick it up to do that as well as committing it.


Ah, good news!

I hope it's not coming too late, but I have a final update removing a 
rather pointless palloc() return value check. No changes otherwise.


--
Christian



diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
new file mode 100644
index 3b2935c..f7d7b5a
*** a/doc/src/sgml/client-auth.sgml
--- b/doc/src/sgml/client-auth.sgml
*** omicron bryanh
*** 1097,1102 
--- 1097,1140 
   
  
   
+   compat_realm
+   
+
+ If set to 1, the domain's SAM-compatible name (also known as the
+ NetBIOS name) is used for the include_realm
+ option. This is the default. If set to 0, the true realm name from
+ the Kerberos user principal name is used. Leave this option
+ disabled to maintain compatibility with existing 
+ pg_ident.conf files.
+
+
+ Do not enable this option unless your server runs under a domain
+ account (this includes virtual service accounts on a domain member
+ system) and all clients authenticating through SSPI are also using
+ domain accounts, or authentication will fail.
+
+   
+  
+ 
+  
+   upn_username
+   
+
+ If this option is enabled along with compat_realm,
+ the user name from the Kerberos UPN is used for authentication. If
+ it is disabled (the default), the SAM-compatible user name is used.
+ By default, these two names are identical for new user accounts.
+
+
+ Note that libpq uses the SAM-compatible name if no
+ explicit user name is specified. If you use
+ libpq (e.g. through the ODBC driver), you should
+ leave this option disabled.
+
+   
+  
+ 
+  
map

 
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
new file mode 100644
index 899da71..cedebf1
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
*** typedef SECURITY_STATUS
*** 155,160 
--- 155,165 
(WINAPI * QUERY_SECURITY_CONTEXT_TOKEN_FN) (

   PCtxtHandle, void **);
  static intpg_SSPI_recvauth(Port *port);
+ static intpg_SSPI_make_upn(char *accountname,
+size_t accountnamesize,
+char *domainname,
+size_t domainnamesize,
+bool 
update_accountname);
  #endif
  
  /*
*** pg_SSPI_recvauth(Port *port)
*** 1265,1270 
--- 1270,1284 
  
free(tokenuser);
  
+   if (!port->hba->compat_realm)
+   {
+   int status = pg_SSPI_make_upn(accountname, sizeof(accountname),
+ 
domainname, sizeof(domainname),
+ 
port->hba->upn_username);
+   if (status != STATUS_OK)
+   return status;
+   }
+ 
/*
 * Compare realm/domain if requested. In SSPI, always compare case
 * insensitive.
*** pg_SSPI_recvauth(Port *port)
*** 1300,1305 
--- 1314,1412 
else
return check_usermap(port->hba->usermap, port->user_name, 
accountname, true);
  }
+ 
+ /*
+  * Replaces the domainname with the Kerberos realm name,
+  * and optionally the accountname with the Kerberos user name.
+  */
+ static intpg_SSPI_make_upn(char *accountname,
+size_t accountnamesize,
+char *domainname,
+size_t domainnamesize,
+bool 
update_accountname)
+ {
+   char *samname;
+   char *upname = NULL;
+   char *p = NULL;
+   ULONG upnamesize = 0;
+   size_t upnamerealmsize;
+   BOOLEAN res;
+ 
+   /*
+* Build SAM name (DOMAIN\\user), then translate to UPN
+* (user@kerberos.realm). The realm name is returned in
+* lower case, but that is fine because in SSPI auth,
+* string comparisons are always case-insensitive.
+*/
+ 
+   samname = psprintf("%s\\%s", domainname, accountname);
+   res = TranslateName(samname, NameSamCompatible, NameUserPrincipal,
+  

[HACKERS] Re: [COMMITTERS] pgsql: Sync tzload() and tzparse() APIs with IANA release tzcode2016c.

[Christian Ullrich]

* Tom Lane wrote:


Christian Ullrich  writes:



Anyway, I think Michael's fix is wrong. The bug is that the Win32
version of link() (at the bottom of zic.c) does not set errno if its
attempt to copy the file fails, so what dolink() puts into link_errno is
bogus.


Ah-hah, that explains things nicely.  The previous coding in dolink()
wasn't so dependent on link() returning a valid errno on failure.


Patch attached.


But then, should not this code make sure that errno *always* gets set?


A library function that does not fail does not touch errno. This link() 
replacement is an honorary library function, so neither should it.



I'd be inclined to think we should use _dosmaperr(), too, rather than
hand-coding it.


Yes, of course. If only I had known about it ...

New patch attached.

--
Christian

diff --git a/src/timezone/zic.c b/src/timezone/zic.c
new file mode 100644
index 8d4347a..f9cbac9
*** a/src/timezone/zic.c
--- b/src/timezone/zic.c
*** int
*** 3485,3491 
--- 3485,3494 
  link(const char *oldpath, const char *newpath)
  {
if (!CopyFile(oldpath, newpath, false))
+   {
+   _dosmaperr(GetLastError());
return -1;
+   }
return 0;
  }
  #endif

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

[HACKERS] [PATCH] Improve safety of FormatMessage() calls on Windows

[Christian Ullrich]

There are some instances of calls to FormatMessage() with the 
FORMAT_MESSAGE_FROM_SYSTEM flag that omit the 
FORMAT_MESSAGE_IGNORE_INSERTS flag. The effect of that is that if the 
requested message string contains any insertion markers, the call to 
FormatMessage() will fail because none of these calls pass an argument list.


This patch adds the ...IGNORE_INSERTS flag to these calls.

The documentation for FormatMessage() does not clearly say that a NULL 
argument list is not an implicit IGNORE_INSERTS flag, but Chen does at 
.


--
Christian
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
new file mode 100644
index 7f1ae8c..dfbf278
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
*** pg_SSPI_error(int severity, const char *
*** 1011,1017 
  {
charsysmsg[256];
  
!   if (FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, r, 0,
  sysmsg, sizeof(sysmsg), NULL) == 0)
ereport(severity,
(errmsg_internal("%s", errmsg),
--- 1011,1018 
  {
charsysmsg[256];
  
!   if (FormatMessage(FORMAT_MESSAGE_IGNORE_INSERTS | 
FORMAT_MESSAGE_FROM_SYSTEM,
! NULL, r, 0,
  sysmsg, sizeof(sysmsg), NULL) == 0)
ereport(severity,
(errmsg_internal("%s", errmsg),
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
new file mode 100644
index cd863a5..8fd90fb
*** a/src/interfaces/libpq/fe-auth.c
--- b/src/interfaces/libpq/fe-auth.c
*** pg_SSPI_error(PGconn *conn, const char *
*** 234,240 
  {
charsysmsg[256];
  
!   if (FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, r, 0,
  sysmsg, sizeof(sysmsg), NULL) == 0)
printfPQExpBuffer(&conn->errorMessage, "%s: SSPI error %x\n",
  mprefix, (unsigned int) r);
--- 234,241 
  {
charsysmsg[256];
  
!   if (FormatMessage(FORMAT_MESSAGE_IGNORE_INSERTS | 
FORMAT_MESSAGE_FROM_SYSTEM,
! NULL, r, 0,
  sysmsg, sizeof(sysmsg), NULL) == 0)
printfPQExpBuffer(&conn->errorMessage, "%s: SSPI error %x\n",
  mprefix, (unsigned int) r);
diff --git a/src/port/dirmod.c b/src/port/dirmod.c
new file mode 100644
index 8053d16..5ee5e4e
*** a/src/port/dirmod.c
--- b/src/port/dirmod.c
*** pgsymlink(const char *oldpath, const cha
*** 206,212 
LPSTR   msg;
  
errno = 0;
!   FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER | 
FORMAT_MESSAGE_FROM_SYSTEM,
  NULL, GetLastError(),
  MAKELANGID(LANG_ENGLISH, 
SUBLANG_DEFAULT),
  (LPSTR) &msg, 0, NULL);
--- 206,212 
LPSTR   msg;
  
errno = 0;
!   FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER | 
FORMAT_MESSAGE_IGNORE_INSERTS | FORMAT_MESSAGE_FROM_SYSTEM,
  NULL, GetLastError(),
  MAKELANGID(LANG_ENGLISH, 
SUBLANG_DEFAULT),
  (LPSTR) &msg, 0, NULL);
*** pgreadlink(const char *path, char *buf,
*** 281,287 
LPSTR   msg;
  
errno = 0;
!   FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER | 
FORMAT_MESSAGE_FROM_SYSTEM,
  NULL, GetLastError(),
  MAKELANGID(LANG_ENGLISH, 
SUBLANG_DEFAULT),
  (LPSTR) &msg, 0, NULL);
--- 281,287 
LPSTR   msg;
  
errno = 0;
!   FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER | 
FORMAT_MESSAGE_IGNORE_INSERTS | FORMAT_MESSAGE_FROM_SYSTEM,
  NULL, GetLastError(),
  MAKELANGID(LANG_ENGLISH, 
SUBLANG_DEFAULT),
  (LPSTR) &msg, 0, NULL);

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

[HACKERS] Re: [COMMITTERS] pgsql: Sync tzload() and tzparse() APIs with IANA release tzcode2016c.

[Christian Ullrich]

* Christian Ullrich wrote:


* Tom Lane wrote:



Christian Ullrich  writes:



zic aborts somewhere between writing Etc/UTC and UTC.


Huh ... I would not have guessed that.  Can you track down exactly
where it's failing?


I'd love to, but with 656ee84 I cannot reproduce on my Windows 10
system. I can try on the animals where it actually failed, but now that
there's a fix, that won't be necessary, right?


Weird. vcregress check works, install fails. Perhaps the directories are 
precreated for check?


Anyway, I think Michael's fix is wrong. The bug is that the Win32 
version of link() (at the bottom of zic.c) does not set errno if its 
attempt to copy the file fails, so what dolink() puts into link_errno is 
bogus.


The additional mkdirs() call just papers over the actual bug; the 
existing one in line 802 will do nicely once it actually runs.


Patch attached.

--
Christian

diff --git a/src/timezone/zic.c b/src/timezone/zic.c
new file mode 100644
index 8d4347a..ce30740
*** a/src/timezone/zic.c
--- b/src/timezone/zic.c
*** int
*** 3485,3491 
--- 3485,3496 
  link(const char *oldpath, const char *newpath)
  {
if (!CopyFile(oldpath, newpath, false))
+   {
+   DWORD err = GetLastError();
+   if (err == ERROR_PATH_NOT_FOUND || err == ERROR_FILE_NOT_FOUND)
+   errno = ENOENT;
return -1;
+   }
return 0;
  }
  #endif

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

[HACKERS] Re: [COMMITTERS] pgsql: Sync tzload() and tzparse() APIs with IANA release tzcode2016c.

[Christian Ullrich]

* Tom Lane wrote:


Christian Ullrich  writes:



zic aborts somewhere between writing Etc/UTC and UTC.


Huh ... I would not have guessed that.  Can you track down exactly
where it's failing?


I'd love to, but with 656ee84 I cannot reproduce on my Windows 10 
system. I can try on the animals where it actually failed, but now that 
there's a fix, that won't be necessary, right?



We absorbed some new code in zic.c for creating subdirectories of the
target timezone directory, and said code seemed a bit odd to me,
but I supposed that the IANA guys had debugged it already.  Maybe not.
Or maybe the problem is with some specific input file that gets
reached somewhere in that range?


My first guess was that it stumbles over the two-character directory 
name, but according to Michael Paquier's fix, that guess was 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

[HACKERS] Re: [COMMITTERS] pgsql: Sync tzload() and tzparse() APIs with IANA release tzcode2016c.

[Christian Ullrich]

* Tom Lane wrote:


Michael Paquier  writes:

Buildfarm-not-being-happy-status: woodloose, mastodon, thrips, jacana.
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=woodlouse&dt=2016-03-29%2000%3A42%3A08
The origin of the problem is that, which prevents all the subsequent
queries to fail:
   SET TimeZone to 'UTC';
+ ERROR:  invalid value for parameter "TimeZone": "UTC"


Yeah.  I've been staring at that for awhile, but it's not clear where
the problem is.  There are a bunch of other SET TIME ZONE commands in
the regression tests, how is it that this trivial case fails on the
Windows critters?


I think this is the reason, from the check log on woodlouse (jacana says 
the same in make style):


Generating timezone files...release\zic\zic: Can't create 
C:/buildfarm/buildenv/HEAD/pgsql.build/tmp_install/share/timezone/US/Pacific-New: 
No such file or directory


zic aborts somewhere between writing Etc/UTC and UTC. This is how the 
toplevel directory ends up after the error:


  Africa
  America
  Antarctica
  Arctic
  Asia
  Atlantic
  Australia
 2.102 CET
 2.294 CST6CDT
 1.876 EET
   127 EST
 2.294 EST5EDT
  Etc
  Europe
   264 Factory
   128 HST
  Indian
 2.102 MET
   127 MST
 2.294 MST7MDT
  Pacific
 2.294 PST8PDT
 1.873 WET

After I manually created the "US" directory, zic had no further trouble 
putting files into it.


--
Christian



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

Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Christian Ullrich]

* From: Alvaro Herrera [mailto:alvhe...@2ndquadrant.com]

> Christian Ullrich wrote:

> > * Christian Ullrich wrote:
> >
> > >* From: Magnus Hagander [mailto:mag...@hagander.net]
> 
> > >>Code uses a mix of malloc() and palloc() (through sprintf). Is there
> > >>a reason for that?
> > >
> > >I wasn't sure which to prefer, so I looked around in auth.c, and
> > >other than RADIUS, everything seems to use malloc() (although the
> > >sample size is not too great). Should I use palloc() instead?
> >
> > The single instance of malloc() has been replaced with palloc().
> 
> I'm wary of palloc() in this code actually ... if the allocation fails,
> I'm not sure it's okay to use ereport(ERROR) which is what would happen
> with palloc.  With the malloc code, you report the problem with
> elog(LOG) and then return STATUS_ERROR which lets the calling code
> handle the failure in a different way.  I didn't actually review your
> new code, but I recall this from previous readings of auth code; so if
> you're going to use palloc(), you better audit what happens on OOM.
> 
> For the same reason, using psprintf is probably not acceptable either.

To be honest, I'm not sure what can and cannot be done in auth code. I took 
inspiration from the existing SSPI code and nearly every error check in 
pg_SSPI_recvauth() ends up doing ereport(ERROR) already, directly or via 
pg_SSPI_error(). If this could cause serious trouble, someone would have 
noticed yet.

What *could* happen, anyway? Can ereport(ERROR) in a backend make the 
postmaster panic badly enough to force a shared memory reset?

-- 
Christian



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

Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Christian Ullrich]

* From: Christian Ullrich

> * From: Robbie Harwood [mailto:rharw...@redhat.com]
> 
> > Christian Ullrich  writes:

> > > + /* Replace domainname with realm name. */
> > > + if (upnamerealmsize > domainnamesize)
> > > + {
> > > + pfree(upname);
> > > + ereport(LOG,
> > > + 
> > > (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
> > > +  errmsg("realm name too long")));
> > > +  return STATUS_ERROR;
> > > + }
> > > +
> > > + /* Length is now safe. */
> > > + strcpy(domainname, p+1);
> >
> > Is this an actual fail state or something born out of convenience?  A
> > naive reading of this code doesn't explain why it's forbidden for the
> > upn realm to be longer than the domain name.
> 
> Because it's copied *into* domainname right there on the last line.
> 
> That said, sizeof(domainname) is MAXPGPATH, which is 1024, so there is
> absolutely no chance that the realm could be longer -- it would need an
> AD forest at least 16 domains deep.

Oh, sorry, I misunderstood the question. Yes, it's due to convenience, but
a) it *is* rather convenient given the plentiful buffer I get, and
b) doing it differently involves char** inout parameters and potential
trouble with pointer aliasing in the caller, both things I'd rather avoid.

-- 
Christian



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

Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Christian Ullrich]

On 2016-03-24 16:35, Christian Ullrich wrote:


* From: Robbie Harwood [mailto:rharw...@redhat.com]


Christian Ullrich  writes:



   pg_SSPI_recvauth(Port *port)
   {
int mtype;
+   int status;


The section of this function for include_realm checking already uses an
int status return code (retval).  I would expect to see them share a
variable rather than have both "retval" and "status".


I would not, because retval is local to that last if, but you are right, status
does not need to be in function scope.


Moved declaration.


+   /* Build SAM name (DOMAIN\\user), then translate to UPN
+  (user@kerberos.realm). The realm name is returned in
+  lower case, but that is fine because in SSPI auth,
+  string comparisons are always case-insensitive. */


Since we're already considering changing things: this is not the comment
style for this file (though it is otherwise a good comment).


True. Will fix.


Reformatted.


+   upname = (char*)palloc(upnamesize);


I don't believe this cast is typically included.


Left over from when this was malloc() before Magnus' first look at it.


Removed.

Updated patch attached.

--
Christian

diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
new file mode 100644
index 3b2935c..f7d7b5a
*** a/doc/src/sgml/client-auth.sgml
--- b/doc/src/sgml/client-auth.sgml
*** omicron bryanh
*** 1097,1102 
--- 1097,1140 
   
  
   
+   compat_realm
+   
+
+ If set to 1, the domain's SAM-compatible name (also known as the
+ NetBIOS name) is used for the include_realm
+ option. This is the default. If set to 0, the true realm name from
+ the Kerberos user principal name is used. Leave this option
+ disabled to maintain compatibility with existing 
+ pg_ident.conf files.
+
+
+ Do not enable this option unless your server runs under a domain
+ account (this includes virtual service accounts on a domain member
+ system) and all clients authenticating through SSPI are also using
+ domain accounts, or authentication will fail.
+
+   
+  
+ 
+  
+   upn_username
+   
+
+ If this option is enabled along with compat_realm,
+ the user name from the Kerberos UPN is used for authentication. If
+ it is disabled (the default), the SAM-compatible user name is used.
+ By default, these two names are identical for new user accounts.
+
+
+ Note that libpq uses the SAM-compatible name if no
+ explicit user name is specified. If you use
+ libpq (e.g. through the ODBC driver), you should
+ leave this option disabled.
+
+   
+  
+ 
+  
map

 
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
new file mode 100644
index 7f1ae8c..6830764
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
*** typedef SECURITY_STATUS
*** 155,160 
--- 155,165 
(WINAPI * QUERY_SECURITY_CONTEXT_TOKEN_FN) (

   PCtxtHandle, void **);
  static intpg_SSPI_recvauth(Port *port);
+ static intpg_SSPI_make_upn(char *accountname,
+size_t accountnamesize,
+char *domainname,
+size_t domainnamesize,
+bool 
update_accountname);
  #endif
  
  /*
*** pg_SSPI_recvauth(Port *port)
*** 1263,1268 
--- 1268,1282 
  
free(tokenuser);
  
+   if (!port->hba->compat_realm)
+   {
+   int status = pg_SSPI_make_upn(accountname, sizeof(accountname),
+ 
domainname, sizeof(domainname),
+ 
port->hba->upn_username);
+   if (status != STATUS_OK)
+   return status;
+   }
+ 
/*
 * Compare realm/domain if requested. In SSPI, always compare case
 * insensitive.
*** pg_SSPI_recvauth(Port *port)
*** 1298,1303 
--- 1312,1419 
else
return check_usermap(port->hba->usermap, port->user_name, 
accountname, true);
  }
+ 
+ /*
+  * Replaces the domainname with the Kerberos realm name,
+  * and optionally the accountname with the Kerberos user name.
+  */
+ static intpg_SSPI_make_upn(char *accountname,
+ 

Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Christian Ullrich]

* From: Robbie Harwood [mailto:rharw...@redhat.com]

> Christian Ullrich  writes:
> 
> > Updated patch attached.
> 
> I unfortunately don't have windows machines to test this on, but I
> thought it might be helpful to review this anyway since I'm touching
> code in the same general area (GSSAPI).  And as far as I can tell, you
> don't break anything there; master continues to behave as expected.

Thanks for the review.

> Some comments inline:
> 
> >   pg_SSPI_recvauth(Port *port)
> >   {
> > int mtype;
> > +   int status;
> 
> The section of this function for include_realm checking already uses an
> int status return code (retval).  I would expect to see them share a
> variable rather than have both "retval" and "status".

I would not, because retval is local to that last if, but you are right, status
does not need to be in function scope.

> > +   status = pg_SSPI_make_upn(accountname, sizeof(accountname),
> > + domainname,
> sizeof(domainname),
> > + 
> > port->hba->upn_username);
> 
> This is the only place I see that this function is called.  That being
> the case, why bother with the sizes of parameters?  Why doesn't
> pg_SSPI_make_upn() just call sizeof() by itself rather than taking those
> as arguments?

sizeof(array) != sizeof(pointer).

> > +   /* Build SAM name (DOMAIN\\user), then translate to UPN
> > +  (user@kerberos.realm). The realm name is returned in
> > +  lower case, but that is fine because in SSPI auth,
> > +  string comparisons are always case-insensitive. */
> 
> Since we're already considering changing things: this is not the comment
> style for this file (though it is otherwise a good comment).

True. Will fix.

> > +   upname = (char*)palloc(upnamesize);
> 
> I don't believe this cast is typically included.

Left over from when this was malloc() before Magnus' first look at it.

> > +   /* Replace domainname with realm name. */
> > +   if (upnamerealmsize > domainnamesize)
> > +   {
> > +   pfree(upname);
> > +   ereport(LOG,
> > +   (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
> > +errmsg("realm name too long")));
> > +return STATUS_ERROR;
> > +   }
> > +
> > +   /* Length is now safe. */
> > +   strcpy(domainname, p+1);
> 
> Is this an actual fail state or something born out of convenience?  A
> naive reading of this code doesn't explain why it's forbidden for the
> upn realm to be longer than the domain name.

Because it's copied *into* domainname right there on the last line. 

That said, sizeof(domainname) is MAXPGPATH, which is 1024, so there is
absolutely no chance that the realm could be longer -- it would need an
AD forest at least 16 domains deep.

> > +   /* Replace account name as well (in case UPN != SAM)? */
> > +   if (update_accountname)
> > +   {
> > +   if ((p - upname + 1) > accountnamesize)
> > +   {
> > +   pfree(upname);
> > +   ereport(LOG,
> > +   
> > (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
> > +errmsg("translated account name too
> long")));
> > +return STATUS_ERROR;
> > +   }
> > +
> > +   *p = 0;
> > +   strcpy(accountname, upname);
> 
> Same as above.

Yup.

-- 
Christian



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

Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Christian Ullrich]

* Christian Ullrich wrote:


* From: Magnus Hagander [mailto:mag...@hagander.net]



I don't like the name "real_realm" as a parameter name. I'm wondering if
it might be better to reverse the meaning, and call it sspi_netbios_realm
(and then change the default to on, to be backwards compatible).


What about "compat_realm" instead? "sspi_netbios_realm" is a bit long.
Also, the domain short name is not really a realm name, and this would
feel like implying that it is.


I changed it to "compat_realm".


Code uses a mix of malloc() and palloc() (through sprintf). Is there a
reason for that?


I wasn't sure which to prefer, so I looked around in auth.c, and other than
RADIUS, everything seems to use malloc() (although the sample size is not
too great). Should I use palloc() instead?


The single instance of malloc() has been replaced with palloc().


Another thing: This business of getting a SID, turning it into a user
name/domain pair, then using that to get the UPN (which probably involves
converting it to the SID again somewhere in between) does not look very good
to me. I'll have to see if it works, but what do you think about briefly
impersonating the client, just long enough to get the SAM and UPN names?


I did not pursue this further; it involves quite a bit more code 
including several more functions from secur32.dll. These might be a 
problem on MinGW according to the comment in auth.c regarding 
QuerySecurityContextToken() (if that is still accurate, because my 
mingw\lib\libsecur32.a apparently has the export).


Updated patch attached.

--
Christian

diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
new file mode 100644
index 3b2935c..f7d7b5a
*** a/doc/src/sgml/client-auth.sgml
--- b/doc/src/sgml/client-auth.sgml
*** omicron bryanh
*** 1097,1102 
--- 1097,1140 
   
  
   
+   compat_realm
+   
+
+ If set to 1, the domain's SAM-compatible name (also known as the
+ NetBIOS name) is used for the include_realm
+ option. This is the default. If set to 0, the true realm name from
+ the Kerberos user principal name is used. Leave this option
+ disabled to maintain compatibility with existing 
+ pg_ident.conf files.
+
+
+ Do not enable this option unless your server runs under a domain
+ account (this includes virtual service accounts on a domain member
+ system) and all clients authenticating through SSPI are also using
+ domain accounts, or authentication will fail.
+
+   
+  
+ 
+  
+   upn_username
+   
+
+ If this option is enabled along with compat_realm,
+ the user name from the Kerberos UPN is used for authentication. If
+ it is disabled (the default), the SAM-compatible user name is used.
+ By default, these two names are identical for new user accounts.
+
+
+ Note that libpq uses the SAM-compatible name if no
+ explicit user name is specified. If you use
+ libpq (e.g. through the ODBC driver), you should
+ leave this option disabled.
+
+   
+  
+ 
+  
map

 
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
new file mode 100644
index 7f1ae8c..f31b06f
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
*** typedef SECURITY_STATUS
*** 155,160 
--- 155,165 
(WINAPI * QUERY_SECURITY_CONTEXT_TOKEN_FN) (

   PCtxtHandle, void **);
  static intpg_SSPI_recvauth(Port *port);
+ static intpg_SSPI_make_upn(char *accountname,
+size_t accountnamesize,
+char *domainname,
+size_t domainnamesize,
+bool 
update_accountname);
  #endif
  
  /*
*** static int
*** 1026,1031 
--- 1031,1037 
  pg_SSPI_recvauth(Port *port)
  {
int mtype;
+   int status;
StringInfoData buf;
SECURITY_STATUS r;
CredHandle  sspicred;
*** pg_SSPI_recvauth(Port *port)
*** 1263,1268 
--- 1269,1283 
  
free(tokenuser);
  
+   if (!port->hba->compat_realm)
+   {
+   status = pg_SSPI_make_upn(accountname, sizeof(accountname),
+ domainname, 
sizeof(domainname),
+ 
port->hba->upn_username);
+

Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Christian Ullrich]

* From: Magnus Hagander [mailto:mag...@hagander.net]

> I took a quick look at this one, and have some initial thoughts.
> 
> I don't like the name "real_realm" as a parameter name. I'm wondering if
> it might be better to reverse the meaning, and call it sspi_netbios_realm
> (and then change the default to on, to be backwards compatible).

What about "compat_realm" instead? "sspi_netbios_realm" is a bit long.
Also, the domain short name is not really a realm name, and this would
feel like implying that it is.

> How does the real_realm thing work if you connect with a local account?
> Hostname, or kerberos principal for the host?

It fails. There is no UPN with a local account, so the conversion does not
happen, and I thought it would be best from a security POV to let the logon
fail rather than inventing a reason why it should succeed.

> Code uses a mix of malloc() and palloc() (through sprintf). Is there a
> reason for that?

I wasn't sure which to prefer, so I looked around in auth.c, and other than
RADIUS, everything seems to use malloc() (although the sample size is not 
too great). Should I use palloc() instead?

> Looking at the docs:
> 
> + Note that libpq uses the SAM-compatible name if
> no
> + explicit user name is specified. If you use
> + libpq (e.g. through the ODBC driver), you should
> + leave this option disabled.
> 
> What's the actual usecase where it makes sense to change it? Seems that's
> the more reasonable thing to document, with a reference to active
> directory specifically (or is there also such a compatible name for local
> accounts?)

In an environment where sAMAccountName and userPrincipalName are different,
it might be preferable to have something to map in pg_ident.conf that is
actually a valid user name (UPN, in this case), rather than a mixture of 
both forms that isn't valid for either.

Also, since I already have the UPN, adding the option is basically free and
feels more useful than always throwing away half the information.


Another thing: This business of getting a SID, turning it into a user 
name/domain pair, then using that to get the UPN (which probably involves 
converting it to the SID again somewhere in between) does not look very good 
to me. I'll have to see if it works, but what do you think about briefly
impersonating the client, just long enough to get the SAM and UPN names?

-- 
Christian


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

[HACKERS] New Windows animal, mostly ecpg trouble (was: Crash with old Windows on new CPU)

[Christian Ullrich]

* Alvaro Herrera wrote:


Magnus Hagander wrote:



On Wed, Mar 9, 2016 at 4:36 PM, Christian Ullrich 
wrote:



And apparently not a single one with VS 2013. OK, I'll see what I can do
about setting some up soonish, at least with (server) 2008 and (client) 7.
FWIW, I have a local build of 9.5.1 with this patch in daily use on 2008
now, with no complaints.


Having that added to the buildfarm would definitely be very useful!


Yeah, ideally we would have one buildfarm member for each MSVC compiler
supported by each Windows version.  AFAICS we only have

Windows 8 8.1 Pro Visual Studio 2012 x86_64
Windows Server 2008 R2 (64bit) Visual C++ 2005 AMD64
Windows Server 2003 R2 5.2.3790 MSVC 2005 Express 8.0.50727.42 x86
Vista Ultimate 6.0.6000 MSVC 2005 Pro 8.0.50727.867 x86
Windows XP-PRO SP3 MSVC++ 2008 Express i686


Add to that Windows 7 with Visual Studio 2013 (x64), named woodlouse, 
and there will be another one sometime over the weekend. Setting up 
Windows animals is a major pain; no wonder so few people run any.


I spent some hours getting rid of random linker errors in ecpg 
(kernel32.lib not found, msvcrt.lib not found, or unresolved symbols 
that should be in one of the two); they finally went away when I stopped 
defining the build environment in build-farm.conf and ran vcvarsall.bat 
instead (this was probably pilot error; I think I had a mix of x86 and 
x64 paths there). At that point, I managed one successful run on HEAD. 
Things started failing in ecpg again after that, with crashes of the 
test programs for the thread-thread and thread-prep test cases, 
apparently somewhere in setlocale(). I had to --skip-steps=ecpg-check 
because the crashes cause blocking UI.


The other troublemaker is plperl; it produces a wall of compiler errors 
which I cleverly did not copy, most are about something not being in a 
formal parameter list. Some preprocessor trouble, I suspect. This is 
with both ActiveState and Strawberry Perl.


I did not try Tcl, and plpython3 apparently builds, but I cannot see any 
tests being run.


--
Christian



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

Re: [HACKERS] Crash with old Windows on new CPU

[Christian Ullrich]

* Magnus Hagander wrote:


I did notice the #ifdef's are actually different in the header and body
section of the patch, which seems wrong. I used the one from the actual
implementation (_M_AMD64) for the header includes as, and also merged the
#ifdef's together to a single #if in each section. Please verify!


Should be fine. I remember I had a reason for doing it this way, but it 
can't have been a very good one. Thanks for cleaning it up.


--
Christian



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

Re: [HACKERS] Crash with old Windows on new CPU

[Christian Ullrich]

* Magnus Hagander wrote:


On Wed, Mar 9, 2016 at 4:36 PM, Christian Ullrich 
wrote:


* Magnus Hagander wrote:



How does this work wrt mingw, though? Do we have the same problem there?
AIUI this code can never run on mingw, correct?


Not unless mingw defines _MSC_VER.


The question is then - should it, under some conditions?


I have no clue about MinGW, so all I can say is that msvcrt.dll (even in 
Windows 10) does not use the problematic instructions, nor does it 
export anything with "FMA" in its name. If there is nothing to turn off, 
and nothing to turn it off with, then I suspect we are safe there. (This 
is based on a minimal test program, just to see where MinGW takes its 
log() from; it comes from msvcrt.)


Also, we have confirmation, in the link I sent when I started this 
thread, that the bug is only and specifically in the VS2013 CRT, not 
anywhere else before or since.



I notice the code checks IsWindows7SP1OrGreater() but the comment refers to
W7SP1 *or* 2008R2 SP1. I assume this is correct, or should there actually
be a separate check for server-windows?



No, that is fine. I think it's just to keep the function name from getting
too ridiculously long. The functions in  are all named
for the client versions only, and only check the version number, not the
client/server capability flags. Or, rather, there is a separate function to
determine that.



Presumably the link is documented somewhere (the docs don't seem to say
anything about it).


<https://msdn.microsoft.com/en-us/library/windows/desktop/dn424960(v=vs.85).aspx> 
("IsWindows7SP1OrGreater function"):


> This function does not differentiate between client and server
> releases. It will return true if the current OS version number is
> equal to or higher than the version of the client named in the call.
> For example, a call to IsWindowsXPSP3OrGreater will return true on
> Windows Server 2008. Applications that need to distinguish between
> server and client versions of Windows should call IsWindowsServer.

The internal version numbers (since NT4) are:

 Version | Client | Server
-++
 4.0 | NT4| NT4
 5.0 | 2000   | 2000
 5.1 | XP |
 5.2 || 2003
 6.0 | Vista  | 2008
 6.1 | 7  | 2008R2
 6.2 | 8  | 2012
 6.3 | 8.1| 2012R2
10.0 | 10 | [2016]

The relevant SDK header (), where constants for the version 
numbers are defined, also only has entries named for the client 
versions, with the sole exception of 2008, with the same value as Vista, 
and the special case of 2003 with no equivalent client version (unless 
you count the second attempt at an XP x64 ... but I digress again).


If Microsoft wanted the added complexity of separate symbols for what is 
basically the same code, I rather think they would have them.


--
Christian



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

Re: [HACKERS] Crash with old Windows on new CPU

[Christian Ullrich]

* Magnus Hagander wrote:


On Sat, Feb 13, 2016 at 4:45 PM, Christian Ullrich 
wrote:



On February 13, 2016 4:10:34 PM Tom Lane  wrote:



I'm also suspicious of the "#if _MSC_VER == 1800" tests, that is,
the code compiles on *exactly one* MSVC version.


The bug exists in only that compiler version's CRT, also, that is not the
complete version number. There may be different builds somewhere, but they
all start with 18.0.



IIRC, the CRT itself does explicit checks against _MSC_VER == 1800. As in,
they don't actually bump that number in different build numbers.

How does this work wrt mingw, though? Do we have the same problem there?
AIUI this code can never run on mingw, correct?


Not unless mingw defines _MSC_VER.

(If they do, I suggest we make Yury Zhuravlev cry and drop MinGW support 
instead. IMHO everyone should boycott them anyway until they come up 
with a working libc of their own instead of doing unspeakable things to 
a helpless msvcrt.dll that is not intended for use by non-system 
components at all. But I digress.)



I notice the code checks IsWindows7SP1OrGreater() but the comment refers to
W7SP1 *or* 2008R2 SP1. I assume this is correct, or should there actually
be a separate check for server-windows?


No, that is fine. I think it's just to keep the function name from 
getting too ridiculously long. The functions in  are 
all named for the client versions only, and only check the version 
number, not the client/server capability flags. Or, rather, there is a 
separate function to determine that.



That would
give us some context to estimate the risks of this code executing
when it's not really needed.


Hence all the conditions. The problem is *certain* to occur under these
specific conditions (x64 code on Windows before 7SP1 on a CPU with AVX2
when built with VS2013), and under no others, and these conditions flip the
switch exactly then.



Well, it doesn't flip it based on the specifics "running on a CPU with
AVX2". But presumably turning of AVX2 if the CPU doesn't support it is a
no-op.


Precisely.


Isn't that what the buildfarm is (among other things) for?


The buildfarm doesn't really have a big spread of Windows animals,
unfortunately.


And apparently not a single one with VS 2013. OK, I'll see what I can do 
about setting some up soonish, at least with (server) 2008 and (client) 
7. FWIW, I have a local build of 9.5.1 with this patch in daily use on 
2008 now, with no complaints.


--
Christian



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

Re: [HACKERS] Crash with old Windows on new CPU

[Christian Ullrich]

* Peter Eisentraut wrote:


On 2/12/16 11:24 AM, Christian Ullrich wrote:



Otherwise, it may be time to update the manual (15.6 Supported
Platforms) where it says PostgreSQL "can be expected to work on these
operating systems: [...] Windows (Win2000 SP4 and later), [...]".
Perhaps we could add "except Windows before 7 SP1/2008R2 SP1 when
running in x64 mode on Intel CPUs introduced after May 2013 (Haswell and
later)"?



Wouldn't the fix be for users to upgrade their service packs?


Windows 2008 and 2008R2 are entirely different things: 2008 is the 
server sibling of Vista (internal version 6.0), 2008R2 is that of 
Windows 7 (6.1). There is no version of 2008 that supports AVX2.


--
Christian



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

Re: [HACKERS] pl/pgSQL, get diagnostics and big data

[Christian Ullrich]

* Andreas 'ads' Scherbaum wrote:


Attached is a new version of the patch, with %lu replaced by %zu.
I re-ran all the tests, especially the long test with 2^32+x rows, and
it produces the same result as before.


To paraphrase Twain: "Sire, the Board finds this patch perfect in all 
the requirements and qualifications for inclusion into core, and doth 
hold his case open for decision after due examination by his committer."


The Windows issue is corrected, and all regression tests pass on Windows 
and FreeBSD. I can find no further fault with this patch.


Sorry it took so long, my other PostgreSQL issue happened just when I 
was going to test the updated patch.


--
Christian



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

Re: [HACKERS] Crash with old Windows on new CPU

[Christian Ullrich]

* From: Christian Ullrich

> On February 13, 2016 4:10:34 PM Tom Lane  wrote:
> 
> > Christian Ullrich  writes:

> > Lastly, I'd like to see some discussion of what side effects
> > "_set_FMA3_enable(0);" has ... I rather doubt that it's really
> > a magic-elixir-against-crashes-with-no-downsides.
> 
> It tells the math library (in the CRT, no separate libm on Windows)
> not to use the AVX2-based implementations of log() and possibly
> other functions. AIUI, FMA means "fused multiply-add" and is
> apparently something that increases performance and accuracy in
> transcendental functions.
> 
> I can check the CRT source later today and figure out exactly what
> it does.

OK, it turns out that the CRT source MS ships is not quite as complete as I 
thought it was (up until 2013, at least), so I had a look at the disassembly. 
When the library initializes, it checks whether the CPU supports the FMA 
instructions by looking at a certain bit in the CPUID result. If that is set, 
it sets a flag to use the FMA instructions. Later, in exp(), log(), pow() and 
the trigonometrical functions, it first checks whether that flag is set, and if 
so, uses the AVX-based implementation. If the flag is not set, it falls back to 
an SSE2-based one. So, yes, that function only and specifically disables the 
use of instructions that do not work in the problematic case.

The bug appears to be that it uses all manner of AVX and AVX2 instructions 
based only on the FMA support flag in CPUID, even though AVX2 has its own bit 
there.

To reiterate: The problem occurs because the library only asks the CPU whether 
it is *able* to perform the AVX instructions, but not whether it is *willing* 
to do so. In this particular situation, the former applies but not the latter, 
because the CPU needs OS support (saving the XMM/YMM registers across context 
switches), and the OS has not declared its support for that.

The downside to disabling the AVX implementations is a performance loss 
compared to using it. I ran a microbenchmark (avg(log(x) from 
generate_series(1,1e8))), and the result was that with FMA enabled, it is ~5.5% 
faster than without.

-- 
Christian


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

Re: [HACKERS] Crash with old Windows on new CPU

[Christian Ullrich]

On February 13, 2016 4:10:34 PM Tom Lane  wrote:

> Christian Ullrich  writes:
>> * Robert Haas wrote:
>>> Thanks for the report and patch.  Regrettably I haven't the Windows
>>> knowledge to have any idea whether it's right or wrong, but hopefully
>>> someone who knows Windows will jump in here.
>
>> In commitfest now.
>
> FWIW, I'm a tad suspicious of the notion that it's our job to make this
> case work.  How practical is it really to run a Windows release on
> unsupported-by-Microsoft hardware --- aren't dozens of other programs
> going to have the same issue?

Why would the hardware be unsupported? The problem occurs on new CPUs, not old 
ones, and even the OS (2008) is still in extended support until next year, IIRC.

> I'm also suspicious of the "#if _MSC_VER == 1800" tests, that is,
> the code compiles on *exactly one* MSVC version.  

The bug exists in only that compiler version's CRT, also, that is not the 
complete version number. There may be different builds somewhere, but they all 
start with 18.0. 

After all, MS is in the business of selling new compilers, not maintaining the 
old ones. 

> Maybe that's actually
> what's needed, but it sure looks fishy.  And what connection does the
> build toolchain version have to the runtime environment anyway?

The CRT version is tied to the compiler version. It has mainly to do with 
matching allocators.

> Likewise, how can we know that !IsWindows7SP1OrGreater() is the exactly
> right runtime test?

Because all sources, including Microsoft, say that AVX2 support was added in 
7SP1.

> Lastly, I'd like to see some discussion of what side effects
> "_set_FMA3_enable(0);" has ... I rather doubt that it's really
> a magic-elixir-against-crashes-with-no-downsides.  

It tells the math library (in the CRT, no separate libm on Windows) not to use 
the AVX2-based implementations of log() and possibly other functions. AIUI, FMA 
means "fused multiply-add" and is apparently something that increases 
performance and accuracy in transcendental functions.

I can check the CRT source later today and figure out exactly what it does. 

Also, if you look at the link I sent, you will find that a member of the Visual 
C++ Libraries team at MS is the source for the workaround. They probably know 
what they are doing, present circumstances excepted. 

> That would
> give us some context to estimate the risks of this code executing
> when it's not really needed. 

Hence all the conditions. The problem is *certain* to occur under these 
specific conditions (x64 code on Windows before 7SP1 on a CPU with AVX2 when 
built with VS2013), and under no others, and these conditions flip the switch 
exactly then. 

> Without that, I'd be worried that
> this cure is worse than the disease because it breaks cases that
> weren't broken before.

Isn't that what the buildfarm is (among other things) for?

-- 
Christian Ullrich 


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

Re: [HACKERS] Crash with old Windows on new CPU

[Christian Ullrich]

* Robert Haas wrote:


On Fri, Feb 12, 2016 at 7:26 PM, Christian Ullrich  wrote:



startup_hacks(), I think. Proposed patch attached.


Thanks for the report and patch.  Regrettably I haven't the Windows
knowledge to have any idea whether it's right or wrong, but hopefully
someone who knows Windows will jump in here.


In commitfest now.

--
Christian




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

Re: [HACKERS] Crash with old Windows on new CPU

[Christian Ullrich]

* Christian Ullrich wrote:


Backends (and possibly other processes) crash at the slightest
provocation, such as "SELECT * FROM pg_stat_activity;" or VACUUM. The
log says either "exception 0xC005" (segfault) or "exception
0xC01D" (illegal instruction).

The interesting reason: The old host had a Core-generation CPU, which
does not support the AVX2 instruction set. The new one has a
Haswell-generation one, and this one does. The EDB distribution of 9.4
was built with the Visual Studio 2013 compiler, whose CRT (aka libc) has
a bug where it uses AVX2 instructions if the *CPU* supports them, but
does not care whether the *OS* does, and 2008 doesn't. That support was
added in SP1 for 7/2008R2.



I just tried it, and it appears to work. If there is any interest in
fixing this, I'll be happy to prepare a patch. (Where would be the best
place to put a function call from  that has to be done during
startup of each server process, on Windows only?)


startup_hacks(), I think. Proposed patch attached.

--
Christian
diff --git a/src/backend/main/main.c b/src/backend/main/main.c
index 19176b1..4be21a0 100644
--- a/src/backend/main/main.c
+++ b/src/backend/main/main.c
@@ -26,6 +26,13 @@
 #include 
 #endif
 
+#ifdef _WIN32
+#if _MSC_VER == 1800
+#include 
+#include 
+#endif
+#endif
+
 #include "bootstrap/bootstrap.h"
 #include "common/username.h"
 #include "postmaster/postmaster.h"
@@ -263,6 +270,22 @@ startup_hacks(const char *progname)
 
/* In case of general protection fault, don't show GUI popup 
box */
SetErrorMode(SEM_FAILCRITICALERRORS | SEM_NOGPFAULTERRORBOX);
+
+#ifdef _M_AMD64
+#if _MSC_VER == 1800
+   /*
+* Avoid crashing in certain floating-point operations if
+* we were compiled for x64 with MS Visual Studio 2013 and
+* are running on Windows prior to 7/2008R2 SP1 on an 
+* AVX2-capable CPU.
+*/
+   if (!IsWindows7SP1OrGreater())
+   {
+   _set_FMA3_enable(0);
+   }
+#endif /* _MSC_VER == 1800 */
+#endif /* _M_AMD64 */
+
}
 #endif   /* WIN32 */
 

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

[HACKERS] Crash with old Windows on new CPU

[Christian Ullrich]

Hello,

I just found a compatibility issue when I was migrating an elderly VM to 
a new host. The VM is running Windows Server 2008 SP2, and it has the 
EDB build of PostgreSQL 9.4.5 on it. (9.4.6 behaves the same.) It is 
also not dependent on running in a VM; it would fail on the hardware as 
well.


Backends (and possibly other processes) crash at the slightest 
provocation, such as "SELECT * FROM pg_stat_activity;" or VACUUM. The 
log says either "exception 0xC005" (segfault) or "exception 
0xC01D" (illegal instruction).


The interesting reason: The old host had a Core-generation CPU, which 
does not support the AVX2 instruction set. The new one has a 
Haswell-generation one, and this one does. The EDB distribution of 9.4 
was built with the Visual Studio 2013 compiler, whose CRT (aka libc) has 
a bug where it uses AVX2 instructions if the *CPU* supports them, but 
does not care whether the *OS* does, and 2008 doesn't. That support was 
added in SP1 for 7/2008R2.


There is a workaround, see 
. 
It consists of a single function call, required only if the OS does not 
support AVX2.


I just tried it, and it appears to work. If there is any interest in 
fixing this, I'll be happy to prepare a patch. (Where would be the best 
place to put a function call from  that has to be done during 
startup of each server process, on Windows only?)


Otherwise, it may be time to update the manual (15.6 Supported 
Platforms) where it says PostgreSQL "can be expected to work on these 
operating systems: [...] Windows (Win2000 SP4 and later), [...]". 
Perhaps we could add "except Windows before 7 SP1/2008R2 SP1 when 
running in x64 mode on Intel CPUs introduced after May 2013 (Haswell and 
later)"?


--
Christian



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

Re: [HACKERS] pl/pgSQL, get diagnostics and big data

[Christian Ullrich]

Ah, so it turns out I should have used the commitfest tool. My 
apologies; I will send the whole thing through that again. Please 
disregard the earlier message.




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

Re: [HACKERS] pl/pgSQL, get diagnostics and big data

[Christian Ullrich]

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   tested, failed
Spec compliant:   not tested
Documentation:not tested

* Andreas 'ads' Scherbaum wrote:

> one of our customers approached us and complained, that GET DIAGNOSTICS
> row_count returns invalid results if the number of rows is > 2^31. It's

> Attached patch expands the row_count to 64 bit.
>
> diagnostics=# select testfunc_pg((2^32 + 5)::bigint);
>   testfunc_pg
> -
>4295017296
> (1 row)

This is my first patch review, but I have to get my feet wet at some point, and 
this is a nice, small patch to do that.

Following the checklist from the wiki:

- Is the patch in context format: Yes.

- Does it apply cleanly to master: Yes.

- Does it include reasonable tests, doc patches, etc.: No. While
  it would be nice if it had some, a test that inserts 2^32 rows
  will take a while and can hardly be called reasonable.


The patch is designed to expand the size of the "affected records" count in the 
command tag from 32 to 64 bits.

- Does it do that: Yes.

- Do we want that: Yes, because it is motivated by reports from users
  who have queries like that in real life.

- Do we already have it: No.

- Does it follow SQL spec or community-agreed behavior: This is not
  covered by the SQL standard and there has not, to my knowledge, been
  any discussion on this point on -hackers. It is, however, the
  obvious approach to solving the specific issue.

- Does it include pg_dump support: n/a

- Are there dangers: Existing applications and client libraries must
  support the increased maximum size (up to nine additional digits) and
  maximum value. libpq apparently does not parse the command tag, only
  stores it as a string for retrieval by PQcmdStatus(), so it is not
  affected in terms of parsing the value, and for storage, it uses a
  64-character buffer, which will overflow if the command name part of
  the tag exceeds 32 characters (63 - 19 [row count] - 10 [OID] - 2
  [spaces]). The longest command name I can think of is "REFRESH
  MATERIALIZED VIEW" which, at 25 characters, stays comfortably below
  this limit, and does not include a row count anyway.

- Have all the bases been covered: The patch changes all locations
  where the command tag is formatted, and where the record count is
  retrieved by PL/pgSQL.

- Does the patch follow the coding guidelines: I believe so.

- Are there portability issues/Will it work on Windows/BSD etc.:

  No, it will not work correctly on Windows when built with MSVC,
  although it may work with MinGW.

  +++ postgresql-9.5.0/src/backend/tcop/pquery.c
  @@ -195,7 +195,7 @@
   {
 case CMD_SELECT:
 snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
  - "SELECT %u", queryDesc->estate->es_processed);
  + "SELECT %lu", queryDesc->estate->es_processed);


  %lu formats unsigned long. "long" is problematic in terms of
  portability, because sizeof(long) is different everywhere. It is 32
  bits on Windows and on 32-bit *nix, and 64 bits on 64-bit *nix.

  I added the following line to the INSERT formatting in pquery.c:

queryDesc->estate->es_processed += 471147114711LL;

  This number is 0x6DB28E70D7; so inserting one row should return
  "INSERT 0 2995679448" (0xB28E70D8):

postgres=# insert into t1 values (0);
INSERT 0 2995679448

  To fix this, I think it will be enough to change the format strings to
  use "%zu" instead of "%lu". pg_snprintf() is selected by configure if
  the platform's snprintf() does not support the "z" conversion. I tried
  this, and it appears to work:

postgres=# insert into t1 values (0);
INSERT 0 471147114712

  I have looked for other uses of "%lu", and found none that may cause
  the same issue; apparently they are all used with values that clearly
  have 32-bit type; actually, most of them are used to format error
  codes in Windows-specific code.

- Are the comments sufficient and accurate: Yes.

- Does it do what it says, correctly: Yes, except for the Windows thing.

- Does it produce compiler warnings: No. First, pg_snprintf() does not
  use the system implementation, and second, a warning (C4477) for
  this kind of format string mismatch was only added in VS2015, which
  is not officially supported (it works for me).

- Can you make it crash: No. The problematic argument always appears
  last in the sprintf() calls, so the format string issue should not
  be exploitable.

I did not run the regression tests or do the "performance" sections after I 
found the Windows issue. I do not think it will negatively affect performance, 
though.

In all, if replacing four "l"s with "z"s is indeed enough, I think this patch 
is an appropriate solution for solving the underlying issue.

The new status of this patch is: Waiting on Author

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your 

Re: [HACKERS] pl/pgSQL, get diagnostics and big data

[Christian Ullrich]

* Andreas 'ads' Scherbaum wrote:


one of our customers approached us and complained, that GET DIAGNOSTICS
row_count returns invalid results if the number of rows is > 2^31. It's



Attached patch expands the row_count to 64 bit.

diagnostics=# select testfunc_pg((2^32 + 5)::bigint);
  testfunc_pg
-
   4295017296
(1 row)


This is my first patch review, but I have to get my feet wet at some 
point, and this is a nice, small patch to do that.


Following the checklist from the wiki:

- Is the patch in context format: Yes.

- Does it apply cleanly to master: Yes.

- Does it include reasonable tests, doc patches, etc.: No. While
  it would be nice if it had some, a test that inserts 2^32 rows
  will take a while and can hardly be called reasonable.


The patch is designed to expand the size of the "affected records" count 
in the command tag from 32 to 64 bits.


- Does it do that: Yes.

- Do we want that: Yes, because it is motivated by reports from users
  who have queries like that in real life.

- Do we already have it: No.

- Does it follow SQL spec or community-agreed behavior: This is not
  covered by the SQL standard and there has not, to my knowledge, been
  any discussion on this point on -hackers. It is, however, the
  obvious approach to solving the specific issue.

- Does it include pg_dump support: n/a

- Are there dangers: Existing applications and client libraries must
  support the increased maximum size (up to nine additional digits) and
  maximum value. libpq apparently does not parse the command tag, only
  stores it as a string for retrieval by PQcmdStatus(), so it is not
  affected in terms of parsing the value, and for storage, it uses a
  64-character buffer, which will overflow if the command name part of
  the tag exceeds 32 characters (63 - 19 [row count] - 10 [OID] - 2
  [spaces]). The longest command name I can think of is "REFRESH
  MATERIALIZED VIEW" which, at 25 characters, stays comfortably below
  this limit, and does not include a row count anyway.

- Have all the bases been covered: The patch changes all locations
  where the command tag is formatted, and where the record count is
  retrieved by PL/pgSQL.

- Does the patch follow the coding guidelines: I believe so.

- Are there portability issues/Will it work on Windows/BSD etc.:

  No, it will not work correctly on Windows when built with MSVC,
  although it may work with MinGW.

  +++ postgresql-9.5.0/src/backend/tcop/pquery.c
  @@ -195,7 +195,7 @@
   {
case CMD_SELECT:
snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
  -  "SELECT %u", queryDesc->estate->es_processed);
  +  "SELECT %lu", queryDesc->estate->es_processed);


  %lu formats unsigned long. "long" is problematic in terms of
  portability, because sizeof(long) is different everywhere. It is 32
  bits on Windows and on 32-bit *nix, and 64 bits on 64-bit *nix.

  I added the following line to the INSERT formatting in pquery.c:

queryDesc->estate->es_processed += 471147114711LL;

  This number is 0x6DB28E70D7; so inserting one row should return
  "INSERT 0 2995679448" (0xB28E70D8):

postgres=# insert into t1 values (0);
INSERT 0 2995679448

  To fix this, I think it will be enough to change the format strings to
  use "%zu" instead of "%lu". pg_snprintf() is selected by configure if
  the platform's snprintf() does not support the "z" conversion. I tried
  this, and it appears to work:

postgres=# insert into t1 values (0);
INSERT 0 471147114712

  I have looked for other uses of "%lu", and found none that may cause
  the same issue; apparently they are all used with values that clearly
  have 32-bit type; actually, most of them are used to format error
  codes in Windows-specific code.

- Are the comments sufficient and accurate: Yes.

- Does it do what it says, correctly: Yes, except for the Windows thing.

- Does it produce compiler warnings: No. First, pg_snprintf() does not
  use the system implementation, and second, a warning (C4477) for
  this kind of format string mismatch was only added in VS2015, which
  is not officially supported (it works for me).

- Can you make it crash: No. The problematic argument always appears
  last in the sprintf() calls, so the format string issue should not
  be exploitable.

I did not run the regression tests or do the "performance" sections 
after I found the Windows issue. I do not think it will negatively 
affect performance, though.


In all, if replacing four "l"s with "z"s is indeed enough, I think this 
patch is an appropriate solution for solving the underlying issue.


--
Christian



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

Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Christian Ullrich]

* Christian Ullrich wrote:


* Christian Ullrich wrote:


* Christian Ullrich wrote:

> According to the release notes, the default for the "include_realm"
> option in SSPI authentication was changed from off to on in 9.5 for

 > > improved security. However, the authenticated user name, with the
 > > option enabled, includes the NetBIOS domain name, *not* the Kerberos

> realm name:



Below is a patch to correct this behavior. I suspect it has some
serious compatibility issues, so I would appreciate feedback.


Updated patch, sorry. The first one worked by accident only.


Another update. This time even the documentation builds.

One thing I'm fairly sure I need advice on is error handling and/or 
error codes. Right now I use ERROR_INVALID_ROLE_SPECIFICATION just about 
everywhere (because the surrounding SSPI code does as well), and that is 
probably not the best choice in some places.


--
Christian


diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 3b2935c..cb4fe5e 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -1097,6 +1097,41 @@ omicron bryanh  guest1
  
 
  
+  real_realm
+  
+   
+If set to 0, the domain's SAM-compatible name (also known as the
+NetBIOS name) is used for the include_realm
+option. This is the default. If set to 1, the true realm name from
+the Kerberos user principal name is used. Leave this option
+disabled to maintain compatibility with existing 
+pg_ident.conf files.
+   
+   
+Do not enable this option unless your server runs under a domain
+account (this includes virtual service accounts on a domain member
+system) and all clients authenticating through SSPI are also using
+domain accounts, or authentication will fail.
+   
+  
+ 
+
+ 
+  upn_username
+  
+   
+If this option is enabled along with real_realm,
+the user name from the Kerberos UPN is used for authentication. If
+it is disabled (the default), the SAM-compatible user name is used.
+Note that libpq uses the SAM-compatible name if no
+explicit user name is specified. If you use
+libpq (e.g. through the ODBC driver), you should
+leave this option disabled.
+   
+  
+ 
+
+ 
   map
   

diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 57c2f48..0a0f5e3 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -155,6 +155,11 @@ typedef SECURITY_STATUS
(WINAPI * QUERY_SECURITY_CONTEXT_TOKEN_FN) (

   PCtxtHandle, void **);
 static int pg_SSPI_recvauth(Port *port);
+static int pg_SSPI_make_upn(char *accountname,
+size_t accountnamesize,
+char *domainname,
+size_t domainnamesize,
+bool 
update_accountname);
 #endif
 
 /*
@@ -1026,6 +1031,7 @@ static int
 pg_SSPI_recvauth(Port *port)
 {
int mtype;
+   int status;
StringInfoData buf;
SECURITY_STATUS r;
CredHandle  sspicred;
@@ -1263,6 +1269,15 @@ pg_SSPI_recvauth(Port *port)
 
free(tokenuser);
 
+   if (port->hba->real_realm)
+   {
+   status = pg_SSPI_make_upn(accountname, sizeof(accountname),
+ domainname, 
sizeof(domainname),
+ 
port->hba->upn_username);
+   if (status != STATUS_OK)
+   return status;
+   }
+
/*
 * Compare realm/domain if requested. In SSPI, always compare case
 * insensitive.
@@ -1298,6 +1313,102 @@ pg_SSPI_recvauth(Port *port)
else
return check_usermap(port->hba->usermap, port->user_name, 
accountname, true);
 }
+
+static int pg_SSPI_make_upn(char *accountname,
+size_t accountnamesize,
+char *domainname,
+size_t domainnamesize,
+bool 
update_accountname)
+{
+   char *samname;
+   char *upname = NULL;
+   char *p = NULL;
+   ULONG upnamesize = 0;
+   size_t upnamerealmsize;
+   BOOLEAN res;
+
+   /* Build SAM name (DOMAIN\\user), then translate to UPN
+

[HACKERS] Close handle leak in SSPI auth

[Christian Ullrich]

Hello,

here's a one-line patch to close a handle leak in pg_SSPI_recvauth().

According to the docs, the token retrieved with 
QuerySecurityContextToken() must be closed.


--
Christian

diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
new file mode 100644
index 0131bfd..57c2f48
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
*** pg_SSPI_recvauth(Port *port)
*** 1253,1258 
--- 1253,1260 
(errmsg_internal("could not get user token: 
error code %lu",
 
GetLastError(;
  
+   CloseHandle(token);
+ 
if (!LookupAccountSid(NULL, tokenuser->User.Sid, accountname, 
&accountnamesize,
  domainname, &domainnamesize, 
&accountnameuse))
ereport(ERROR,

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

Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Christian Ullrich]

* Christian Ullrich wrote:


* Christian Ullrich wrote:

> According to the release notes, the default for the "include_realm"
> option in SSPI authentication was changed from off to on in 9.5 for

> > improved security. However, the authenticated user name, with the
> > option enabled, includes the NetBIOS domain name, *not* the Kerberos

> realm name:



Below is a patch to correct this behavior. I suspect it has some
serious compatibility issues, so I would appreciate feedback.


Updated patch, sorry. The first one worked by accident only.

--
Christian


diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
new file mode 100644
index 3b2935c..7236459
*** a/doc/src/sgml/client-auth.sgml
--- b/doc/src/sgml/client-auth.sgml
*** omicron bryanh
*** 1097,1102 
--- 1097,1132 
   
  
   
+   real_realm
+   
+
+ If set to 0, the domain's SAM-compatible name (also known as the
+ NetBIOS name) is used for the include_realm
+ option. This is the default. If set to 1, the true realm name from
+ the Kerberos user principal name is used. If you used the
+ include_realm option, you can leave this option
+ disabled to maintain compatibility with existing 
+ pg_ident.conf files.
+
+   
+  
+ 
+  
+   upn_username
+   
+
+ If this option is enabled along with real_realm,
+ the user name from the Kerberos UPN is used for authentication. If
+ it is disabled (the default), the SAM-compatible user name is used.
+ Note that libpq uses the SAM-compatible name if no
+ explicit user name is specified. If you use
+ libpq (e.g. through the ODBC driver), you should
+ leave this option disabled.
+
+   
+  
+ 
+  
map

 
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
new file mode 100644
index 0131bfd..0f28f54
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
*** typedef SECURITY_STATUS
*** 155,160 
--- 155,165 
(WINAPI * QUERY_SECURITY_CONTEXT_TOKEN_FN) (

   PCtxtHandle, void **);
  static intpg_SSPI_recvauth(Port *port);
+ static intpg_SSPI_make_upn(char *accountname,
+size_t accountnamesize,
+char *domainname,
+size_t domainnamesize,
+bool 
update_accountname);
  #endif
  
  /*
*** static int
*** 1026,1031 
--- 1031,1037 
  pg_SSPI_recvauth(Port *port)
  {
int mtype;
+   int status;
StringInfoData buf;
SECURITY_STATUS r;
CredHandle  sspicred;
*** pg_SSPI_recvauth(Port *port)
*** 1261,1266 
--- 1267,1281 
  
free(tokenuser);
  
+   if (port->hba->real_realm) {
+   status = pg_SSPI_make_upn(accountname, sizeof(accountname),
+ domainname, 
sizeof(domainname),
+ 
port->hba->upn_username);
+   if (status != STATUS_OK) {
+   return status;
+   }
+   }
+ 
/*
 * Compare realm/domain if requested. In SSPI, always compare case
 * insensitive.
*** pg_SSPI_recvauth(Port *port)
*** 1296,1301 
--- 1311,1407 
else
return check_usermap(port->hba->usermap, port->user_name, 
accountname, true);
  }
+ 
+ static intpg_SSPI_make_upn(char *accountname,
+size_t accountnamesize,
+char *domainname,
+size_t domainnamesize,
+bool 
update_accountname)
+ {
+   char *samname;
+   char *upname = NULL;
+   char *p = NULL;
+   ULONG upnamesize = 0;
+   size_t upnamerealmsize;
+   BOOLEAN res;
+ 
+   /* Build SAM name (DOMAIN\\user), then translate to UPN
+  (user@kerberos.realm). The realm name is returned in
+  lower case, but that is fine because in SSPI auth,
+  string comparisons are always case-insensitive. */
+ 
+   samname = psprintf("%s\\%s", domainname, accountname);
+   res = TranslateName(samname, NameSamCompatible, NameUserPrincipal,
+

Re: [HACKERS] libxml2 2.9.3 breaks xml test output

[Christian Ullrich]

* Michael Paquier wrote:


On Sat, Dec 5, 2015 at 4:38 PM, Christian Ullrich  wrote:

I have zero experience with libxml2, so no idea if the previous context
level can be turned on again. IMHO, the libxml2 change is a bug in itself;
PostgreSQL's error messages are more readable with the XML text in them.


See here for example you are not the only one to see this problem:
http://www.postgresql.org/message-id/cafj8pra4xjqfgnqcqmcygx-umgmr3stt3xfeuw7kbsoiovg...@mail.gmail.com
https://bugzilla.redhat.com/show_bug.cgi?id=1286692


Right. Sorry, I had checked the archives for earlier mentions of the 
issue, but had not found Pavel's post.


--
Christian





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

[HACKERS] libxml2 2.9.3 breaks xml test output

[Christian Ullrich]

Hello,

I just noticed that last night all built branches failed on my buildfarm 
animal, jaguarundi. They all failed on the "xml" test, and the output is 
essentially the same everywhere:


***
*** 9,16 
  LINE 1: INSERT INTO xmltest VALUES (3, '  SELECT xmlconcat('', NULL, 'standalone="no"?>');

xmlconcat
  --

The last thing I did in regard to XML on this system was upgrade libxml2 
from 2.9.2 to 2.9.3. I bisected it a a bit and the guilty commit to 
libxml2 is ce0b0d0d8 "Do not print error context when there is none" 
from about two weeks ago.


I have zero experience with libxml2, so no idea if the previous context 
level can be turned on again. IMHO, the libxml2 change is a bug in 
itself; PostgreSQL's error messages are more readable with the XML text 
in them.


--
Christian



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

Re: [HACKERS] pg_xlog -> pg_xjournal?

[Christian Ullrich]

* Michael Nolan wrote:

> Why not take a simpler approach and create a zero length file in directories 
> that should not be fiddled with by non-experts using a file name something 
> like "DO.NOT.DELETE.THESE.FILES"? 

Move the critical things into a new subdirectory?
 $PGDATA/pg_critical/pg_xlog? Perhaps "pg_internal" instead, or
 "pg_indispensable", or "pg_here_be_dragons", or
 "pg_delete_this_or_anything_in_here_and_you_wil
l_have_utterly_destroyed_your_database"? That last one should be
 clear enough, if a tad long for deeply nested PGDATA
 locations.


Symlinking the old path to the new one would be impossible, of
 course, if the intent is to protect against "rm -rf *log*/*"
 fiends.

-- 

Christian



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

Re: [HACKERS] hstore_plpython regression test does not work on Python 3

[Christian Ullrich]

* Peter Eisentraut wrote:


On 5/16/15 12:06 PM, Tom Lane wrote:

As exhibited for instance here:

http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=spoonbill&dt=2015-05-16%2011%3A00%3A07

I've been able to replicate this on a Fedora 21 box: works fine with
Python 2, fails with Python 3.  Seems like we still have an issue
with reliance on a system-provided sort method.


Pushed a fix, tested with 2.3 .. 3.4.


There is still a sorting problem (of sorts). jaguarundi [1] keeps 
failing intermittently like this:


*** 47,53 
  return len(val)
  $$;
  SELECT test1arr(array['aa=>bb, cc=>NULL'::hstore, 'dd=>ee']);
! INFO:  [{'aa': 'bb', 'cc': None}, {'dd': 'ee'}]
  CONTEXT:  PL/Python function "test1arr"
   test1arr
  --
--- 47,53 
  return len(val)
  $$;
  SELECT test1arr(array['aa=>bb, cc=>NULL'::hstore, 'dd=>ee']);
! INFO:  [{'cc': None, 'aa': 'bb'}, {'dd': 'ee'}]
  CONTEXT:  PL/Python function "test1arr"
   test1arr
  --

I cannot find any other animal that does the same, but I doubt it's due 
to CCA this time.


Should dict tests perhaps output sorted(thedict.items()) instead? 
Testing dict formatting could be done with single-item dicts.


[1] http://pgbuildfarm.org/cgi-bin/show_history.pl?nm=jaguarundi&br=HEAD

--
Christian




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

Re: [HACKERS] transforms vs. CLOBBER_CACHE_ALWAYS

[Christian Ullrich]

* Tom Lane wrote:


Christian Ullrich  writes:

* Peter Eisentraut wrote:

On 4/30/15 2:49 PM, Andrew Dunstan wrote:

friarbird is a FreeBSD buildfarm animal running with
-DCLOBBER_CACHE_ALWAYS. It usually completes a run in about 6.5 hours.
However, it's been stuck since Monday running the plpython regression
tests. The only relevant commit seems to be the transforms feature.



I can reproduce it.  I'll look into it.



I looked over the CCA animals and noticed that pademelon and gaur are
apparently unaffected;


pademelon and gaur do not run CCA (if they did, it would take weeks for
a run to complete :-().


Ah. I obviously had associated the "note" icon with CCA. Sorry. OTOH, if 
I had noticed that, I might not have gone into more detail ...


--
Christian





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

Re: [HACKERS] transforms vs. CLOBBER_CACHE_ALWAYS

[Christian Ullrich]

* Peter Eisentraut wrote:


On 4/30/15 2:49 PM, Andrew Dunstan wrote:



friarbird is a FreeBSD buildfarm animal running with
-DCLOBBER_CACHE_ALWAYS. It usually completes a run in about 6.5 hours.
However, it's been stuck since Monday running the plpython regression
tests. The only relevant commit seems to be the transforms feature.


I can reproduce it.  I'll look into it.


I looked over the CCA animals and noticed that pademelon and gaur are 
apparently unaffected; what they have in common is the OS (HP-UX) and 
the Python version (2.5). There's nothing I can do about OS-related 
differences, but I thought I'd check the Python angle.


With Python 2.5.6, jaguarundi locks up on the plpython tests just the 
same as with 3.4, and friarbird with 2.7. So that is not the culprit, 
either.


I ran make check by hand, and noticed three tests where it seemed to 
hang (I gave it at least three minutes each, and the functions in the 
queries are simple):


plpython_spiSELECT cursor_plan();
plpython_setof  SELECT test_setof_as_list(1, 'list');
plpython_composite  SELECT multiout_simple_setof();

These are the only plpython tests that mention SETOF at all, and the 
queries that hung are the first ones in their respective tests to 
actually build a set.


Does that help?

--
Christian




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

Re: [HACKERS] transforms vs. CLOBBER_CACHE_ALWAYS

[Christian Ullrich]

* Andrew Dunstan:


friarbird is a FreeBSD buildfarm animal running with
-DCLOBBER_CACHE_ALWAYS. It usually completes a run in about 6.5 hours.
However, it's been stuck since Monday running the plpython regression
tests. The only relevant commit seems to be the transforms feature.
Here's what it's been doing:



query| SELECT cursor_plan();


Same here, on jaguarundi. I actually killed it intentionally this 
morning, hoping that whatever the problem was might have been fixed 
already. No such luck.


I would suspect that it might have something to do with the OS, if all 
the other CCA animals weren't lining up nicely behind in the buildfarm 
status page.



I imagine it was in some sort of infinite loop. gdb says it's all in
src/backend/utils/cache/plancache.c, although not the same line each
time I run it.


I ktrace'd it this morning, but cleverly did not keep the dump. It 
looked much the same to me, though, it was reading the same filenode 
over and over again.


--
Christian




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

Re: [HACKERS] [committers] pgsql: RLS fixes, new hooks, and new test module

[Christian Ullrich]

* Stephen Frost wrote:


RLS fixes, new hooks, and new test module


The buildfarm says that with -DCLOBBER_CACHE_ALWAYS, the RLS violations 
get blamed on the wrong tables. Mostly, they are catalogs (I have seen 
pg_opclass, pg_am, and pg_amproc), but some also come up with binary 
garbage instead, e.g.


- 
http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=markhor&dt=2015-04-23%2000%3A00%3A12
- 
http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=friarbird&dt=2015-04-23%2004%3A20%3A00
- 
http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=tick&dt=2015-04-22%2019%3A56%3A53


--
Christian




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

Re: [HACKERS] New Python vs. old PG on raccoon and jaguarundi

[Christian Ullrich]

* From: Noah Misch [mailto:n...@leadboat.com]

> Buildfarm member jaguarundi, which has Python 3.4, activated --with-
> python for REL9_1_STABLE as of its 2014-12-15 run.  Please remove --
> with-python or test against an older Python.  It already omits --with-
> python for REL9_0_STABLE.

Done; sorry for the noise.

-- 
Christian



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

Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED

[Christian Ullrich]

* Alvaro Herrera wrote:


Michael Paquier wrote:


Btw, perhaps this diff should be pushed as a different patch as this is a
rather different thing:
-   if (heapRelation->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED
&&
+   if (indexRelation->rd_rel->relpersistence ==
RELPERSISTENCE_UNLOGGED &&
 !smgrexists(indexRelation->rd_smgr, INIT_FORKNUM)
As this is a correctness fix, it does not seem necessary to back-patch it:
the parent relation always has the same persistence as its indexes.


There was an argument for doing it this way that only applies if this
patch went in, but I can't remember now what it was.

Anyway I pushed the patch after some slight additional changes.  Thanks!


The buildfarm says -DCLOBBER_CACHE_ALWAYS does not like this patch.

--
Christian





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

Re: [HACKERS] PostgreSQL in Windows console and Ctrl-C

[Christian Ullrich]

* From: Noah Misch [mailto:n...@leadboat.com]

> On Mon, Jun 30, 2014 at 07:28:03PM +0000, Christian Ullrich wrote:

> > * From: Noah Misch [mailto:n...@leadboat.com]

> > > I liked the proposal here; was there a problem with it?
> > > http://www.postgresql.org/message-
> > > id/ca+tgmoz3ake4enctmqmzsykc_0pjl_u4c_x47ge48uy1upb...@mail.gmail.co
> > > m
> >
> > You're referring to the suggestion of accepting and ignoring the
> > option on non-Windows, right? I can do that, I just don't see the
> > point as long as pg_ctl has a separate code path (well, #ifdef) for
> > Windows anyway.
> 
> Yes.  We normally recognize platform-specific options on every platform.
> For example, the effective_io_concurrency GUC exists on all platforms, but
> you can only change it on platforms where it matters.  In that light, one
> could argue for raising an error for --background on non-Windows systems.
> I don't have a strong opinion on raising an error vs. ignoring the option,
> but I do think the outcome of --background should be distinguishable from
> the outcome of --sometypo on all platforms.

I'm convinced, will change to --sometypo treatment.

> > > The pg_upgrade test suite and the $(prove_check)-based test suites
> > > rely on their pg_ctl-started postmasters receiving any console ^C.
> > > pg_ctl deserves a --foreground or --no-background option for callers
> > > that prefer the current behavior.  That, or those tests need a new
> > > way to launch the postmaster.
> >
> > Ah. More good news. Just to confirm, this is only about the tests,
> > right, not the code they are testing?
> 
> Yes; the consequence of ignoring ^C is that the test postmaster would
> persist indefinitely after the ^C.  The system under test doesn't care per

No it won't, please see below.

> > If so, is there even a way to run either on Windows? The pg_upgrade
> > test suite is a shell script, and prove_check is defined in
> > Makefile.global and definitely not applicable to Windows.
> 
> contrib/pg_upgrade/test.sh works under MSYS.  Perhaps $(prove_check)
> currently doesn't, but that's something to be fixed, not relied upon.

Yes. That doesn't matter, though.

> There's a clone of contrib/pg_upgrade/test.sh in
> src/tools/msvc/vcregress.pl, and I expect it would have the same problem.

Oh, right. I didn't notice that because it doesn't mention upgradecheck in its 
usage message.

I think I know where we're talking past each other. You may be assuming that 
kill(postmaster, SIGINT) would be affected by my patch. It would not. 
PostgreSQL's signal emulation on Windows works completely separately from the 
actual Windows analogy to signals in console processes. My patch drops these 
"analogous" events, but the emulated signals still work the same way as before.

When Ctrl-C is pressed in a console window, pg_console_handler() in 
backend/port/win32/signal.c is called with a CTRL_C_EVENT, and converts that to 
an emulated SIGINT (unless I tell it not to). That emulated SIGINT is then 
processed as usual. pg_ctl -m fast stop sends the emulated SIGINT directly.

Anyway, I just ran upgradecheck, and it reported success without leaving any 
stale postmasters around.

-- 
Christian


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

Re: [HACKERS] PostgreSQL in Windows console and Ctrl-C

[Christian Ullrich]

* From: Noah Misch [mailto:n...@leadboat.com]

> I liked the proposal here; was there a problem with it?
> http://www.postgresql.org/message-
> id/ca+tgmoz3ake4enctmqmzsykc_0pjl_u4c_x47ge48uy1upb...@mail.gmail.com

You're referring to the suggestion of accepting and ignoring the option on
non-Windows, right? I can do that, I just don't see the point as long as
pg_ctl has a separate code path (well, #ifdef) for Windows anyway.

> The pg_upgrade test suite and the $(prove_check)-based test suites rely on
> their pg_ctl-started postmasters receiving any console ^C.  pg_ctl
> deserves a --foreground or --no-background option for callers that prefer
> the current behavior.  That, or those tests need a new way to launch the
> postmaster.

Ah. More good news. Just to confirm, this is only about the tests, right,
not the code they are testing?

If so, is there even a way to run either on Windows? The pg_upgrade test
suite is a shell script, and prove_check is defined in Makefile.global and 
definitely not applicable to Windows.

-- 
Christian


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

Re: [HACKERS] PostgreSQL in Windows console and Ctrl-C

[Christian Ullrich]

* From: MauMau [mailto:maumau...@gmail.com]

> From: "Christian Ullrich" 

> > OK, here is the first draft against current master. It builds on Windows
> > with VS 2012 and on FreeBSD 10 with clang 3.3. I ran the regression
> > tests on Windows, they all pass.
> >
> > The changed behavior is limited to Windows, where it now silently
> > ignores Ctrl-C and Ctrl-Break when started via pg_ctl start.
> >
> > I don't think there is currently any support for switch-type long
> > options, so rather than invent my own, I squeezed the two lines I added
> > into postmaster.c where they fit best; unfortunately, the result is
> > quite ugly. I'll be happy to refine that if someone can give me a hint
> > on how to do it.
> 
> Overall, the patch seems good as it is based on the discussion.  I found a
> few problems:

Thank you for the review. I will rebase, retest, and resubmit as soon as I find 
the time, certainly sometime this week.

> (2)
> Although I haven't tried, doesn't pg_ctl start fail on non-Windows
> platforms
> because of the following check?
> 
> !if (opt == '-')
> ! ereport(ERROR,
> !   (errcode(ERRCODE_SYNTAX_ERROR),
> !errmsg("--%s requires a value",
> ! optarg)));

On non-Windows platforms, the --background option is not passed, and the option 
handling is unmodified except for an additional pair of braces. The postmaster 
does not pass the option to its children on any platform.

> And, in the postgres reference page,
> 
> http://www.postgresql.org/docs/devel/static/app-postgres.html
> 
> there's a paragraph:
> 
> "The -- options will not work on FreeBSD or OpenBSD. Use -c instead. This
> is a bug in the affected operating systems; a future release of PostgreSQL
> willprovide a workaround if this is not fixed."
> 
> Would --background work on FreeBSD and OpenBSD (i.e. would pg_ctl start
> succeed)?  I don't have access to those platforms.

pg_ctl does not pass the option anywhere but on Windows, and postmaster.c does 
not recognize it anywhere else. If it is encountered on a platform where it 
does not make sense, it will be treated like any other (unknown) long option.

This is actually the weakest point of the existing patch, in my opinion. 
Jamming the long option handling into postmaster.c by way of #ifdef WIN32 feels 
wrong, but I could not figure out a better way to do it.

> (3)
> --background will also be used by restart subcommand, won't it?
> 
> + in a console window. It is used automatically by
> + pg_ctl when called with the
> + start subcommand.

Restart is implemented as stop/start, so, yes.

-- 
Christian

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

Re: [HACKERS] PostgreSQL in Windows console and Ctrl-C

[Christian Ullrich]

OK, here is the first draft against current master. It builds on Windows 
with VS 2012 and on FreeBSD 10 with clang 3.3. I ran the regression 
tests on Windows, they all pass.


The changed behavior is limited to Windows, where it now silently 
ignores Ctrl-C and Ctrl-Break when started via pg_ctl start.


I don't think there is currently any support for switch-type long 
options, so rather than invent my own, I squeezed the two lines I added 
into postmaster.c where they fit best; unfortunately, the result is 
quite ugly. I'll be happy to refine that if someone can give me a hint 
on how to do it.


Patch attached, will add to CF soon.

--
Christian

diff --git a/doc/src/sgml/ref/postgres-ref.sgml 
b/doc/src/sgml/ref/postgres-ref.sgml
new file mode 100644
index 8e225e4..731682c
*** a/doc/src/sgml/ref/postgres-ref.sgml
--- b/doc/src/sgml/ref/postgres-ref.sgml
*** PostgreSQL documentation
*** 590,595 
--- 590,620 
   
  
 
+ 
+
+ Platform-specific Options
+ 
+   
+--background
+
+ background execution
+ Windows
+
+
+ 
+  This option is effective on Windows only and ignored on all other 
platforms.
+ 
+ 
+  It instructs the server to ignore the signals caused by pressing
+  CtrlC or
+  CtrlBreak
+  in a console window. It is used automatically by
+  pg_ctl when called with the
+  start subcommand.
+ 
+
+   
+
   
  
   
diff --git a/src/backend/port/win32/signal.c b/src/backend/port/win32/signal.c
new file mode 100644
index 322b857..91f8243
*** a/src/backend/port/win32/signal.c
--- b/src/backend/port/win32/signal.c
*** static pqsigfunc pg_signal_defaults[PG_S
*** 41,46 
--- 41,48 
  static DWORD WINAPI pg_signal_thread(LPVOID param);
  static BOOL WINAPI pg_console_handler(DWORD dwCtrlType);
  
+ extern bool IsBackgroundPostmaster;
+ extern bool IsUnderPostmaster;
  
  /*
   * pg_usleep --- delay the specified number of microseconds, but
*** pg_signal_thread(LPVOID param)
*** 346,358 
  static BOOL WINAPI
  pg_console_handler(DWORD dwCtrlType)
  {
!   if (dwCtrlType == CTRL_C_EVENT ||
!   dwCtrlType == CTRL_BREAK_EVENT ||
!   dwCtrlType == CTRL_CLOSE_EVENT ||
!   dwCtrlType == CTRL_SHUTDOWN_EVENT)
{
pg_queue_signal(SIGINT);
!   return TRUE;
}
!   return FALSE;
  }
--- 348,369 
  static BOOL WINAPI
  pg_console_handler(DWORD dwCtrlType)
  {
!   switch (dwCtrlType)
{
+   case CTRL_C_EVENT:
+   case CTRL_BREAK_EVENT:
+   /* Ignore if started with "pg_ctl start". */
+   if (IsUnderPostmaster || IsBackgroundPostmaster)
+   break;
+   /* fall through */
+   case CTRL_CLOSE_EVENT:
+   case CTRL_SHUTDOWN_EVENT:
pg_queue_signal(SIGINT);
!   break;
!   default:
!   return FALSE;
}
! 
!   return TRUE;
  }
+ 
diff --git a/src/backend/postmaster/postmaster.c 
b/src/backend/postmaster/postmaster.c
new file mode 100644
index b573fd8..d1ce978
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
*** PostmasterMain(int argc, char *argv[])
*** 751,771 
   *value;
  
ParseLongOption(optarg, &name, &value);
!   if (!value)
{
!   if (opt == '-')
!   ereport(ERROR,
!   
(errcode(ERRCODE_SYNTAX_ERROR),
!
errmsg("--%s requires a value",
!   
optarg)));
!   else
!   ereport(ERROR,
!   
(errcode(ERRCODE_SYNTAX_ERROR),
!
errmsg("-c %s requires a value",
!   
optarg)));
}
  
-   SetConfigOption(name, value, 
PGC_POSTMASTER, PGC_S_ARGV);
free(name);
if (value)
free(value);
--- 751,780 
   *value;
  
ParseLongOption(optarg, &name, &value);
! 

Re: [HACKERS] PostgreSQL in Windows console and Ctrl-C

[Christian Ullrich]

* From: Amit Kapila

> On Mon, Apr 14, 2014 at 11:46 AM, Christian Ullrich
>  wrote:


> > * From: Amit Kapila
> >> Do you mean to say use some existing environment variable?
> >> Introducing an environment variable to solve this issue or infact
> >> using some existing environ variable doesn't seem to be the best way
> >> to pass such information.
> >
> > I meant creating a new one, yes. If, say, PGSQL_BACKGROUND_JOB was
> > set, the postmaster etc. would ignore the events.
> 
> Do you plan to reset it and if yes when?
> I think there is chance that after setting this environment variable,
> some other instance of server (postmaster) can read it and missed the
> signal which it should have otherwise processed.

We have decided not to go this way, but just for completeness:

Environment inheritance works the same way on Windows as on Unix. When
a process is started with a modified environment (one of the plentiful
arguments of CreateProcess() et al.), only that process and its 
descendants see the modification. I had not planned to set a system-level
or user-level variable.

-- 
Christian

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

Re: [HACKERS] PostgreSQL in Windows console and Ctrl-C

[Christian Ullrich]

* From: Bruce Momjian

> On Mon, Apr 14, 2014 at 09:34:14AM +0530, Amit Kapila wrote:
> > The problem can be solved this way, but the only question here is
> > whether it is acceptable for users to have a new console window for
> > server.
> >
> > Can others also please share their opinion if this fix (start server
> > in new console) seems acceptable or shall we try by passing some
> > information from pg_ctl and then ignore CTRL+C && CTRL+BREAK for it?
> 
> I wanted to point out that I think this patch is only appropriate for
> head, not backpatching.  Also, on Unix we have to handle signals that

Yes, of course.

> come from the kill command.  Can you send CTRL+C from other applications
> on Windows?

Yes again, using GenerateConsoleCtrlEvent() you can send these events to
any (console-attached) process you have the required permissions for, 
but that is not an issue for the same reason it isn't one on Unix. All
the target process sees is the event, it cannot determine (nor does it 
care) where the event came from.

-- 
Christian



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

Re: [HACKERS] PostgreSQL in Windows console and Ctrl-C

[Christian Ullrich]

* From: Robert Haas

> On Mon, Apr 14, 2014 at 2:16 AM, Christian Ullrich
>  wrote:

> > I meant creating a new one, yes. If, say, PGSQL_BACKGROUND_JOB was
> > set, the postmaster etc. would ignore the events.
> 
> Why not just pass a command-line switch?

Because, as I wrote in the message you are quoting, I did not think that
having a command-line option for the sole purpose of telling the 
postmaster who its parent is was a suitable solution. 

I had already given up on that idea based on Amit's advice, and I will 
create a patch based on a command-line option.

While I have you here, though, any suggestions on what the name of that
option should be? I think --background is about right. Also, how should
I treat the option on non-Windows platforms? Should it just not be there
(= error), or be ignored if present?

-- 
Christian



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

Re: [HACKERS] PostgreSQL in Windows console and Ctrl-C

[Christian Ullrich]

* From: Amit Kapila

> On Sun, Apr 13, 2014 at 5:59 PM, Christian Ullrich 
> wrote:
> > There are some possible solutions:
> >
> > - pg_ctl could set an environment variable (unless it has to be
> >   compatible with postmasters from different versions, and it does 
> >   not, does it?).
> 
> Do you mean to say use some existing environment variable?
> Introducing an environment variable to solve this issue or infact using
> some existing environ variable doesn't seem to be the best way to pass
> such information.

I meant creating a new one, yes. If, say, PGSQL_BACKGROUND_JOB was set, 
the postmaster etc. would ignore the events.

> >   pg_ctl creates a named job object, and the postmaster has all the
> >   information it needs to reconstruct the name, so it can check if it is
> >   running inside that pg_ctl-created job.
> 
> We are already creating one job object, so may be that itself can be
> used, but not sure if Job Objects are supported on all platforms on which
> postgres works.

I mentioned that in my earlier message; we cannot rely on having that job
object around. If the OS does not support them (not much of a problem
nowadays, I think; anything anyone is going to run the version this will
first be released in on is going to) or, more likely, if something uses
PostgreSQL as "embedded" database and starts it in a job of their own,
pg_ctl will not create its job object.

There may also be software out there that a) runs PostgreSQL in this way,
b) uses the console events to stop it again, and c) does that with or
without a separate job object. Hence, deciding based on the existence of
a job object would break backward compatibility in this case, assuming
it exists in reality.

> If you have to pass such information, then why don't pass some special
> flag in command to CreateProcess()?

Because I wanted to avoid introducing a command line option to tell the
postmaster who started it. If we cannot look at job objects, and an
environment variable is not an option either, then this looks like the
best remaining solution.

I will create a patch based on this approach.

> There is always a chance that we ignore the CTRL+C for some app which
> we don't intend if we solve the problem by passing some info, as some
> other app could also do so, but I think it should not be a big problem.

No. In fact, if whatever starts the postmaster does pass that flag, we
can safely assume that it did not do so just for fun.

-- 
Christian



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

Re: [HACKERS] PostgreSQL in Windows console and Ctrl-C

[Christian Ullrich]

* From: Amit Kapila

> On Sat, Apr 12, 2014 at 12:36 PM, Christian Ullrich 
> wrote:
> > * From: Amit Kapila
> >
> >> Another thing to decide about this fix is that whether it is okay to
> >> fix it for CTRL+C and leave the problem open for CTRL+BREAK?
> >> (The current option used (CREATE_NEW_PROCESS_GROUP) will handle only
> >> CTRL+C).
> >
> > Below is a new (right now very much proof-of-concept) patch to replace
> > my earlier one. It has the same effect on Ctrl-C the change to pg_ctl
> > had, and additionally ignores Ctrl-Break as well.
> 
> This fix won't allow CTRL+C for other cases like when server is started
> directly with postgres binary.
> ./postgres -D 
> I think this doesn't come under your original complaint and as such I
> don't see any problem in allowing CTRL+C for above case.

Good point. I had not thought of that case. Just how do you tell if your 
postmaster was started by pg_ctl or directly on the command line?

- pg_ctl starts the postmaster through an intervening shell, so even if
  it did not exit right after that, we could not check the parent process
  name (assuming nobody renamed pg_ctl).

- pg_ctl starts the postmaster with stdin redirected from the null device,
  but so could anyone else. The same is true for access rights, token
  groups, and most everything else pg_ctl does.

- I don't want to add a new command line flag to postgres.exe just to tell
  it who its parent is.

- Job objects may not be supported on the underlying OS, or creation may
  have failed, or the interactive console may have been running in one
  already, so the sheer existence of a job is no proof it's ours.

There are some possible solutions:

- pg_ctl could set an environment variable (unless it has to be compatible
  with postmasters from different versions, and it does not, does it?).

  pg_ctl creates a named job object, and the postmaster has all the 
  information it needs to reconstruct the name, so it can check if it is 
  running inside that pg_ctl-created job.

I would appreciate some advice on this.

> One another way could be to use CREATE_NEW_CONSOLE instead of
> CREATE_NEW_PROCESS_GROUP in you previous fix, but I am not sure if it's
> acceptable to users to have a new console window for server.

It might. That would also isolate stderr logging from whatever else the
user is doing in the window, and it would work correctly in the pg_ctl
(and by extension the direct-postmaster) case. It would not do anything 
for events generated by keystrokes in the new console window, though.

There would also have to be a command line option to pg_ctl to either
enable or disable it, not sure which.

-- 
Christian

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

Re: [HACKERS] PostgreSQL in Windows console and Ctrl-C

[Christian Ullrich]

* From: Amit Kapila

> Another thing to decide about this fix is that whether it is okay to fix
> it for CTRL+C and leave the problem open for CTRL+BREAK?
> (The current option used (CREATE_NEW_PROCESS_GROUP) will handle only
> CTRL+C).

I can think of three situations in which a postgres process can run on
Windows:

- single backend
- console background via pg_ctl
- service

The only way to deliver a console event to a service process is by
calling GenerateConsoleCtrlEvent() with that process( group)'s PID. If
anyone does that, they will know what they are doing, so we can disregard
that. The other two are tricky. In single-backend mode, we probably expect
both to work as usual (ending the process), while in the pg_ctl case,
they should both be ignored.

Ignoring Ctrl-C in the postmaster and all children is simple, this is what
my patch does. Ctrl-Break is more difficult to do. It is not limited to 
the "foreground" process group, but is delivered to all processes attached
to the console that originates it. To ignore it, every process (postmaster,
backends, workers, etc.) will have to handle it in their own console
event handling function.

backend/port/win32/signal.c explicitly turns several of the console events,
including Ctrl-C and Ctrl-Break, into SIGINT. The simplest fix would be to
ignore Ctrl-Break there, effectively disabling it entirely under all
circumstances. I tried that, and it appears to work, but I don't know 
enough about the signal emulation and the interactions between the various
processes to be sure this is the right place to do it. Single-backend mode
has no need for signal handling and does not use the emulation layer, so
it is unaffected.

Below is a new (right now very much proof-of-concept) patch to replace
my earlier one. It has the same effect on Ctrl-C the change to pg_ctl had,
and additionally ignores Ctrl-Break as well.

Please be gentle.



diff --git a/src/backend/port/win32/signal.c b/src/backend/port/win32/signal.c
index 322b857..7ce5051 100644
--- a/src/backend/port/win32/signal.c
+++ b/src/backend/port/win32/signal.c
@@ -347,8 +347,12 @@ static BOOL WINAPI
 pg_console_handler(DWORD dwCtrlType)
 {
if (dwCtrlType == CTRL_C_EVENT ||
-   dwCtrlType == CTRL_BREAK_EVENT ||
-   dwCtrlType == CTRL_CLOSE_EVENT ||
+   dwCtrlType == CTRL_BREAK_EVENT)
+   {
+   /* Ignore */
+   return TRUE;
+   }
+   else if (dwCtrlType == CTRL_CLOSE_EVENT ||
dwCtrlType == CTRL_SHUTDOWN_EVENT)
{
pg_queue_signal(SIGINT);

-- 
Christian


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

[HACKERS] PostgreSQL in Windows console and Ctrl-C

[Christian Ullrich]

Hello all,

when pg_ctl start is used to run PostgreSQL in a console window on 
Windows, it runs in the background (it is terminated by closing the 
window, but that is probably inevitable). There is one problem, however: 
The first Ctrl-C in that window, no matter in which situation, will 
cause the background postmaster to exit. If you, say, ping something, 
and press Ctrl-C to stop ping, you probably don't want the database to 
go away, too.


The reason is that Windows delivers the Ctrl-C event to all processes 
using that console, not just to the foreground one.


Here's a patch to fix that. "pg_ctl stop" still works, and it has no 
effect when running as a service, so it should be safe. It starts the 
postmaster in a new process group (similar to calling setpgrp() after 
fork()) that does not receive Ctrl-C events from the console window.


--
Christian
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
new file mode 100644
index 50d4586..89a9544
*** a/src/bin/pg_ctl/pg_ctl.c
--- b/src/bin/pg_ctl/pg_ctl.c
*** CreateRestrictedProcess(char *cmd, PROCE
*** 1561,1566 
--- 1561,1567 
HANDLE  restrictedToken;
SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};
SID_AND_ATTRIBUTES dropSids[2];
+   DWORD   flags;
  
/* Functions loaded dynamically */
__CreateRestrictedToken _CreateRestrictedToken = NULL;
*** CreateRestrictedProcess(char *cmd, PROCE
*** 1636,1642 
AddUserToTokenDacl(restrictedToken);
  #endif
  
!   r = CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL, TRUE, 
CREATE_SUSPENDED, NULL, NULL, &si, processInfo);
  
Kernel32Handle = LoadLibrary("KERNEL32.DLL");
if (Kernel32Handle != NULL)
--- 1637,1650 
AddUserToTokenDacl(restrictedToken);
  #endif
  
!   flags = CREATE_SUSPENDED;
! 
!   /* Protect console process from Ctrl-C */
!   if (!as_service) {
!   flags |= CREATE_NEW_PROCESS_GROUP;
!   }
! 
!   r = CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL, TRUE, 
flags, NULL, NULL, &si, processInfo);
  
Kernel32Handle = LoadLibrary("KERNEL32.DLL");
if (Kernel32Handle != NULL)

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

[HACKERS] Re: PostgreSQL Service on Windows does not start. ~ "is not a valid Win32 application"

[Christian Ullrich]

* Tom Lane wrote:


I looked at this patch a bit.  I agree that we need to fix
pgwin32_CommandLine to double-quote the executable name, but it needs a
great deal more work than that :-(.  Whoever wrote this code was


One additional issue is that the path to the service executable should 
use backslashes exclusively. Currently, the last directory separator in 
the service command line (the one before "pg_ctl.exe") is a forward 
slash. I recently had trouble with Symantec Backup Exec (not sure which 
versions are affected); it fails to do system state backups when a 
service registered using pg_ctl is present on the system.


See  for the same issue 
involving a different service.


The EDB installer does not cause that problem, although I don't know if 
that is because it does not use pg_ctl to register the service or 
because it fixes the path afterwards.


--
Christian




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