Re: Avoiding memory leak when compilation of a function fails

On Tue, May 27, 2025 at 4:23 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> 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?
>

The patch seems reasonable and the changes appear straightforward
enough for a backport. However, I am not sure about the backporting,
as the leak doesn't seem to occur very frequently.

Regards,
Amul