Обсуждение: pq_setkeepalives* functions
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
Вложения
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
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
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 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;
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
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 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
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 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;
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
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
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