Lists: | pgsql-hackers |
---|
From: | Peter Eisentraut <peter_e(at)gmx(dot)net> |
---|---|
To: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | possible connection leak in dblink? |
Date: | 2011-06-14 20:34:09 |
Message-ID: | 1308083649.29840.5.camel@vanquo.pezone.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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?
From: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com> |
---|---|
To: | Peter Eisentraut <peter_e(at)gmx(dot)net> |
Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: possible connection leak in dblink? |
Date: | 2011-06-15 02:41:18 |
Message-ID: | BANLkTinOnN5JpEXKSEg3XUJj1PeZu-gU-w@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
On Wed, Jun 15, 2011 at 5:34 AM, Peter Eisentraut <peter_e(at)gmx(dot)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 | Content-Type | Size |
---|---|---|
dblink_bugfix_v1.patch | text/x-patch | 1.3 KB |
From: | Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> |
---|---|
To: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com> |
Cc: | Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: possible connection leak in dblink? |
Date: | 2011-06-15 02:53:16 |
Message-ID: | BANLkTikUyAQRqgA6uWxRZ3TvyhjGzZkP-A@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
On Wed, Jun 15, 2011 at 11:41, Fujii Masao <masao(dot)fujii(at)gmail(dot)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
From: | Peter Eisentraut <peter_e(at)gmx(dot)net> |
---|---|
To: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com> |
Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: possible connection leak in dblink? |
Date: | 2011-06-17 19:21:01 |
Message-ID: | 1308338461.16852.0.camel@vanquo.pezone.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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?
From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Peter Eisentraut <peter_e(at)gmx(dot)net> |
Cc: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: possible connection leak in dblink? |
Date: | 2011-06-17 20:05:07 |
Message-ID: | 23310.1308341107@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
Peter Eisentraut <peter_e(at)gmx(dot)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
From: | Joe Conway <mail(at)joeconway(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Peter Eisentraut <peter_e(at)gmx(dot)net>, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: possible connection leak in dblink? |
Date: | 2011-06-18 03:25:54 |
Message-ID: | 4DFC1AC2.6020409@joeconway.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
On 06/17/2011 01:05 PM, Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)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
From: | Bruce Momjian <bruce(at)momjian(dot)us> |
---|---|
To: | Joe Conway <mail(at)joeconway(dot)com> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Peter Eisentraut <peter_e(at)gmx(dot)net>, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: possible connection leak in dblink? |
Date: | 2011-06-20 17:24:38 |
Message-ID: | 201106201724.p5KHOcb09708@momjian.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
Joe Conway wrote:
-- Start of PGP signed section.
> On 06/17/2011 01:05 PM, Tom Lane wrote:
> > Peter Eisentraut <peter_e(at)gmx(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
From: | Joe Conway <mail(at)joeconway(dot)com> |
---|---|
To: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com> |
Cc: | Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: possible connection leak in dblink? |
Date: | 2011-06-25 20:36:33 |
Message-ID: | 4E0646D1.7070409@joeconway.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
On 06/14/2011 07:41 PM, Fujii Masao wrote:
> On Wed, Jun 15, 2011 at 5:34 AM, Peter Eisentraut <peter_e(at)gmx(dot)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
From: | Peter Eisentraut <peter_e(at)gmx(dot)net> |
---|---|
To: | Joe Conway <mail(at)joeconway(dot)com> |
Cc: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: possible connection leak in dblink? |
Date: | 2011-06-25 21:11:48 |
Message-ID: | 1309036308.23159.0.camel@vanquo.pezone.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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.