Leaking memory in text_overlay function

Started by Dirk Rudolphover 9 years ago4 messages

dirk.rudolph@netcentric.biz

over 9 years ago

1 attachment(s)

Hi,

while implementing my own C function, I mentioned that some memory is not freed by the text_overlay function in varlena.c

Particularly I mean the both substrings allocated by text_substring (according to the documentation of text_substring "The result is always a freshly palloc'd datum.") and one of the concatenation results. I’m aware of the MemoryContext being deleted in my case but IMHO builtin functions should be memory safe. (or at least the others I used are).

See attached a patch that fixes that against HEAD.

Cheers,
Dirk Rudolph | Senior Software Engineer

Netcentric AG

M: +41 79 642 37 11
D: +49 174 966 84 34

dirk.rudolph@netcentric.biz <mailto:dirk.rudolph@netcentric.biz> | www.netcentric.biz <http://www.netcentric.biz/>

Attachments:

text_overlay_pfree.patchapplication/octet-stream; name=text_overlay_pfree.patchDownload

diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index a869e85..bd8db41 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -1034,7 +1034,8 @@ textoverlay_no_len(PG_FUNCTION_ARGS)
 static text *
 text_overlay(text *t1, text *t2, int sp, int sl)
 {
-	text	   *result;
+	text	   *result1;
+	text	   *result2;
 	text	   *s1;
 	text	   *s2;
 	int			sp_pl_sl;
@@ -1056,10 +1057,14 @@ text_overlay(text *t1, text *t2, int sp, int sl)
 
 	s1 = text_substring(PointerGetDatum(t1), 1, sp - 1, false);
 	s2 = text_substring(PointerGetDatum(t1), sp_pl_sl, -1, true);
-	result = text_catenate(s1, t2);
-	result = text_catenate(result, s2);
+	result1 = text_catenate(s1, t2);
+	result2 = text_catenate(result1, s2);
 
-	return result;
+	pfree(s1);
+	pfree(s2);
+	pfree(result1);
+
+	return result2;
 }
 
 /*

Tom Lane

tgl@sss.pgh.pa.us

over 9 years ago

In reply to: Dirk Rudolph (#1)

Re: Leaking memory in text_overlay function

Dirk Rudolph <dirk.rudolph@netcentric.biz> writes:

while implementing my own C function, I mentioned that some memory is not freed by the text_overlay function in varlena.c

By and large, that's intentional. SQL-called functions normally run
in short-lived memory contexts, so that any memory they don't bother to
pfree will be cleaned up automatically at the end of the tuple cycle.
If we did not follow that approach, there would need to be many thousands
more explicit pfree calls than there are today, and the system would be
net slower overall because multiple retail pfree's are slower than a
MemoryContextReset.

There are places where it's important to avoid leaking memory, certainly.
But I don't think this is one of them, unless you can demonstrate a
scenario with query-lifespan or worse leakage.

Particularly I mean the both substrings allocated by text_substring (according to the documentation of text_substring "The result is always a freshly palloc'd datum.") and one of the concatenation results. I’m aware of the MemoryContext being deleted in my case but IMHO builtin functions should be memory safe.

That is not the project policy.

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

Dirk Rudolph

dirk.rudolph@netcentric.biz

over 9 years ago

In reply to: Tom Lane (#2)

Re: Leaking memory in text_overlay function

Well that's good to know. It was just curious that, in my case, I only saw
this in this particular function.

Anyway. I will consider to handle the memory the same way, if this is the
recommendation.

Many thanks.

/Closed

On Sat, Jul 2, 2016 at 4:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Dirk Rudolph <dirk.rudolph@netcentric.biz> writes:

while implementing my own C function, I mentioned that some memory is

not freed by the text_overlay function in varlena.c

By and large, that's intentional. SQL-called functions normally run
in short-lived memory contexts, so that any memory they don't bother to
pfree will be cleaned up automatically at the end of the tuple cycle.
If we did not follow that approach, there would need to be many thousands
more explicit pfree calls than there are today, and the system would be
net slower overall because multiple retail pfree's are slower than a
MemoryContextReset.

There are places where it's important to avoid leaking memory, certainly.
But I don't think this is one of them, unless you can demonstrate a
scenario with query-lifespan or worse leakage.

Particularly I mean the both substrings allocated by text_substring

(according to the documentation of text_substring "The result is always a
freshly palloc'd datum.") and one of the concatenation results. I’m aware
of the MemoryContext being deleted in my case but IMHO builtin functions
should be memory safe.

That is not the project policy.

regards, tom lane

Dirk Rudolph | Senior Software Engineer

Netcentric AG

M: +41 79 642 37 11
D: +49 174 966 84 34

dirk.rudolph@netcentric.biz | www.netcentric.biz

Anastasia Lubennikova

a.lubennikova@postgrespro.ru

over 9 years ago

In reply to: Dirk Rudolph (#3)

Re: Leaking memory in text_overlay function

I found this thread on the open CF. As I see, the discussion is ended
with decision to reject patch.

I've just changed the status to "Rejected".

02.07.2016 18:11, Dirk Rudolph:

Well that's good to know. It was just curious that, in my case, I only
saw this in this particular function.

Anyway. I will consider to handle the memory the same way, if this is
the recommendation.

Many thanks.

/Closed

On Sat, Jul 2, 2016 at 4:45 PM, Tom Lane <tgl@sss.pgh.pa.us
<mailto:tgl@sss.pgh.pa.us>> wrote:

Dirk Rudolph <dirk.rudolph@netcentric.biz
<mailto:dirk.rudolph@netcentric.biz>> writes:

while implementing my own C function, I mentioned that some

memory is not freed by the text_overlay function in varlena.c

By and large, that's intentional. SQL-called functions normally run
in short-lived memory contexts, so that any memory they don't
bother to
pfree will be cleaned up automatically at the end of the tuple cycle.
If we did not follow that approach, there would need to be many
thousands
more explicit pfree calls than there are today, and the system
would be
net slower overall because multiple retail pfree's are slower than a
MemoryContextReset.

There are places where it's important to avoid leaking memory,
certainly.
But I don't think this is one of them, unless you can demonstrate a
scenario with query-lifespan or worse leakage.

Particularly I mean the both substrings allocated by

text_substring (according to the documentation of text_substring
"The result is always a freshly palloc'd datum.") and one of the
concatenation results. I’m aware of the MemoryContext being
deleted in my case but IMHO builtin functions should be memory safe.

That is not the project policy.

regards, tom lane

--

Dirk Rudolph | Senior Software Engineer

Netcentric AG

M: +41 79 642 37 11
D: +49 174 966 84 34

dirk.rudolph@netcentric.biz <mailto:dirk.rudolph@netcentric.biz> |
www.netcentric.biz <http://www.netcentric.biz/>

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company