Avoiding memory leak when compilation of a function fails

On Tue, May 27, 2025 at 4:23 AM Tom Lane <tgl@sss.pgh.pa.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

út 27. 5. 2025 v 7:27 odesílatel Amul Sul <sulamul@gmail.com> napsal:

On Tue, May 27, 2025 at 4:23 AM Tom Lane <tgl@sss.pgh.pa.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.

+1

Regards

Pavel