Re: calling procedures is slow and consumes extra much memory against calling function - Mailing list pgsql-hackers

From Pavel Stehule
Subject Re: calling procedures is slow and consumes extra much memory against calling function
Date
Msg-id CAFj8pRBMDBxFVEMnz8Nd-xKt6zuu7KQoh4EXVn5v71Gpwv86mg@mail.gmail.com
Whole thread Raw
In response to Re: calling procedures is slow and consumes extra much memory against calling function  (Tom Lane <tgl@sss.pgh.pa.us>)
Responses Re: calling procedures is slow and consumes extra much memory against calling function
List pgsql-hackers


po 28. 9. 2020 v 3:04 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
Pavel Stehule <pavel.stehule@gmail.com> writes:
> I am sending another patch that tries to allow CachedPlans for CALL
> statements. I think this patch is very accurate, but it is not nice,
> because it is smudging very precious reference counting for CachedPlans.

I spent some time testing this.  Although the #1 patch gets rid of
the major memory leak of cached plans, the original test case still
shows a pretty substantial leak across repeated executions of a CALL.
The reason is that the stanza for rebuilding stmt->target also gets
executed each time through, and that leaks not only the relatively
small PLpgSQL_row datum but also a bunch of catalog lookup cruft
created on the way to building the datum.  Basically this code forgot
that plpgsql's outer execution layer can't assume that it's running
in a short-lived context.

I attach a revised #1 that takes care of that problem, and also
cleans up what seems to me to be pretty sloppy thinking in both
the original code and Pavel's #1 patch: we should be restoring
the previous value of expr->plan, not cavalierly assuming that
it was necessarily NULL.  I didn't care for looking at the plan's
"saved" field to decide what was happening, either.  We really
should have a local flag variable clearly defining which behavior
it is that we're implementing.

With this patch, I see zero memory bloat on Pavel's original example,
even with a much larger repeat count.

I don't like much of anything about plpgsql-stmt_call-fix-2.patch.
It feels confused and poorly documented, possibly because "fragile"
is not a very clear term for whatever property it is you're trying to
attribute to plans.  But in any case, I think it's fixing the problem
in the wrong place.  I think the right way to fix it probably is to
manage a CALL's saved plan the same as every other plpgsql plan,
but arrange for the transient refcount on that plan to be held by a
ResourceOwner that is not a child of any transaction resowner, but
rather belongs to the procedure's execution and will be released on
the way out of the procedure.

In any case, I doubt we'd risk back-patching either the #2 patch
or any other approach to avoiding the repeat planning.  We need a
back-patchable fix that at least tamps down the memory bloat,
and this seems like it'd do.

I agree with these conclusions.  I'll try to look if I can do #2 patch better for pg14. Probably it can fix more issues related to CALL statement, and I agree so this should not be backapatched.

It can be great to use CALL without memory leaks (and it can be better (in future) if the performance of CALL statements should be good).

Thank you for enhancing and fixing this patch

Regards

Pavel


                        regards, tom lane

pgsql-hackers by date:

Previous
From: Ajin Cherian
Date:
Subject: Re: [HACKERS] logical decoding of two-phase transactions
Next
From: Amit Kapila
Date:
Subject: Re: Parallel copy