Re: [PATCH v1] fix potential memory leak in untransformRelOptions

On 2022-Sep-01, Tom Lane wrote:

> Junwang Zhao <zhjwpku@gmail.com> 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 Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/