Re: [HACKERS] [bug fix] Memory leak in dblink
From: Tom Lane t...@sss.pgh.pa.us In the first query, the MemoryContextReset is nearly free since there's nothing to free (and we've special-cased that path in aset.c). It's certainly a measurement artifact that it measures out faster, which says to me that these numbers can only be trusted within a couple percent; but at least we're not taking much hit in cases where the patch isn't actually conferring any benefit. For the second query, losing 1% to avoid memory bloat seems well worthwhile. Barring objections I'll apply and back-patch this. So this patch would solve memory leak issues if other modules had similar bugs, in addition to the original dblink problem, wouldn't this? Definitely +1. The original OP wants to use 9.2. I'll report to him when you've committed this nce patch. Thanks, Tom san. Regards MauMau -- 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] [bug fix] Memory leak in dblink
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/19/2014 03:38 PM, MauMau wrote: From: Joe Conway m...@joeconway.com Fair enough -- this patch does it at that level in materializeQueryResult() I'm in favor of this. TBH, I did this at first, but I was afraid this would be rejected due to the reason mentioned earlier. if statement in PG_TRY block is not necessary like this, because sinfo is zero-cleared. Right -- pushed that way, back-patched through 9.2 Joe - -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 Support -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTpIv6AAoJEDfy90M199hlQtIP/ilpExgyHa86DaMCej4+oRI/ nO06AujnF+wcw/xre8xpu3sYCLebBnSbmKlqv17ry+mPfWD2KsrwMnqFgm2UoQGQ zDB/fpz4ALfQHlWrdvBqKd/J2IcdtuWaS9xi0kqShuSXWWm4XMaUIZ+gxDAA+x4U OXRumrv+kipHTONwMporZfby5DPtaRLpuV/o4ioOB4elb2VQAlajR5Vpmguhihdf A6exIN6dHIKTT2jNUHfkqnd7bZ2anVaohuociM/j+JwKaRct9K+anR3bokfLKLW9 l/OQ+BHzLBDz/Pi7GB/ImmkrIKL33phbhCWWPN1nkD6OYfYohkYvl+aixZ+kvUav wEBXTUkkJkuIL4at/5v4jbDDlPXAaYdBmGLQ/riwAOzISq2weAqcAQqhidJUbY1a JRSgojNHbo4v8IfjEj3BMRmPlKhY6Z/eMELr2Yi+K+54Hk4Fy+UDaatyDpEo2iwm cx2auCWBaT5SzI+KbebO0WZNhSrt7m1OEWN2tmPyTsnPsGg7I1oyQ/UtybaNjGtD G16HIfe12wca9Sm8zu+ypnxl3gUeku5KB0ZtigNwMxZSJXm/qa4kZqGn1RoItDnh kcfk8LTGNR1xMrPCiD+rUHyY573g8WK1XpdTWDBER29vgETdaswqXDeWsoPWmX/3 KICzxLurjqvyiJW9L4O+ =6It5 -END PGP SIGNATURE- -- 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] [bug fix] Memory leak in dblink
From: Joe Conway m...@joeconway.com I think the context deletion was missed in the first place because storeRow() is the wrong place to create the context. Rather than creating the context in storeRow() and deleting it two levels up in materializeQueryResult(), I think it should be created and deleted in the interim layer, storeQueryResult(). Patch along those lines attached. Any objections to me back-patching it this way? I thought the same at first before creating the patch, but I reconsidered. If the query executed by dblink() doesn't return any row, the context creation and deletion is a waste of processing. I think the original author wanted to eliminate this waste by creating the context when dblink() should return a row. I'd like to respect his thought. Regards MauMau -- 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] [bug fix] Memory leak in dblink
MauMau maumau...@gmail.com writes: From: Joe Conway m...@joeconway.com Any objections to me back-patching it this way? I thought the same at first before creating the patch, but I reconsidered. If the query executed by dblink() doesn't return any row, the context creation and deletion is a waste of processing. I think the original author wanted to eliminate this waste by creating the context when dblink() should return a row. I'd like to respect his thought. I don't think speed is really worth worrying about. The remote query will take orders of magnitude more time than the possibly-useless context creation. The code is really designed to put all the setup for storeRow into one place; but I concur with Joe that having teardown in a different place from setup isn't very nice. An alternative that might be worth thinking about is putting both the context creation and deletion at the outermost level where the storeInfo struct is defined. That seems possibly a shade less surprising than having it at the intermediate level. 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] [bug fix] Memory leak in dblink
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/18/2014 08:50 PM, Joe Conway wrote: On 06/18/2014 08:45 PM, Tom Lane wrote: Well, we usually think memory leaks are back-patchable bugs. I'm a bit worried about the potential performance impact of an extra memory context creation/deletion though. It's probably not noticeable in this test case, but that's just because dblink() is such a spectacularly expensive function. Probably so. I'll try to scrounge up some time to test the performance impact of your patch. Not the most scientific of tests, but I think a reasonable one: 8--- create function testcontext(arg int) returns setof int as $$select $1$$ language sql; do $$ declare i int; rec record; begin for i in 1..100 loop select * into rec from testcontext(42); end loop; end $$; 8--- I threw out the first run (not shown) in each group below. without patch - - Time: 9930.320 ms Time: 9963.114 ms Time: 10048.067 ms Time: 9899.810 ms Time: 9926.066 ms Time: 9996.044 ms Time: 9919.095 ms Time: 9921.482 ms Time: 9904.839 ms Time: 9990.285 ms = Avg: 9949.912 ms with patch - - Time: 10172.148 ms Time: 10203.585 ms Time: 10142.779 ms Time: 10211.159 ms Time: 10109.001 ms Time: 10216.619 ms Time: 10221.820 ms Time: 10153.220 ms Time: 10147.540 ms Time: 10176.478 ms == Avg: 10175.435 ms without patch - - Time: 9897.838 ms Time: 9848.947 ms Time: 9830.405 ms Time: 9824.837 ms Time: 9828.743 ms Time: 9990.998 ms Time: 9820.717 ms Time: 9853.545 ms Time: 9850.613 ms Time: 9836.452 ms = Avg: 9858.310 ms 2.7% performance penalty Joe - -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 Support -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTowRSAAoJEDfy90M199hl2kUP+QGkKWJ8MkIOFIH7YlsGwGEK I2jZvgTA84FBmoKuKSRJmUTXenzh3KnINHmsJxywqH3QAjYt3WFZia1OucQQgdZ5 YIpDWyN7FBS2NEwXhDSp2X/Wpqw9ZcLY1cyivUFruRTYm4viw10InNKFe3+396i/ zVt1+e0NlxJKAl4wdtk29q8rlmSJ2ej5fGATgrdd1I6C0kLhBaAxYWqMCC81JQrT slbE/y6qeLUkyCEbvrRPj+J8rCO5sCpXvWA691x5qFSrhFaI1jE62QGq6sz4eB1F gUBNn2c57A5sTtqZDz704FbxHAv6mXZpwb4g7jYT5bV7NBlDUxaUURoWQxLvZ9Iy 6CKZ7eM7yU0k2wpHF7bnOVt5YGtF9spd4MZOkrxSjUJ1XwdBS7IKtdymtUleTRup 5T3oFTQ/joaAGKbO3Ioq2PgcDlVgfq/x2rf/veQXV4AdlvymWamnygZ/Nf91w4QA GpN+cOtsvLVNejqdxR4CoXWA9xu6gfmjnATaVkBQ8vQb61OGMmLmxtJWljp785zL 3jyhISMvcW2Yn7Gv07f7cV89YzfxTwl1EY34DhT9hTKXlim7qu0w0kgR4gp/MKX/ DelfTIZz1JUVwfRDwhOo3cMUGD/ru6H8N/FgtQGycXxYfLg7yK69egxpM3+oZF2t NaEbghbhHXn4LPEbSt0L =2yrG -END PGP SIGNATURE- -- 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] [bug fix] Memory leak in dblink
Joe Conway wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/18/2014 08:50 PM, Joe Conway wrote: On 06/18/2014 08:45 PM, Tom Lane wrote: Well, we usually think memory leaks are back-patchable bugs. I'm a bit worried about the potential performance impact of an extra memory context creation/deletion though. It's probably not noticeable in this test case, but that's just because dblink() is such a spectacularly expensive function. Probably so. I'll try to scrounge up some time to test the performance impact of your patch. Not the most scientific of tests, but I think a reasonable one: Is this an assert-enabled build? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] [bug fix] Memory leak in dblink
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/19/2014 08:50 AM, Alvaro Herrera wrote: Joe Conway wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/18/2014 08:50 PM, Joe Conway wrote: On 06/18/2014 08:45 PM, Tom Lane wrote: Well, we usually think memory leaks are back-patchable bugs. I'm a bit worried about the potential performance impact of an extra memory context creation/deletion though. It's probably not noticeable in this test case, but that's just because dblink() is such a spectacularly expensive function. Probably so. I'll try to scrounge up some time to test the performance impact of your patch. Not the most scientific of tests, but I think a reasonable one: Is this an assert-enabled build? No: CFLAGS = -O2 -Wall -Wmissing-prototypes -Wpointer-arith - -Wdeclaration-after-statement -Wendif-labels - -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing - -fwrapv -fexcess-precision=standard -g CONFIGURE = '--prefix=/usr/local/pgsql-head' '--with-pgport=55437' '--enable-debug' '--enable-nls' '--with-pam' '--enable-thread-safety' '--with-openssl' '--with-krb5' '--with-includes=/usr/include/kerberosIV' '--with-includes=/usr/include/et' '--enable-integer-datetimes' '--with-libxml' '' 'build_alias=' 'host_alias=' 'target_alias=' 'DOCBOOKSTYLE=/usr/share/sgml/docbook/stylesheet/dsssl/modular' - -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 Support -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTowsYAAoJEDfy90M199hll48QAJVSEiaYAAG4f50OyYpS5uka y85ceg+EfE9UMG6FKbEK7z6+a0cQUumvuGOxd0TxztujHLvRCFqW+UlCALRwl5fi d169MI+GYucHoKUj/CObM6dM8qYKK8OAVsMerpNLs97kW2Qsny1Jt930RhHxUgZe ZyUvRsQssInMQ5JUcCy5IinENxhry9fhCMkMIIPVuCu4aF4DeIy7T/cBQkY3S+C3 5DcpHoubwcorL2lFZSpJorQ6w5bueHp0jVsWo1IfP/XlrRckaS1MIgW95YKKw/Ok //F/CoFd9nSkyFmvZw8JW2R9o8Tv1HTZTMvzVCFK7yJGGXRQeTLEGHbYMkkaHgtr piHf89p1wvYOMaGtjQ2pAp4oGjMQ2g3DefvmU3UiM2g9wQ9WgRBMkwnWl3IHms6g OkozqzNtpBVyAxgoumuD2ohdnoHt521v0W4vGisLMv5ZbxHbTZBDmHBptiRkSxYH i6jWlAnTPjXmjy335QQSMe17XAa/vE+0v3ut/vS80lGj+nAKiZnvd4XgR8VwAN8f 6qWU3E754ZnMdYaIrjdrcJ6F0L75tsvM/bS73IN+OsefiYM+BVPv4M2kiwUtOl3z sPNl1uedv5QeF/B/69FBr6zRVBMUoxy6NTdwhZEf4aL9WnIxRO461NOlcEbBNjaC Pds6HML5iBRZixluCHuM =7cT8 -END PGP SIGNATURE- -- 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] [bug fix] Memory leak in dblink
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/19/2014 09:18 AM, Tom Lane wrote: I think we could mitigate this by allocating the argument context once in nodeFunctionscan setup, and passing it into ExecMakeTableFunctionResult; so we'd only do a memory context reset not a create/delete in each cycle. That would make the patch a bit more invasive, but not much. Back-patchability would depend on whether you think there's any third party code calling ExecMakeTableFunctionResult; I kinda doubt that, but I wonder if anyone has a different opinion. Seems unlikely to me. Joe - -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 Support -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTow9pAAoJEDfy90M199hlFIgQAIh8/65UIWiFzACoprPJI3O1 fLoU3mEAErCw1IE6pl+eYNiUNauvSGoizrDlqFdlcVs6PVbgirjcXYvMKNExkIoz eKooxhb9hsfyV0iK6vDOzCWnvLIuarJJgYnu3lFJw5diEoCAywtUE8o3X3/RScQW glMBiIuiGW2EmYkqBQTSc/lkmKIvCB9rrn1XPymvD2riTFvP466MY7gnt7vFTNjD Cq/9PGTxdXOirSPMvIxUmZkbPOLmucDWJCl5CC8E7mYeG4XDj7YoV8KMn4zRKda9 ZUZk72BXpnsqMak0q+tHa9A9KgMkRB8Sw2YE16/qD6et+jbm1OoiD3prpBEdficZ HQiSL/yyFjCX383ySAZ5AbfAlnnDpfWCgk9xjBWZecIt0GGNzRKBbIvO3+maAIok XcotFHkAcFu4C0ssIJdL58tDOoqwaNshgTn9CUK6g4jqZlny9WF8RcRe6yx2XokY x20oRcA4nZ0bDH6ZvxqBFWK0eAfc15zVH0EZaCWDYxX7LEHyr6dxNtTIj38xBt6d RT5ioKR3k+v/mpTq8xKBedtKWsb3BHhvZ+WnncBOU/tjZJcjC6sEZ89O2rElxJ09 FalqGuioi3gR8YgfbNMAbemEwaPg8OP45ChN/SA3cEjX3OJLqHA/WZNmqO50oLZl M3zNfviiWzHG/OjBxp3o =8S6U -END PGP SIGNATURE- -- 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] [bug fix] Memory leak in dblink
Joe Conway m...@joeconway.com writes: Probably so. I'll try to scrounge up some time to test the performance impact of your patch. Not the most scientific of tests, but I think a reasonable one: ... 2.7% performance penalty Thanks. While that's not awful, it's enough to be annoying. I think we could mitigate this by allocating the argument context once in nodeFunctionscan setup, and passing it into ExecMakeTableFunctionResult; so we'd only do a memory context reset not a create/delete in each cycle. That would make the patch a bit more invasive, but not much. Back-patchability would depend on whether you think there's any third party code calling ExecMakeTableFunctionResult; I kinda doubt that, but I wonder if anyone has a different opinion. 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] [bug fix] Memory leak in dblink
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 06/19/2014 08:18 AM, Tom Lane wrote:
The code is really designed to put all the setup for storeRow into
one place; but I concur with Joe that having teardown in a
different place from setup isn't very nice.
An alternative that might be worth thinking about is putting both
the context creation and deletion at the outermost level where the
storeInfo struct is defined. That seems possibly a shade less
surprising than having it at the intermediate level.
Fair enough -- this patch does it at that level in
materializeQueryResult()
Joe
- --
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, 24x7 Support
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJToxoJEDfy90M199hl6okP/0B5Pf+SAk7uNpLhDgE5oR27
vWpzhZHau2Yo9RkPqXwJoHRIIwluBqAjp3Ps2b9qLCPpyPFWbxll/f2K2C3LTgYi
ZcpFWQqrCdDMFbHphE642mAt8oy9xAXHsCCH4ONAUTGfhOagRKP2oKGhwbMOah74
KDfwDuyXyXEhzOjX538/3WhGXgZV5pmBUp9+QKZ8KhLy6GdAwg5h8Q2QFXy5UVuf
pZyoXP8kxuN6ubYMlRnWpUK1yxhks9Hqjahx3dU4MDrKyxPGKJL2p5jpU04an8RD
JuswYNlZpMWbaF9C1AWM7C++COWPzSx5tULOJFWNhVgmrZj6jvtKR0SVB8GK2K4G
8xepGXREj6wAzFEY3FqOmihcKi0gk6jvJ6BJocZDAB+UTSSIMvqxfj+RDtX1CYWT
YKEh5Wzs4D1bK7eKxDxWRj+EzdxJVXq6nNSUdQHwq6HwR8XcT7R7/AP6qUSl90EY
pj+8iKu+c3u/fLogqH1xcLx0d5WpDttosjvJ2d1pA6iCBTRZJofj9Q80TDSXYEn8
/gdYHjam9U11wUzNhT5n7IV+vxMdNYxlHWX076xIA0wgrz7SIAp0DBkCu844OAl7
4DJ966m3QbWUHQgzNPE0nyHpEMUOpKMTAKxxeXMDik1U/q/GUx/dekjeL09W2qvh
O//08pMr5h+y6NAjbOMQ
=9P3q
-END PGP SIGNATURE-
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index a81853f..957fe01 100644
*** a/contrib/dblink/dblink.c
--- b/contrib/dblink/dblink.c
*** materializeQueryResult(FunctionCallInfo
*** 977,982
--- 977,990
PG_TRY();
{
+ /* Create short-lived memory context for data conversions */
+ if (!sinfo.tmpcontext)
+ sinfo.tmpcontext = AllocSetContextCreate(CurrentMemoryContext,
+ dblink temporary context,
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_INITSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
+
/* execute query, collecting any tuples into the tuplestore */
res = storeQueryResult(sinfo, conn, sql);
*** materializeQueryResult(FunctionCallInfo
*** 1041,1046
--- 1049,1060
PQclear(res);
res = NULL;
}
+
+ /* clean up data conversion short-lived memory context */
+ if (sinfo.tmpcontext != NULL)
+ MemoryContextDelete(sinfo.tmpcontext);
+ sinfo.tmpcontext = NULL;
+
PQclear(sinfo.last_res);
sinfo.last_res = NULL;
PQclear(sinfo.cur_res);
*** storeRow(storeInfo *sinfo, PGresult *res
*** 1204,1218
if (sinfo-cstrs)
pfree(sinfo-cstrs);
sinfo-cstrs = (char **) palloc(nfields * sizeof(char *));
-
- /* Create short-lived memory context for data conversions */
- if (!sinfo-tmpcontext)
- sinfo-tmpcontext =
- AllocSetContextCreate(CurrentMemoryContext,
- dblink temporary context,
- ALLOCSET_DEFAULT_MINSIZE,
- ALLOCSET_DEFAULT_INITSIZE,
- ALLOCSET_DEFAULT_MAXSIZE);
}
/* Should have a single-row result if we get here */
--- 1218,1223
--
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] [bug fix] Memory leak in dblink
From: Joe Conway m...@joeconway.com
Fair enough -- this patch does it at that level in
materializeQueryResult()
I'm in favor of this. TBH, I did this at first, but I was afraid this would
be rejected due to the reason mentioned earlier.
if statement in PG_TRY block is not necessary like this, because sinfo is
zero-cleared.
PG_TRY();
{
+ /* Create short-lived memory context for data conversions */
+ sinfo.tmpcontext = AllocSetContextCreate(CurrentMemoryContext,
+dblink temporary context,
Regards
MauMau
--
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] [bug fix] Memory leak in dblink
Joe Conway m...@joeconway.com writes:
Not the most scientific of tests, but I think a reasonable one:
...
2.7% performance penalty
I made a slightly different test based on the original example.
This uses generate_series() which is surely faster than a SQL
function, so it shows the problem to a larger degree:
create table t1 as select x from generate_series(1,1000) x;
vacuum t1;
-- time repeated executions of this:
select count(*) from t1
where exists (select 1 from generate_series(x,x+ (random()*10)::int));
-- and this:
select count(*) from t1
where exists (select 1 from
generate_series(x,x+ (random()*10)::int::text::int));
The first of these test queries doesn't leak memory because the argument
expressions are all pass-by-value; the second one adds some useless text
conversions just to provoke the leak (it eats about 1GB in HEAD).
HEAD, with asserts off:
first query:
Time: 21694.557 ms
Time: 21629.107 ms
Time: 21593.510 ms
second query:
Time: 26698.445 ms
Time: 26699.408 ms
Time: 26751.742 ms
With yesterday's patch:
first query:
Time: 23919.438 ms
Time: 24053.108 ms
Time: 23884.989 ms
... so about 10.7% slower, not good
second query no longer bloats, but:
Time: 29986.850 ms
Time: 29951.179 ms
Time: 29771.533 ms
... almost 12% slower
With the attached patch instead, I get:
first query:
Time: 21017.503 ms
Time: 21113.656 ms
Time: 21107.617 ms
... 2.5% faster??
second query:
Time: 27033.626 ms
Time: 26997.907 ms
Time: 26984.397 ms
... about 1% slower
In the first query, the MemoryContextReset is nearly free since there's
nothing to free (and we've special-cased that path in aset.c). It's
certainly a measurement artifact that it measures out faster, which says
to me that these numbers can only be trusted within a couple percent;
but at least we're not taking much hit in cases where the patch isn't
actually conferring any benefit. For the second query, losing 1% to avoid
memory bloat seems well worthwhile.
Barring objections I'll apply and back-patch this.
regards, tom lane
diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index f162e92..bc79e3a 100644
*** a/src/backend/executor/execQual.c
--- b/src/backend/executor/execQual.c
*** ExecMakeFunctionResultNoSets(FuncExprSta
*** 2002,2007
--- 2002,2008
Tuplestorestate *
ExecMakeTableFunctionResult(ExprState *funcexpr,
ExprContext *econtext,
+ MemoryContext argContext,
TupleDesc expectedDesc,
bool randomAccess)
{
*** ExecMakeTableFunctionResult(ExprState *f
*** 2083,2094
/*
* Evaluate the function's argument list.
*
! * Note: ideally, we'd do this in the per-tuple context, but then the
! * argument values would disappear when we reset the context in the
! * inner loop. So do it in caller context. Perhaps we should make a
! * separate context just to hold the evaluated arguments?
*/
argDone = ExecEvalFuncArgs(fcinfo, fcache-args, econtext);
/* We don't allow sets in the arguments of the table function */
if (argDone != ExprSingleResult)
ereport(ERROR,
--- 2084,2101
/*
* Evaluate the function's argument list.
*
! * We can't do this in the per-tuple context: the argument values
! * would disappear when we reset that context in the inner loop. And
! * the caller's CurrentMemoryContext is typically a query-lifespan
! * context, so we don't want to leak memory there. We require the
! * caller to pass a separate memory context that can be used for this,
! * and can be reset each time through to avoid bloat.
*/
+ MemoryContextReset(argContext);
+ oldcontext = MemoryContextSwitchTo(argContext);
argDone = ExecEvalFuncArgs(fcinfo, fcache-args, econtext);
+ MemoryContextSwitchTo(oldcontext);
+
/* We don't allow sets in the arguments of the table function */
if (argDone != ExprSingleResult)
ereport(ERROR,
diff --git a/src/backend/executor/nodeFunctionscan.c b/src/backend/executor/nodeFunctionscan.c
index da5d8c1..945a414 100644
*** a/src/backend/executor/nodeFunctionscan.c
--- b/src/backend/executor/nodeFunctionscan.c
***
*** 28,33
--- 28,34
#include nodes/nodeFuncs.h
#include parser/parsetree.h
#include utils/builtins.h
+ #include utils/memutils.h
/*
*** FunctionNext(FunctionScanState *node)
*** 94,99
--- 95,101
node-funcstates[0].tstore = tstore =
ExecMakeTableFunctionResult(node-funcstates[0].funcexpr,
node-ss.ps.ps_ExprContext,
+ node-argcontext,
node-funcstates[0].tupdesc,
node-eflags EXEC_FLAG_BACKWARD);
*** FunctionNext(FunctionScanState *node)
*** 152,157
--- 154,160
fs-tstore =
ExecMakeTableFunctionResult(fs-funcexpr,
node-ss.ps.ps_ExprContext,
+ node-argcontext,
fs-tupdesc,
Re: [HACKERS] [bug fix] Memory leak in dblink
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/19/2014 04:36 PM, Tom Lane wrote: In the first query, the MemoryContextReset is nearly free since there's nothing to free (and we've special-cased that path in aset.c). It's certainly a measurement artifact that it measures out faster, which says to me that these numbers can only be trusted within a couple percent; but at least we're not taking much hit in cases where the patch isn't actually conferring any benefit. For the second query, losing 1% to avoid memory bloat seems well worthwhile. Barring objections I'll apply and back-patch this. Nice work! +1 Joe - -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 Support -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTo4XHAAoJEDfy90M199hlEDwP/1eqjD1lu9Dk5zRCJk239I7D hLC0DguDNs/cwPCm4tC7ngtOLBUtrLVyvWYXQw9Yxs+8JYm2rx2fpD3bq5scZfXh mWaaIM5plYI6v1QeMtg7u8LUEuH8dw0Ycbse2oZRWwZwn9dAEBHHdQTTtrm0bCSu cKS402hTSUYMHhyhoURN4CzK1zzmoV8kb3ZhyCx+4HQNNJ3zf/yl/fONkHdc5aar 8M66g+Emf9Qpddx8+wWUL8xcqagIU0k36tJk1lM2vT5tLQ8ezYlNwLxC674ifIZn dlsu127uSanEegYis+lFFQriRaYl2Bs7kZfYcas33RtuTlCJN4TK5AcRmgzPs+hT WH2Kj/cVAMd7fwrogHQzGEnEGPRRfETWj+GPL9uibgrfY6mLaV5PLEosWTIRDsq/ 6aSGNrBL5jp958k+d2bNjv/WTj/LZrZTSMRA//8mc3wbae5YDFEn3o6ADvm5/3Gn xj+ijVOYFAgnNq4P5o1TIWoWqu+GA7ExfD8FW369Hgfi58o6KKRbpM4httJz/3fH 3q3pbDHp7dNFsM5AnmjFltg97X148u6dRd7sHDMEgSmGxJQiJGLEqNyROATRVTj1 DRxxmIsVcE52Rpm0Mz7SnmoSM+1RdtVO8N9Lz9ygcokP8XbbWlXUbEKl8f3S7v+P ANXyc09bdqXYi40afE1d =7MYr -END PGP SIGNATURE- -- 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] [bug fix] Memory leak in dblink
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 06/10/2014 02:57 AM, MauMau wrote:
From: Amit Kapila amit.kapil...@gmail.com
Is there a need to free memory context in PG_CATCH()? In error
path the memory due to this temporary context will get freed as
it will delete the transaction context and this temporary context
will definitely be in the hierarchy of it, so it should also get
deleted. I think such handling can be useful incase we use
PG_CATCH() to suppress the error.
I thought the same, but I also felt that I should make an effort
to release resources as soon as possible, considering the memory
context auto deletion as a last resort. However, looking at other
places where PG_CATCH() is used, memory context is not deleted.
So, I removed the modification from PG_CATCH() block. Thanks.
I think the context deletion was missed in the first place because
storeRow() is the wrong place to create the context. Rather than
creating the context in storeRow() and deleting it two levels up in
materializeQueryResult(), I think it should be created and deleted in
the interim layer, storeQueryResult(). Patch along those lines attached.
Any objections to me back-patching it this way?
Joe
- --
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, 24x7 Support
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJTod+UAAoJEDfy90M199hlaWgP/iitSQm7m+Sf2i8hP73TAdTX
Naw+TKtag822xzVT6AoVJafeZvEv0PNRYurP69y4zE11cwiG9u+RfoO1X1hP7FYu
mdHo5++YG+znmqDVC57Qav0qAheYaCk2Y9RKEZk9gEJLUO+HJRxuzc+L277lsAni
Iyu3DyaiogzmGBtjrPPex4VMtpFAPHzHWfABaL11kvNBXXpUjO/Z02ex+1nuZDV7
+wNSV4IdTTsmpdbbi/NRhDeU7sZOerIAY29B6vI/GXfHdwhDhg+3tG4PuoDg6YG2
jX/H37UE0zq0+J3ySnM92HZtn9RxfrjdkHTz/X7DAjZjHNwBNVi18oNfrsXUyqHO
3j3VEaRelow2+tUDaTxziEkZRA0vCRoeG20B7dgQY3op60wm9lpJ+pCQH8UGwPYC
HDxaOYcVKmxbWq+j86l2ZyYlfP8sVbqTMEc00dnYoc2sdDzU36+KJadIqoMoUgds
G5uNYOZ5eTxumua9exz2cqGVOdgzcDD3r/ZAUUVM0jk3LwdA4CKLorsy26Ce59lF
mSIO58uAJe198tyOCmbpZjbypIRRO3Qm73SfUmioCbcUzSg2tDdPpf0LP62V/YEm
gFIsPHNyZnaVm0baLilbJFg+88sxFSo1hvpoaUfNPVww7woAfFxi6uBt5h5KthUO
JoDWxYjYUy9VFDdC4rh4
=gj6p
-END PGP SIGNATURE-
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index a81853f..46c4a51 100644
*** a/contrib/dblink/dblink.c
--- b/contrib/dblink/dblink.c
*** storeQueryResult(storeInfo *sinfo, PGcon
*** 1076,1081
--- 1076,1089
if (!PQsetSingleRowMode(conn)) /* shouldn't fail */
elog(ERROR, failed to set single-row mode for dblink query);
+ /* Create short-lived memory context for data conversions */
+ if (!sinfo-tmpcontext)
+ sinfo-tmpcontext = AllocSetContextCreate(CurrentMemoryContext,
+ dblink temporary context,
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_INITSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
+
for (;;)
{
CHECK_FOR_INTERRUPTS();
*** storeQueryResult(storeInfo *sinfo, PGcon
*** 1119,1124
--- 1127,1136
/* clean up GUC settings, if we changed any */
restoreLocalGucs(nestlevel);
+ /* clean up data conversion short-lived memory context */
+ if (sinfo-tmpcontext != NULL)
+ MemoryContextDelete(sinfo-tmpcontext);
+
/* return last_res */
res = sinfo-last_res;
sinfo-last_res = NULL;
*** storeRow(storeInfo *sinfo, PGresult *res
*** 1204,1218
if (sinfo-cstrs)
pfree(sinfo-cstrs);
sinfo-cstrs = (char **) palloc(nfields * sizeof(char *));
-
- /* Create short-lived memory context for data conversions */
- if (!sinfo-tmpcontext)
- sinfo-tmpcontext =
- AllocSetContextCreate(CurrentMemoryContext,
- dblink temporary context,
- ALLOCSET_DEFAULT_MINSIZE,
- ALLOCSET_DEFAULT_INITSIZE,
- ALLOCSET_DEFAULT_MAXSIZE);
}
/* Should have a single-row result if we get here */
--- 1216,1221
--
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] [bug fix] Memory leak in dblink
Joe Conway m...@joeconway.com writes: I think the context deletion was missed in the first place because storeRow() is the wrong place to create the context. Rather than creating the context in storeRow() and deleting it two levels up in materializeQueryResult(), I think it should be created and deleted in the interim layer, storeQueryResult(). Patch along those lines attached. Since the storeInfo struct is longer-lived than storeQueryResult(), it'd probably be better if the cleanup looked like + if (sinfo-tmpcontext != NULL) + MemoryContextDelete(sinfo-tmpcontext); + sinfo-tmpcontext = NULL; I find myself a bit suspicious of this whole thing though. If it's necessary to explicitly clean up the tmpcontext, why not also the sinfo-cstrs allocation? And what about the tupdesc, attinmeta, etc created further up in that if (first) block? I'd have expected that all this stuff gets allocated in a context that's short-lived enough that we don't really need to clean it up explicitly. 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] [bug fix] Memory leak in dblink
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/18/2014 12:09 PM, Tom Lane wrote: Joe Conway m...@joeconway.com writes: I think the context deletion was missed in the first place because storeRow() is the wrong place to create the context. Rather than creating the context in storeRow() and deleting it two levels up in materializeQueryResult(), I think it should be created and deleted in the interim layer, storeQueryResult(). Patch along those lines attached. Since the storeInfo struct is longer-lived than storeQueryResult(), it'd probably be better if the cleanup looked like + if (sinfo-tmpcontext != NULL) + MemoryContextDelete(sinfo-tmpcontext); + sinfo-tmpcontext = NULL; good point I find myself a bit suspicious of this whole thing though. If it's necessary to explicitly clean up the tmpcontext, why not also the sinfo-cstrs allocation? And what about the tupdesc, attinmeta, etc created further up in that if (first) block? I'd have expected that all this stuff gets allocated in a context that's short-lived enough that we don't really need to clean it up explicitly. Yeah, I thought about that too. My testing showed a small amount of remaining leakage -- like 20 MB on 100,000 iterations -- I wasn't sure that it was worthwhile to worry about. The memory context leak was much larger. Joe - -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 Support -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJToeUQAAoJEDfy90M199hls8kP/0hhsVdn8HKcY7OZ64cKju4e dH0SwQNMZxklpQkjEqb6vpbTmqaQ/7At/i7b6RGTMWFbIBu7L5Vpz+NZnJ7SyfaC UjSgKfSe6M2auDIXlLCiZ62d8KZJJmVQj6U2h8ljhqorJq22kvClgXAUlcxZMoVv SEthE1y7Udxgf3IqBO0AN6SMPrxP8bZDgrPbtZqw9UEHkCGZK0MDdH8TSRip7p6V heyg9GbWeTvwFWBUYomMnMEUB6UlgBRnmHYsZIAjmUgxZfGiKhlydGOrb7MDHopz ejyb06fg2MsSQnrEnCElLbonUqb5S9bD4p9BZHF/bz67AhVieJvDvnsSIoDjzR1a +JIYe36ZDqo2kIGx4PuESgGX4ZTxxsrw033AQhVBW+KGkIuxjnvhh7tvEj3ACSjd 0bC1ztrnzCJLyNFd6jKaq5KAcNCEb/zvfKQAdHygJ87JkwE8I6u+ms1hdDpc10uZ w484lcv/qP/JO2SaqPerDMwRsDXaovT8kEcsbqr1D7XFXu4cufHVGwe64IJYMjt+ 9Y09/+i+Xt9DKXfkxC68Q2QWxwst5NfShRWDF3NG02iWlMqU2OsoVTQB6137DCiB 7hBKoNBmOApLJIjwCzEjB5IJmeeAz9x7YxgsqicRPnXoCKO+aF3dTbS+1u04va84 5XQ0p0lwUZaNfYr/xbi6 =H5bR -END PGP SIGNATURE- -- 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] [bug fix] Memory leak in dblink
Joe Conway m...@joeconway.com writes: On 06/18/2014 12:09 PM, Tom Lane wrote: I find myself a bit suspicious of this whole thing though. If it's necessary to explicitly clean up the tmpcontext, why not also the sinfo-cstrs allocation? And what about the tupdesc, attinmeta, etc created further up in that if (first) block? I'd have expected that all this stuff gets allocated in a context that's short-lived enough that we don't really need to clean it up explicitly. Yeah, I thought about that too. My testing showed a small amount of remaining leakage -- like 20 MB on 100,000 iterations -- I wasn't sure that it was worthwhile to worry about. The memory context leak was much larger. [ Thinks for awhile... ] Ah. The memory context is a child of the econtext's ecxt_per_tuple_memory, which is supposed to be short-lived, but we only do MemoryContextReset() on that after each tuple, not MemoryContextResetAndDeleteChildren(). I recall there's been debate about changing that, but it's not something we can risk changing in back branches, for sure. So I concur we need an explicit context delete here. I do see growth in the per-query context as well. I'm not sure where that's coming from, but we probably should try to find out. A couple hundred bytes per iteration is going to add up, even if it's not as fast as 8K per iteration. I'm not sure it's dblink's fault, because I don't see anything in dblink.c that is allocating anything in the per-query context, except for the returned tuplestores, which somebody else should clean up. 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] [bug fix] Memory leak in dblink
I wrote: I do see growth in the per-query context as well. I'm not sure where that's coming from, but we probably should try to find out. A couple hundred bytes per iteration is going to add up, even if it's not as fast as 8K per iteration. I'm not sure it's dblink's fault, because I don't see anything in dblink.c that is allocating anything in the per-query context, except for the returned tuplestores, which somebody else should clean up. I poked at this example some more, and found that the additional memory leak is occurring during evaluation of the arguments to be passed to dblink(). There's been a comment there for a very long time suggesting that we might need to do something about that ... With the attached patch on top of yours, I see no leak anymore. regards, tom lane diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c index f162e92..cf8cbb1 100644 *** a/src/backend/executor/execQual.c --- b/src/backend/executor/execQual.c *** ExecMakeTableFunctionResult(ExprState *f *** 2014,2019 --- 2014,2020 PgStat_FunctionCallUsage fcusage; ReturnSetInfo rsinfo; HeapTupleData tmptup; + MemoryContext argContext = NULL; MemoryContext callerContext; MemoryContext oldcontext; bool direct_function_call; *** ExecMakeTableFunctionResult(ExprState *f *** 2083,2094 /* * Evaluate the function's argument list. * ! * Note: ideally, we'd do this in the per-tuple context, but then the ! * argument values would disappear when we reset the context in the ! * inner loop. So do it in caller context. Perhaps we should make a ! * separate context just to hold the evaluated arguments? */ argDone = ExecEvalFuncArgs(fcinfo, fcache-args, econtext); /* We don't allow sets in the arguments of the table function */ if (argDone != ExprSingleResult) ereport(ERROR, --- 2084,2104 /* * Evaluate the function's argument list. * ! * We can't do this in the per-tuple context: the argument values ! * would disappear when we reset that context in the inner loop. And ! * the caller's context is typically a query-lifespan context, so we ! * don't want to leak memory there. So make a separate context just ! * to hold the evaluated arguments. */ + argContext = AllocSetContextCreate(callerContext, + Table function arguments, + ALLOCSET_DEFAULT_MINSIZE, + ALLOCSET_DEFAULT_INITSIZE, + ALLOCSET_DEFAULT_MAXSIZE); + oldcontext = MemoryContextSwitchTo(argContext); argDone = ExecEvalFuncArgs(fcinfo, fcache-args, econtext); + MemoryContextSwitchTo(oldcontext); + /* We don't allow sets in the arguments of the table function */ if (argDone != ExprSingleResult) ereport(ERROR, *** no_function_result: *** 2321,2328 --- 2331,2342 FreeTupleDesc(rsinfo.setDesc); } + /* Clean up */ MemoryContextSwitchTo(callerContext); + if (argContext) + MemoryContextDelete(argContext); + /* All done, pass back the tuplestore */ return rsinfo.setResult; } -- 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] [bug fix] Memory leak in dblink
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/18/2014 07:29 PM, Tom Lane wrote: I wrote: I do see growth in the per-query context as well. I'm not sure where that's coming from, but we probably should try to find out. A couple hundred bytes per iteration is going to add up, even if it's not as fast as 8K per iteration. I'm not sure it's dblink's fault, because I don't see anything in dblink.c that is allocating anything in the per-query context, except for the returned tuplestores, which somebody else should clean up. I poked at this example some more, and found that the additional memory leak is occurring during evaluation of the arguments to be passed to dblink(). There's been a comment there for a very long time suggesting that we might need to do something about that ... With the attached patch on top of yours, I see no leak anymore. I can confirm that -- rock solid with 1 million iterations. I assume that should not be back-patched though? On a side note, while perusing this section of code: 8-- at dblink.c:1176 -- /* make sure we have a persistent copy of the tupdesc */ tupdesc = CreateTupleDescCopy(tupdesc); /* check result and tuple descriptor have the same number of columns */ if (nfields != tupdesc-natts) ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), errmsg(remote query result rowtype does not match the specified FROM clause rowtype))); /* Prepare attinmeta for later data conversions */ sinfo-attinmeta = TupleDescGetAttInMetadata(tupdesc); /* Create a new, empty tuplestore */ oldcontext = MemoryContextSwitchTo(rsinfo-econtext-ecxt_per_query_memory); sinfo-tuplestore = tuplestore_begin_heap(true, false, work_mem); rsinfo-setResult = sinfo-tuplestore; rsinfo-setDesc = tupdesc; MemoryContextSwitchTo(oldcontext); 8-- at dblink.c:1194 -- Shouldn't that CreateTupleDescCopy() happen in ecxt_per_query_memory? Joe - -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 Support -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTolNHAAoJEDfy90M199hlEN8QAIi3eq5D93Y4CqWWS8Uulqx1 jOBQUoLTwF7/CkYM0F+tGvX1QO1BAIDAxS6jwWXK/c9NqesVuS5tNwQcdrLb69Pm me4hQ2ZYtoCA4Y8SiFL0sOjUGuads2QEjhL3HUMQDBXTMjCpzamotIBGvYWu1OOe bf1VaY83bwGa6XvXYcfFFyqyUz+kMHvcM/Rq9Mj7pLrGT9Lqvec9RL/1FhFYamrI 2ewaKYC4htWXwk1xIvpDZtWTjLyv3BUl3X41BzPOecChWqXmYCpaPo+dR6V9LSm1 OxGFQL4Z3pM7VZDWk5FOj3vOFz1AJhWXEh8fyzlLKeDm7FFaApyf3rPLULBRO4sj 4C88lUdSbhzgTlMreq/wqBh2bKFmLGUA8M9sSwd+2DMc6QQQN0DWCgDtSZq/3Fwr Tc/0LWvHwh+/Tozx0kk0DxIQzS0BOsWHzjtOMwb1p3aLZXlG9FP5IrrPJlIyDOQK feVB9JNH5kGUFEU0OkGaqPaQy1lTVTizIA/FmTqV9QeJo2+vKSNPt2Ys4dM5jevo Tpp6ZAjvC6sAoIoWmvtazV5WOL7FwXwf8AQRJRgmh8JqHw4C2nZt9R+CNQqbGPa2 hxiTWi5EMufBmVOJeaEX867CUTLL6qzCtr/I2a2XZyMuPD5dQbS3l2MaYw1l2ucU o9x6G78hBR32xagpqPCE =LqzA -END PGP SIGNATURE- -- 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] [bug fix] Memory leak in dblink
Joe Conway m...@joeconway.com writes: On a side note, while perusing this section of code: 8-- at dblink.c:1176 -- /* make sure we have a persistent copy of the tupdesc */ tupdesc = CreateTupleDescCopy(tupdesc); Shouldn't that CreateTupleDescCopy() happen in ecxt_per_query_memory? Not necessary (we'd have seen crashes long since if it was). ExecMakeTableFunctionResult doesn't need the tupdesc to persist past return. Actually, I was wondering whether we couldn't remove that CreateTupleDescCopy call entirely. The risk would be if get_call_result_type returned a pointer into relcache or some other cached tuple descriptor, which might be subject to a cache flush --- but AFAICS it always returns a freshly created or copied tupdesc. (This might not have been true originally, which could explain why dblink thinks it needs to copy.) 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] [bug fix] Memory leak in dblink
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/18/2014 08:19 PM, Tom Lane wrote: Actually, I was wondering whether we couldn't remove that CreateTupleDescCopy call entirely. Apparently not, at least without some additional surgery. ExecMakeTableFunctionResult() tries to free the tupledesc and segfaults. Joe - -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 Support -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTolmPAAoJEDfy90M199hlz7YQAInPRKkGLskcqx4ujmsNpbEG RS2zSll4PqCELa/wMcWDJUsoVkzjybuw6A/1MSLGtERIamHDcmDIbTFwx9Z02n0u nuFGgyds9auIqn+AB18rvwgas+gL6tRHOUe4bagiuqNzywnOceW98PT0NttWt86y Fsyz/xfapIoV+kS1k9FAXC5B/PfayYoPq3cB3qWGNX/vor+xIgw3BGT9Bk3DbN2J IqD75HqoFV5jEwiStwxLaLvgqiLPVMzI/HdiQhprbveTa1e2vFM7Tu6n02i8uFVt fVu4UCtBtOWRIC9oOO0QhVtqnETMpwsxwWIxC5RhWScL7M8CT3Z9cw5zCIJWo2Q8 VaUDa+gpXukisg8OUfK2AhSduxcPYJ8I+b/VMS9p6j5P/87slnLuMaTnxU7afVCM 3F2UrDOgv53t43NMeD3xou8J4ntf/NJbaOsXCQx9yXjcq3UMzT0g3OSwxPbfViE+ YN6GCelnt6e0mT3Uk/pZDm0+QwYeMv6ZHjYD3y7vD4+Ipnt/HNAhO6r/HyRDk7/x DvSeO0okJXwUqTq/xcJ6wBE7frBqTpfzLxEMLXEVMMRCZWpKOAwK05afIZsadKqP wgeJETiSirKfWUWb0/bmMIVv2vA4fRZYpLW31pBr6OLS1GlwsrsfuYNxCm8ur1tG gUe/trrEIMBo6iyE//N5 =i7jE -END PGP SIGNATURE- -- 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] [bug fix] Memory leak in dblink
Joe Conway m...@joeconway.com writes:
On 06/18/2014 08:19 PM, Tom Lane wrote:
Actually, I was wondering whether we couldn't remove that
CreateTupleDescCopy call entirely.
Apparently not, at least without some additional surgery.
ExecMakeTableFunctionResult() tries to free the tupledesc and segfaults.
Hmm ... oh, I missed this bit:
/* We must get the tupledesc from call context */
if (rsinfo IsA(rsinfo, ReturnSetInfo)
rsinfo-expectedDesc != NULL)
{
result = TYPEFUNC_COMPOSITE;
if (resultTupleDesc)
*resultTupleDesc = rsinfo-expectedDesc;
/* Assume no polymorphic columns here, either */
}
So it's passing back the same tupdesc passed in by the caller of
ExecMakeTableFunctionResult. We can free that all right, but the caller
won't be happy :-(
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] [bug fix] Memory leak in dblink
Joe Conway m...@joeconway.com writes: On 06/18/2014 07:29 PM, Tom Lane wrote: With the attached patch on top of yours, I see no leak anymore. I can confirm that -- rock solid with 1 million iterations. I assume that should not be back-patched though? Well, we usually think memory leaks are back-patchable bugs. I'm a bit worried about the potential performance impact of an extra memory context creation/deletion though. It's probably not noticeable in this test case, but that's just because dblink() is such a spectacularly expensive function. 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] [bug fix] Memory leak in dblink
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/18/2014 08:45 PM, Tom Lane wrote: Joe Conway m...@joeconway.com writes: On 06/18/2014 07:29 PM, Tom Lane wrote: With the attached patch on top of yours, I see no leak anymore. I can confirm that -- rock solid with 1 million iterations. I assume that should not be back-patched though? Well, we usually think memory leaks are back-patchable bugs. I'm a bit worried about the potential performance impact of an extra memory context creation/deletion though. It's probably not noticeable in this test case, but that's just because dblink() is such a spectacularly expensive function. Probably so. I'll try to scrounge up some time to test the performance impact of your patch. Joe - -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 Support -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTol38AAoJEDfy90M199hleycP/2kOi2Pa6vcVXKxhNQo3mSdO A84Ae/4LTfUbeVzUTf+uBRcz6LOtOlrHATZOcftJMlyTNmM++JJvF3YYMpGgmxO/ UfiykDs2bqDgPrIxbPxAEpgdeXWcsdJZzzOV1YWurU/qnTdoKD2ArPQhakWLGZH0 CRc46Cn2Qb3NCvnuO5R+ZhGPXS0t6EqTiGlmWtk9ZaI8MHmv1qVKMOKBor3v+2lk /wdlc5lypPnZ07NKIjCVN0gzEJ+RV9nxQk1M3QkNYNsHOBiexEmaucXo6ab4derO nXoOGo/0XwMhlhA6vrKlAKhxjkTNnJulVHQOWVLMCiNvcfX0KISJZVIoT/NraR94 Hc5ZZMjmhosbU8sgQiKjGoFSJq2Wld6SADuLt6xvsY9k5AiuEcPFbfVjAWlCIaEm lOQ2cOrk+4nhEA1ygIsRw96GMT2WaEtOek4l3hJs6yd3zuzXoouO9i02QaXBqgR8 QmIJ+tOjwKnOPFThbJMjxlsrQMwJ6mPywhwt6E74YsKV6ndGFigBOfzjZlOn3OKX DM60oWFhuCfHQdOlid1d6Uyuc4yeFb4g4XSS4sXW9wLPpvve63NxxBQ8ez0r3Up8 nLwcCqxFZRFwKX2Wp6fgtpmhgxolLF29XvpfTUMR6pPai+/Ei59vU4JXqqz0haqa 3UHpQ3AznN5vm+UvZJYe =pvQS -END PGP SIGNATURE- -- 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] [bug fix] Memory leak in dblink
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 06/18/2014 12:14 PM, Joe Conway wrote:
On 06/18/2014 12:09 PM, Tom Lane wrote:
Joe Conway m...@joeconway.com writes:
I think the context deletion was missed in the first place
because storeRow() is the wrong place to create the context.
Rather than creating the context in storeRow() and deleting it
two levels up in materializeQueryResult(), I think it should
be created and deleted in the interim layer,
storeQueryResult(). Patch along those lines attached.
Since the storeInfo struct is longer-lived than
storeQueryResult(), it'd probably be better if the cleanup
looked like
+if (sinfo-tmpcontext != NULL) +
MemoryContextDelete(sinfo-tmpcontext); +sinfo-tmpcontext =
NULL;
good point
If there is no further discussion, I will commit this version of the
patch back to 9.2 where this leak was introduced.
Joe
- --
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, 24x7 Support
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJTompDAAoJEDfy90M199hlZoMP/333eXbmFvh1WCowMehKPEy1
WV3VgyZx7hBvSr/cP/HUMjXTb34hRgi5uGa79ZMyAW+U+jDxrIN4ozxp54LlgU5x
pTzD8J8VviunvWzf6stgHTe5bfcBJ9kA9oZlgHApH+JRGeySsYALI4ZBluJa1tRa
gf6ePd8Kwl4AUucbM3v7kJGxtVS5XRGKcffJnATg+OLiBUReVv9ZCjxeYasyOaRt
K2Q8ijW878UgV9HViTCsl8THelnlh7q0BpbKaSZBJG847CgmrVxfRt/l8Od8a4G/
ODoQ/fV0ZOI3h5j9oirL4/RC9HWOqchJBvBd3CK7caWLwNKVwqqEWGV3uqEO2TnA
NH3QgHb4u8WGNhoWbwL6dGWa27s2EntlWLpJRuavKxCIN2owvVBKSZ6H8d3dSlI5
AqaGnxOPvxWEB5K2w0wBynkZ9nrk4PYuIVKADu8fqyYyDsG3wGsZmI1trLNXj5sm
uE/FTbdvUjcU2uIVMMJSbPxa5JvvfR/+9rM/AF8VP5PMnSs1CyeLQXkW7nazRZOP
7zHUY6N4vwem8tVQuuXPouLu2B/eTJoXaUTm7iQvXztJDmwKxKgYCzLW/LxfvI4Q
mDIY/Arh/4RdC7kVXu6m5BEisNIbBuKtcA6f0DM+4G9i0Wtft450fgajYV4xcic9
DMPyBMwD7csz3ssF04Ox
=PJyj
-END PGP SIGNATURE-
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index a81853f..2e43c8f 100644
*** a/contrib/dblink/dblink.c
--- b/contrib/dblink/dblink.c
*** storeQueryResult(storeInfo *sinfo, PGcon
*** 1076,1081
--- 1076,1089
if (!PQsetSingleRowMode(conn)) /* shouldn't fail */
elog(ERROR, failed to set single-row mode for dblink query);
+ /* Create short-lived memory context for data conversions */
+ if (!sinfo-tmpcontext)
+ sinfo-tmpcontext = AllocSetContextCreate(CurrentMemoryContext,
+ dblink temporary context,
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_INITSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
+
for (;;)
{
CHECK_FOR_INTERRUPTS();
*** storeQueryResult(storeInfo *sinfo, PGcon
*** 1119,1124
--- 1127,1137
/* clean up GUC settings, if we changed any */
restoreLocalGucs(nestlevel);
+ /* clean up data conversion short-lived memory context */
+ if (sinfo-tmpcontext != NULL)
+ MemoryContextDelete(sinfo-tmpcontext);
+ sinfo-tmpcontext = NULL;
+
/* return last_res */
res = sinfo-last_res;
sinfo-last_res = NULL;
*** storeRow(storeInfo *sinfo, PGresult *res
*** 1204,1218
if (sinfo-cstrs)
pfree(sinfo-cstrs);
sinfo-cstrs = (char **) palloc(nfields * sizeof(char *));
-
- /* Create short-lived memory context for data conversions */
- if (!sinfo-tmpcontext)
- sinfo-tmpcontext =
- AllocSetContextCreate(CurrentMemoryContext,
- dblink temporary context,
- ALLOCSET_DEFAULT_MINSIZE,
- ALLOCSET_DEFAULT_INITSIZE,
- ALLOCSET_DEFAULT_MAXSIZE);
}
/* Should have a single-row result if we get here */
--- 1217,1222
--
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] [bug fix] Memory leak in dblink
From: Amit Kapila amit.kapil...@gmail.com Is there a need to free memory context in PG_CATCH()? In error path the memory due to this temporary context will get freed as it will delete the transaction context and this temporary context will definitely be in the hierarchy of it, so it should also get deleted. I think such handling can be useful incase we use PG_CATCH() to suppress the error. I thought the same, but I also felt that I should make an effort to release resources as soon as possible, considering the memory context auto deletion as a last resort. However, looking at other places where PG_CATCH() is used, memory context is not deleted. So, I removed the modification from PG_CATCH() block. Thanks. Regards MauMau dblink_memleak_v2.patch Description: Binary data -- 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] [bug fix] Memory leak in dblink
On Tue, Jun 10, 2014 at 12:27 AM, Amit Kapila amit.kapil...@gmail.com wrote: Is there a need to free memory context in PG_CATCH()? In error path the memory due to this temporary context will get freed as it will delete the transaction context and this temporary context will definitely be in the hierarchy of it, so it should also get deleted. I think such handling can be useful incase we use PG_CATCH() to suppress the error. Using PG_CATCH() to suppress an error is pretty much categorically unsafe. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] [bug fix] Memory leak in dblink
On Tue, Jun 10, 2014 at 7:30 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Jun 10, 2014 at 12:27 AM, Amit Kapila amit.kapil...@gmail.com wrote: Is there a need to free memory context in PG_CATCH()? In error path the memory due to this temporary context will get freed as it will delete the transaction context and this temporary context will definitely be in the hierarchy of it, so it should also get deleted. I think such handling can be useful incase we use PG_CATCH() to suppress the error. Using PG_CATCH() to suppress an error is pretty much categorically unsafe. In some cases like for handling exceptions in plpgsql, PG_CATCH() is used to handle the exception and then take an appropriate action based on exception, so in some such cases it might be right to free the context memory depending on situation. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] [bug fix] Memory leak in dblink
Amit Kapila amit.kapil...@gmail.com writes: On Tue, Jun 10, 2014 at 7:30 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Jun 10, 2014 at 12:27 AM, Amit Kapila amit.kapil...@gmail.com wrote: Is there a need to free memory context in PG_CATCH()? In error path the memory due to this temporary context will get freed as it will delete the transaction context and this temporary context will definitely be in the hierarchy of it, so it should also get deleted. I think such handling can be useful incase we use PG_CATCH() to suppress the error. Using PG_CATCH() to suppress an error is pretty much categorically unsafe. In some cases like for handling exceptions in plpgsql, PG_CATCH() is used to handle the exception and then take an appropriate action based on exception, so in some such cases it might be right to free the context memory depending on situation. Robert's point is that the only safe way to suppress an error is to do a (sub)transaction rollback. That will take care of cleaning up appropriate memory contexts, along with much else. I don't see the value of adding any single-purpose cleanups when they'd just be subsumed by the transaction rollback anyhow. The reason there's a PG_CATCH block here at all is to clean up libpq PGresults that the transaction rollback logic won't be aware of. We could instead have built infrastructure to allow rollback to clean those up, but it'd be a lot more code, for not a lot of benefit given the current complexity of dblink.c. Maybe sometime in the future it'll be worth doing it that way. 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] [bug fix] Memory leak in dblink
On Wed, Jun 11, 2014 at 10:46 AM, Tom Lane t...@sss.pgh.pa.us wrote: Amit Kapila amit.kapil...@gmail.com writes: In some cases like for handling exceptions in plpgsql, PG_CATCH() is used to handle the exception and then take an appropriate action based on exception, so in some such cases it might be right to free the context memory depending on situation. Robert's point is that the only safe way to suppress an error is to do a (sub)transaction rollback. That will take care of cleaning up appropriate memory contexts, along with much else. I don't see the value of adding any single-purpose cleanups when they'd just be subsumed by the transaction rollback anyhow. Agreed in general there won't be need of any such special-purpose cleanups and that's the main reason I have mentioned upthread to remove context cleanup from PG_CATCH(), however for certain special case where some situation want to allocate memory in context higher level than transaction to retain data across transaction boundary, it might be needed. This is just a hypothetical scenario came to my mind and I am not sure if there will be any actual need for such a thing. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] [bug fix] Memory leak in dblink
On Mon, Jun 9, 2014 at 10:07 AM, MauMau maumau...@gmail.com wrote: Hello, I've fixed and tested a memory leak bug in dblink. Could you review and commit this? I'll add this to the CommitFest shortly. I think there no need to add it to the commitfest, because it's a bugfix and not a new feature. Or am I missing something? Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
Re: [HACKERS] [bug fix] Memory leak in dblink
From: Fabrízio de Royes Mello fabriziome...@gmail.com I think there no need to add it to the commitfest, because it's a bugfix and not a new feature. Or am I missing something? The CommitFest app has an option bug fix in the list of topic choices. I suppose the reason is that if the bug fix is only posted to pgsql-hackers and/or pgsql-bugs, it might be forgotten. Regards MauMau -- 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] [bug fix] Memory leak in dblink
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/09/2014 08:58 AM, MauMau wrote: From: Fabrízio de Royes Mello fabriziome...@gmail.com I think there no need to add it to the commitfest, because it's a bugfix and not a new feature. Or am I missing something? The CommitFest app has an option bug fix in the list of topic choices. I suppose the reason is that if the bug fix is only posted to pgsql-hackers and/or pgsql-bugs, it might be forgotten. Yes, please add it to the commitfest and I'll claim it. Thanks, Joe - -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 Support -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTlccRAAoJEDfy90M199hln1oP/i0FO8d9j6c8TAbmDHHh3p2Q xjKvUSmnk6crAZR43M5wkUt3bj/qp58evYLJG6x0i71tJLGVjHByT2GrfFTjyWdB hfBQy7Su1t6QsqXuOEvL4KksscRp8zGQC+vqBks69zbfi3IcfF0nAnnHzk+qWmfL WZ7k7hhbtI03llWU7QB/JZxjKt8H1tR1kauDrQroZv0uNL4qbG6darLxt53h9WaG 0K1m/iVdrhbSYzxwMzdrvKhtYexLWA1iLje6u9lZSYhXQtTD/J+gCcSkE+VngF9I hjVixfnWbB+Y8VF2Fwee0wbIV0C/9L1OVodFFIaGIPyLUc2bbSI9KkknK4CCfR3M s7/mpSUPod4JKZxmNNSll/ituUV1sWq9DJ1RhiXqLU+dAxCQGTG5jxw9dHBwC0fO giQ/srh0lnR6C3SOjgGb3mC1+uNPxNWJOt+kyL+5GIQ+RyRFBPfo5hMvlwgUnj/V 764CAJIn2IpoqEondkKRGVfJMEp3Xg/WhlXEed/hMGwoC7DT0z1GKXxWBuqJ74eA YYAp8EeHREIs0SMcPT9qUi/iSvS3jbq6U0BQM/qRPNE6yEujJ630VXHpgwTCCcuB 37yRrGl++J91UGY3pSPCOzq14G9Mscfm4a55MD+4Svuf5lOTTAr9BLzjs6XftHev 5Mx+HaOg58xVRQBA6NVB =F4pB -END PGP SIGNATURE- -- 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] [bug fix] Memory leak in dblink
On Mon, Jun 9, 2014 at 6:37 PM, MauMau maumau...@gmail.com wrote: Hello, I've fixed and tested a memory leak bug in dblink. Could you review and commit this? I'll add this to the CommitFest shortly. Is there a need to free memory context in PG_CATCH()? In error path the memory due to this temporary context will get freed as it will delete the transaction context and this temporary context will definitely be in the hierarchy of it, so it should also get deleted. I think such handling can be useful incase we use PG_CATCH() to suppress the error. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
