Re: CI/windows docker vs "am a service" autodetection on windows

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