Re: [HACKERS] Re: pgsql: Add support for TCP keepalives on Windows, both for backend and

Log Message:
-----------
Add support for TCP keepalives on Windows, both for backend and the new
libpq support.

Modified Files:
--------------
pgsql/src/backend/libpq:
pqcomm.c (r1.210 -> r1.211)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/libpq/pqcomm.c?r1=1.210&r2=1.211)
pgsql/src/interfaces/libpq:
fe-connect.c (r1.396 -> r1.397)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/interfaces/libpq/fe-connect.c?r1=1.396&r2=1.397)
pgsql/doc/src/sgml:
config.sgml (r1.293 -> r1.294)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/config.sgml?r1=1.293&r2=1.294)
libpq.sgml (r1.312 -> r1.313)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/libpq.sgml?r1=1.312&r2=1.313)

mha(at)postgresql(dot)org (Magnus Hagander) writes:
> Log Message:
> -----------
> Add support for TCP keepalives on Windows, both for backend and the new
> libpq support.

Buildfarm member narwhal doesn't like this patch. You have about six
or eight hours to fix or revert it before beta3 wraps.

regards, tom lane

On Thu, Jul 8, 2010 at 17:04, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> mha(at)postgresql(dot)org (Magnus Hagander) writes:
>> Log Message:
>> -----------
>> Add support for TCP keepalives on Windows, both for backend and the new
>> libpq support.
>
> Buildfarm member narwhal doesn't like this patch.  You have about six
> or eight hours to fix or revert it before beta3 wraps.

Gah. Seems mingw is out of date with reality again. I'll go look for a
vm with it on and see if I can find how to do it there.

(and yes, I even asked Dave to do a special run with that bf member
for me, and then forgot to check the result. sorry!)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Thu, Jul 8, 2010 at 17:11, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Thu, Jul 8, 2010 at 17:04, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> mha(at)postgresql(dot)org (Magnus Hagander) writes:
>>> Log Message:
>>> -----------
>>> Add support for TCP keepalives on Windows, both for backend and the new
>>> libpq support.
>>
>> Buildfarm member narwhal doesn't like this patch.  You have about six
>> or eight hours to fix or revert it before beta3 wraps.
>
> Gah. Seems mingw is out of date with reality again. I'll go look for a
> vm with it on and see if I can find how to do it there.
>
> (and yes, I even asked Dave to do a special run with that bf member
> for me, and then forgot to check the result. sorry!)

Seems pretty simple - mingw doesn't have support for this. We have two
ways to deal with that I think:
1) Disable it on mingw.
2) Include it in our custom headers.

For #2, what we need to include is the define of SIO_KEEPALIVE_VALS as
well as the definition of struct tcp_keepalive.

We've done #2 before at least once, which worked well until mingw
suddenly caught up and added it a while later. It's not like this is a
new definition in windows, but we need to be ready for them to
eventually do that.

I guess there is:
3) write an autoconf test and provide them only when mingw doesn't have it.

if we're going with #3, I'll respectfully have to ask somebod yelse to
write the autoconf test, that's beyond me I think :-)

Opinions on which way to go?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Seems pretty simple - mingw doesn't have support for this. We have two
> ways to deal with that I think:
> 1) Disable it on mingw.
> 2) Include it in our custom headers.

> For #2, what we need to include is the define of SIO_KEEPALIVE_VALS as
> well as the definition of struct tcp_keepalive.

> We've done #2 before at least once, which worked well until mingw
> suddenly caught up and added it a while later. It's not like this is a
> new definition in windows, but we need to be ready for them to
> eventually do that.

Yeah. I'm satisfied with doing #1 and waiting for them to fix it.

> I guess there is:
> 3) write an autoconf test and provide them only when mingw doesn't have it.
> if we're going with #3, I'll respectfully have to ask somebod yelse to
> write the autoconf test, that's beyond me I think :-)

An easy approximation would be to make the code #ifdef SIO_KEEPALIVE_VALS.
That would fail if the mingw guys decide to provide the #define without
adding the struct at the same time, but that seems moderately unlikely.

regards, tom lane

On Thu, Jul 8, 2010 at 17:39, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Seems pretty simple - mingw doesn't have support for this. We have two
>> ways to deal with that I think:
>> 1) Disable it on mingw.
>> 2) Include it in our custom headers.
>
>> For #2, what we need to include is the define of SIO_KEEPALIVE_VALS as
>> well as the definition of struct tcp_keepalive.
>
>> We've done #2 before at least once, which worked well until mingw
>> suddenly caught up and added it a while later. It's not like this is a
>> new definition in windows, but we need to be ready for them to
>> eventually do that.
>
> Yeah.  I'm satisfied with doing #1 and waiting for them to fix it.
>
>> I guess there is:
>> 3) write an autoconf test and provide them only when mingw doesn't have it.
>> if we're going with #3, I'll respectfully have to ask somebod yelse to
>> write the autoconf test, that's beyond me I think :-)
>
> An easy approximation would be to make the code #ifdef SIO_KEEPALIVE_VALS.
> That would fail if the mingw guys decide to provide the #define without
> adding the struct at the same time, but that seems moderately unlikely.

Seems reasonable. I'll go do something along that line and verify that
it actually works :-)

That laves the questions of docs - right now the docs just say it
works on windows. I guess we need to add some kind of disclaimer
around that, but the fact is that for 99+% of our windows users it
will work - since they use the binaries, and the binaries are built
with the full api - so we shouldn't make it *too* prominent..

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>
>> Seems pretty simple - mingw doesn't have support for this. We have two
>> ways to deal with that I think:
>> 1) Disable it on mingw.
>> 2) Include it in our custom headers.
>>
>
>
>> For #2, what we need to include is the define of SIO_KEEPALIVE_VALS as
>> well as the definition of struct tcp_keepalive.
>>
>
>
>> We've done #2 before at least once, which worked well until mingw
>> suddenly caught up and added it a while later. It's not like this is a
>> new definition in windows, but we need to be ready for them to
>> eventually do that.
>>
>
> Yeah. I'm satisfied with doing #1 and waiting for them to fix it.
>
>
>> I guess there is:
>> 3) write an autoconf test and provide them only when mingw doesn't have it.
>> if we're going with #3, I'll respectfully have to ask somebod yelse to
>> write the autoconf test, that's beyond me I think :-)
>>
>
> An easy approximation would be to make the code #ifdef SIO_KEEPALIVE_VALS.
> That would fail if the mingw guys decide to provide the #define without
> adding the struct at the same time, but that seems moderately unlikely.
>
>
>

+1 for this course of action.

cheers

andrew

On Thu, Jul 8, 2010 at 17:45, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
>
> Tom Lane wrote:
>>
>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>
>>>
>>> Seems pretty simple - mingw doesn't have support for this. We have two
>>> ways to deal with that I think:
>>> 1) Disable it on mingw.
>>> 2) Include it in our custom headers.
>>>
>>
>>
>>>
>>> For #2, what we need to include is the define of SIO_KEEPALIVE_VALS as
>>> well as the definition of struct tcp_keepalive.
>>>
>>
>>
>>>
>>> We've done #2 before at least once, which worked well until mingw
>>> suddenly caught up and added it a while later. It's not like this is a
>>> new definition in windows, but we need to be ready for them to
>>> eventually do that.
>>>
>>
>> Yeah.  I'm satisfied with doing #1 and waiting for them to fix it.
>>
>>
>>>
>>> I guess there is:
>>> 3) write an autoconf test and provide them only when mingw doesn't have
>>> it.
>>> if we're going with #3, I'll respectfully have to ask somebod yelse to
>>> write the autoconf test, that's beyond me I think :-)
>>>
>>
>> An easy approximation would be to make the code #ifdef SIO_KEEPALIVE_VALS.
>> That would fail if the mingw guys decide to provide the #define without
>> adding the struct at the same time, but that seems moderately unlikely.
>>
>>
>>
>
> +1 for this course of action.

Here's what I came up with and will apply as soon as my msvc build
completes. (the mingw one works with this)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Thu, Jul 8, 2010 at 18:06, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Thu, Jul 8, 2010 at 17:45, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>>
>>
>> Tom Lane wrote:
>>>
>>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>>
>>>>
>>>> Seems pretty simple - mingw doesn't have support for this. We have two
>>>> ways to deal with that I think:
>>>> 1) Disable it on mingw.
>>>> 2) Include it in our custom headers.
>>>>
>>>
>>>
>>>>
>>>> For #2, what we need to include is the define of SIO_KEEPALIVE_VALS as
>>>> well as the definition of struct tcp_keepalive.
>>>>
>>>
>>>
>>>>
>>>> We've done #2 before at least once, which worked well until mingw
>>>> suddenly caught up and added it a while later. It's not like this is a
>>>> new definition in windows, but we need to be ready for them to
>>>> eventually do that.
>>>>
>>>
>>> Yeah.  I'm satisfied with doing #1 and waiting for them to fix it.
>>>
>>>
>>>>
>>>> I guess there is:
>>>> 3) write an autoconf test and provide them only when mingw doesn't have
>>>> it.
>>>> if we're going with #3, I'll respectfully have to ask somebod yelse to
>>>> write the autoconf test, that's beyond me I think :-)
>>>>
>>>
>>> An easy approximation would be to make the code #ifdef SIO_KEEPALIVE_VALS.
>>> That would fail if the mingw guys decide to provide the #define without
>>> adding the struct at the same time, but that seems moderately unlikely.
>>>
>>>
>>>
>>
>> +1 for this course of action.
>
> Here's what I came up with and will apply as soon as my msvc build
> completes. (the mingw one works with this)

... and applied.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Here's what I came up with and will apply as soon as my msvc build
> completes. (the mingw one works with this)

This is still going to need manual adjustment in the future, since
probably when mingw adds the #define, they will put it in <mstcpip.h>,
and this code will not see it. So at some point we'll need to add an
autoconf test for <mstcpip.h> and replace those #ifdef
WIN32_ONLY_COMPILER checks with #ifdef HAVE_MSTCPIP_H. But I see no
need to bother until such a version of mingw exists, and I'd just as
soon not be making such changes on the day of a wrap. So this is just
a note for later.

regards, tom lane

On Thu, Jul 8, 2010 at 18:37, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Here's what I came up with and will apply as soon as my msvc build
>> completes. (the mingw one works with this)
>
> This is still going to need manual adjustment in the future, since
> probably when mingw adds the #define, they will put it in <mstcpip.h>,
> and this code will not see it.  So at some point we'll need to add an
> autoconf test for <mstcpip.h> and replace those #ifdef
> WIN32_ONLY_COMPILER checks with #ifdef HAVE_MSTCPIP_H.  But I see no
> need to bother until such a version of mingw exists, and I'd just as
> soon not be making such changes on the day of a wrap.  So this is just
> a note for later.

Agreed.

And it's far from certain they'll actually add it to mstcpip.h -
sometimes they stick it in a different header...

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Thu, Jul 8, 2010 at 18:37, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> This is still going to need manual adjustment in the future,

> And it's far from certain they'll actually add it to mstcpip.h -
> sometimes they stick it in a different header...

Yeah, that's the other reason for waiting for them to fix it before
we invest more effort on our end.

regards, tom lane

Magnus Hagander wrote:
> > An easy approximation would be to make the code #ifdef SIO_KEEPALIVE_VALS.
> > That would fail if the mingw guys decide to provide the #define without
> > adding the struct at the same time, but that seems moderately unlikely.
>
> Seems reasonable. I'll go do something along that line and verify that
> it actually works :-)
>
> That laves the questions of docs - right now the docs just say it
> works on windows. I guess we need to add some kind of disclaimer
> around that, but the fact is that for 99+% of our windows users it
> will work - since they use the binaries, and the binaries are built
> with the full api - so we shouldn't make it *too* prominent..

Wow, how would they know if the binaries are MinGW compiled? Does it
show in version()?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ None of us is going to be here forever. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Magnus Hagander wrote:
>> That laves the questions of docs - right now the docs just say it
>> works on windows. I guess we need to add some kind of disclaimer
>> around that, but the fact is that for 99+% of our windows users it
>> will work - since they use the binaries, and the binaries are built
>> with the full api - so we shouldn't make it *too* prominent..

> Wow, how would they know if the binaries are MinGW compiled? Does it
> show in version()?

AFAIK there is nobody distributing mingw-built binaries. If somebody
has one, they'd have built it themselves, and they'd know.

regards, tom lane

On lör, 2010-07-10 at 16:23 -0400, Bruce Momjian wrote:
> Wow, how would they know if the binaries are MinGW compiled? Does it
> show in version()?

Yes, I think so.

On Sun, Jul 11, 2010 at 00:05, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On lör, 2010-07-10 at 16:23 -0400, Bruce Momjian wrote:
>> Wow, how would they know if the binaries are MinGW compiled?  Does it
>> show in version()?
>
> Yes, I think so.

It definitely does.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/