Back in [1], Andres complained that repeated attempts to create
an invalid plpgsql function (one that fails initial compilation)
leak memory, for example
DO $do$
BEGIN
FOR i IN 1 .. 100000 LOOP
BEGIN
CREATE OR REPLACE FUNCTION foo() RETURNS VOID
LANGUAGE plpgsql AS $f$BEGIN frakbar; END;$f$;
EXCEPTION WHEN others THEN
END;
END LOOP;
END;$do$;
The reason is that we create the long-lived function cache context
and detect the syntax error only while trying to fill it in.
As I remarked at the time, we could make that better by making
the cache context initially short-lived and reparenting it only
after it's known good. The attached patch does that.
I noted that the CachedFunction struct made by funccache.c gets
leaked too. (That's not new, but the blame used to fall on plpgsql's
equivalent of that code.) That's not hard to fix in typical cases,
at the price of an extra PG_TRY, which seems okay in a code path that
is setting up a long-lived cache entry. Also done in the attached.
I thought that SQL-language functions might have this issue too,
but they do not, because sql_compile_callback already uses the
reparenting trick. (I followed its lead in making the function
contexts live under CacheMemoryContext not TopMemoryContext.)
If you run the above example long enough, you will also observe a
slow leak in TopTransactionContext. AFAICT that is from accumulating
invalidation messages from the failed pg_proc insertions, so it's not
specific to functions but applies to any DDL in a loop. Fixing that
seems outside the scope of this patch.
I think this is a bug fix, so I'm inclined to squeeze it into v18.
I am not sure if it's worth developing a back-patchable version.
The pl_comp.c change probably applies easily further back, and
would be enough to get the bulk of the benefit.
Comments?
regards, tom lane
[1]
https://www.postgresql.org/message-id/20210317055718.v6qs3ltzrform...@alap3.anarazel.de
diff --git a/src/backend/utils/cache/funccache.c b/src/backend/utils/cache/funccache.c
index 150c502a612..a5e9408cf55 100644
--- a/src/backend/utils/cache/funccache.c
+++ b/src/backend/utils/cache/funccache.c
@@ -491,6 +491,7 @@ cached_function_compile(FunctionCallInfo fcinfo,
CachedFunctionHashKey hashkey;
bool function_valid = false;
bool hashkey_valid = false;
+ bool new_function = false;
/*
* Lookup the pg_proc tuple by Oid; we'll need it in any case
@@ -570,13 +571,15 @@ recheck:
/*
* Create the new function struct, if not done already. The function
- * structs are never thrown away, so keep them in TopMemoryContext.
+ * cache entry will be kept for the life of the backend, so put it in
+ * TopMemoryContext.
*/
Assert(cacheEntrySize >= sizeof(CachedFunction));
if (function == NULL)
{
function = (CachedFunction *)
MemoryContextAllocZero(TopMemoryContext, cacheEntrySize);
+ new_function = true;
}
else
{
@@ -585,17 +588,35 @@ recheck:
}
/*
- * Fill in the CachedFunction part. fn_hashkey and use_count remain
- * zeroes for now.
+ * However, if function compilation fails, we'd like not to leak the
+ * function struct, so use a PG_TRY block to prevent that. (It's up
+ * to the compile callback function to avoid its own internal leakage
+ * in such cases.) Unfortunately, freeing the struct is only safe if
+ * we just allocated it: otherwise there are probably fn_extra
+ * pointers to it.
*/
- function->fn_xmin = HeapTupleHeaderGetRawXmin(procTup->t_data);
- function->fn_tid = procTup->t_self;
- function->dcallback = dcallback;
+ PG_TRY();
+ {
+ /*
+ * Fill in the CachedFunction part. fn_hashkey and use_count
+ * remain zeroes for now.
+ */
+ function->fn_xmin = HeapTupleHeaderGetRawXmin(procTup->t_data);
+ function->fn_tid = procTup->t_self;
+ function->dcallback = dcallback;
- /*
- * Do the hard, language-specific part.
- */
- ccallback(fcinfo, procTup, &hashkey, function, forValidator);
+ /*
+ * Do the hard, language-specific part.
+ */
+ ccallback(fcinfo, procTup, &hashkey, function, forValidator);
+ }
+ PG_CATCH();
+ {
+ if (new_function)
+ pfree(function);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
/*
* Add the completed struct to the hash table.
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index 5f3e8646fbb..22af973aae2 100644
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -230,8 +230,13 @@ plpgsql_compile_callback(FunctionCallInfo fcinfo,
/*
* All the permanent output of compilation (e.g. parse tree) is kept in a
* per-function memory context, so it can be reclaimed easily.
+ *
+ * While the func_cxt needs to be long-lived, we initially make it a child
+ * of the assumed-short-lived caller's context, and reparent it under
+ * CacheMemoryContext only upon success. This arrangement avoids memory
+ * leakage during compilation of a faulty function.
*/
- func_cxt = AllocSetContextCreate(TopMemoryContext,
+ func_cxt = AllocSetContextCreate(CurrentMemoryContext,
"PL/pgSQL function",
ALLOCSET_DEFAULT_SIZES);
plpgsql_compile_tmp_cxt = MemoryContextSwitchTo(func_cxt);
@@ -707,6 +712,11 @@ plpgsql_compile_callback(FunctionCallInfo fcinfo,
if (plpgsql_DumpExecTree)
plpgsql_dumptree(function);
+ /*
+ * All is well, so make the func_cxt long-lived
+ */
+ MemoryContextSetParent(func_cxt, CacheMemoryContext);
+
/*
* Pop the error context stack
*/
