Обсуждение: Re: CI/windows docker vs "am a service" autodetection on windows

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

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