stat() vs cygwin

Magnus Hagander <magnus@hagander.net> writes:

It seems my fix for stat() broke cygwin, because it doesn't have
dosmaperr() available.

Are you sure you aren't just missing an #include? The other places
where we call _dosmaperr don't seem to be protected by anything more
than #ifdef WIN32.

regards, tom lane

Magnus, was this fixed/resolved?

---------------------------------------------------------------------------

Magnus Hagander wrote:

It seems my fix for stat() broke cygwin, because it doesn't have
dosmaperr() available. The way I see it there are two ways to fix this:

1) Don't apply the stat fix for cygwin.

2) Make our dosmaperr() function be used on cygwin.

I don't know if the fix is actually needed on cygwin. Can someone with
access to such an environment test it and see?

The easy check, easier than the table, goes something along the line
of:
CREATE TABLE test(t int);
INSERT INTO test(t) SELECT * FROM generate_series(1,100000);
SELECT pg_relation_size('t');
SELECT pg_sleep(5);
SELECT pg_relation_size('t');

Without the patch on win32, the first pg_relation_size comes out as 0,
and the second one correct. With the patch, they come out equal. They
should, of course, always come out equal.

//Magnus

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

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Yes.

As in the cygwin build does build. Nobody really has verified if the fix
is needed there. But frankly, if you are likely to care about the
effects of this issue, you won't be running cygwin anyway. It's mostly a
dead platform for postgresql anyway, AFAICS we only keep it building for
legacy compatibility. Once it starts taking lots of resources to keep
building (which it doesn't now), I think we should just drop it instead...

//Magnus

Bruce Momjian wrote:

On Tue, Jun 24, 2008 at 9:32 AM, Magnus Hagander <magnus@hagander.net> wrote:

Yes.

As in the cygwin build does build. Nobody really has verified if the fix
is needed there. But frankly, if you are likely to care about the
effects of this issue, you won't be running cygwin anyway. It's mostly a
dead platform for postgresql anyway, AFAICS we only keep it building for
legacy compatibility. Once it starts taking lots of resources to keep
building (which it doesn't now), I think we should just drop it instead...

FWIW, the most recent packages from Cygwin themselves are 8.2.5.

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com

One motivation for keeping it working on Cygwin, is that in some
environments it is not allowed to install native Windows apps but
they allow the use of the Cygwin environment. Of course if it takes
too many resources to support, then dropping support would be an
option. I would check this for you, but I am in the middle of moving
and my Windows/Cygwin box is not available right now.

Cheers,
Ken

Kenneth Marshall wrote:

One motivation for keeping it working on Cygwin, is that in some
environments it is not allowed to install native Windows apps but
they allow the use of the Cygwin environment. Of course if it takes
too many resources to support, then dropping support would be an
option. I would check this for you, but I am in the middle of moving
and my Windows/Cygwin box is not available right now.

Does anybody seriously have such a broken policy? I know a lot of places
who have inverse policy, where they don't allow cygwin, but I've never
heard of anybody refusing native programs and only allowing cygwin. Just
like I've heard of no linux shops requiring that you run your database
under wine...

//Magnus

The case I am referring to has a "developer" clause that allows
Cygwin applications to be used for development only. I agree that
the policy is odd.

Ken

Magnus Hagander wrote:

Kenneth Marshall wrote:

One motivation for keeping it working on Cygwin, is that in some
environments it is not allowed to install native Windows apps but
they allow the use of the Cygwin environment. Of course if it takes
too many resources to support, then dropping support would be an
option. I would check this for you, but I am in the middle of moving
and my Windows/Cygwin box is not available right now.

Does anybody seriously have such a broken policy? I know a lot of places
who have inverse policy, where they don't allow cygwin, but I've never
heard of anybody refusing native programs and only allowing cygwin. Just
like I've heard of no linux shops requiring that you run your database
under wine...

This whole argument is pointless, ISTM. We are not in the business of
telling people what environment to use Postgres in.

Using Cygwin is still the best way I know of to use psql on Windows, and
it works just fine as a development environment.

By contrast, setting up a development environment for the native build
in either supported flavor is distinctly non-trivial.

(And yes I know about the problems Cygwin causes if you put it in the
System PATH. Don't do that.)

More to the point: I thought this had been tested. I will test it today
so we can put this whole thread to rest.

cheers

andrew

Andrew Dunstan wrote:

Magnus Hagander wrote:

Kenneth Marshall wrote:

One motivation for keeping it working on Cygwin, is that in some
environments it is not allowed to install native Windows apps but
they allow the use of the Cygwin environment. Of course if it takes
too many resources to support, then dropping support would be an
option. I would check this for you, but I am in the middle of moving
and my Windows/Cygwin box is not available right now.

Does anybody seriously have such a broken policy? I know a lot of places
who have inverse policy, where they don't allow cygwin, but I've never
heard of anybody refusing native programs and only allowing cygwin. Just
like I've heard of no linux shops requiring that you run your database
under wine...

This whole argument is pointless, ISTM. We are not in the business of
telling people what environment to use Postgres in.

Well, agreed, the argument was that it should be dropped *IF* it turns
into a maintenance burden. Which it hasn't yet done.

More to the point: I thought this had been tested. I will test it today
so we can put this whole thread to rest.

IIRC it was only tested insofar that it doesn't actually break. Not if
it returns proper results.

Buf if my memory isn't completely off, there are other such cases as
well around the code, where we've done proper fixes for native win32 and
left cygwin alone. The argument being that for a developer system, it
doesn't really matter if things aren't entirely reliable, and that
nobody should be using cygwin for a production server. (I have nothing
against using it for a dev box, though I wouldn't do it myself)

//Magnus

Magnus Hagander wrote:

More to the point: I thought this had been tested. I will test it today
so we can put this whole thread to rest.

IIRC it was only tested insofar that it doesn't actually break. Not if
it returns proper results.

I have tested it using the suggested script (corrected) and it passes
(both sizes the same) consistently, as I expected.

Buf if my memory isn't completely off, there are other such cases as
well around the code, where we've done proper fixes for native win32 and
left cygwin alone. The argument being that for a developer system, it
doesn't really matter if things aren't entirely reliable, and that
nobody should be using cygwin for a production server. (I have nothing
against using it for a dev box, though I wouldn't do it myself)

I don't recall any. But I could be wrong.

cheers

andrew

Dave Page schrieb:

On Tue, Jun 24, 2008 at 9:32 AM, Magnus Hagander <magnus@hagander.net> wrote:

Yes.

As in the cygwin build does build. Nobody really has verified if the fix
is needed there. But frankly, if you are likely to care about the
effects of this issue, you won't be running cygwin anyway. It's mostly a
dead platform for postgresql anyway, AFAICS we only keep it building for
legacy compatibility. Once it starts taking lots of resources to keep
building (which it doesn't now), I think we should just drop it instead...

"Dead" is interesting. We see a lot of cygwin users having postgresql
installed.

FWIW, the most recent packages from Cygwin themselves are 8.2.5.

Update: 8.2.9 is latest.
8.3.x not because the new SSPI doesn't work yet.

currently failing is: --with-gssapi --with-krb5 --with-tcl --with-java
--with-ossp-uuid --with-ldap
(but ldap works okay with 8.2.9)

currently testing is: --enable-nls --with-CXX --with-openssl --with-perl
--with-python --with-libxml --with-libxslt

current cygwin patch in testing is attached.
--
Reini Urban
postgresql cygwin maintainer

Reini Urban wrote:

Dave Page schrieb:

On Tue, Jun 24, 2008 at 9:32 AM, Magnus Hagander <magnus@hagander.net>
wrote:

Yes.

As in the cygwin build does build. Nobody really has verified if the fix
is needed there. But frankly, if you are likely to care about the
effects of this issue, you won't be running cygwin anyway. It's mostly a
dead platform for postgresql anyway, AFAICS we only keep it building for
legacy compatibility. Once it starts taking lots of resources to keep
building (which it doesn't now), I think we should just drop it
instead...

"Dead" is interesting. We see a lot of cygwin users having postgresql
installed.

Heh. Maybe not dead, but certainly not really alive either ;-) Given the
evidence in your patch that clearly 8.3 isn't quite up to speed on
cygwin, and nobody has really noticed until now.

FWIW, the most recent packages from Cygwin themselves are 8.2.5.

Update: 8.2.9 is latest.

Good!

8.3.x not because the new SSPI doesn't work yet.

currently failing is: --with-gssapi --with-krb5 --with-tcl --with-java
--with-ossp-uuid --with-ldap
(but ldap works okay with 8.2.9)

currently testing is: --enable-nls --with-CXX --with-openssl --with-perl
--with-python --with-libxml --with-libxslt

current cygwin patch in testing is attached.

I assume this is a WIP and not actually for application, right? Please
look it over because it contains a number of pure-whitespace changes
that make it harder to read, and that will just end up being undone by
pgindent at a later date anyway.

I also notice this in auth.c:
+#ifdef�__CYGWIN__
+#define�WIN32
+#endif

What is the need to change this for just one file? Seems very fragile -
the rest of our codebase assumes WIN32 != CYGWIN, and I think we should
keep that consistent.

There's also a number of:
-#ifndef�WIN32
+#if�!defined(WIN32)�||�defined(__CYGWIN__)

If I read that right, it shouldn't be necessary as long as the WIN32
define is not set on CYGWIN?

And finally:
-������������VALUE�"OriginalFilename",�"libpq.dll\0"
+������������VALUE�"OriginalFilename",�"cygpq.dll\0"

This obviously has to be done another way, because that change will
affect the win32 platform as well...

//Magnus

Magnus Hagander wrote:

8.3.x not because the new SSPI doesn't work yet.

current cygwin patch in testing is attached.

I assume this is a WIP and not actually for application, right? Please
look it over because it contains a number of pure-whitespace changes
that make it harder to read, and that will just end up being undone by
pgindent at a later date anyway.

The patch also seems to hint that pgwin32_waitforsinglesocket needs a
specific Cygwin implementation too (rather than patching every caller).

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Magnus Hagander schrieb:

Reini Urban wrote:

Dave Page schrieb:

On Tue, Jun 24, 2008 at 9:32 AM, Magnus Hagander <magnus@hagander.net>
wrote:

Yes.

As in the cygwin build does build. Nobody really has verified if the fix
is needed there. But frankly, if you are likely to care about the
effects of this issue, you won't be running cygwin anyway. It's mostly a
dead platform for postgresql anyway, AFAICS we only keep it building for
legacy compatibility. Once it starts taking lots of resources to keep
building (which it doesn't now), I think we should just drop it
instead...

"Dead" is interesting. We see a lot of cygwin users having postgresql
installed.

Heh. Maybe not dead, but certainly not really alive either ;-) Given the
evidence in your patch that clearly 8.3 isn't quite up to speed on
cygwin, and nobody has really noticed until now.

FWIW, the most recent packages from Cygwin themselves are 8.2.5.

Update: 8.2.9 is latest.

Good!

8.3.x not because the new SSPI doesn't work yet.

currently failing is: --with-gssapi --with-krb5 --with-tcl --with-java
--with-ossp-uuid --with-ldap
(but ldap works okay with 8.2.9)

currently testing is: --enable-nls --with-CXX --with-openssl --with-perl
--with-python --with-libxml --with-libxslt

current cygwin patch in testing is attached.

I assume this is a WIP and not actually for application, right? Please
look it over because it contains a number of pure-whitespace changes
that make it harder to read, and that will just end up being undone by
pgindent at a later date anyway.

Sure. This is just the current status of my patch (still from 8.3beta2),
nothing to actually submit.

I also notice this in auth.c:
+#ifdef�__CYGWIN__
+#define�WIN32
+#endif

What is the need to change this for just one file? Seems very fragile -
the rest of our codebase assumes WIN32 != CYGWIN, and I think we should
keep that consistent.

SSPI has some direct winapi calls to libsecure32 which are simpliest to
declare by this cygwin == WIN32 declaration in the backend.
For the client libpq this is not so easy, I still have troubles
seperating this.

There's also a number of:
-#ifndef�WIN32
+#if�!defined(WIN32)�||�defined(__CYGWIN__)

If I read that right, it shouldn't be necessary as long as the WIN32
define is not set on CYGWIN?

This is only for the special case cygwin == WIN32. Just to be sure while
testing I wrote it this way.

And finally:
-������������VALUE�"OriginalFilename",�"libpq.dll\0"
+������������VALUE�"OriginalFilename",�"cygpq.dll\0"

This obviously has to be done another way, because that change will
affect the win32 platform as well...

Sure :) This is only vendor private.
--
Reini

Magnus Hagander wrote:

Heh. Maybe not dead, but certainly not really alive either ;-) Given the
evidence in your patch that clearly 8.3 isn't quite up to speed on
cygwin, and nobody has really noticed until now.

AIUI, only the gssapi stuff is broken. Most users are not likely to want
it on Cygwin I should think. (And plenty of distros are still on 8.2 and
earlier, anyway).

What would be nice would be for Reini to set up a Cygwin buildfarm
member that uses all the switches that the Cygwin distro wants to use.

I also notice this in auth.c:
+#ifdef�__CYGWIN__
+#define�WIN32
+#endif

What is the need to change this for just one file? Seems very fragile -
the rest of our codebase assumes WIN32 != CYGWIN, and I think we should
keep that consistent.

Right. We have had significant grief from this in the past, and don't
need to return there. If we need it to get correct behaviour from some
header file, then it needs to be heavily commented and localised. But I
bet there are other ways to get the right result - that's what we've
tended to find in the past.

cheers

andrew

Andrew Dunstan schrieb:

Magnus Hagander wrote:

Heh. Maybe not dead, but certainly not really alive either ;-) Given the
evidence in your patch that clearly 8.3 isn't quite up to speed on
cygwin, and nobody has really noticed until now.

AIUI, only the gssapi stuff is broken. Most users are not likely to want
it on Cygwin I should think. (And plenty of distros are still on 8.2 and
earlier, anyway).

Well, native windows users is a very nice to have. Actually a killer
feature.

What would be nice would be for Reini to set up a Cygwin buildfarm
member that uses all the switches that the Cygwin distro wants to use.

Without ENABLE_SSPI I just need
--enable-nls --with-CXX --with-openssl --with-perl --with-python
--with-libxml --with-libxslt --with-ldap
and these build out of the box.

I also notice this in auth.c:
+#ifdef�__CYGWIN__
+#define�WIN32
+#endif

What is the need to change this for just one file? Seems very fragile -
the rest of our codebase assumes WIN32 != CYGWIN, and I think we should
keep that consistent.

Right. We have had significant grief from this in the past, and don't
need to return there. If we need it to get correct behaviour from some
header file, then it needs to be heavily commented and localised. But I
bet there are other ways to get the right result - that's what we've
tended to find in the past.

Ok, I copy then the required lines from WIN32.
This was the shortest patch I could come up with and it
worked nice for the auth backend, with SSPI enabled.
--
Reini

Where are we on this patch?

---------------------------------------------------------------------------

Reini Urban wrote:

Dave Page schrieb:

On Tue, Jun 24, 2008 at 9:32 AM, Magnus Hagander <magnus@hagander.net> wrote:

Yes.

As in the cygwin build does build. Nobody really has verified if the fix
is needed there. But frankly, if you are likely to care about the
effects of this issue, you won't be running cygwin anyway. It's mostly a
dead platform for postgresql anyway, AFAICS we only keep it building for
legacy compatibility. Once it starts taking lots of resources to keep
building (which it doesn't now), I think we should just drop it instead...

"Dead" is interesting. We see a lot of cygwin users having postgresql
installed.

FWIW, the most recent packages from Cygwin themselves are 8.2.5.

Update: 8.2.9 is latest.
8.3.x not because the new SSPI doesn't work yet.

currently failing is: --with-gssapi --with-krb5 --with-tcl --with-java
--with-ossp-uuid --with-ldap
(but ldap works okay with 8.2.9)

currently testing is: --enable-nls --with-CXX --with-openssl --with-perl
--with-python --with-libxml --with-libxslt

current cygwin patch in testing is attached.
--
Reini Urban
postgresql cygwin maintainer

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Where are we on this? The patch was not acceptable for several reasons;
for one:

And finally:
-????????????VALUE?"OriginalFilename",?"libpq.dll\0"
+????????????VALUE?"OriginalFilename",?"cygpq.dll\0"

This obviously has to be done another way, because that change will
affect the win32 platform as well...

---------------------------------------------------------------------------

Reini Urban wrote:

Dave Page schrieb:

On Tue, Jun 24, 2008 at 9:32 AM, Magnus Hagander <magnus@hagander.net> wrote:

Yes.

As in the cygwin build does build. Nobody really has verified if the fix
is needed there. But frankly, if you are likely to care about the
effects of this issue, you won't be running cygwin anyway. It's mostly a
dead platform for postgresql anyway, AFAICS we only keep it building for
legacy compatibility. Once it starts taking lots of resources to keep
building (which it doesn't now), I think we should just drop it instead...

"Dead" is interesting. We see a lot of cygwin users having postgresql
installed.

FWIW, the most recent packages from Cygwin themselves are 8.2.5.

Update: 8.2.9 is latest.
8.3.x not because the new SSPI doesn't work yet.

currently failing is: --with-gssapi --with-krb5 --with-tcl --with-java
--with-ossp-uuid --with-ldap
(but ldap works okay with 8.2.9)

currently testing is: --enable-nls --with-CXX --with-openssl --with-perl
--with-python --with-libxml --with-libxslt

current cygwin patch in testing is attached.
--
Reini Urban
postgresql cygwin maintainer

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

If we have no plan to apply this patch, do we need to remove Cygwin as a
supported platform?

---------------------------------------------------------------------------

Bruce Momjian wrote:

Where are we on this? The patch was not acceptable for several reasons;
for one:

And finally:
-????????????VALUE?"OriginalFilename",?"libpq.dll\0"
+????????????VALUE?"OriginalFilename",?"cygpq.dll\0"

This obviously has to be done another way, because that change will
affect the win32 platform as well...

---------------------------------------------------------------------------

Reini Urban wrote:

Dave Page schrieb:

On Tue, Jun 24, 2008 at 9:32 AM, Magnus Hagander <magnus@hagander.net> wrote:

Yes.

As in the cygwin build does build. Nobody really has verified if the fix
is needed there. But frankly, if you are likely to care about the
effects of this issue, you won't be running cygwin anyway. It's mostly a
dead platform for postgresql anyway, AFAICS we only keep it building for
legacy compatibility. Once it starts taking lots of resources to keep
building (which it doesn't now), I think we should just drop it instead...

"Dead" is interesting. We see a lot of cygwin users having postgresql
installed.

FWIW, the most recent packages from Cygwin themselves are 8.2.5.

Update: 8.2.9 is latest.
8.3.x not because the new SSPI doesn't work yet.

currently failing is: --with-gssapi --with-krb5 --with-tcl --with-java
--with-ossp-uuid --with-ldap
(but ldap works okay with 8.2.9)

currently testing is: --enable-nls --with-CXX --with-openssl --with-perl
--with-python --with-libxml --with-libxslt

current cygwin patch in testing is attached.
--
Reini Urban
postgresql cygwin maintainer

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

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

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +