CI/windows docker vs "am a service" autodetection on windows
Hi,
When one tests postgres in a some of the popular CI systems (all that
use docker for windows), some of the tests fail in weird ways. Like
/messages/by-id/20210303052011.ybplxw6q4tafwogk@alap3.anarazel.de
t/003_recovery_targets.pl ............ 7/9
# Failed test 'multiple conflicting settings'
# at t/003_recovery_targets.pl line 151.# Failed test 'recovery end before target reached is a fatal error'
# at t/003_recovery_targets.pl line 177.
t/003_recovery_targets.pl ............ 9/9 # Looks like you failed 2 tests of 9.
t/003_recovery_targets.pl ............ Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/9 subtestsI think it's pretty dangerous if we have a substantial number of tests
that aren't run on windows - I think a lot of us just assume that the
BF would catch windows specific problems...
A lot of debugging later I figured out that the problem is that postgres
decides not to write anything to stderr, but send everything to the
windows event log instead. This includes error messages when starting
postgres with wrong parameters or such...
The reason for that elog.c and pg_ctl.c use
src/port/win32security.c:pgwin32_is_service() to detect whether they're
running as a service:
static void
send_message_to_server_log(ErrorData *edata)
...
/*
* In a win32 service environment, there is no usable stderr. Capture
* anything going there and write it to the eventlog instead.
*
* If stderr redirection is active, it was OK to write to stderr above
* because that's really a pipe to the syslogger process.
*/
else if (pgwin32_is_service())
write_eventlog(edata->elevel, buf.data, buf.len);
..
void
write_stderr(const char *fmt,...)
...
/*
* 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 */
{
write_eventlog(ERROR, errbuf, strlen(errbuf));
but pgwin32_is_service() doesn't actually reliably detect if running as
a service - it's a heuristic that also triggers when running postgres
within a windows docker container (presumably because that itself is run
from within a service?).
ISTM that that's a problem, and is likely to become more of a problem
going forward (assuming that docker on windows will become more
popular).
My opinion is that the whole attempt at guessing whether we are running
as a service is a bad idea. This isn't the first time to be a problem,
see e.g. [1]commit ff30aec759bdc4de78912d91f650ec8fd95ff6bc Author: Heikki Linnakangas <heikki.linnakangas@iki.fi> Date: 2017-03-17 11:14:01 +0200.
Why don't we instead have pgwin32_doRegister() include a parameter that
indicates we're running as a service and remove all the heuristics?
I tried to look around to see if there's a simple way to drop the
problematic memberships that trigger pgwin32_is_service() - but there
seem to be no commandline tools doing so (but there are C APIs).
Does anybody have an alternative way of fixing this?
Greetings,
Andres Freund
[1]: commit ff30aec759bdc4de78912d91f650ec8fd95ff6bc Author: Heikki Linnakangas <heikki.linnakangas@iki.fi> Date: 2017-03-17 11:14:01 +0200
commit ff30aec759bdc4de78912d91f650ec8fd95ff6bc
Author: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: 2017-03-17 11:14:01 +0200
Fix and simplify check for whether we're running as Windows service.
On 3/4/21 2:08 PM, Andres Freund wrote:
[...] pgwin32_is_service() doesn't actually reliably detect if running as
a service - it's a heuristic that also triggers when running postgres
within a windows docker container (presumably because that itself is run
from within a service?).ISTM that that's a problem, and is likely to become more of a problem
going forward (assuming that docker on windows will become more
popular).My opinion is that the whole attempt at guessing whether we are running
as a service is a bad idea. This isn't the first time to be a problem,
see e.g. [1].Why don't we instead have pgwin32_doRegister() include a parameter that
indicates we're running as a service and remove all the heuristics?
I assume you mean a postmaster parameter, that would be set via pg_ctl?
Seems reasonable.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Thu, Mar 4, 2021 at 8:33 PM Andrew Dunstan <andrew@dunslane.net> wrote:
On 3/4/21 2:08 PM, Andres Freund wrote:
[...] pgwin32_is_service() doesn't actually reliably detect if running as
a service - it's a heuristic that also triggers when running postgres
within a windows docker container (presumably because that itself is run
from within a service?).ISTM that that's a problem, and is likely to become more of a problem
going forward (assuming that docker on windows will become more
popular).My opinion is that the whole attempt at guessing whether we are running
as a service is a bad idea. This isn't the first time to be a problem,
see e.g. [1].Why don't we instead have pgwin32_doRegister() include a parameter that
indicates we're running as a service and remove all the heuristics?I assume you mean a postmaster parameter, that would be set via pg_ctl?
Seems reasonable.
The problem with doing it at register time is that everybody who
builds an installer for PostgreSQL will then have to do it in their
own registration (I'm pretty sure most of them don't use pg_ctl
register).
The same thing in pgwin32_doRunAsService() might help with that. But
then we'd have to figure out what to do if pg_ctl fails prior to
reaching that point... There aren't that many such paths, but there
are some.
Just throwing out ideas without spending time thinking about it, maybe
log to *both* in the case when we pick by it by autodetection?
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
Hi,
On 2021-03-04 21:08:30 +0100, Magnus Hagander wrote:
The problem with doing it at register time is that everybody who
builds an installer for PostgreSQL will then have to do it in their
own registration (I'm pretty sure most of them don't use pg_ctl
register).
Well, hm, maybe they should change that?
The same thing in pgwin32_doRunAsService() might help with that.
What do you mean by this?
But then we'd have to figure out what to do if pg_ctl fails prior to
reaching that point... There aren't that many such paths, but there
are some.Just throwing out ideas without spending time thinking about it, maybe
log to *both* in the case when we pick by it by autodetection?
I think that's a good answer for pg_ctl - not so sure about postgres
itself, at least once it's up and running. I don't know what lead to all
of this autodetection stuff, but is there the possibility of blocking on
whatever stderr is set too as a service?
Perhaps we could make the service detection more reliable by checking
whether stderr is actually something useful?
There does seem to be isatty(), so we could improve the case of
pg_ctl/postgres run interactively without breaking a sweat. And there is
fstat() too, so if stderr in a service is something distinguishable...
Greetings,
Andres Freund
On Thu, Mar 4, 2021 at 9:30 PM Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2021-03-04 21:08:30 +0100, Magnus Hagander wrote:
The problem with doing it at register time is that everybody who
builds an installer for PostgreSQL will then have to do it in their
own registration (I'm pretty sure most of them don't use pg_ctl
register).Well, hm, maybe they should change that?
The same thing in pgwin32_doRunAsService() might help with that.
What do you mean by this?
I mean controlling this flag by entry into pgwin32_doRunAsService().
So that when we start *postgres* we pass a parameter along saying that
it's a service and should use eventlog for the early exit.
pgwin32_doRunAsService() will (of course) only be called when started
with runservice. That would, I think, sort out the problem for the
postgres processes, and leave us with just pg_ctl to figure out.
But then we'd have to figure out what to do if pg_ctl fails prior to
reaching that point... There aren't that many such paths, but there
are some.Just throwing out ideas without spending time thinking about it, maybe
log to *both* in the case when we pick by it by autodetection?I think that's a good answer for pg_ctl - not so sure about postgres
itself, at least once it's up and running. I don't know what lead to all
of this autodetection stuff, but is there the possibility of blocking on
whatever stderr is set too as a service?Perhaps we could make the service detection more reliable by checking
whether stderr is actually something useful?
So IIRC, and mind that this is like 15 years ago, there is something
that looks like stderr, but the contents are thrown away. It probably
exists specifically so that programs won't crash when run as a
service...
There does seem to be isatty(), so we could improve the case of
pg_ctl/postgres run interactively without breaking a sweat. And there is
fstat() too, so if stderr in a service is something distinguishable...
We seem to have used that at some point, but commit
a967613911f7ef7b6387b9e8718f0ab8f0c4d9c8 got rid of it... But maybe
apply it in a combination.
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
Hi,
On 2021-03-04 21:36:23 +0100, Magnus Hagander wrote:
I think that's a good answer for pg_ctl - not so sure about postgres
itself, at least once it's up and running. I don't know what lead to all
of this autodetection stuff, but is there the possibility of blocking on
whatever stderr is set too as a service?Perhaps we could make the service detection more reliable by checking
whether stderr is actually something useful?So IIRC, and mind that this is like 15 years ago, there is something
that looks like stderr, but the contents are thrown away. It probably
exists specifically so that programs won't crash when run as a
service...
Yea, that'd make sense.
I wish we had tests for the service stuff, but that's from long before
there were tap tests...
There does seem to be isatty(), so we could improve the case of
pg_ctl/postgres run interactively without breaking a sweat. And there is
fstat() too, so if stderr in a service is something distinguishable...We seem to have used that at some point, but commit
a967613911f7ef7b6387b9e8718f0ab8f0c4d9c8 got rid of it...
Hm. The bug #13592 referenced in that commit appears to be about
something else. Looks to be #13594
/messages/by-id/20150828104658.2089.83265@wrigleys.postgresql.org
But maybe apply it in a combination.
Yea, that's what I was thinking.
Gah, I don't really want to know anything about windows, I just want to
hack on aio with proper working CI.
Greetings,
Andres Freund
On 2021-Mar-04, Andres Freund wrote:
There does seem to be isatty(), so we could improve the case of
pg_ctl/postgres run interactively without breaking a sweat. And there is
fstat() too, so if stderr in a service is something distinguishable...We seem to have used that at some point, but commit
a967613911f7ef7b6387b9e8718f0ab8f0c4d9c8 got rid of it...Hm. The bug #13592 referenced in that commit appears to be about
something else. Looks to be #13594
/messages/by-id/20150828104658.2089.83265@wrigleys.postgresql.org
Yeah, that's a typo in the commit message.
But maybe apply it in a combination.
Yea, that's what I was thinking.
That makes sense. At the time we were not thinking (*I* was not
thinking, for sure) that you could have a not-a-service process that
runs inside a service. The fixed bug was in the same direction that you
want to fix now, just differently: the bare "isatty" test was
considering too many things as under a service, and replaced it with the
pgwin32_is_service which considers a different set of too many things as
under a service. I agree with the idea that *both* tests have to pass
in order to consider it as under a service.
--
�lvaro Herrera 39�49'30"S 73�17'W
"No hay hombre que no aspire a la plenitud, es decir,
la suma de experiencias de que un hombre es capaz"
Hi,
On 2021-03-04 12:48:59 -0800, Andres Freund wrote:
On 2021-03-04 21:36:23 +0100, Magnus Hagander wrote:
I think that's a good answer for pg_ctl - not so sure about postgres
itself, at least once it's up and running. I don't know what lead to all
of this autodetection stuff, but is there the possibility of blocking on
whatever stderr is set too as a service?Perhaps we could make the service detection more reliable by checking
whether stderr is actually something useful?So IIRC, and mind that this is like 15 years ago, there is something
that looks like stderr, but the contents are thrown away. It probably
exists specifically so that programs won't crash when run as a
service...Yea, that'd make sense.
I wish we had tests for the service stuff, but that's from long before
there were tap tests...
After fighting with a windows VM for a bit (ugh), it turns out that yes,
there is stderr, but that fileno(stderr) returns -2, and
GetStdHandle(STD_ERROR_HANDLE) returns NULL (not INVALID_HANDLE_VALUE).
The complexity however is that while that's true for pg_ctl within
pgwin32_ServiceMain:
checking what stderr=00007FF8687DFCB0 is (handle: 0, fileno=-2)
but not for postmaster or backends
WARNING: 01000: checking what stderr=00007FF880F5FCB0 is (handle: 92, fileno=2)
which makes sense in a way, because we don't tell CreateProcessAsUser()
that it should pass stdin/out/err down (which then seems to magically
get access to the "topmost" console applications output - damn, this
stuff is weird).
You'd earlier mentioned that other distributions may not use pg_ctl
register - but I assume they use pg_ctl runservice? Or do they actually
re-implement all those magic incantations in pg_ctl.c?
Greetings,
Andres Freund
Hi,
On 2021-03-05 10:57:52 -0800, Andres Freund wrote:
On 2021-03-04 12:48:59 -0800, Andres Freund wrote:
On 2021-03-04 21:36:23 +0100, Magnus Hagander wrote:
I think that's a good answer for pg_ctl - not so sure about postgres
itself, at least once it's up and running. I don't know what lead to all
of this autodetection stuff, but is there the possibility of blocking on
whatever stderr is set too as a service?Perhaps we could make the service detection more reliable by checking
whether stderr is actually something useful?So IIRC, and mind that this is like 15 years ago, there is something
that looks like stderr, but the contents are thrown away. It probably
exists specifically so that programs won't crash when run as a
service...Yea, that'd make sense.
I wish we had tests for the service stuff, but that's from long before
there were tap tests...After fighting with a windows VM for a bit (ugh), it turns out that yes,
there is stderr, but that fileno(stderr) returns -2, and
GetStdHandle(STD_ERROR_HANDLE) returns NULL (not INVALID_HANDLE_VALUE).The complexity however is that while that's true for pg_ctl within
pgwin32_ServiceMain:
checking what stderr=00007FF8687DFCB0 is (handle: 0, fileno=-2)
but not for postmaster or backends
WARNING: 01000: checking what stderr=00007FF880F5FCB0 is (handle: 92, fileno=2)which makes sense in a way, because we don't tell CreateProcessAsUser()
that it should pass stdin/out/err down (which then seems to magically
get access to the "topmost" console applications output - damn, this
stuff is weird).
That part is not too hard to address - it seems we only need to do that
in pg_ctl pgwin32_doRunAsService(). It seems that the
stdin/stderr/stdout being set to invalid will then be passed down to
postmaster children.
https://docs.microsoft.com/en-us/windows/console/getstdhandle
"If an application does not have associated standard handles, such as a
service running on an interactive desktop, and has not redirected them,
the return value is NULL."
There does seem to be some difference between what services get as std*
- GetStdHandle() returns NULL, and what explicitly passing down invalid
handles to postmaster does - GetStdHandle() returns
INVALID_HANDLE_VALUE. But passing down NULL rather than
INVALID_HANDLE_VALUE to postmaster seems to lead to postmaster
re-opening console buffers.
Patch attached.
I'd like to commit something to address this issue to master soon - it
allows us to run a lot more tests in cirrus-ci... But probably not
backpatch it [yet] - there've not really been field complaints, and who
knows if there end up being some unintentional side-effects...
You'd earlier mentioned that other distributions may not use pg_ctl
register - but I assume they use pg_ctl runservice? Or do they actually
re-implement all those magic incantations in pg_ctl.c?
It seems that we, in addition to the above patch, should add a guc that
pg_ctl runservice passes down to postgres. And then rip out the call to
pgwin32_is_service() from the backend. That doesn't require other
distributions to use pg_ctl register, just pg_ctl runservice - which I
think they need to do anyway, unless they want to duplicate all the
logic around pgwin32_SetServiceStatus()?
Greetings,
Andres Freund
Attachments:
v2-0001-windows-Only-consider-us-to-be-running-as-service.patchtext/x-diff; charset=us-asciiDownload+44-3
Hi,
Magnus, Michael, Anyone - I'd appreciate a look.
On 2021-03-05 12:55:37 -0800, Andres Freund wrote:
After fighting with a windows VM for a bit (ugh), it turns out that yes,
there is stderr, but that fileno(stderr) returns -2, and
GetStdHandle(STD_ERROR_HANDLE) returns NULL (not INVALID_HANDLE_VALUE).The complexity however is that while that's true for pg_ctl within
pgwin32_ServiceMain:
checking what stderr=00007FF8687DFCB0 is (handle: 0, fileno=-2)
but not for postmaster or backends
WARNING: 01000: checking what stderr=00007FF880F5FCB0 is (handle: 92, fileno=2)which makes sense in a way, because we don't tell CreateProcessAsUser()
that it should pass stdin/out/err down (which then seems to magically
get access to the "topmost" console applications output - damn, this
stuff is weird).That part is not too hard to address - it seems we only need to do that
in pg_ctl pgwin32_doRunAsService(). It seems that the
stdin/stderr/stdout being set to invalid will then be passed down to
postmaster children.https://docs.microsoft.com/en-us/windows/console/getstdhandle
"If an application does not have associated standard handles, such as a
service running on an interactive desktop, and has not redirected them,
the return value is NULL."There does seem to be some difference between what services get as std*
- GetStdHandle() returns NULL, and what explicitly passing down invalid
handles to postmaster does - GetStdHandle() returns
INVALID_HANDLE_VALUE. But passing down NULL rather than
INVALID_HANDLE_VALUE to postmaster seems to lead to postmaster
re-opening console buffers.Patch attached.
I'd like to commit something to address this issue to master soon - it
allows us to run a lot more tests in cirrus-ci... But probably not
backpatch it [yet] - there've not really been field complaints, and who
knows if there end up being some unintentional side-effects...
Because it'd allow us to run more tests as part of cfbot and other CI
efforts, I'd like to push this forward. So I'm planning to commit this
to master soon-ish, unless somebody wants to take this over? I'm really
not a windows person...
Greetings,
Andres Freund
Em sex., 13 de ago. de 2021 às 10:03, Andres Freund <andres@anarazel.de>
escreveu:
Hi,
Magnus, Michael, Anyone - I'd appreciate a look.
On 2021-03-05 12:55:37 -0800, Andres Freund wrote:
After fighting with a windows VM for a bit (ugh), it turns out that
yes,
there is stderr, but that fileno(stderr) returns -2, and
GetStdHandle(STD_ERROR_HANDLE) returns NULL (not INVALID_HANDLE_VALUE).The complexity however is that while that's true for pg_ctl within
pgwin32_ServiceMain:
checking what stderr=00007FF8687DFCB0 is (handle: 0, fileno=-2)
but not for postmaster or backends
WARNING: 01000: checking what stderr=00007FF880F5FCB0 is (handle: 92,fileno=2)
which makes sense in a way, because we don't tell CreateProcessAsUser()
that it should pass stdin/out/err down (which then seems to magically
get access to the "topmost" console applications output - damn, this
stuff is weird).That part is not too hard to address - it seems we only need to do that
in pg_ctl pgwin32_doRunAsService(). It seems that the
stdin/stderr/stdout being set to invalid will then be passed down to
postmaster children.https://docs.microsoft.com/en-us/windows/console/getstdhandle
"If an application does not have associated standard handles, such as a
service running on an interactive desktop, and has not redirected them,
the return value is NULL."There does seem to be some difference between what services get as std*
- GetStdHandle() returns NULL, and what explicitly passing down invalid
handles to postmaster does - GetStdHandle() returns
INVALID_HANDLE_VALUE. But passing down NULL rather than
INVALID_HANDLE_VALUE to postmaster seems to lead to postmaster
re-opening console buffers.Patch attached.
I'd like to commit something to address this issue to master soon - it
allows us to run a lot more tests in cirrus-ci... But probably not
backpatch it [yet] - there've not really been field complaints, and who
knows if there end up being some unintentional side-effects...Because it'd allow us to run more tests as part of cfbot and other CI
efforts, I'd like to push this forward. So I'm planning to commit this
to master soon-ish, unless somebody wants to take this over? I'm really
not a windows person...
Hi Andres,
I found this function on the web, from OpenSSL, but I haven't tested it.
I think that there is one more way to test if a service is running
(SECURITY_INTERACTIVE_RID).
Can you test on a Windows VM?
If this works I can elaborate a bit.
Attached.
regards,
Ranier Vilela
Attachments:
pgwin32_is_service.ctext/plain; charset=US-ASCII; name=pgwin32_is_service.cDownload
On Fri, Aug 13, 2021 at 3:03 PM Andres Freund <andres@anarazel.de> wrote:
Hi,
Magnus, Michael, Anyone - I'd appreciate a look.
On 2021-03-05 12:55:37 -0800, Andres Freund wrote:
After fighting with a windows VM for a bit (ugh), it turns out that yes,
there is stderr, but that fileno(stderr) returns -2, and
GetStdHandle(STD_ERROR_HANDLE) returns NULL (not INVALID_HANDLE_VALUE).The complexity however is that while that's true for pg_ctl within
pgwin32_ServiceMain:
checking what stderr=00007FF8687DFCB0 is (handle: 0, fileno=-2)
but not for postmaster or backends
WARNING: 01000: checking what stderr=00007FF880F5FCB0 is (handle: 92, fileno=2)which makes sense in a way, because we don't tell CreateProcessAsUser()
that it should pass stdin/out/err down (which then seems to magically
get access to the "topmost" console applications output - damn, this
stuff is weird).That part is not too hard to address - it seems we only need to do that
in pg_ctl pgwin32_doRunAsService(). It seems that the
stdin/stderr/stdout being set to invalid will then be passed down to
postmaster children.https://docs.microsoft.com/en-us/windows/console/getstdhandle
"If an application does not have associated standard handles, such as a
service running on an interactive desktop, and has not redirected them,
the return value is NULL."There does seem to be some difference between what services get as std*
- GetStdHandle() returns NULL, and what explicitly passing down invalid
handles to postmaster does - GetStdHandle() returns
INVALID_HANDLE_VALUE. But passing down NULL rather than
INVALID_HANDLE_VALUE to postmaster seems to lead to postmaster
re-opening console buffers.Patch attached.
I'd like to commit something to address this issue to master soon - it
allows us to run a lot more tests in cirrus-ci... But probably not
backpatch it [yet] - there've not really been field complaints, and who
knows if there end up being some unintentional side-effects...Because it'd allow us to run more tests as part of cfbot and other CI
efforts, I'd like to push this forward. So I'm planning to commit this
to master soon-ish, unless somebody wants to take this over? I'm really
not a windows person...
It certainly sounds reasonable. It does make me wonder why we didn't
use that GetStdHandle in the first place -- mostly in that "did we try
that and it didn't work", but that was long enough ago that I really
can't remember, and I am unable to find any references in my mail
history either. So it may very well just be that we missed it. But
give the number of times we've had issues around this, it makes me
wonder. It could of course also be something that didn't use to be
reliable but us now -- the world of Windows has changed a lot since
that was written.
It wouldn't surprise me if it does break some *other* weird
cornercase, but based on the docs page you linked to it doesn't look
like it would break any of the normal/standard usecases. But I'm also
very much not a Windows person these days, and most of my knowledge on
the API side is quite outdated by now -- so I can only base that on
reading the same manual page you did...
Gaining better testability definitely seems worth it, so I think an
approach of "push to master and see what explodes" is reasonable :)
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
Hi,
On 2021-08-15 11:25:01 -0300, Ranier Vilela wrote:
I found this function on the web, from OpenSSL, but I haven't tested it.
I think that there is one more way to test if a service is running
(SECURITY_INTERACTIVE_RID).
I don't think that really addresses the issue. If a service starts postgres
somewhere within, it won't have SECURITY_INTERACTIVE_RID, but postgres isn't
quite running as a service nevertheless. I think that's the situation in the
CI case that triggered me to look into this.
Regards,
Andres
Hi,
On 2021-08-16 15:34:51 +0200, Magnus Hagander wrote:
It wouldn't surprise me if it does break some *other* weird
cornercase, but based on the docs page you linked to it doesn't look
like it would break any of the normal/standard usecases.
Yea, me neither...
I do suspect that it'd have been better to have a --windows-service flag to
postgres. But we can't easily change the past...
Gaining better testability definitely seems worth it, so I think an
approach of "push to master and see what explodes" is reasonable :)
Done.
Greetings,
Andres Freund