pq_setkeepalives* functions

Started by Jaime Casanovaalmost 16 years ago11 messages

jcasanov@systemguards.com.ec

almost 16 years ago

1 attachment(s)

Hi,

In 2 of 3 pq_setkeepalives* functions we have the #DEFINE involving
even this conditional:
"""
if (port == NULL || IS_AF_UNIX(port->laddr.addr.ss_family))
return STATUS_OK;
"""

but in pq_setkeepalivesinterval() the #DEFINE is after the
conditional, doesn't seems to affect anything but for consistency with
the other two :)

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

Attachments:

tcpkeep.patchtext/x-patch; charset=US-ASCII; name=tcpkeep.patchDownload

Index: src/backend/libpq/pqcomm.c
===================================================================
RCS file: /home/postgres/pgrepo/pgsql/src/backend/libpq/pqcomm.c,v
retrieving revision 1.205
diff -c -r1.205 pqcomm.c
*** src/backend/libpq/pqcomm.c	26 Feb 2010 02:00:43 -0000	1.205
--- src/backend/libpq/pqcomm.c	13 Mar 2010 04:39:19 -0000
***************
*** 1418,1427 ****
  int
  pq_setkeepalivesinterval(int interval, Port *port)
  {
  	if (port == NULL || IS_AF_UNIX(port->laddr.addr.ss_family))
  		return STATUS_OK;
  
- #ifdef TCP_KEEPINTVL
  	if (interval == port->keepalives_interval)
  		return STATUS_OK;
  
--- 1418,1427 ----
  int
  pq_setkeepalivesinterval(int interval, Port *port)
  {
+ #ifdef TCP_KEEPINTVL
  	if (port == NULL || IS_AF_UNIX(port->laddr.addr.ss_family))
  		return STATUS_OK;
  
  	if (interval == port->keepalives_interval)
  		return STATUS_OK;

Bruce Momjian

bruce@momjian.us

almost 16 years ago

In reply to: Jaime Casanova (#1)

Re: pq_setkeepalives* functions

Jaime Casanova wrote:

Hi,

In 2 of 3 pq_setkeepalives* functions we have the #DEFINE involving
even this conditional:
"""
if (port == NULL || IS_AF_UNIX(port->laddr.addr.ss_family))
return STATUS_OK;
"""

but in pq_setkeepalivesinterval() the #DEFINE is after the
conditional, doesn't seems to affect anything but for consistency with
the other two :)

Thanks, applied.

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

PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do

Tom Lane

tgl@sss.pgh.pa.us

almost 16 years ago

In reply to: Bruce Momjian (#2)

Re: pq_setkeepalives* functions

Bruce Momjian <bruce@momjian.us> writes:

Jaime Casanova wrote:

but in pq_setkeepalivesinterval() the #DEFINE is after the
conditional, doesn't seems to affect anything but for consistency with
the other two :)

Thanks, applied.

This makes the function *not* like the other two, rather than
improving consistency.

regards, tom lane

Bruce Momjian

bruce@momjian.us

almost 16 years ago

In reply to: Tom Lane (#3)

1 attachment(s)

Re: pq_setkeepalives* functions

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

Jaime Casanova wrote:

but in pq_setkeepalivesinterval() the #DEFINE is after the
conditional, doesn't seems to affect anything but for consistency with
the other two :)

Thanks, applied.

This makes the function *not* like the other two, rather than
improving consistency.

Well, it makes it like some of the existing functions, but not like
others. This applied patch fixes them all.

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

PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do

Attachments:

/rtmp/difftext/x-diffDownload

Index: src/backend/libpq/pqcomm.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/libpq/pqcomm.c,v
retrieving revision 1.206
diff -c -c -r1.206 pqcomm.c
*** src/backend/libpq/pqcomm.c	13 Mar 2010 15:35:46 -0000	1.206
--- src/backend/libpq/pqcomm.c	13 Mar 2010 16:38:57 -0000
***************
*** 1346,1355 ****
  int
  pq_setkeepalivesidle(int idle, Port *port)
  {
  	if (port == NULL || IS_AF_UNIX(port->laddr.addr.ss_family))
  		return STATUS_OK;
  
- #ifdef TCP_KEEPIDLE
  	if (idle == port->keepalives_idle)
  		return STATUS_OK;
  
--- 1346,1355 ----
  int
  pq_setkeepalivesidle(int idle, Port *port)
  {
+ #ifdef TCP_KEEPIDLE
  	if (port == NULL || IS_AF_UNIX(port->laddr.addr.ss_family))
  		return STATUS_OK;
  
  	if (idle == port->keepalives_idle)
  		return STATUS_OK;
  
***************
*** 1490,1499 ****
  int
  pq_setkeepalivescount(int count, Port *port)
  {
  	if (port == NULL || IS_AF_UNIX(port->laddr.addr.ss_family))
  		return STATUS_OK;
  
- #ifdef TCP_KEEPCNT
  	if (count == port->keepalives_count)
  		return STATUS_OK;
  
--- 1490,1499 ----
  int
  pq_setkeepalivescount(int count, Port *port)
  {
+ #ifdef TCP_KEEPCNT
  	if (port == NULL || IS_AF_UNIX(port->laddr.addr.ss_family))
  		return STATUS_OK;
  
  	if (count == port->keepalives_count)
  		return STATUS_OK;

Tom Lane

tgl@sss.pgh.pa.us

almost 16 years ago

In reply to: Bruce Momjian (#4)

Re: pq_setkeepalives* functions

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

This makes the function *not* like the other two, rather than
improving consistency.

Well, it makes it like some of the existing functions, but not like
others. This applied patch fixes them all.

This is making things worse, not better. You have just changed the
behavior, and not in a good way. Formerly these were no-ops on
a unix socket connection, and now they can throw errors.

regards, tom lane

Bruce Momjian

bruce@momjian.us

almost 16 years ago

In reply to: Tom Lane (#5)

Re: pq_setkeepalives* functions

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

This makes the function *not* like the other two, rather than
improving consistency.

Well, it makes it like some of the existing functions, but not like
others. This applied patch fixes them all.

This is making things worse, not better. You have just changed the
behavior, and not in a good way. Formerly these were no-ops on
a unix socket connection, and now they can throw errors.

OK, reverted. Let me study the entire file.

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

PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do

Bruce Momjian

bruce@momjian.us

almost 16 years ago

In reply to: Bruce Momjian (#6)

Re: pq_setkeepalives* functions

Bruce Momjian wrote:

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

This makes the function *not* like the other two, rather than
improving consistency.

Well, it makes it like some of the existing functions, but not like
others. This applied patch fixes them all.

This is making things worse, not better. You have just changed the
behavior, and not in a good way. Formerly these were no-ops on
a unix socket connection, and now they can throw errors.

OK, reverted. Let me study the entire file.

Clarification, both changes reverted.

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

PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do

Bruce Momjian

bruce@momjian.us

almost 16 years ago

In reply to: Tom Lane (#5)

1 attachment(s)

Re: pq_setkeepalives* functions

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

This makes the function *not* like the other two, rather than
improving consistency.

Well, it makes it like some of the existing functions, but not like
others. This applied patch fixes them all.

This is making things worse, not better. You have just changed the
behavior, and not in a good way. Formerly these were no-ops on
a unix socket connection, and now they can throw errors.

Is this the proper way to fix the issue? Patch attached.

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

PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do

Attachments:

/pgpatches/keepalivetext/x-diffDownload

Index: src/backend/libpq/pqcomm.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/libpq/pqcomm.c,v
retrieving revision 1.208
diff -c -c -r1.208 pqcomm.c
*** src/backend/libpq/pqcomm.c	13 Mar 2010 16:56:37 -0000	1.208
--- src/backend/libpq/pqcomm.c	13 Mar 2010 17:01:14 -0000
***************
*** 1317,1326 ****
  int
  pq_getkeepalivesidle(Port *port)
  {
- #ifdef TCP_KEEPIDLE
  	if (port == NULL || IS_AF_UNIX(port->laddr.addr.ss_family))
  		return 0;
  
  	if (port->keepalives_idle != 0)
  		return port->keepalives_idle;
  
--- 1317,1326 ----
  int
  pq_getkeepalivesidle(Port *port)
  {
  	if (port == NULL || IS_AF_UNIX(port->laddr.addr.ss_family))
  		return 0;
  
+ #ifdef TCP_KEEPIDLE
  	if (port->keepalives_idle != 0)
  		return port->keepalives_idle;
  
***************
*** 1389,1398 ****
  int
  pq_getkeepalivesinterval(Port *port)
  {
- #ifdef TCP_KEEPINTVL
  	if (port == NULL || IS_AF_UNIX(port->laddr.addr.ss_family))
  		return 0;
  
  	if (port->keepalives_interval != 0)
  		return port->keepalives_interval;
  
--- 1389,1398 ----
  int
  pq_getkeepalivesinterval(Port *port)
  {
  	if (port == NULL || IS_AF_UNIX(port->laddr.addr.ss_family))
  		return 0;
  
+ #ifdef TCP_KEEPINTVL
  	if (port->keepalives_interval != 0)
  		return port->keepalives_interval;
  
***************
*** 1461,1470 ****
  int
  pq_getkeepalivescount(Port *port)
  {
- #ifdef TCP_KEEPCNT
  	if (port == NULL || IS_AF_UNIX(port->laddr.addr.ss_family))
  		return 0;
  
  	if (port->keepalives_count != 0)
  		return port->keepalives_count;
  
--- 1461,1470 ----
  int
  pq_getkeepalivescount(Port *port)
  {
  	if (port == NULL || IS_AF_UNIX(port->laddr.addr.ss_family))
  		return 0;
  
+ #ifdef TCP_KEEPCNT
  	if (port->keepalives_count != 0)
  		return port->keepalives_count;

Tom Lane

tgl@sss.pgh.pa.us

almost 16 years ago

In reply to: Bruce Momjian (#8)

Re: pq_setkeepalives* functions

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

This is making things worse, not better. You have just changed the
behavior, and not in a good way. Formerly these were no-ops on
a unix socket connection, and now they can throw errors.

Is this the proper way to fix the issue? Patch attached.

AFAICS there is no issue, and the code is fine as-is.

Modifying the "get" functions as you propose would be harmless, but it's
also not an improvement, since it would result in redundant code in the
functions when those macros aren't defined.

I don't see any real advantage in making the set and get functions
look slightly more alike. They're doing different things.

regards, tom lane

#10

Bruce Momjian

bruce@momjian.us

almost 16 years ago

In reply to: Tom Lane (#9)

Re: pq_setkeepalives* functions

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

This is making things worse, not better. You have just changed the
behavior, and not in a good way. Formerly these were no-ops on
a unix socket connection, and now they can throw errors.

Is this the proper way to fix the issue? Patch attached.

AFAICS there is no issue, and the code is fine as-is.

Modifying the "get" functions as you propose would be harmless, but it's
also not an improvement, since it would result in redundant code in the
functions when those macros aren't defined.

I don't see any real advantage in making the set and get functions
look slightly more alike. They're doing different things.

OK, thanks for the analysis.

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

PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do

#11

Jaime Casanova

jcasanov@systemguards.com.ec

almost 16 years ago

In reply to: Tom Lane (#9)

Re: pq_setkeepalives* functions

On Sat, Mar 13, 2010 at 12:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

This is making things worse, not better. You have just changed the
behavior, and not in a good way. Formerly these were no-ops on
a unix socket connection, and now they can throw errors.

Is this the proper way to fix the issue? Patch attached.

AFAICS there is no issue, and the code is fine as-is.

ah! now i see this clearer... sorry for the noise

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157