pgsql: Fix and simplify check for whether we're running as Windows serv

Heikki Linnakangas <heikki.linnakangas@iki.fi> writes:

Fix and simplify check for whether we're running as Windows service.

This seems to have broken narwhal:

Creating library file: libpostgres.a
../../src/port/libpgport_srv.a(win32security_srv.o)(.text+0x109): In function `pgwin32_is_admin':
C:/msys/1.0/local/pgbuildfarm/buildroot/HEAD/pgsql.build/src/port/win32security.c:77: undefined reference to `CheckTokenMembership'
../../src/port/libpgport_srv.a(win32security_srv.o)(.text+0x127):C:/msys/1.0/local/pgbuildfarm/buildroot/HEAD/pgsql.build/src/port/win32security.c:77: undefined reference to `CheckTokenMembership'
../../src/port/libpgport_srv.a(win32security_srv.o)(.text+0x264): In function `pgwin32_is_service':
C:/msys/1.0/local/pgbuildfarm/buildroot/HEAD/pgsql.build/src/port/win32security.c:138: undefined reference to `CheckTokenMembership'
../../src/port/libpgport_srv.a(win32security_srv.o)(.text+0x302):C:/msys/1.0/local/pgbuildfarm/buildroot/HEAD/pgsql.build/src/port/win32security.c:163: undefined reference to `CheckTokenMembership'
collect2: ld returned 1 exit status
make[2]: *** [postgres] Error 1

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 03/17/2017 07:57 PM, Tom Lane wrote:

Heikki Linnakangas <heikki.linnakangas@iki.fi> writes:

Fix and simplify check for whether we're running as Windows service.

This seems to have broken narwhal:

Creating library file: libpostgres.a
../../src/port/libpgport_srv.a(win32security_srv.o)(.text+0x109): In function `pgwin32_is_admin':
C:/msys/1.0/local/pgbuildfarm/buildroot/HEAD/pgsql.build/src/port/win32security.c:77: undefined reference to `CheckTokenMembership'
../../src/port/libpgport_srv.a(win32security_srv.o)(.text+0x127):C:/msys/1.0/local/pgbuildfarm/buildroot/HEAD/pgsql.build/src/port/win32security.c:77: undefined reference to `CheckTokenMembership'
../../src/port/libpgport_srv.a(win32security_srv.o)(.text+0x264): In function `pgwin32_is_service':
C:/msys/1.0/local/pgbuildfarm/buildroot/HEAD/pgsql.build/src/port/win32security.c:138: undefined reference to `CheckTokenMembership'
../../src/port/libpgport_srv.a(win32security_srv.o)(.text+0x302):C:/msys/1.0/local/pgbuildfarm/buildroot/HEAD/pgsql.build/src/port/win32security.c:163: undefined reference to `CheckTokenMembership'
collect2: ld returned 1 exit status
make[2]: *** [postgres] Error 1

Hmm. The other MinGW animals are not having that problem.

According to the MSDN docs [1]https://msdn.microsoft.com/en-us/library/windows/desktop/aa376389%28v=vs.85%29.aspx, CheckTokenMembership() is defined in the
Advapi32 library. In pg_ctl.c and in
src/include/common/restricted_token.c, we currently do this, for another
function that's in Advapi32:

/*
* Mingw headers are incomplete, and so are the libraries. So we have to load
* a whole lot of API functions dynamically. Since we have to do this anyway,
* also load the couple of functions that *do* exist in minwg headers but not
* on NT4. That way, we don't break on NT4.
*/
typedef BOOL (WINAPI * __CreateRestrictedToken) (HANDLE, DWORD, DWORD, PSID_AND_ATTRIBUTES, DWORD, PLUID_AND_ATTRIBUTES, DWORD, PSID_AND_ATTRIBUTES, PHANDLE);
typedef BOOL (WINAPI * __IsProcessInJob) (HANDLE, HANDLE, PBOOL);
typedef HANDLE (WINAPI * __CreateJobObject) (LPSECURITY_ATTRIBUTES, LPCTSTR);
typedef BOOL (WINAPI * __SetInformationJobObject) (HANDLE, JOBOBJECTINFOCLASS, LPVOID, DWORD);
typedef BOOL (WINAPI * __AssignProcessToJobObject) (HANDLE, HANDLE);
typedef BOOL (WINAPI * __QueryInformationJobObject) (HANDLE, JOBOBJECTINFOCLASS, LPVOID, DWORD, LPDWORD);

CheckTokenMembership() is probably one of those missing functions, like
CreateRestrictedToken.

I'd like to bump our minimum requirements,and stop supporting old MinGW
versions that don't have those headers. I'm not sure what version they
were added in, but frogmouth doesn't have this problem, and it uses gcc
4.5.0. Once we do that, we can simplify pg_ctl.c and restricted_token.c
to not open advapi32.dll dynamically, and call those functions the usual
way.

It's not very nice to change the requirements in a minor version, but I
don't think this would be a real problem for anyone. Not many people
build PostgreSQL using MinGW, let alone with an ancient version of it.
But if people don't agree, we could instead revert this patch and apply
the smaller V2 patch [2]/messages/by-id/CAB7nPqSvfu=KpJ=NX+YAHmgAmQdzA7N5h31BjzXeMgczhGCC+Q@mail.gmail.com instead, in the back-branches.

Thoughts? Any objections to requiring a newer version of MinGW? Any
objections to do so in the next minor release?

[1]: https://msdn.microsoft.com/en-us/library/windows/desktop/aa376389%28v=vs.85%29.aspx
https://msdn.microsoft.com/en-us/library/windows/desktop/aa376389%28v=vs.85%29.aspx

[2]: /messages/by-id/CAB7nPqSvfu=KpJ=NX+YAHmgAmQdzA7N5h31BjzXeMgczhGCC+Q@mail.gmail.com
/messages/by-id/CAB7nPqSvfu=KpJ=NX+YAHmgAmQdzA7N5h31BjzXeMgczhGCC+Q@mail.gmail.com

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Heikki Linnakangas <hlinnaka@iki.fi> writes:

On 03/17/2017 07:57 PM, Tom Lane wrote:

This seems to have broken narwhal:

It's not very nice to change the requirements in a minor version, but I
don't think this would be a real problem for anyone. Not many people
build PostgreSQL using MinGW, let alone with an ancient version of it.
But if people don't agree, we could instead revert this patch and apply
the smaller V2 patch [2] instead, in the back-branches.

Thoughts? Any objections to requiring a newer version of MinGW? Any
objections to do so in the next minor release?

Hm. I'm +1 for doing that in HEAD, but less so for the back branches.

Can we get some fix on when the functions in question were added
to MinGW? If we knew how new a toolchain we'd be requiring here,
that would help make this decision.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 03/19/2017 11:40 PM, Tom Lane wrote:

Heikki Linnakangas <hlinnaka@iki.fi> writes:

On 03/17/2017 07:57 PM, Tom Lane wrote:

This seems to have broken narwhal:

It's not very nice to change the requirements in a minor version, but I
don't think this would be a real problem for anyone. Not many people
build PostgreSQL using MinGW, let alone with an ancient version of it.
But if people don't agree, we could instead revert this patch and apply
the smaller V2 patch [2] instead, in the back-branches.

Thoughts? Any objections to requiring a newer version of MinGW? Any
objections to do so in the next minor release?

Hm. I'm +1 for doing that in HEAD, but less so for the back branches.

Can we get some fix on when the functions in question were added
to MinGW? If we knew how new a toolchain we'd be requiring here,
that would help make this decision.

I did some archeology, and found CheckTokenMembership() in MinGW's
w32api packages version 3.14
(https://sourceforge.net/projects/mingw/files/MinGW/Base/w32api/w32api-3.14/,
in include/winbase.h). According to the timestamps on that download
page, that was released in 2009. That was the oldest version I could
find, so it might go even further back.

Dave, do you know exactly what version of MinGW narwhal is running? And
how difficult is it to upgrade to something slightly more modern? Ease
of upgrade is another good data point on how far we need to support old
versions.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Heikki Linnakangas wrote:

I did some archeology, and found CheckTokenMembership() in MinGW's w32api
packages version 3.14
(https://sourceforge.net/projects/mingw/files/MinGW/Base/w32api/w32api-3.14/,
in include/winbase.h). According to the timestamps on that download page,
that was released in 2009. That was the oldest version I could find, so it
might go even further back.

Dave, do you know exactly what version of MinGW narwhal is running? And how
difficult is it to upgrade to something slightly more modern? Ease of
upgrade is another good data point on how far we need to support old
versions.

Given that this was backpatched and that it broke narwhal in all
branches, I think the solution needs to make narwhal work again without
requiring it to upgrade; so we should acquire CheckTokenMembership via
dynloading just like we do the other functions. If we want to require a
newer mingw version in pg10, that's acceptable, but it should be a
separate patch.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Mar 22, 2017 at 10:13 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Heikki Linnakangas wrote:

I did some archeology, and found CheckTokenMembership() in MinGW's w32api
packages version 3.14
(https://sourceforge.net/projects/mingw/files/MinGW/Base/w32api/w32api-3.14/,
in include/winbase.h). According to the timestamps on that download page,
that was released in 2009. That was the oldest version I could find, so it
might go even further back.

Dave, do you know exactly what version of MinGW narwhal is running? And how
difficult is it to upgrade to something slightly more modern? Ease of
upgrade is another good data point on how far we need to support old
versions.

Given that this was backpatched and that it broke narwhal in all
branches, I think the solution needs to make narwhal work again without
requiring it to upgrade; so we should acquire CheckTokenMembership via
dynloading just like we do the other functions. If we want to require a
newer mingw version in pg10, that's acceptable, but it should be a
separate patch.

+1 for not moving the minimum system requirements in the back-branches.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 03/22/2017 07:44 PM, Robert Haas wrote:

On Wed, Mar 22, 2017 at 10:13 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Heikki Linnakangas wrote:

I did some archeology, and found CheckTokenMembership() in MinGW's w32api
packages version 3.14
(https://sourceforge.net/projects/mingw/files/MinGW/Base/w32api/w32api-3.14/,
in include/winbase.h). According to the timestamps on that download page,
that was released in 2009. That was the oldest version I could find, so it
might go even further back.

Dave, do you know exactly what version of MinGW narwhal is running? And how
difficult is it to upgrade to something slightly more modern? Ease of
upgrade is another good data point on how far we need to support old
versions.

Given that this was backpatched and that it broke narwhal in all
branches, I think the solution needs to make narwhal work again without
requiring it to upgrade; so we should acquire CheckTokenMembership via
dynloading just like we do the other functions. If we want to require a
newer mingw version in pg10, that's acceptable, but it should be a
separate patch.

+1 for not moving the minimum system requirements in the back-branches.

Ok. I reverted this patch in the back-branches, and applied the much
less invasive "V2" patch [1]/messages/by-id/CAB7nPqSvfu=KpJ=NX+YAHmgAmQdzA7N5h31BjzXeMgczhGCC+Q@mail.gmail.com instead. HEAD is unchanged, so narwhal
still fails there.

Dave: the consensus is that we no longer support the old version of
MinGW that narwhal is using, for PostgreSQL v 10. Can you modify the
configuration of narwhal to not try building 'master' anymore, or
upgrade the toolchain, please?

[1]: /messages/by-id/CAB7nPqSvfu=KpJ=NX+YAHmgAmQdzA7N5h31BjzXeMgczhGCC+Q@mail.gmail.com
/messages/by-id/CAB7nPqSvfu=KpJ=NX+YAHmgAmQdzA7N5h31BjzXeMgczhGCC+Q@mail.gmail.com

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Friday, March 24, 2017, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 03/22/2017 07:44 PM, Robert Haas wrote:

On Wed, Mar 22, 2017 at 10:13 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Heikki Linnakangas wrote:

I did some archeology, and found CheckTokenMembership() in MinGW's
w32api
packages version 3.14
(https://sourceforge.net/projects/mingw/files/MinGW/Base/
w32api/w32api-3.14/,
in include/winbase.h). According to the timestamps on that download
page,
that was released in 2009. That was the oldest version I could find, so
it
might go even further back.

Dave, do you know exactly what version of MinGW narwhal is running? And
how
difficult is it to upgrade to something slightly more modern? Ease of
upgrade is another good data point on how far we need to support old
versions.

Given that this was backpatched and that it broke narwhal in all
branches, I think the solution needs to make narwhal work again without
requiring it to upgrade; so we should acquire CheckTokenMembership via
dynloading just like we do the other functions. If we want to require a
newer mingw version in pg10, that's acceptable, but it should be a
separate patch.

+1 for not moving the minimum system requirements in the back-branches.

Ok. I reverted this patch in the back-branches, and applied the much less
invasive "V2" patch [1] instead. HEAD is unchanged, so narwhal still fails
there.

Dave: the consensus is that we no longer support the old version of MinGW
that narwhal is using, for PostgreSQL v 10. Can you modify the
configuration of narwhal to not try building 'master' anymore, or upgrade
the toolchain, please?

I've disabled it. Thanks.

--
Dave Page
PostgreSQL Core Team
http://www.postgresql.org/