út 27. 5. 2025 v 7:27 odesílatel Amul Sul <sula...@gmail.com> napsal:
> On Tue, May 27, 2025 at 4:23 AM Tom Lane <t...@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 > > Regards, > Amul > > >
