Re: [HACKERS] [COMMITTERS] pgsql: Add TCP keepalive support to libpq.

2010-06-25 Thread Robert Haas
On Thu, Jun 24, 2010 at 11:50 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Thu, Jun 24, 2010 at 1:38 PM, Simon Riggs si...@2ndquadrant.com wrote:
 On Thu, 2010-06-24 at 11:15 -0400, Tom Lane wrote:
 It might be a good idea to add a sentence to the documentation, though,
 just pointing out that these control client-side keepalive probes rather
 than server-side.

 Yes please. I was confused; I think others will be also.

 Do either of you have any thoughts about where would be the best place
 to add such a sentence?

 In the hunk you added to libpq.sgml would be fine by me.

Well, the trick is that the hunk that I added is really four separate
sections, and adding a whole sentence to each one seems like overkill.
 Here's an attempt at rewording the sections so that the information
is included in each parameter's description without taking up a whole
sentence.

Thoughts?

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


libpq-keepalive-docs.patch
Description: Binary data

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


Re: [HACKERS] [COMMITTERS] pgsql: Add TCP keepalive support to libpq.

2010-06-24 Thread Simon Riggs
On Wed, 2010-06-23 at 21:54 +, Robert Haas wrote:
 Log Message:
 ---
 Add TCP keepalive support to libpq.

I misunderstood the earlier discussion on this and didn't realise you
were considering committing in this way. For me, this is two patches,
not one. I object to one and like the other.

 This adds four additional connection parameters to libpq: keepalives,
 keepalives_idle, keepalives_count, and keepalives_interval.
 keepalives default to on, per discussion, but can be turned off by
 specifying keepalives=0.  The remaining parameters, where supported,
 can be used to adjust how often keepalives are sent and how many
 can be lost before the connection is broken.

There isn't any need at at all for this. We can already add options on
the libpq connection line.

options = '-o tcp_keepalives_idle=X
tcp_keepalives_interval=Y
tcp_keepalives_count=Z'

I see zero need to further complicate the libpq interface. If it changes
frequently between releases then supporting PostgreSQL programs becomes
much harder and leads to software not working correctly with Postgres.
Connecting to Postgres is already too complex.

At most it needs an example in the manual to show how to do this the
existing way.

I'm sorry to express this opinion now, again because I misunderstood.

 The immediate motivation for this patch is to make sure that
 walreceiver will eventually notice if the master reboots without
 closing the connection cleanly, but it should be helpful in other
 cases as well.

My understanding is that the motivation for this is that the above
options parameter would not have worked? So I regard making that work as
a bug fix, so agree with some parts of this patch, I think.

-- 
 Simon Riggs   www.2ndQuadrant.com


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


Re: [HACKERS] [COMMITTERS] pgsql: Add TCP keepalive support to libpq.

2010-06-24 Thread Tom Lane
Simon Riggs si...@2ndquadrant.com writes:
 On Wed, 2010-06-23 at 21:54 +, Robert Haas wrote:
 This adds four additional connection parameters to libpq: keepalives,
 keepalives_idle, keepalives_count, and keepalives_interval.
 keepalives default to on, per discussion, but can be turned off by
 specifying keepalives=0.  The remaining parameters, where supported,
 can be used to adjust how often keepalives are sent and how many
 can be lost before the connection is broken.

 There isn't any need at at all for this. We can already add options on
 the libpq connection line.

 options = '-o tcp_keepalives_idle=X
 tcp_keepalives_interval=Y
 tcp_keepalives_count=Z'

Huh?  The above is 100% fanciful; there was no code in libpq or anywhere
else that would have processed such a thing.

The bigger picture is that this patch is needed, not only for
walreceiver but for many other purposes --- the feature has been
requested repeatedly over the years and was already in the 9.1
commitfest queue.  We moved it up because it seemed fairly important for
walreceiver's purposes, but it would have gotten done in the very near
future anyway.

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


Re: [HACKERS] [COMMITTERS] pgsql: Add TCP keepalive support to libpq.

2010-06-24 Thread Robert Haas
On Thu, Jun 24, 2010 at 10:13 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Simon Riggs si...@2ndquadrant.com writes:
 On Wed, 2010-06-23 at 21:54 +, Robert Haas wrote:
 This adds four additional connection parameters to libpq: keepalives,
 keepalives_idle, keepalives_count, and keepalives_interval.
 keepalives default to on, per discussion, but can be turned off by
 specifying keepalives=0.  The remaining parameters, where supported,
 can be used to adjust how often keepalives are sent and how many
 can be lost before the connection is broken.

 There isn't any need at at all for this. We can already add options on
 the libpq connection line.

 options = '-o tcp_keepalives_idle=X
 tcp_keepalives_interval=Y
 tcp_keepalives_count=Z'

 Huh?  The above is 100% fanciful; there was no code in libpq or anywhere
 else that would have processed such a thing.

You can do this:

psql host=127.0.0.1 options='-c tcp_keepalives_idle=1'

...but it doesn't do the same thing as this patch.  It lets you set
the TCP keepalive parameters on the server side, whereas what this
patch does is let you set them on the client side.  Only setting them
on the client side will allow the client to notice when the server has
gone away.

There is still an open question in my mind as to whether this is
really an adequate solution to the walrecevier problem, but as you
say, if it turns out not to be, it's got other value.

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

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


Re: [HACKERS] [COMMITTERS] pgsql: Add TCP keepalive support to libpq.

2010-06-24 Thread Florian Pflug
On Jun 24, 2010, at 16:30 , Robert Haas wrote:
 On Thu, Jun 24, 2010 at 10:13 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Simon Riggs si...@2ndquadrant.com writes:
 On Wed, 2010-06-23 at 21:54 +, Robert Haas wrote:
 This adds four additional connection parameters to libpq: keepalives,
 keepalives_idle, keepalives_count, and keepalives_interval.
 keepalives default to on, per discussion, but can be turned off by
 specifying keepalives=0.  The remaining parameters, where supported,
 can be used to adjust how often keepalives are sent and how many
 can be lost before the connection is broken.
 
 There isn't any need at at all for this. We can already add options on
 the libpq connection line.
 
 options = '-o tcp_keepalives_idle=X
 tcp_keepalives_interval=Y
 tcp_keepalives_count=Z'
 
 Huh?  The above is 100% fanciful; there was no code in libpq or anywhere
 else that would have processed such a thing.
 
 You can do this:
 
 psql host=127.0.0.1 options='-c tcp_keepalives_idle=1'

Hm, seems a bit error-prone though. The difference between the above

psql host=127.0.0.1 keepalives=1

isn't immediately obvious I'd say.

Should we maybe rename the libpq-side parameters to tcp_client_keepalives, 
tcp_client_keepalives_idle, tcp_client_keepalives_count and 
tcp_client_keepalives_interval? Or do we expect people who fiddle with those 
parameters to understand the subtle difference?

best regards,
Florian Pflug


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


Re: [HACKERS] [COMMITTERS] pgsql: Add TCP keepalive support to libpq.

2010-06-24 Thread Robert Haas
On Thu, Jun 24, 2010 at 10:40 AM, Florian Pflug f...@phlo.org wrote:
 On Jun 24, 2010, at 16:30 , Robert Haas wrote:
 On Thu, Jun 24, 2010 at 10:13 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Simon Riggs si...@2ndquadrant.com writes:
 There isn't any need at at all for this. We can already add options on
 the libpq connection line.

 options = '-o tcp_keepalives_idle=X
 tcp_keepalives_interval=Y
 tcp_keepalives_count=Z'

 Huh?  The above is 100% fanciful; there was no code in libpq or anywhere
 else that would have processed such a thing.

 You can do this:

 psql host=127.0.0.1 options='-c tcp_keepalives_idle=1'

 Hm, seems a bit error-prone though. The difference between the above

 psql host=127.0.0.1 keepalives=1

 isn't immediately obvious I'd say.

 Should we maybe rename the libpq-side parameters to tcp_client_keepalives, 
 tcp_client_keepalives_idle, tcp_client_keepalives_count and 
 tcp_client_keepalives_interval? Or do we expect people who fiddle with those 
 parameters to understand the subtle difference?

I think the existing names are fine - people should understand that
options means server-side options and that anything else is a
client-side option.  However, if there's a strong consensus the other
way and someone feels like working up a patch, that's fine too.

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

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


Re: [HACKERS] [COMMITTERS] pgsql: Add TCP keepalive support to libpq.

2010-06-24 Thread Florian Pflug
On Jun 24, 2010, at 16:45 , Robert Haas wrote:
 On Thu, Jun 24, 2010 at 10:40 AM, Florian Pflug f...@phlo.org wrote:
 On Jun 24, 2010, at 16:30 , Robert Haas wrote:
 On Thu, Jun 24, 2010 at 10:13 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Simon Riggs si...@2ndquadrant.com writes:
 There isn't any need at at all for this. We can already add options on
 the libpq connection line.
 
 options = '-o tcp_keepalives_idle=X
 tcp_keepalives_interval=Y
 tcp_keepalives_count=Z'
 
 Huh?  The above is 100% fanciful; there was no code in libpq or anywhere
 else that would have processed such a thing.
 
 You can do this:
 
 psql host=127.0.0.1 options='-c tcp_keepalives_idle=1'
 
 Hm, seems a bit error-prone though. The difference between the above
 
 psql host=127.0.0.1 keepalives=1
 
 isn't immediately obvious I'd say.
 
 Should we maybe rename the libpq-side parameters to tcp_client_keepalives, 
 tcp_client_keepalives_idle, tcp_client_keepalives_count and 
 tcp_client_keepalives_interval? Or do we expect people who fiddle with those 
 parameters to understand the subtle difference?
 
 I think the existing names are fine - people should understand that
 options means server-side options and that anything else is a
 client-side option.  However, if there's a strong consensus the other
 way and someone feels like working up a patch, that's fine too.

I'd volunteer to create the patch if people think renaming the libpq options is 
a good idea.

best regards,
Florian Pflug


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


Re: [HACKERS] [COMMITTERS] pgsql: Add TCP keepalive support to libpq.

2010-06-24 Thread Tom Lane
Florian Pflug f...@phlo.org writes:
 On Jun 24, 2010, at 16:45 , Robert Haas wrote:
 I think the existing names are fine - people should understand that
 options means server-side options and that anything else is a
 client-side option.  However, if there's a strong consensus the other
 way and someone feels like working up a patch, that's fine too.

 I'd volunteer to create the patch if people think renaming the libpq options 
 is a good idea.

I'm with Robert: the names are fine as-is.  We've not had complaints
about the libpq SSL parameters being confusingly like server-side SSL
parameters, for instance.

It might be a good idea to add a sentence to the documentation, though,
just pointing out that these control client-side keepalive probes rather
than server-side.

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


Re: [HACKERS] [COMMITTERS] pgsql: Add TCP keepalive support to libpq.

2010-06-24 Thread Simon Riggs
On Thu, 2010-06-24 at 11:15 -0400, Tom Lane wrote:
 Florian Pflug f...@phlo.org writes:
  On Jun 24, 2010, at 16:45 , Robert Haas wrote:
  I think the existing names are fine - people should understand that
  options means server-side options and that anything else is a
  client-side option.  However, if there's a strong consensus the other
  way and someone feels like working up a patch, that's fine too.
 
  I'd volunteer to create the patch if people think renaming the libpq 
  options is a good idea.
 
 I'm with Robert: the names are fine as-is.  We've not had complaints
 about the libpq SSL parameters being confusingly like server-side SSL
 parameters, for instance.
 
 It might be a good idea to add a sentence to the documentation, though,
 just pointing out that these control client-side keepalive probes rather
 than server-side.

Yes please. I was confused; I think others will be also.

-- 
 Simon Riggs   www.2ndQuadrant.com



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


Re: [HACKERS] [COMMITTERS] pgsql: Add TCP keepalive support to libpq.

2010-06-24 Thread Simon Riggs
On Thu, 2010-06-24 at 10:30 -0400, Robert Haas wrote:

 It lets you set
 the TCP keepalive parameters on the server side, whereas what this
 patch does is let you set them on the client side.  Only setting them
 on the client side will allow the client to notice when the server has
 gone away.

Thank you for explaining. Now I understand.

It sort of begs the question: why need they be different?

-- 
 Simon Riggs   www.2ndQuadrant.com


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


Re: [HACKERS] [COMMITTERS] pgsql: Add TCP keepalive support to libpq.

2010-06-24 Thread Robert Haas
On Thu, Jun 24, 2010 at 1:40 PM, Simon Riggs si...@2ndquadrant.com wrote:
 On Thu, 2010-06-24 at 10:30 -0400, Robert Haas wrote:

 It lets you set
 the TCP keepalive parameters on the server side, whereas what this
 patch does is let you set them on the client side.  Only setting them
 on the client side will allow the client to notice when the server has
 gone away.

 Thank you for explaining. Now I understand.

 It sort of begs the question: why need they be different?

In most cases, they probably don't; in fact, I suspect most people
don't bother adjusting these parameters at all.  Essentially, they
just control the amount of time that can pass before you decide that
the other guy is dead.  And if the client thinks it's right to declare
the server dead after X minutes, it's probably reasonable for the
server to declare the client dead after X minutes, too.  On the other
hand, the client and server may be under different administrative
control.  What the DBA wants the database to do to avoid running out
of connection slots might not be the same as what the application
writer wants to do to detect when the database has crashed.  For that
exact reason, it's actually slightly strange that we make these
parameters PGC_USERSET on the server side - but not strange enough
that I can get excited about changing it.  If an application writer
wants to make trouble for the DBA, he certainly has better ways to do
it than this...

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

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


Re: [HACKERS] [COMMITTERS] pgsql: Add TCP keepalive support to libpq.

2010-06-24 Thread Robert Haas
On Thu, Jun 24, 2010 at 1:38 PM, Simon Riggs si...@2ndquadrant.com wrote:
 On Thu, 2010-06-24 at 11:15 -0400, Tom Lane wrote:
 Florian Pflug f...@phlo.org writes:
  On Jun 24, 2010, at 16:45 , Robert Haas wrote:
  I think the existing names are fine - people should understand that
  options means server-side options and that anything else is a
  client-side option.  However, if there's a strong consensus the other
  way and someone feels like working up a patch, that's fine too.

  I'd volunteer to create the patch if people think renaming the libpq 
  options is a good idea.

 I'm with Robert: the names are fine as-is.  We've not had complaints
 about the libpq SSL parameters being confusingly like server-side SSL
 parameters, for instance.

 It might be a good idea to add a sentence to the documentation, though,
 just pointing out that these control client-side keepalive probes rather
 than server-side.

 Yes please. I was confused; I think others will be also.

Do either of you have any thoughts about where would be the best place
to add such a sentence?

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

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


Re: [HACKERS] [COMMITTERS] pgsql: Add TCP keepalive support to libpq.

2010-06-24 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Thu, Jun 24, 2010 at 1:38 PM, Simon Riggs si...@2ndquadrant.com wrote:
 On Thu, 2010-06-24 at 11:15 -0400, Tom Lane wrote:
 It might be a good idea to add a sentence to the documentation, though,
 just pointing out that these control client-side keepalive probes rather
 than server-side.
 
 Yes please. I was confused; I think others will be also.

 Do either of you have any thoughts about where would be the best place
 to add such a sentence?

In the hunk you added to libpq.sgml would be fine by me.

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