[PATCH] Fix leak when a plpgsql exception block catches an error from CALL

Hi,

A PL/pgSQL procedure, called outside a transaction block, that calls
another procedure inside an exception block keeps the plan of every
CALL that fails until it returns, so a loop that catches such failures
uses memory without limit:

CREATE PROCEDURE p_fail(k int) LANGUAGE plpgsql AS $$
BEGIN
RAISE EXCEPTION 'fail %', k;
END $$;

CREATE PROCEDURE p_loop(n int) LANGUAGE plpgsql AS $$
DECLARE
c0 bigint;
c1 bigint;
BEGIN
SELECT count(*) INTO c0 FROM pg_backend_memory_contexts
WHERE name = 'CachedPlan';
FOR i IN 1..n LOOP
BEGIN
CALL p_fail(i);
EXCEPTION WHEN OTHERS THEN
NULL;
END;
END LOOP;
SELECT count(*) INTO c1 FROM pg_backend_memory_contexts
WHERE name = 'CachedPlan';
RAISE NOTICE '% more CachedPlan contexts', c1 - c0;
END $$;

CALL p_loop(20000);
NOTICE: 20005 more CachedPlan contexts

That is about 41MB, 2kB per caught failure, held until p_loop
returns; a COMMIT inside the loop does not release any of it. (The
other 5 contexts are the plans of the two procedures' own statements;
with the attached patch, the NOTICE says 5.) It is the same on master
and on the heads of 14 through 19. The pattern is an ordinary one
for a batch procedure: call a procedure for each row inside an
exception block, commit now and then, and log the rows that fail
instead of stopping. A million failed rows hold about 2GB until the
batch procedure returns.

The cause: since ee895a655ce, a non-atomic procedure that contains
CALL or DO statements pins each such statement's plan in a
procedure-lifespan resource owner, so that the pin survives a COMMIT
in the called procedure (exec_stmt_call() passes
estate->procedure_resowner to SPI_execute_plan_extended()).
_SPI_execute_plan() drops the pin when the statement is done, but an
error in the CALL (in the called procedure, or while evaluating its
arguments) skips that, and rolling back the exception block's
subtransaction does not touch the procedure's resource owner. A CALL
whose arguments use PL/pgSQL variables gets a new custom plan every
time, because choose_custom_plan() compares a generic cost of 0 with
an average custom cost of 0 and never switches to the generic plan;
that is why each failure keeps a whole CachedPlan and not just a
reference. Without such arguments, or with plan_cache_mode =
force_generic_plan, each failure still leaves a reference count and a
resource owner entry behind. (Separately, this means every CALL whose
arguments use variables builds and throws away a custom plan even when
it succeeds; that may deserve a look of its own, but this fix does not
depend on it.)

DO blocks have the same problem, in atomic contexts too:
plpgsql_inline_handler() uses the block's simple-expression resource
owner as the procedure resource owner whether or not the block is
atomic, so the same loop in a DO block inside BEGIN ... COMMIT keeps
every failed plan until the block ends. A procedure called in an
atomic context, or a function, is not affected.

The procedure-lifespan resource owner is only needed when the called
procedure can end the transaction, and that is only possible when
_SPI_execute_plan() runs the CALL non-atomically: in a non-atomic
context with no subtransaction active (the rule from c96de42c4b5,
which SPI_inside_nonatomic_context() also implements). The attached
patch uses it only when SPI_inside_nonatomic_context() is true, and
otherwise leaves options.owner NULL, so SPI uses the current resource
owner as it does for any other statement. Since an error from the
CALL can only be caught inside a subtransaction, every pin that used
to leak now belongs to that subtransaction (or one nested in it), and
aborting it releases the pin. This also brings DO blocks in line with
what ee895a655ce describes: "In an atomic context, we just use
CurrentResourceOwner, as before." The fix adds one cheap test per
CALL (timings below).

I also tried giving each CALL a short-lived child of the procedure
resource owner and releasing it in a PG_FINALLY block. That fixes the
leak too, but it creates and deletes a resource owner on every CALL,
which costs about 0.13-0.18us per CALL (median of 11 runs of 1,000,000
CALLs of an empty procedure, release build: +4% without arguments, +3%
with one), and it buys nothing over the above, since outside a
subtransaction an error from the CALL can't be caught anyway.

With the patch, 1,000,000 CALLs of an empty procedure from a
non-atomic procedure take 3.07s, against 3.01s without it (release
build, median of 11 runs; 6.21s against 6.31s with one argument), so
the difference is within run-to-run noise.

The patch adds a test to plpgsql_call.sql that counts CachedPlan
contexts around ten caught failures, in a procedure and in an atomic
DO block: without the fix it reports 10 for each, with the fix 0. It
gives the same output with debug_discard_caches = 1 and with either
forced plan_cache_mode. I found no existing test that uses
pg_backend_memory_contexts to catch a leak; if that is thought too
fragile for the regression suite, the test can be dropped. With the
fix, a loop like the one above keeps the backend's memory flat: across
20,000 caught failures, with or without a COMMIT every 100 rows, the
sum of total_bytes over pg_backend_memory_contexts does not change.

On master, check-world passes (without the TAP tests), and the PL/pgSQL
tests pass with the server running under Valgrind. The patch
cherry-picks cleanly to REL_14_STABLE through REL_19_STABLE; on each of
them the PL/pgSQL and core regression tests pass with it, and the new
test fails without it.

I think this should be back-patched to 14, as a6b1f5365d5 ("Fix memory
leak in plpgsql's CALL processing") was back-patched to 11.

Regards,
Muzzammil Sarwar