[bug fix] dblink leaks unnamed connections

Started by Tsunakawa, Takayukialmost 9 years ago5 messages
#1Tsunakawa, Takayuki
tsunakawa.takay@jp.fujitsu.com
1 attachment(s)

Hello,

dblink fails to close the unnamed connection as follows when a new unnamed connection is requested. The attached patch fixes this.

postgres=# select count(*) from pg_stat_activity;
count
-------
1
(1 row)

postgres=# select dblink_connect('dbname=postgres');
dblink_connect
----------------
OK
(1 row)

postgres=# select count(*) from pg_stat_activity;
count
-------
2
(1 row)

postgres=# select dblink_connect('dbname=postgres');
dblink_connect
----------------
OK
(1 row)

postgres=# select count(*) from pg_stat_activity;
count
-------
3
(1 row)

Regards
Takayuki Tsunakawa

Attachments:

dblink_leak_unnamed_conn.patchapplication/octet-stream; name=dblink_leak_unnamed_conn.patchDownload
diff -rc a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
*** a/contrib/dblink/dblink.c	Wed Mar  8 15:46:00 2017
--- b/contrib/dblink/dblink.c	Wed Mar  8 15:46:18 2017
***************
*** 276,281 ****
--- 276,286 ----
  
  	/* check password in connection string if not superuser */
  	dblink_connstr_check(connstr);
+ 	if (!connname && pconn->conn)
+ 	{
+ 		PQfinish(pconn->conn);
+ 		pconn->conn = NULL;
+ 	}
  	conn = PQconnectdb(connstr);
  
  	if (PQstatus(conn) == CONNECTION_BAD)
#2Fujii Masao
masao.fujii@gmail.com
In reply to: Tsunakawa, Takayuki (#1)
Re: [bug fix] dblink leaks unnamed connections

On Wed, Mar 8, 2017 at 3:52 PM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:

Hello,

dblink fails to close the unnamed connection as follows when a new unnamed connection is requested. The attached patch fixes this.

This issue was reported about ten years ago and added as TODO item.
http://archives.postgresql.org/pgsql-hackers/2007-10/msg00895.php

I agree that this is a bug, and am tempted to back-patch to all the supported
versions. But it had not been fixed in many years since the first report of
the issue. So I'm not sure if it's ok to just treat this as a bug right now and
back-patch. Or we should fix this only in HEAD? Anyway I'd like to hear
more opinions about this.

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Fujii Masao (#2)
Re: [bug fix] dblink leaks unnamed connections

Fujii Masao <masao.fujii@gmail.com> writes:

On Wed, Mar 8, 2017 at 3:52 PM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:

dblink fails to close the unnamed connection as follows when a new unnamed connection is requested. The attached patch fixes this.

This issue was reported about ten years ago and added as TODO item.
http://archives.postgresql.org/pgsql-hackers/2007-10/msg00895.php

I agree that this is a bug, and am tempted to back-patch to all the supported
versions. But it had not been fixed in many years since the first report of
the issue. So I'm not sure if it's ok to just treat this as a bug right now and
back-patch. Or we should fix this only in HEAD? Anyway I'd like to hear
more opinions about this.

It looks to me like the issue simply fell through the cracks because Joe
wasn't excited about fixing it. Now that we have a second complaint,
I think it's worth treating as a bug and back-patching.

(I've not read this particular patch and am not expressing an opinion
whether it's correct.)

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

#4Joe Conway
mail@joeconway.com
In reply to: Tom Lane (#3)
Re: [bug fix] dblink leaks unnamed connections

On 03/09/2017 07:54 AM, Tom Lane wrote:

Fujii Masao <masao.fujii@gmail.com> writes:

On Wed, Mar 8, 2017 at 3:52 PM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:

dblink fails to close the unnamed connection as follows when a new unnamed connection is requested. The attached patch fixes this.

This issue was reported about ten years ago and added as TODO item.
http://archives.postgresql.org/pgsql-hackers/2007-10/msg00895.php

I agree that this is a bug, and am tempted to back-patch to all the supported
versions. But it had not been fixed in many years since the first report of
the issue. So I'm not sure if it's ok to just treat this as a bug right now and
back-patch. Or we should fix this only in HEAD? Anyway I'd like to hear
more opinions about this.

It looks to me like the issue simply fell through the cracks because Joe
wasn't excited about fixing it. Now that we have a second complaint,
I think it's worth treating as a bug and back-patching.

(I've not read this particular patch and am not expressing an opinion
whether it's correct.)

Ok, will take another look.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

#5Joe Conway
mail@joeconway.com
In reply to: Joe Conway (#4)
Re: [bug fix] dblink leaks unnamed connections

On 03/09/2017 08:31 AM, Joe Conway wrote:

On 03/09/2017 07:54 AM, Tom Lane wrote:

Fujii Masao <masao.fujii@gmail.com> writes:

On Wed, Mar 8, 2017 at 3:52 PM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:

dblink fails to close the unnamed connection as follows when a new unnamed connection is requested. The attached patch fixes this.

This issue was reported about ten years ago and added as TODO item.
http://archives.postgresql.org/pgsql-hackers/2007-10/msg00895.php

I agree that this is a bug, and am tempted to back-patch to all the supported
versions. But it had not been fixed in many years since the first report of
the issue. So I'm not sure if it's ok to just treat this as a bug right now and
back-patch. Or we should fix this only in HEAD? Anyway I'd like to hear
more opinions about this.

It looks to me like the issue simply fell through the cracks because Joe
wasn't excited about fixing it. Now that we have a second complaint,
I think it's worth treating as a bug and back-patching.

(I've not read this particular patch and am not expressing an opinion
whether it's correct.)

Ok, will take another look.

I pushed a fix to all supported branches.

Thanks,

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development