Thread: possible connection leak in dblink?
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?
On Wed, Jun 15, 2011 at 5:34 AM, Peter Eisentraut <peter_e@gmx.net> 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
Attachment
On Wed, Jun 15, 2011 at 11:41, Fujii Masao <masao.fujii@gmail.com> 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
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?
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.
regards, tom lane
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
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. +
On 06/14/2011 07:41 PM, Fujii Masao wrote: > On Wed, Jun 15, 2011 at 5:34 AM, Peter Eisentraut <peter_e@gmx.net> 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
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.