Re: CI/windows docker vs "am a service" autodetection on windows - Mailing list pgsql-hackers

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/