Thread: BUG #13594: pg_ctl.exe redirects stderr to Windows Events Log if stderr is redirected to pipe

The following bug has been logged on the website:

Bug reference:      13594
Logged by:          Egon Kocjan
Email address:      ekocjan@gmail.com
PostgreSQL version: 9.5alpha2
Operating system:   Windows 8.1
Description:

Hello

Context:

I run PostgreSQL as a part of a larger software package, pg_ctl is used to
control PostgreSQL. I capture all subprocess stderr logs to make a complete
report.

Problem:

On Windows (but not Unix!), PostgreSQL (any version) pg_ctl stops writing
into stderr if stderr is not a character device. As a result, it's not
possible to capture stderr of pg_ctl when using CreateProcess and
STARTUPINFO.hStdError. It might be possible to create a workaround with a
console buffer, but I think it's quite ugly:
http://www.codeproject.com/Articles/16163/Real-Time-Console-Output-Redirection

Solution:

Fix pg_ctl to write into stderr, if stderr is available and not just a
character device. Possibly, add a flag to force stderr. I don't have the big
picture, but this is the quick solution that I use right now:

diff -ur postgresql-9.3.6.orig\src\bin\pg_ctl\pg_ctl.c
postgresql-9.3.6\src\bin\pg_ctl\pg_ctl.c
--- postgresql-9.3.6.orig\src\bin\pg_ctl\pg_ctl.c    Mon Feb 02 22:43:50 2015
+++ postgresql-9.3.6\src\bin\pg_ctl\pg_ctl.c    Fri Aug 28 08:21:01 2015
@@ -215,7 +215,7 @@
      * On Win32, we print to stderr if running on a console, or write to
      * eventlog if running as a service
      */
-    if (!isatty(fileno(stderr)))    /* Running as a service */
+    if (getenv("PG_CTL_STDERR") == NULL && !isatty(fileno(stderr)))    /* Running
as a service */
     {
         char        errbuf[2048];        /* Arbitrary size? */


Then in my main process: _putenv("PG_CTL_STDERR=1")

I also noticed, that write_stderr() in src/backend/utils/error/elog.c uses a
different check (not isatty):

if (pgwin32_is_service())    /* Running as a service */

Thank you

On Fri, Aug 28, 2015 at 7:46 PM, <ekocjan@gmail.com> wrote:
>
> I run PostgreSQL as a part of a larger software package, pg_ctl is used to
> control PostgreSQL. I capture all subprocess stderr logs to make a complete
> report.
>
> Problem:
>
> On Windows (but not Unix!), PostgreSQL (any version) pg_ctl stops writing
> into stderr if stderr is not a character device. As a result, it's not
> possible to capture stderr of pg_ctl when using CreateProcess and
> STARTUPINFO.hStdError. It might be possible to create a workaround with a
> console buffer, but I think it's quite ugly:
> http://www.codeproject.com/Articles/16163/Real-Time-Console-Output-Redirection


Yep, that's ugly.

>
> Solution:
>
> Fix pg_ctl to write into stderr, if stderr is available and not just a
> character device. Possibly, add a flag to force stderr. I don't have the big
> picture, but this is the quick solution that I use right now:
>
> diff -ur postgresql-9.3.6.orig\src\bin\pg_ctl\pg_ctl.c
> postgresql-9.3.6\src\bin\pg_ctl\pg_ctl.c
> --- postgresql-9.3.6.orig\src\bin\pg_ctl\pg_ctl.c       Mon Feb 02 22:43:50 2015
> +++ postgresql-9.3.6\src\bin\pg_ctl\pg_ctl.c    Fri Aug 28 08:21:01 2015
> @@ -215,7 +215,7 @@
>          * On Win32, we print to stderr if running on a console, or write to
>          * eventlog if running as a service
>          */
> -       if (!isatty(fileno(stderr)))    /* Running as a service */
> +       if (getenv("PG_CTL_STDERR") == NULL && !isatty(fileno(stderr))) /* Running
> as a service */
>         {
>                 char            errbuf[2048];           /* Arbitrary size? */
>
>
> Then in my main process: _putenv("PG_CTL_STDERR=1")


Honestly I don't like that much.

>
> I also noticed, that write_stderr() in src/backend/utils/error/elog.c uses a
> different check (not isatty):
>
> if (pgwin32_is_service())       /* Running as a service */


The last time I poked at that, switching to use pgwin32_is_service was
not that straight-forward as it is a backend-only routine, relying on
write_stderr, write_eventlog and write_console, and pg_ctl has its own
concept of those routines:
http://www.postgresql.org/message-id/CAB7nPqSxWcUFpzL4LaOZAt4k-FPM7fy6Et7p50w8S5NbRfCwFw@mail.gmail.com
But actually looking at it with fresh eyes it is not that complicated
if we switch backend/port/win32/security.c to port/win32security.c to
make pgwin32_is_service available as well for frontends. And it would
obviously still be an improvement to switch isatty() to
pgwin32_is_service() in pg_ctl.c, the trick being to remove the
dependency to write_stderr when pgwin32_is_admin is called from
frontend so as it does not interact badly with the existing logging
infrastructure of elog.c and the definition of write_stderr in
pg_ctl.c. Note that in any case we cannot rely on ereport when calling
pgwin32_is_service on backend because it is called very early at
process startup, so there is no need to dance with ifdef FRONTEND
there.

This change is too invasive so I think that making it HEAD-only is
better, and that's a change of behavior. I should perhaps have split
the patch into two: one for the move to src/port, the other for pg_ctl
but it's not really a big deal. I'll add it to the next CF I think
that's worth considering it.

Egon, would the patch attached help in your case?
Regards,
--
Michael

On 31.8.2015 8:51, Michael Paquier wrote:
> On Fri, Aug 28, 2015 at 7:46 PM, <ekocjan@gmail.com> wrote:
>> I also noticed, that write_stderr() in src/backend/utils/error/elog.c uses a
>> different check (not isatty):
>>
>> if (pgwin32_is_service())       /* Running as a service */
>
> The last time I poked at that, switching to use pgwin32_is_service was
> not that straight-forward as it is a backend-only routine, relying on
> write_stderr, write_eventlog and write_console, and pg_ctl has its own
> concept of those routines:
> http://www.postgresql.org/message-id/CAB7nPqSxWcUFpzL4LaOZAt4k-FPM7fy6Et7p50w8S5NbRfCwFw@mail.gmail.com
> But actually looking at it with fresh eyes it is not that complicated
> if we switch backend/port/win32/security.c to port/win32security.c to
> make pgwin32_is_service available as well for frontends. And it would
> obviously still be an improvement to switch isatty() to
> pgwin32_is_service() in pg_ctl.c, the trick being to remove the
> dependency to write_stderr when pgwin32_is_admin is called from
> frontend so as it does not interact badly with the existing logging
> infrastructure of elog.c and the definition of write_stderr in
> pg_ctl.c. Note that in any case we cannot rely on ereport when calling
> pgwin32_is_service on backend because it is called very early at
> process startup, so there is no need to dance with ifdef FRONTEND
> there.
>
> This change is too invasive so I think that making it HEAD-only is
> better, and that's a change of behavior. I should perhaps have split
> the patch into two: one for the move to src/port, the other for pg_ctl
> but it's not really a big deal. I'll add it to the next CF I think
> that's worth considering it.
>
> Egon, would the patch attached help in your case?
>

I tested with these two changes:

diff -ur postgresql-9.6devel.orig\src\bin\pg_ctl\pg_ctl.c
postgresql-9.6devel\src\bin\pg_ctl\pg_ctl.c
--- postgresql-9.6devel.orig\src\bin\pg_ctl\pg_ctl.c    Mon Aug 31
13:16:51 2015
+++ postgresql-9.6devel\src\bin\pg_ctl\pg_ctl.c    Mon Aug 31 15:12:54 2015
@@ -215,7 +215,7 @@
       * On Win32, we print to stderr if running on a console, or write to
       * eventlog if running as a service
       */
-    if (!pgwin32_is_service())    /* Running as a service */
+    if (pgwin32_is_service())    /* Running as a service */
      {
          char        errbuf[2048];        /* Arbitrary size? */

@@ -2131,6 +2131,9 @@
      progname = get_progname(argv[0]);
      set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_ctl"));
      start_time = time(NULL);
+
+    write_stderr("%s: test test\n",
+                     progname);

      /*
       * save argv[0] so do_start() can look for the postmaster if
necessary. we


1) It now works ok for development under regular account.

2) However there is no output on stderr when pg_ctl is running as a
subprocess of a service. So I would still have to patch pg_ctl if I want
stderr. I haven't looked into elog.c yet, maybe stderr from there would
be good to have as well? So far, pg_ctl output and pg_logs\* was enough
for me to debug the installations. Technically, I could also monitor
Windows event log and dump events on the fly into my report file. But it
seems simpler to just patch PostgreSQL locally to force stderr and reuse
all the logic from the Unix port...

Thank you for looking into this,
Egon

On Mon, Aug 31, 2015 at 11:56 PM, Egon Kocjan wrote:
> On 31.8.2015 8:51, Michael Paquier wrote:
>> Egon, would the patch attached help in your case?
>>
>
> diff -ur postgresql-9.6devel.orig\src\bin\pg_ctl\pg_ctl.c
> postgresql-9.6devel\src\bin\pg_ctl\pg_ctl.c
> --- postgresql-9.6devel.orig\src\bin\pg_ctl\pg_ctl.c    Mon Aug 31 13:16:51
> 2015
> +++ postgresql-9.6devel\src\bin\pg_ctl\pg_ctl.c    Mon Aug 31 15:12:54 2015
> @@ -215,7 +215,7 @@
>       * On Win32, we print to stderr if running on a console, or write to
>       * eventlog if running as a service
>       */
> -    if (!pgwin32_is_service())    /* Running as a service */
> +    if (pgwin32_is_service())    /* Running as a service */
>      {
>          char        errbuf[2048];        /* Arbitrary size? */

This one is obviously a stupid mistake of my patch :)

> @@ -2131,6 +2131,9 @@
>      progname = get_progname(argv[0]);
>      set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_ctl"));
>      start_time = time(NULL);
> +
> +    write_stderr("%s: test test\n",
> +                     progname);
>
>      /*
>       * save argv[0] so do_start() can look for the postmaster if necessary.
> we
>
>
> 1) It now works ok for development under regular account.

Thanks for the additional tests, that's still a win so this patch is a
good thing. Relying on isatty is definitely something that we should
treat as a bug here and backpatch. On master and REL9_5_STABLE, what I
sent previously, with the call of pgwin32_is_service fixed in pg_ctl.c
is fine. For back branches, we could simply copy pgwin32_is_service in
pg_ctl.c and rely on that. It seems like the safer approach. Thoughts
from other hackers on the matter?

> 2) However there is no output on stderr when pg_ctl is running as a
> subprocess of a service. So I would still have to patch pg_ctl if I want
> stderr. I haven't looked into elog.c yet, maybe stderr from there would be
> good to have as well? So far, pg_ctl output and pg_logs\* was enough for me
> to debug the installations. Technically, I could also monitor Windows event
> log and dump events on the fly into my report file. But it seems simpler to
> just patch PostgreSQL locally to force stderr and reuse all the logic from
> the Unix port...

I wished we had such a switch a couple of times and I have as well
some code paths that would benefit a redirection of those logs to
stderr instead of the event log. Still this sounds like a new feature
to me. Hence, what about for example adding a Windows-only option that
can be used to enforce where logs are redirected, let's say
--log-output with the following values:
- default, which is what we have now
- stderr, to enforce the output to stderr
- eventlog, to enforce redirection to the event logs.
Would you like to work on this patch?
Regards,
--
Michael

On 2015-08-31 15:51:57 +0900, Michael Paquier wrote:
>      if (!OpenProcessToken(GetCurrentProcess(), TOKEN_READ, &AccessToken))
>      {
> +#ifndef FRONTEND
>          write_stderr("could not open process token: error code %lu\n",
>                       GetLastError());
> +#else
> +        fprintf(stderr, "could not open process token: error code %lu\n",
> +                GetLastError());
> +#endif
>          exit(1);
>      }

I find these kind of ifdefs rather ugly - why not just introduce a
wrapper?

Greetings,

Andres Freund

On Thu, Sep 3, 2015 at 8:36 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-08-31 15:51:57 +0900, Michael Paquier wrote:
>>       if (!OpenProcessToken(GetCurrentProcess(), TOKEN_READ, &AccessToken))
>>       {
>> +#ifndef FRONTEND
>>               write_stderr("could not open process token: error code %lu\n",
>>                                        GetLastError());
>> +#else
>> +             fprintf(stderr, "could not open process token: error code %lu\n",
>> +                             GetLastError());
>> +#endif
>>               exit(1);
>>       }
>
> I find these kind of ifdefs rather ugly - why not just introduce a
> wrapper?

I thought it was just not worth it for this file. I don't mind
updating if you think that's cleaner this way.
--
Michael

On 1.9.2015 3:43, Michael Paquier wrote:
> On Mon, Aug 31, 2015 at 11:56 PM, Egon Kocjan wrote:
>> 2) However there is no output on stderr when pg_ctl is running as a
>> subprocess of a service. So I would still have to patch pg_ctl if I want
>> stderr. I haven't looked into elog.c yet, maybe stderr from there would be
>> good to have as well? So far, pg_ctl output and pg_logs\* was enough for me
>> to debug the installations. Technically, I could also monitor Windows event
>> log and dump events on the fly into my report file. But it seems simpler to
>> just patch PostgreSQL locally to force stderr and reuse all the logic from
>> the Unix port...
> I wished we had such a switch a couple of times and I have as well
> some code paths that would benefit a redirection of those logs to
> stderr instead of the event log. Still this sounds like a new feature
> to me. Hence, what about for example adding a Windows-only option that
> can be used to enforce where logs are redirected, let's say
> --log-output with the following values:
> - default, which is what we have now
> - stderr, to enforce the output to stderr
> - eventlog, to enforce redirection to the event logs.
> Would you like to work on this patch?
>

Yes, I would start with pg_ctl itself. Then we can look into what else
needs to be done for the backend, I'm not yet 100% sure how it all fits
together.

Regards
Egon

On Fri, Sep 4, 2015 at 10:28 AM, Michael Paquier wrote:
> New patch attached, updated in consequence.

This was registered in 2015-11 CF, and that's a bug fix for a
long-existing bug. I am moving it to next CF of January.
--
Michael

Michael Paquier wrote:
> On Mon, Aug 31, 2015 at 11:56 PM, Egon Kocjan wrote:

> Thanks for the additional tests, that's still a win so this patch is a
> good thing. Relying on isatty is definitely something that we should
> treat as a bug here and backpatch. On master and REL9_5_STABLE, what I
> sent previously, with the call of pgwin32_is_service fixed in pg_ctl.c
> is fine. For back branches, we could simply copy pgwin32_is_service in
> pg_ctl.c and rely on that. It seems like the safer approach. Thoughts
> from other hackers on the matter?

I looked at applying this patch, but as it turns out we don't have the
backpatchable version.  I don't think it's a good idea to backpatch the
move of the file from backend to src/port -- there's precedent for
copying the entire function (and assorted infrastructure) to pg_ctl
instead.  Would you submit a patch to do that?

Here's a rebased version of what you submitted last.  I tweaked a few
lines here and there, nothing major.  I thought your patch is ready to
apply and we only need the backpatchable versions.

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

On Thu, Jan 7, 2016 at 6:42 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>> On Mon, Aug 31, 2015 at 11:56 PM, Egon Kocjan wrote:
>
>> Thanks for the additional tests, that's still a win so this patch is a
>> good thing. Relying on isatty is definitely something that we should
>> treat as a bug here and backpatch. On master and REL9_5_STABLE, what I
>> sent previously, with the call of pgwin32_is_service fixed in pg_ctl.c
>> is fine. For back branches, we could simply copy pgwin32_is_service in
>> pg_ctl.c and rely on that. It seems like the safer approach. Thoughts
>> from other hackers on the matter?
>
> I looked at applying this patch, but as it turns out we don't have the
> backpatchable version.  I don't think it's a good idea to backpatch the
> move of the file from backend to src/port -- there's precedent for
> copying the entire function (and assorted infrastructure) to pg_ctl
> instead.  Would you submit a patch to do that?

Yes (like restricted tokens and pg_upgrade stuff we did), but I was
just waiting that we reach an agreement for the version of master
before moving on with patches for back branches.

> Here's a rebased version of what you submitted last.  I tweaked a few
> lines here and there, nothing major.  I thought your patch is ready to
> apply and we only need the backpatchable versions.

This rebased patch looks fine for me. I am attaching as well versions
for back-branches for 9.1~9.5. In this case it is just necessary to
duplicate pgwin32_get_dynamic_tokeninfo and pgwin32_is_service to
pg_ctl.c to make things work properly.
Regards,
--
Michael

Michael Paquier wrote:
> On Thu, Jan 7, 2016 at 6:42 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

> > I looked at applying this patch, but as it turns out we don't have the
> > backpatchable version.  I don't think it's a good idea to backpatch the
> > move of the file from backend to src/port -- there's precedent for
> > copying the entire function (and assorted infrastructure) to pg_ctl
> > instead.  Would you submit a patch to do that?
>
> Yes (like restricted tokens and pg_upgrade stuff we did), but I was
> just waiting that we reach an agreement for the version of master
> before moving on with patches for back branches.

Right.

> > Here's a rebased version of what you submitted last.  I tweaked a few
> > lines here and there, nothing major.  I thought your patch is ready to
> > apply and we only need the backpatchable versions.
>
> This rebased patch looks fine for me. I am attaching as well versions
> for back-branches for 9.1~9.5. In this case it is just necessary to
> duplicate pgwin32_get_dynamic_tokeninfo and pgwin32_is_service to
> pg_ctl.c to make things work properly.

Pushed.  Obviously I didn't compile any of this, so let's see how the
buildfarm likes it.

Thanks!

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

On Fri, Jan 8, 2016 at 12:11 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Pushed.  Obviously I didn't compile any of this, so let's see how the
> buildfarm likes it.

It didn't like it much. Thanks for the push and for the post-turbulence fixes.
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Fri, Jan 8, 2016 at 12:11 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> Pushed.  Obviously I didn't compile any of this, so let's see how the
>> buildfarm likes it.

> It didn't like it much. Thanks for the push and for the post-turbulence fixes.

brolga is still unhappy:

gcc-4 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -g -O2 -I../../../src/interfaces/libpq
-I../../../src/include-I/usr/include/libxml2   -c -o pg_ctl.o pg_ctl.c 
pg_ctl.c: In function ‘write_stderr’:
pg_ctl.c:219: warning: implicit declaration of function ‘pgwin32_is_service’
gcc-4 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -g -O2 pg_ctl.o  -L../../../src/common
-lpgcommon-L../../../src/port -lpgport -L../../../src/interfaces/libpq -lpq  -L../../../src/port -L../../../src/common
-Wl,--allow-multiple-definition-Wl,--enable-auto-import -L/usr/lib  -L/usr/local/lib -Wl,--as-needed   -lpgcommon
-lpgport-lxslt -lxml2 -lz -lreadline -lcrypt -o pg_ctl.exe 
pg_ctl.o: In function `write_stderr':
/home/andrew/bf/root/HEAD/pgsql.build/src/bin/pg_ctl/pg_ctl.c:219: undefined reference to `_pgwin32_is_service'
collect2: ld returned 1 exit status
make[3]: *** [pg_ctl] Error 1

Too soon to tell about the other Windows critters.

            regards, tom lane

On Fri, Jan 8, 2016 at 11:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> On Fri, Jan 8, 2016 at 12:11 AM, Alvaro Herrera
>> <alvherre@2ndquadrant.com> wrote:
>>> Pushed.  Obviously I didn't compile any of this, so let's see how the
>>> buildfarm likes it.
>
>> It didn't like it much. Thanks for the push and for the post-turbulence =
fixes.
>
> brolga is still unhappy:
>
> gcc-4 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-stat=
ement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-stri=
ct-aliasing -fwrapv -g -O2 -I../../../src/interfaces/libpq -I../../../src/i=
nclude -I/usr/include/libxml2   -c -o pg_ctl.o pg_ctl.c
> pg_ctl.c: In function =E2=80=98write_stderr=E2=80=99:
> pg_ctl.c:219: warning: implicit declaration of function =E2=80=98pgwin32_=
is_service=E2=80=99
> gcc-4 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-stat=
ement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-stri=
ct-aliasing -fwrapv -g -O2 pg_ctl.o  -L../../../src/common -lpgcommon -L../=
../../src/port -lpgport -L../../../src/interfaces/libpq -lpq  -L../../../sr=
c/port -L../../../src/common -Wl,--allow-multiple-definition -Wl,--enable-a=
uto-import -L/usr/lib  -L/usr/local/lib -Wl,--as-needed   -lpgcommon -lpgpo=
rt -lxslt -lxml2 -lz -lreadline -lcrypt -o pg_ctl.exe
> pg_ctl.o: In function `write_stderr':
> /home/andrew/bf/root/HEAD/pgsql.build/src/bin/pg_ctl/pg_ctl.c:219: undefi=
ned reference to `_pgwin32_is_service'
> collect2: ld returned 1 exit status
> make[3]: *** [pg_ctl] Error 1
>
> Too soon to tell about the other Windows critters.

Hm. It is not that straight-forward... and I have to admit that I will
have little time to look into that until Monday/Tuesday with a real
working environment. Now, looking at those logs, none of the win32*.c
files actually are included in the OBJS list in src/port/Makefile, so
it would seem that the correct way of going is to have the definition
of those new routines in include/port.h for all cases, and include
win32security.c in the list of objects compiled. But those are only
assumptions as I would need a cygwin environment to check that
properly at this stage.

Now, backbranches are proving that compiling pg_ctl.c with those
routines copied in it actually works fine, so based on my limited time
for the next couple of days, it may be better to just revert the patch
entirely on HEAD and apply what has been pushed into back branches to
stabilize the buildfarm, this will address the bug entirely. And it is
no good to keep the buildfarm red for long,
--=20
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Fri, Jan 8, 2016 at 11:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> brolga is still unhappy:

> ... Now, backbranches are proving that compiling pg_ctl.c with those
> routines copied in it actually works fine, so based on my limited time
> for the next couple of days, it may be better to just revert the patch
> entirely on HEAD and apply what has been pushed into back branches to
> stabilize the buildfarm, this will address the bug entirely. And it is
> no good to keep the buildfarm red for long,

Hm, narwhal just reported in green, which I was not expecting, and so did
frogmouth.  So brolga seems like an outlier.  I think reverting might be
an overreaction.  At this point the question is more like "how is brolga
different from the other non-MSVC Windows critters?".  Perhaps Andrew
can offer some insight.

            regards, tom lane

On Fri, Jan 8, 2016 at 3:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> On Fri, Jan 8, 2016 at 11:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> brolga is still unhappy:
>
>> ... Now, backbranches are proving that compiling pg_ctl.c with those
>> routines copied in it actually works fine, so based on my limited time
>> for the next couple of days, it may be better to just revert the patch
>> entirely on HEAD and apply what has been pushed into back branches to
>> stabilize the buildfarm, this will address the bug entirely. And it is
>> no good to keep the buildfarm red for long,
>
> Hm, narwhal just reported in green, which I was not expecting, and so did
> frogmouth.  So brolga seems like an outlier.  I think reverting might be
> an overreaction.  At this point the question is more like "how is brolga
> different from the other non-MSVC Windows critters?".  Perhaps Andrew
> can offer some insight.

That's a good sign at least :)

Looking at the logs, the main difference is that win32security.c is
not compiled in -lpgport for cygwin, and cygwin goes through
src/include/port/win32.h to refer to some routines, so I would think
that the correct fix is to add win32security.c to the list of OBJS for
cygwin. So the attached is correct perhaps? That's a blind fix I don't
have cygwin set up on my machines now.
--
Michael

On 01/08/2016 02:11 AM, Michael Paquier wrote:
> On Fri, Jan 8, 2016 at 3:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Michael Paquier <michael.paquier@gmail.com> writes:
>>> On Fri, Jan 8, 2016 at 11:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> brolga is still unhappy:
>>> ... Now, backbranches are proving that compiling pg_ctl.c with those
>>> routines copied in it actually works fine, so based on my limited time
>>> for the next couple of days, it may be better to just revert the patch
>>> entirely on HEAD and apply what has been pushed into back branches to
>>> stabilize the buildfarm, this will address the bug entirely. And it is
>>> no good to keep the buildfarm red for long,
>> Hm, narwhal just reported in green, which I was not expecting, and so did
>> frogmouth.  So brolga seems like an outlier.  I think reverting might be
>> an overreaction.  At this point the question is more like "how is brolga
>> different from the other non-MSVC Windows critters?".  Perhaps Andrew
>> can offer some insight.
> That's a good sign at least :)
>
> Looking at the logs, the main difference is that win32security.c is
> not compiled in -lpgport for cygwin, and cygwin goes through
> src/include/port/win32.h to refer to some routines, so I would think
> that the correct fix is to add win32security.c to the list of OBJS for
> cygwin. So the attached is correct perhaps? That's a blind fix I don't
> have cygwin set up on my machines now.


I'll test it when I get a moment. Thanks for the patch.

I see this service code goes back probably to the first commit of
pg_ctl.c, which means it's going to have been either my fault or Bruce's
:-) TBH I'm not sure how to run postgres as a service on Cygwin. In
general I would expect it to be via cygrunsrv, like other services I do
actually run (cron and cygserver), rather than via pg_ctl. Anyway, for
now lets see if we can get it building again.

cheers

andrew

Andrew Dunstan wrote:

> On 01/08/2016 02:11 AM, Michael Paquier wrote:

> >Looking at the logs, the main difference is that win32security.c is
> >not compiled in -lpgport for cygwin, and cygwin goes through
> >src/include/port/win32.h to refer to some routines, so I would think
> >that the correct fix is to add win32security.c to the list of OBJS for
> >cygwin. So the attached is correct perhaps? That's a blind fix I don't
> >have cygwin set up on my machines now.

> I'll test it when I get a moment. Thanks for the patch.
>
> I see this service code goes back probably to the first commit of pg_ctl.c,
> which means it's going to have been either my fault or Bruce's :-)

Per IM conversation with Andrew, I pushed it blind and he'll set up an
immediate BF run on brolga so we know quickly whether it worked.

> TBH I'm not sure how to run postgres as a service on Cygwin. In
> general I would expect it to be via cygrunsrv, like other services I
> do actually run (cron and cygserver), rather than via pg_ctl. Anyway,
> for now lets see if we can get it building again.

Yeah, I guess you'd normally you'd do that, but the point is that the OP
was using pg_ctl to control one that's *not* a service and the logs were
still sent to Event Log because of the bug fixed here.

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

On Fri, Jan 8, 2016 at 11:55 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Andrew Dunstan wrote:
>
>> On 01/08/2016 02:11 AM, Michael Paquier wrote:
>
>> >Looking at the logs, the main difference is that win32security.c is
>> >not compiled in -lpgport for cygwin, and cygwin goes through
>> >src/include/port/win32.h to refer to some routines, so I would think
>> >that the correct fix is to add win32security.c to the list of OBJS for
>> >cygwin. So the attached is correct perhaps? That's a blind fix I don't
>> >have cygwin set up on my machines now.
>
>> I'll test it when I get a moment. Thanks for the patch.
>>
>> I see this service code goes back probably to the first commit of pg_ctl.c,
>> which means it's going to have been either my fault or Bruce's :-)
>
> Per IM conversation with Andrew, I pushed it blind and he'll set up an
> immediate BF run on brolga so we know quickly whether it worked.

And it failed. That's frustrating. I'll try to set up a cygwin
environment and address that.
--
Michael