Re: "tupdesc reference is not owned by resource owner Portal"

The following testcase(extracted from a much much larger production code
sample) results in

WARNING: TupleDesc reference leak: TupleDesc 0xb3573b88 (2249,1) still
referenced
CONTEXT: PL/pgSQL function "foo" line 4 at block variables initialization
ERROR: tupdesc reference 0xb3573b88 is not owned by resource owner Portal
CONTEXT: PL/pgSQL function "foo" while casting return value to
function's return type

on 8.2 and -HEAD.

8.1 seems to work fine.

Stefan

CREATE OR REPLACE FUNCTION public.foo() RETURNS INTEGER AS $$
DECLARE
v_var INTEGER;
BEGIN
BEGIN
v_var := (bar()).error_code;
EXCEPTION WHEN others THEN
RETURN 0;
END;
RETURN 0;
END;
$$ LANGUAGE plpgsql;

CREATE OR REPLACE FUNCTION public.bar(OUT error_code INTEGER, OUT new_id
INTEGER) RETURNS RECORD AS $$
BEGIN
error_code := 1;
new_id := 1;
RETURN;
END;
$$ LANGUAGE plpgsql;

SELECT * FROM public.foo();

Stefan Kaltenbrunner <stefan(at)kaltenbrunner(dot)cc> writes:
> The following testcase(extracted from a much much larger production code
> sample) results in

> WARNING: TupleDesc reference leak: TupleDesc 0xb3573b88 (2249,1) still
> referenced
> CONTEXT: PL/pgSQL function "foo" line 4 at block variables initialization
> ERROR: tupdesc reference 0xb3573b88 is not owned by resource owner Portal
> CONTEXT: PL/pgSQL function "foo" while casting return value to
> function's return type

Hmm. What's happening is that the record-function call creates a
reference-counted TupleDesc, and tracking of the TupleDesc is
assigned to the subtransaction resource owner because we're inside
an EXCEPTION-block subtransaction. But the pointer is held by the
function's eval_context which lives throughout the function call,
and so the free happens long after exiting the subtransaction, and
the resource owner code quite properly complains about this.

In this particular case the worst consequence would be a short-term
memory leak, but I think there are probably variants with worse
problems, because anything done by a RegisterExprContextCallback()
callback is equally at risk.

I think the proper fix is probably to establish a new eval_context
when we enter an EXCEPTION block, and destroy it again on the way out.
Slightly annoying, but probably small next to the other overhead of
a subtransaction. Comments?

BTW, both of the CONTEXT lines are misleading. The WARNING happens
during exit from the begin-block, not entry to it; and the ERROR
happens after we've finished fooling with the result value. I'm
tempted to add a few more assignments to err_text to make this nicer.

regards, tom lane

Tom Lane wrote:
> Stefan Kaltenbrunner <stefan(at)kaltenbrunner(dot)cc> writes:
>> The following testcase(extracted from a much much larger production code
>> sample) results in
>
>> WARNING: TupleDesc reference leak: TupleDesc 0xb3573b88 (2249,1) still
>> referenced
>> CONTEXT: PL/pgSQL function "foo" line 4 at block variables initialization
>> ERROR: tupdesc reference 0xb3573b88 is not owned by resource owner Portal
>> CONTEXT: PL/pgSQL function "foo" while casting return value to
>> function's return type
>
> Hmm. What's happening is that the record-function call creates a
> reference-counted TupleDesc, and tracking of the TupleDesc is
> assigned to the subtransaction resource owner because we're inside
> an EXCEPTION-block subtransaction. But the pointer is held by the
> function's eval_context which lives throughout the function call,
> and so the free happens long after exiting the subtransaction, and
> the resource owner code quite properly complains about this.
>
> In this particular case the worst consequence would be a short-term
> memory leak, but I think there are probably variants with worse
> problems, because anything done by a RegisterExprContextCallback()
> callback is equally at risk.
>
> I think the proper fix is probably to establish a new eval_context
> when we enter an EXCEPTION block, and destroy it again on the way out.
> Slightly annoying, but probably small next to the other overhead of
> a subtransaction. Comments?

we use exception blocks heavily here so anything that makes them slower
is not nice but if it fixes the issue at hand I'm all for it ...

>
> BTW, both of the CONTEXT lines are misleading. The WARNING happens
> during exit from the begin-block, not entry to it; and the ERROR
> happens after we've finished fooling with the result value. I'm
> tempted to add a few more assignments to err_text to make this nicer.

yeah wondered about that too when I tried to produce a simple testcase -
the errors did't seem to make much sense in the context of what
triggered them. Improving that would be a very godd thing to do.

Stefan

Stefan Kaltenbrunner <stefan(at)kaltenbrunner(dot)cc> writes:
> Tom Lane wrote:
>> I think the proper fix is probably to establish a new eval_context
>> when we enter an EXCEPTION block, and destroy it again on the way out.
>> Slightly annoying, but probably small next to the other overhead of
>> a subtransaction. Comments?

> we use exception blocks heavily here so anything that makes them slower
> is not nice but if it fixes the issue at hand I'm all for it ...

This turned out a bit uglier than I thought --- the real problem is that
plpgsql's "simple eval econtext" management is much too stupid to
survive in a subtransaction world. There was a comment in the code
worrying about this, but I guess we never investigated closely.

The attached patch (against 8.2) appears to fix the reported problem,
but it could use some more testing before I push it into the stable
branches. Can you try it in the production situation that exposed the
problem? Aside from not failing, do you see any performance loss?

regards, tom lane

Tom Lane wrote:
> Stefan Kaltenbrunner <stefan(at)kaltenbrunner(dot)cc> writes:
>> Tom Lane wrote:
>>> I think the proper fix is probably to establish a new eval_context
>>> when we enter an EXCEPTION block, and destroy it again on the way out.
>>> Slightly annoying, but probably small next to the other overhead of
>>> a subtransaction. Comments?
>
>> we use exception blocks heavily here so anything that makes them slower
>> is not nice but if it fixes the issue at hand I'm all for it ...
>
> This turned out a bit uglier than I thought --- the real problem is that
> plpgsql's "simple eval econtext" management is much too stupid to
> survive in a subtransaction world. There was a comment in the code
> worrying about this, but I guess we never investigated closely.
>
> The attached patch (against 8.2) appears to fix the reported problem,
> but it could use some more testing before I push it into the stable
> branches. Can you try it in the production situation that exposed the
> problem? Aside from not failing, do you see any performance loss?

thanks - this seems to fix the problem on the development system but it
might take a while to get some performance testing done.

Stefan