Re: [HACKERS] possible connection leak in dblink?
On 06/14/2011 07:41 PM, Fujii Masao wrote: On Wed, Jun 15, 2011 at 5:34 AM, Peter Eisentraut pete...@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 signature.asc Description: OpenPGP digital signature
Re: [HACKERS] possible connection leak in dblink?
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] possible connection leak in dblink?
Joe Conway wrote: -- Start of PGP signed section. On 06/17/2011 01:05 PM, Tom Lane wrote: Peter Eisentraut pete...@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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] possible connection leak in dblink?
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? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] possible connection leak in dblink?
Peter Eisentraut pete...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] possible connection leak in dblink?
On 06/17/2011 01:05 PM, Tom Lane wrote: Peter Eisentraut pete...@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 signature.asc Description: OpenPGP digital signature
[HACKERS] 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? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] possible connection leak in dblink?
On Wed, Jun 15, 2011 at 5:34 AM, Peter Eisentraut pete...@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
*** a/contrib/dblink/dblink.c
--- b/contrib/dblink/dblink.c
***
*** 613,628 Datum
dblink_send_query(PG_FUNCTION_ARGS)
{
PGconn *conn = NULL;
- char *connstr = NULL;
char *sql = NULL;
remoteConn *rconn = NULL;
- char *msg;
- bool freeconn = false;
int retval;
if (PG_NARGS() == 2)
{
! DBLINK_GET_CONN;
sql = text_to_cstring(PG_GETARG_TEXT_PP(1));
}
else
--- 613,625
dblink_send_query(PG_FUNCTION_ARGS)
{
PGconn *conn = NULL;
char *sql = NULL;
remoteConn *rconn = NULL;
int retval;
if (PG_NARGS() == 2)
{
! DBLINK_GET_NAMED_CONN;
sql = text_to_cstring(PG_GETARG_TEXT_PP(1));
}
else
***
*** 711,723 dblink_record_internal(FunctionCallInfo fcinfo, bool is_async)
if (PG_NARGS() == 2)
{
/* text,bool */
! DBLINK_GET_CONN;
fail = PG_GETARG_BOOL(1);
}
else if (PG_NARGS() == 1)
{
/* text */
! DBLINK_GET_CONN;
}
else
/* shouldn't happen */
--- 708,720
if (PG_NARGS() == 2)
{
/* text,bool */
! DBLINK_GET_NAMED_CONN;
fail = PG_GETARG_BOOL(1);
}
else if (PG_NARGS() == 1)
{
/* text */
! DBLINK_GET_NAMED_CONN;
}
else
/* shouldn't happen */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] possible connection leak in dblink?
On Wed, Jun 15, 2011 at 11:41, Fujii Masao masao.fu...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
