Re: [PATCH v1] fix potential memory leak in untransformRelOptions
On 2022-Sep-09, Alvaro Herrera wrote: > Keeping 's' and removing the pstrdups better uses memory, because we > have a single palloc'ed chunk per option rather than two. Pushed. This is pretty much cosmetic, so no backpatch. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "The Gord often wonders why people threaten never to come back after they've been told never to return" (www.actsofgord.com)
Re: [PATCH v1] fix potential memory leak in untransformRelOptions
On 2022-Sep-01, Tom Lane wrote:
> Junwang Zhao writes:
> > result = lappend(result, makeDefElem(pstrdup(s), val, -1));
> > + pfree(s);
>
> I wonder why it's pstrdup'ing s in the first place.
Yeah, I think both the pstrdups in that function are useless. The
DefElems can just point to the correct portion of the (already pstrdup'd
by TextDatumGetCString) copy of optiondatums[i]. We modify that copy to
install \0 in the place where the = is, and that copy is not freed
anywhere.
diff --git a/src/backend/access/common/reloptions.c
b/src/backend/access/common/reloptions.c
index 609329bb21..0aa4b334ab 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1357,9 +1357,9 @@ untransformRelOptions(Datum options)
if (p)
{
*p++ = '\0';
- val = (Node *) makeString(pstrdup(p));
+ val = (Node *) makeString(p);
}
- result = lappend(result, makeDefElem(pstrdup(s), val, -1));
+ result = lappend(result, makeDefElem(s, val, -1));
}
return result;
I think these pstrdups were already not necessary when the function was
added in 265f904d8f25, because textout() was already known to return a
palloc'ed copy of its input; but later 220db7ccd8c8 made this contract
even more explicit.
Keeping 's' and removing the pstrdups better uses memory, because we
have a single palloc'ed chunk per option rather than two.
--
Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: [PATCH v1] fix potential memory leak in untransformRelOptions
got it, thanks. Tom Lane 于2022年9月2日 周五01:13写道： > Junwang Zhao writes: > > I'm a little confused when we should call *pfree* and when we should not. > > A few lines before there is a call *text_to_cstring* in which it invokes > > *pfree* to free the unpacked text [0]. I'm just thinking that since *s* > has > > been duplicated, we should free it, that's where the patch comes from. > > By and large, the server is designed so that small memory leaks don't > matter: the space will be reclaimed when the current memory context > is deleted, and most code runs in reasonably short-lived contexts. > Individually pfree'ing such allocations is actually a net negative, > because it costs cycles and code space. > > There are places where a leak *does* matter, but unless you can > demonstrate that this is one, it's not worth changing. > > regards, tom lane > -- Regards Junwang Zhao
Re: [PATCH v1] fix potential memory leak in untransformRelOptions
Junwang Zhao writes: > I'm a little confused when we should call *pfree* and when we should not. > A few lines before there is a call *text_to_cstring* in which it invokes > *pfree* to free the unpacked text [0]. I'm just thinking that since *s* has > been duplicated, we should free it, that's where the patch comes from. By and large, the server is designed so that small memory leaks don't matter: the space will be reclaimed when the current memory context is deleted, and most code runs in reasonably short-lived contexts. Individually pfree'ing such allocations is actually a net negative, because it costs cycles and code space. There are places where a leak *does* matter, but unless you can demonstrate that this is one, it's not worth changing. regards, tom lane
Re: [PATCH v1] fix potential memory leak in untransformRelOptions
On Thu, Sep 1, 2022 at 10:10 PM Tom Lane wrote:
>
> Junwang Zhao writes:
> > result = lappend(result, makeDefElem(pstrdup(s), val, -1));
> > + pfree(s);
>
> I wonder why it's pstrdup'ing s in the first place.
>
Maybe it's pstrdup'ing s so that the caller should take care of the free?
I'm a little confused when we should call *pfree* and when we should not.
A few lines before there is a call *text_to_cstring* in which it invokes
*pfree* to free the unpacked text [0]. I'm just thinking that since *s* has
been duplicated, we should free it, that's where the patch comes from.
[0]:
```
char *
text_to_cstring(const text *t)
{
/* must cast away the const, unfortunately */
text *tunpacked = pg_detoast_datum_packed(unconstify(text *, t));
int len = VARSIZE_ANY_EXHDR(tunpacked);
char *result;
result = (char *) palloc(len + 1);
memcpy(result, VARDATA_ANY(tunpacked), len);
result[len] = '\0';
if (tunpacked != t)
pfree(tunpacked);
return result;
}
```
> regards, tom lane
--
Regards
Junwang Zhao
Re: [PATCH v1] fix potential memory leak in untransformRelOptions
Junwang Zhao writes: > result = lappend(result, makeDefElem(pstrdup(s), val, -1)); > + pfree(s); I wonder why it's pstrdup'ing s in the first place. regards, tom lane
Re: [PATCH v1] fix potential memory leak in untransformRelOptions
> On 1 Sep 2022, at 10:36, Junwang Zhao wrote: > *TextDatumGetCString* calls palloc to alloc memory for the option > text datum, in some cases the the memory is allocated in > *TopTransactionContext*, this may cause memory leak for a long > running backend. Wouldn't that be a fairly small/contained leak in comparison to memory spent during a long running transaction? Do you have any example of transforming reloptions in a loop into TopTransactionContext where it might add up? -- Daniel Gustafsson https://vmware.com/
