Keepalives win32

Magnus Hagander <magnus@hagander.net> writes:

[ can't read system's keepalive values in windows ]

The way I see it, we have two options:
1) Read the default value from the registry. That's some fairly ugly code, imho.
2) Ignore the registry value and use the default value of 2 hours/1
second. That will override any changes the user made in the registry,
which seems pretty ugly.
3) Require that these two parameters are always specified together (on
windows). Which is annoying.

I vote for #2. It's the least inconsistent --- we don't pay attention
to the registry for much of anything else, do we?

In practice I think people who were setting either would set both, so
it's not worth a huge amount of effort to have an unsurprising behavior
when only one is set.

regards, tom lane

On Mon, Jun 28, 2010 at 20:45, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

[ can't read system's keepalive values in windows ]

The way I see it, we have two options:
1) Read the default value from the registry. That's some fairly ugly code, imho.
2) Ignore the registry value and use the default value of 2 hours/1
second. That will override any changes the user made in the registry,
which seems pretty ugly.
3) Require that these two parameters are always specified together (on
windows). Which is annoying.

I vote for #2.  It's the least inconsistent --- we don't pay attention
to the registry for much of anything else, do we?

Directly, no? Indirectly, we do. For every other TCP parameter
(because the registry controls what we'll get as the default when we
"just use things")

In practice I think people who were setting either would set both, so
it's not worth a huge amount of effort to have an unsurprising behavior
when only one is set.

There's unsurprising, and downright hostile (the way we get by default
is if you don't set keepalive_time, it'll spew keepalive packages
continuously, which is certainly not good). In tha tcase, it's
probably better to throw an error (which would be trivial to do, of
course)

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

Magnus Hagander <magnus@hagander.net> writes:

On Mon, Jun 28, 2010 at 20:45, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I vote for #2. �It's the least inconsistent --- we don't pay attention
to the registry for much of anything else, do we?

Directly, no? Indirectly, we do. For every other TCP parameter
(because the registry controls what we'll get as the default when we
"just use things")

Not if we make the code use the RFC values as the defaults. I'm
envisioning the GUC assign hooks doing something like

#ifdef WIN32
if (newval == 0)
newval = RFC-specified-default;
#endif

so that the main GUC logic can still think that zero means "use the
default". We're just redefining where the default comes from.

This would be a change from previous behavior, but so what?
Implementing any functionality at all here is a change from previous
behavior on Windows. I don't have the slightest problem with saying
"as of 9.0, set these values via postgresql.conf, not the registry".

regards, tom lane

The way I see it, we have two options:
1) Read the default value from the registry. That's some fairly ugly code, imho.

It seems faily simple to yank these values out, no? Even easier if you
use the all-in-wonder shell function SHGetValue().

HKEY_LOCAL_MACHINE\System\CurrentControlSet\Services\Tcpip\Parameters
Values: KeepAliveTime, KeepAliveInterval
Type: DWORD

The only annoying thing is that the values may not exist. Well, it is
also rather annoying there is no way to set the counter.

The API used is documented at:
http://msdn.microsoft.com/en-us/library/dd877220(v=VS.85).aspx
Patch as it looks now (libpq only, and with obvious problems with this
issue): http://github.com/mhagander/postgres/compare/master...win32keepalive

and here :)

http://archives.postgresql.org/pgsql-hackers/2009-05/msg01099.php

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

On Mon, Jun 28, 2010 at 21:03, Andrew Chernow <ac@esilo.com> wrote:

The way I see it, we have two options:
1) Read the default value from the registry. That's some fairly ugly code,
imho.

It seems faily simple to yank these values out, no?  Even easier if you use
the all-in-wonder shell function SHGetValue().

We don't want to use that function, because it brings in a bunch of
extra dependencies. This makes libpq.dll more heavyweight and more
importantly, decreases the number of parallell connections we can deal
with on the server side (on win32 at least, not sure about win64).

HKEY_LOCAL_MACHINE\System\CurrentControlSet\Services\Tcpip\Parameters
Values: KeepAliveTime, KeepAliveInterval
Type: DWORD

The only annoying thing is that the values may not exist.  Well, it is also

Right, we'd need an fallback in case they don't exist as well.

rather annoying there is no way to set the counter.

Yeah, but that's at least well documented how it behaves. In fact,
there used to be a way to set that (via registry key), but they
removed it in Vista.

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

On Mon, Jun 28, 2010 at 21:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

On Mon, Jun 28, 2010 at 20:45, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I vote for #2.  It's the least inconsistent --- we don't pay attention
to the registry for much of anything else, do we?

Directly, no? Indirectly, we do. For every other TCP parameter
(because the registry controls what we'll get as the default when we
"just use things")

Not if we make the code use the RFC values as the defaults.  I'm
envisioning the GUC assign hooks doing something like

#ifdef WIN32
       if (newval == 0)
               newval = RFC-specified-default;
#endif

Right. (I've only looked at the libpq side so far)

Also, we could avoid caling it *at all* if neither one of those
parameters is set. That'll take a bit more code (using the
unix-codepath of setsockopt() to enable keepalives at all), but it
shouldn't amount to many lines..

so that the main GUC logic can still think that zero means "use the
default".  We're just redefining where the default comes from.

Yeah.

This would be a change from previous behavior, but so what?
Implementing any functionality at all here is a change from previous
behavior on Windows.  I don't have the slightest problem with saying
"as of 9.0, set these values via postgresql.conf, not the registry".

Works for me.

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

It seems faily simple to yank these values out, no? Even easier if you use
the all-in-wonder shell function SHGetValue().

We don't want to use that function, because it brings in a bunch of
extra dependencies. This makes libpq.dll more heavyweight and more
importantly, decreases the number of parallell connections we can deal
with on the server side (on win32 at least, not sure about win64).

Oh, didn't know that. Are the standard reg functions, open/query/close
really that bad? Can't be any worse than the security api or MAPI hell ;)

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

On Mon, Jun 28, 2010 at 21:10, Magnus Hagander <magnus@hagander.net> wrote:

On Mon, Jun 28, 2010 at 21:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

On Mon, Jun 28, 2010 at 20:45, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I vote for #2.  It's the least inconsistent --- we don't pay attention
to the registry for much of anything else, do we?

Directly, no? Indirectly, we do. For every other TCP parameter
(because the registry controls what we'll get as the default when we
"just use things")

Not if we make the code use the RFC values as the defaults.  I'm
envisioning the GUC assign hooks doing something like

#ifdef WIN32
       if (newval == 0)
               newval = RFC-specified-default;
#endif

Right. (I've only looked at the libpq side so far)

Also, we could avoid caling it *at all* if neither one of those
parameters is set. That'll take a bit more code (using the
unix-codepath of setsockopt() to enable keepalives at all), but it
shouldn't amount to many lines..

Here's what I'm thinking, for the libpq side. Similar change on the
server side. Seems ok?

(still http://github.com/mhagander/postgres/compare/master...win32keepalive
for those that prefer that interface)

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

Magnus Hagander <magnus@hagander.net> writes:

Here's what I'm thinking, for the libpq side. Similar change on the
server side. Seems ok?

I had in mind just legislating that the defaults are the RFC values,
none of this "try to use the registry values in one case" business.
I don't believe that you can make the server side act that way without
much more ugliness than is warranted. Also, at least on the libpq side
there is no backwards compatibility argument to be made, because we
never turned on keepalives at all on that side in previous releases.

regards, tom lane

On Mon, Jun 28, 2010 at 22:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

Here's what I'm thinking, for the libpq side. Similar change on the
server side. Seems ok?

I had in mind just legislating that the defaults are the RFC values,
none of this "try to use the registry values in one case" business.

Um, if you look at that patch, it doesn't try to use the registry. It
falls back directly to the system default, ignoring the registry. The
only special case is where the user doesn't specify any of the
parameters.

I don't believe that you can make the server side act that way without
much more ugliness than is warranted.  Also, at least on the libpq side
there is no backwards compatibility argument to be made, because we
never turned on keepalives at all on that side in previous releases.

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

Magnus Hagander <magnus@hagander.net> writes:

On Mon, Jun 28, 2010 at 22:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I had in mind just legislating that the defaults are the RFC values,
none of this "try to use the registry values in one case" business.

Um, if you look at that patch, it doesn't try to use the registry. It
falls back directly to the system default, ignoring the registry. The
only special case is where the user doesn't specify any of the
parameters.

What I was trying to say is I think we could dispense with the
setsockopt() code path, and just always use the WSAIoctl() path anytime
keepalives are turned on. I don't know what "system default values"
you're speaking of, if they're not the registry entries; and I
definitely don't see the point of consulting such values if they aren't
user-settable. We might as well just consult the RFCs and be done.

regards, tom lane

On Mon, Jun 28, 2010 at 8:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

On Mon, Jun 28, 2010 at 22:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I had in mind just legislating that the defaults are the RFC values,
none of this "try to use the registry values in one case" business.

Um, if you look at that patch, it doesn't try to use the registry. It
falls back directly to the system default, ignoring the registry. The
only special case is where the user doesn't specify any of the
parameters.

What I was trying to say is I think we could dispense with the
setsockopt() code path, and just always use the WSAIoctl() path anytime
keepalives are turned on.  I don't know what "system default values"
you're speaking of, if they're not the registry entries; and I
definitely don't see the point of consulting such values if they aren't
user-settable.  We might as well just consult the RFCs and be done.

FWIW, I think I prefer Magnus's approach, but I'm not 100% sure I can
defend that preference...

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

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Jun 28, 2010 at 8:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

What I was trying to say is I think we could dispense with the
setsockopt() code path, and just always use the WSAIoctl() path anytime
keepalives are turned on. �I don't know what "system default values"
you're speaking of, if they're not the registry entries; and I
definitely don't see the point of consulting such values if they aren't
user-settable. �We might as well just consult the RFCs and be done.

FWIW, I think I prefer Magnus's approach, but I'm not 100% sure I can
defend that preference...

Well, basically what I don't like about Magnus' proposal is that setting
one of the two values changes the default that will be used for the
other one. (Or, if it does not change the default, the extra code is
useless anyway.) If we just always go through the WSAIoctl() path then
we can clearly document "the default for this on Windows is so-and-so".
How is the documentation going to explain the behavior of the proposed
code?

regards, tom lane

Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Jun 28, 2010 at 8:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

What I was trying to say is I think we could dispense with the
setsockopt() code path, and just always use the WSAIoctl() path anytime
keepalives are turned on. ���I don't know what "system default values"
you're speaking of, if they're not the registry entries; and I
definitely don't see the point of consulting such values if they aren't
user-settable. ���We might as well just consult the RFCs and be done.

FWIW, I think I prefer Magnus's approach, but I'm not 100% sure I can
defend that preference...

Well, basically what I don't like about Magnus' proposal is that setting
one of the two values changes the default that will be used for the
other one. (Or, if it does not change the default, the extra code is
useless anyway.) If we just always go through the WSAIoctl() path then
we can clearly document "the default for this on Windows is so-and-so".
How is the documentation going to explain the behavior of the proposed
code?

Let's look at the usage probabilities. 99% of Win32 users will not use
any of these settings. I would hate to come up with a solution that
changes the default behavior for that 99%.

Therefore, I think using hard-coded defaults only for cases where
someone sets one but not all settings is appropriate. The documentation
text would be:

On Windows, if a keepalive settings is set, then defaults will be
used for any unset values, specifically, keepalives_idle (200) and
keepalives_interval(4). Windows does not allow control of
keepalives_count.

Seems simple enough.

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

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

Hello, Bruce.

You wrote:

BM> Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Jun 28, 2010 at 8:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

What I was trying to say is I think we could dispense with the
setsockopt() code path, and just always use the WSAIoctl() path anytime
keepalives are turned on. пїЅI don't know what "system default values"
you're speaking of, if they're not the registry entries; and I
definitely don't see the point of consulting such values if they aren't
user-settable. пїЅWe might as well just consult the RFCs and be done.

FWIW, I think I prefer Magnus's approach, but I'm not 100% sure I can
defend that preference...

Well, basically what I don't like about Magnus' proposal is that setting
one of the two values changes the default that will be used for the
other one. (Or, if it does not change the default, the extra code is
useless anyway.) If we just always go through the WSAIoctl() path then
we can clearly document "the default for this on Windows is so-and-so".
How is the documentation going to explain the behavior of the proposed
code?

BM> Let's look at the usage probabilities. 99% of Win32 users will not use
BM> any of these settings.

Let me disagree with this statement. As DAC developer I'm faced with
opposite reality. There are a lot of users demanding this
functionality.

BM> I would hate to come up with a solution that
BM> changes the default behavior for that 99%.

BM> Therefore, I think using hard-coded defaults only for cases where
BM> someone sets one but not all settings is appropriate. The documentation
BM> text would be:

BM> On Windows, if a keepalive settings is set, then defaults will be
BM> used for any unset values, specifically, keepalives_idle (200) and
BM> keepalives_interval(4). Windows does not allow control of
BM> keepalives_count.

BM> Seems simple enough.

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

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

--
With best wishes,
Pavel mailto:pavel@gf.microolap.com

2010/6/30 Pavel Golub <pavel@microolap.com>:

Hello, Bruce.

You wrote:

BM> Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Jun 28, 2010 at 8:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

What I was trying to say is I think we could dispense with the
setsockopt() code path, and just always use the WSAIoctl() path anytime
keepalives are turned on.  I don't know what "system default values"
you're speaking of, if they're not the registry entries; and I
definitely don't see the point of consulting such values if they aren't
user-settable.  We might as well just consult the RFCs and be done.

FWIW, I think I prefer Magnus's approach, but I'm not 100% sure I can
defend that preference...

Well, basically what I don't like about Magnus' proposal is that setting
one of the two values changes the default that will be used for the
other one.  (Or, if it does not change the default, the extra code is
useless anyway.)  If we just always go through the WSAIoctl() path then
we can clearly document "the default for this on Windows is so-and-so".
How is the documentation going to explain the behavior of the proposed
code?

BM> Let's look at the usage probabilities.  99% of Win32 users will not use
BM> any of these settings.

Let me disagree with this statement. As DAC developer I'm faced with
opposite reality. There are a lot of users demanding this
functionality.

It's very intersting to hear from somebody who expects to actually use
this. But are you aware that we're only talking about *adjusting* the
keepalive values, not enabling them? Because we will, as the code
stands now, enable keepalive by defaults - just use the system default
values for timeout intervals. (Meaning this is how we do it on Unix as
of HEAD, irregardless of my patch)

Do you have an opinion on the two choices for handling keepalives_idle
and keepalives_interval? They basically are:

1) When not configured, use system defaults. When only one of the two
parameters configured, use RFC default for the other one (overwrite
system default).

2) When not configured, use RFC defaults (overwrite system defaults).
When only one of the two parameters configured, use RFC default for
the other one (overwrite system default)

3) When not configured, use system defaults. When only one of the two
parameters configured, throw error.

I can see pros and cons with both. Given that I still think *most*
people will not configure the intervals at all, I think #1 is the one
that follows "principle of least surprise". Perhaps option *3* is the
one that does this for partial configuration?

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

Hello, Magnus.

You wrote:

MH> 2010/6/30 Pavel Golub <pavel@microolap.com>:

Hello, Bruce.

You wrote:

BM> Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Jun 28, 2010 at 8:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

What I was trying to say is I think we could dispense with the
setsockopt() code path, and just always use the WSAIoctl() path anytime
keepalives are turned on. �I don't know what "system default values"
you're speaking of, if they're not the registry entries; and I
definitely don't see the point of consulting such values if they aren't
user-settable. �We might as well just consult the RFCs and be done.

FWIW, I think I prefer Magnus's approach, but I'm not 100% sure I can
defend that preference...

Well, basically what I don't like about Magnus' proposal is that setting
one of the two values changes the default that will be used for the
other one. �(Or, if it does not change the default, the extra code is
useless anyway.) �If we just always go through the WSAIoctl() path then
we can clearly document "the default for this on Windows is so-and-so".
How is the documentation going to explain the behavior of the proposed
code?

BM> Let's look at the usage probabilities. �99% of Win32 users will not use
BM> any of these settings.

Let me disagree with this statement. As DAC developer I'm faced with
opposite reality. There are a lot of users demanding this
functionality.

MH> It's very intersting to hear from somebody who expects to actually use
MH> this. But are you aware that we're only talking about *adjusting* the
MH> keepalive values, not enabling them? Because we will, as the code
MH> stands now, enable keepalive by defaults - just use the system default
MH> values for timeout intervals. (Meaning this is how we do it on Unix as
MH> of HEAD, irregardless of my patch)

MH> Do you have an opinion on the two choices for handling keepalives_idle
MH> and keepalives_interval? They basically are:

MH> 1) When not configured, use system defaults. When only one of the two
MH> parameters configured, use RFC default for the other one (overwrite
MH> system default).

MH> 2) When not configured, use RFC defaults (overwrite system defaults).
MH> When only one of the two parameters configured, use RFC default for
MH> the other one (overwrite system default)

MH> 3) When not configured, use system defaults. When only one of the two
MH> parameters configured, throw error.

MH> I can see pros and cons with both. Given that I still think *most*
MH> people will not configure the intervals at all, I think #1 is the one
MH> that follows "principle of least surprise". Perhaps option *3* is the
MH> one that does this for partial configuration?

Frankly speaking I cannot decide what is the best approach. :) It's up
to you guys.

--
With best wishes,
Pavel mailto:pavel@gf.microolap.com

Magnus Hagander <magnus@hagander.net> writes:

Do you have an opinion on the two choices for handling keepalives_idle
and keepalives_interval? They basically are:

1) When not configured, use system defaults. When only one of the two
parameters configured, use RFC default for the other one (overwrite
system default).

2) When not configured, use RFC defaults (overwrite system defaults).
When only one of the two parameters configured, use RFC default for
the other one (overwrite system default)

3) When not configured, use system defaults. When only one of the two
parameters configured, throw error.

It's hard to argue about this when most of us have no idea what these
"system defaults" are, or whether they really are any different from the
RFC values in the first place, or whether ordinary users know how to
alter them or even find out their values. Please provide some
background if you want intelligent comments.

regards, tom lane

On Wed, Jun 30, 2010 at 16:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

Do you have an opinion on the two choices for handling keepalives_idle
and keepalives_interval? They basically are:

1) When not configured, use system defaults. When only one of the two
parameters configured, use RFC default for the other one (overwrite
system default).

2) When not configured, use RFC defaults (overwrite system defaults).
When only one of the two parameters configured, use RFC default for
the other one (overwrite system default)

3) When not configured, use system defaults. When only one of the two
parameters configured, throw error.

It's hard to argue about this when most of us have no idea what these
"system defaults" are, or whether they really are any different from the
RFC values in the first place, or whether ordinary users know how to
alter them or even find out their values.  Please provide some
background if you want intelligent comments.

The system defaults are whatever the user has configured at a machine
level (by editing the registry, by hand or by tool (including
policies)). I doubt many users have configured them by hand. There may
well be tools that do it for them.

Anyway, after some checking i realized #3 can't be implemented anyway
in the backend, since guc won't let us know early enough. So that's
out.

Thus, let's go with #2. Which was your suggestion :)

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