[bug fix] Memory leak in dblink
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.
[Problem]
A user reported a problem in pgsql-jp ML that he encountered a "out of
memory" error when he ran the ran the following function on 32-bit
PostgreSQL 9.3:
CREATE OR REPLACE FUNCTION aaa(
character varying)
RETURNS character varying AS
$BODY$
DECLARE
...
BEGIN
PERFORM (SELECT DBLINK_CONNECT('conn','dbname=DB-B user=postgres'));
DELETE FROM tbl0010 dba
WHERE EXISTS
(
SELECT tbl0010_cd FROM tbl0010
INNER JOIN (
SELECT * FROM DBLINK
('conn','
SELECT tbl0411_cd FROM tbl0411
INNER JOIN(
...
The above query calls dblink() hundreds of thousands of times. You should
reproduce the problem with a simpler query like this:
CREATE TABLE mytable (col int);
INSERT INTO mytable ...; /* insert many rows */
SELECT *
FROM mytable
WHERE EXISTS
(SELECT *
FROM dblink(
'con',
'SELECT * FROM mytable WHERE col = ' || col)
t(col));
[Cause]
Hundreds of thousands of the following same line were output in the server
log:
dblink temporary context: 8192 total in 1 blocks; 8176 free (0 chunks); 16
used
Each dblink function call creates an instance of this memory context, but it
fails to delete it. This bug seems to have been introduced in 9.2.0 by this
performance improvement:
Improve efficiency of dblink by using libpq's new single-row
processingmode(Kyotaro Horiguchi, Marko Kreen)
Regards
MauMau
Attachments:
dblink_memleak.patchapplication/octet-stream; name=dblink_memleak.patchDownload+4-0
On Mon, Jun 9, 2014 at 10:07 AM, MauMau <maumau307@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
Show quoted text
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
From: "Fabrízio de Royes Mello" <fabriziomello@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
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 06/09/2014 08:58 AM, MauMau wrote:
From: "Fabrízio de Royes Mello" <fabriziomello@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
On Mon, Jun 9, 2014 at 6:37 PM, MauMau <maumau307@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
From: "Amit Kapila" <amit.kapila16@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
Attachments:
dblink_memleak_v2.patchapplication/octet-stream; name=dblink_memleak_v2.patchDownload+2-0
On Tue, Jun 10, 2014 at 12:27 AM, Amit Kapila <amit.kapila16@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
On Tue, Jun 10, 2014 at 7:30 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Jun 10, 2014 at 12:27 AM, Amit Kapila <amit.kapila16@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
Amit Kapila <amit.kapila16@gmail.com> writes:
On Tue, Jun 10, 2014 at 7:30 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Jun 10, 2014 at 12:27 AM, Amit Kapila <amit.kapila16@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
On Wed, Jun 11, 2014 at 10:46 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Amit Kapila <amit.kapila16@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
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 06/10/2014 02:57 AM, MauMau wrote:
From: "Amit Kapila" <amit.kapila16@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-----Attachments:
dblink_memleak_v3.patchtext/x-diff; name=dblink_memleak_v3.patchDownload+12-9
Joe Conway <mail@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
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 06/18/2014 12:09 PM, Tom Lane wrote:
Joe Conway <mail@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
Joe Conway <mail@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
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
Attachments:
ExecMakeTableFunctionResult-mem-leak.patchtext/x-diff; charset=us-ascii; name=ExecMakeTableFunctionResult-mem-leak.patchDownload+22-9
-----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
Joe Conway <mail@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
-----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
Joe Conway <mail@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
Joe Conway <mail@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
