FAQ
With gcc 4.6, I get this warning:

dblink.c: In function ‘dblink_send_query’:
dblink.c:620:7: warning: variable ‘freeconn’ set but not used [-Wunused-but-set-variable]

I don't know much about the internals of dblink, but judging from the
surrounding code, I guess that this fix is necessary:

diff --git i/contrib/dblink/dblink.c w/contrib/dblink/dblink.c
index 19b98fb..e014c1a 100644
--- i/contrib/dblink/dblink.c
+++ w/contrib/dblink/dblink.c
@@ -634,6 +634,10 @@ dblink_send_query(PG_FUNCTION_ARGS)
if (retval != 1)
elog(NOTICE, "%s", PQerrorMessage(conn));

+ /* if needed, close the connection to the database and cleanup */
+ if (freeconn)
+ PQfinish(conn);
+
PG_RETURN_INT32(retval);
}

Otherwise the connection might not get freed. Could someone verify
that?

Search Discussions

  • Fujii Masao at Jun 15, 2011 at 2:41 am

    On Wed, Jun 15, 2011 at 5:34 AM, Peter Eisentraut wrote:
    With gcc 4.6, I get this warning:

    dblink.c: In function ‘dblink_send_query’:
    dblink.c:620:7: warning: variable ‘freeconn’ set but not used [-Wunused-but-set-variable]

    I don't know much about the internals of dblink, but judging from the
    surrounding code, I guess that this fix is necessary:

    diff --git i/contrib/dblink/dblink.c w/contrib/dblink/dblink.c
    index 19b98fb..e014c1a 100644
    --- i/contrib/dblink/dblink.c
    +++ w/contrib/dblink/dblink.c
    @@ -634,6 +634,10 @@ dblink_send_query(PG_FUNCTION_ARGS)
    if (retval != 1)
    elog(NOTICE, "%s", PQerrorMessage(conn));

    +       /* if needed, close the connection to the database and cleanup */
    +       if (freeconn)
    +               PQfinish(conn);
    +
    PG_RETURN_INT32(retval);
    }

    Otherwise the connection might not get freed.  Could someone verify
    that?
    ISTM that the root problem is that dblink_send_query calls DBLINK_GET_CONN
    though it doesn't accept the connection string as an argument. Since the first
    argument in dblink_send_query must be the connection name, dblink_send_query
    should call DBLINK_GET_NAMED_CONN instead. The variable 'freeconn' is used
    only when DBLINK_GET_CONN is called. So, if dblink_send_query uses
    DBLINK_GET_NAMED_CONN instead, the variable 'freeconn' is no longer necessary.

    The similar problem exists in dblink_get_result and dblink_record_internal.
    Attached patch fixes those problems.

    Regards,

    --
    Fujii Masao
    NIPPON TELEGRAPH AND TELEPHONE CORPORATION
    NTT Open Source Software Center
  • Itagaki Takahiro at Jun 15, 2011 at 2:53 am

    On Wed, Jun 15, 2011 at 11:41, Fujii Masao wrote:
    ISTM that the root problem is that dblink_send_query calls DBLINK_GET_CONN
    though it doesn't accept the connection string as an argument.
    +1 for the fix. I have the same conclusion at the first glance.
    Since the first
    argument in dblink_send_query must be the connection name, dblink_send_query
    should call DBLINK_GET_NAMED_CONN instead. The variable 'freeconn' is used
    only when DBLINK_GET_CONN is called. So, if dblink_send_query uses
    DBLINK_GET_NAMED_CONN instead, the variable 'freeconn' is no longer necessary.

    The similar problem exists in dblink_get_result and dblink_record_internal.
    Attached patch fixes those problems.
    --
    Itagaki Takahiro
  • Peter Eisentraut at Jun 17, 2011 at 7:21 pm

    On ons, 2011-06-15 at 11:41 +0900, Fujii Masao wrote:
    ISTM that the root problem is that dblink_send_query calls DBLINK_GET_CONN
    though it doesn't accept the connection string as an argument. Since the first
    argument in dblink_send_query must be the connection name, dblink_send_query
    should call DBLINK_GET_NAMED_CONN instead. The variable 'freeconn' is used
    only when DBLINK_GET_CONN is called. So, if dblink_send_query uses
    DBLINK_GET_NAMED_CONN instead, the variable 'freeconn' is no longer necessary.

    The similar problem exists in dblink_get_result and dblink_record_internal.
    Attached patch fixes those problems.
    Is this a bug fix that should be backpatched?
  • Tom Lane at Jun 17, 2011 at 8:05 pm

    Peter Eisentraut writes:
    Is this a bug fix that should be backpatched?
    I pinged Joe Conway about this. He is jetlagged from a trip to the Far
    East but promised to take care of it soon. I think we can wait for his
    review.

    regards, tom lane
  • Joe Conway at Jun 18, 2011 at 3:26 am

    On 06/17/2011 01:05 PM, Tom Lane wrote:
    Peter Eisentraut <peter_e@gmx.net> writes:
    Is this a bug fix that should be backpatched?
    I pinged Joe Conway about this. He is jetlagged from a trip to the Far
    East but promised to take care of it soon. I think we can wait for his
    review.
    Sorry for the delay. I'm finally caught up on most of my obligations,
    and have plowed through the 1500 or so pgsql mailing list messages that
    I was behind. But if everyone is OK with it I would like to aim to
    commit a fix mid next week, because I still have to get through my
    daughter's high school graduation tomorrow, and an out of state funeral
    for my father-in-law Sunday/Monday.

    That said, I really would like to commit this myself, as I have yet to
    be brave enough to commit anything under git :-(

    Joe

    --
    Joe Conway
    credativ LLC: http://www.credativ.us
    Linux, PostgreSQL, and general Open Source
    Training, Service, Consulting, & 24x7 Support
  • Bruce Momjian at Jun 20, 2011 at 5:24 pm
    Joe Conway wrote:
    -- Start of PGP signed section.
    On 06/17/2011 01:05 PM, Tom Lane wrote:
    Peter Eisentraut <peter_e@gmx.net> writes:
    Is this a bug fix that should be backpatched?
    I pinged Joe Conway about this. He is jetlagged from a trip to the Far
    East but promised to take care of it soon. I think we can wait for his
    review.
    Sorry for the delay. I'm finally caught up on most of my obligations,
    and have plowed through the 1500 or so pgsql mailing list messages that
    I was behind. But if everyone is OK with it I would like to aim to
    commit a fix mid next week, because I still have to get through my
    daughter's high school graduation tomorrow, and an out of state funeral
    for my father-in-law Sunday/Monday.

    That said, I really would like to commit this myself, as I have yet to
    be brave enough to commit anything under git :-(
    Sounds good. We will be here to help.

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

    + It's impossible for everything to be true. +
  • Joe Conway at Jun 25, 2011 at 8:36 pm

    On 06/14/2011 07:41 PM, Fujii Masao wrote:
    On Wed, Jun 15, 2011 at 5:34 AM, Peter Eisentraut wrote:
    Otherwise the connection might not get freed. Could someone verify
    that?
    ISTM that the root problem is that dblink_send_query calls DBLINK_GET_CONN
    though it doesn't accept the connection string as an argument. Since the first
    argument in dblink_send_query must be the connection name, dblink_send_query
    should call DBLINK_GET_NAMED_CONN instead. The variable 'freeconn' is used
    only when DBLINK_GET_CONN is called. So, if dblink_send_query uses
    DBLINK_GET_NAMED_CONN instead, the variable 'freeconn' is no longer necessary.

    The similar problem exists in dblink_get_result and dblink_record_internal.
    Attached patch fixes those problems.
    Fujii's assessment looks correct, although arguably the change is
    unnecessary for dblink_record_internal.

    Looks like the issue with dblink_send_query goes back through 8.4, while
    dblink_record_internal could be fixed as far back as 8.2.

    However, since this is really just a case of unused variables and not a
    leaked connection, I'm inclined to just fix git master -- comments on that?

    Joe

    --
    Joe Conway
    credativ LLC: http://www.credativ.us
    Linux, PostgreSQL, and general Open Source
    Training, Service, Consulting, & 24x7 Support
  • Peter Eisentraut at Jun 25, 2011 at 9:12 pm

    On lör, 2011-06-25 at 13:36 -0700, Joe Conway wrote:
    However, since this is really just a case of unused variables and not
    a leaked connection, I'm inclined to just fix git master -- comments
    on that?
    Please put it into 9.1 as well, so we can get a clean compile with gcc
    4.6 there.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedJun 14, '11 at 8:34p
activeJun 25, '11 at 9:12p
posts9
users6
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2021 Grokbase