Subtransactions and resource owners and such - Mailing list pgsql-hackers

I've been looking into Frank van Vugt's report here:
http://archives.postgresql.org/pgsql-bugs/2009-07/msg00222.php

The cause of the problem is that while printing the value of the
ROW(NEW.*) expression, we acquire a tupledesc refcount on the
table's rowtype descriptor, and this refcount is assigned to the
CurrentResourceOwner, which at that point is the resowner associated
with the Portal the query is executing in.  During the later
subtransaction abort, we try to release the refcount.  But at that
point CurrentResourceOwner has been reset to the subtransaction's
resowner, and so we get
ERROR:  tupdesc reference 0x7ffe74f24ad0 is not owned by resource owner SubTransaction
which then results in a lot of WARNING bleats that look scary but
I think are not really problems.

The quick-hack patch I suggested to Frank avoids the issue by not
trying to clean up plpgsql's per-subtransaction ExprContexts during
a subtransaction abort.  This is not very satisfactory, though, as
it reintroduces the memory leaks I was trying to solve here:
http://archives.postgresql.org/pgsql-committers/2009-04/msg00127.php
For example, this function

create or replace function test1(int) returns void as
$$
declare xx text; yy int;
begin for i in 1..$1 loop   begin     xx := repeat('x',1000);     yy := i / 0;   exception     when division_by_zero
then      null;   end; end loop;
 
end$$
language plpgsql;

works fine in 8.4.0 but creates a nasty memory leak with that patch.
(Try running it with a repeat count of a million or so and watch
the backend's memory usage.)

I have thought of a number of possible solutions that might avoid
leakage here:

1. Modify FreeExprContext() so that it's told whether this is a normal
or abort cleanup.  In the abort case it skips calling any registered
callbacks, but still releases the memory belonging to the context.

2. Pass a normal/abort flag to FreeExprContext as above, but have it
still call the callbacks and pass the flag on to them.  This would
provide an opportunity for callbacks to do something during abort,
if they needed to.  However an API change here seems a bit invasive.
I'm not sure if any third-party code uses RegisterExprContextCallback.
I'm also unconvinced that we really need to give the callbacks a
chance to do anything there.  We've never called them during regular
transaction abort.

3. When aborting a transaction or subtransaction, arrange to fold
all child resowners into the (sub)transaction's topmost resowner;
that is, reassign all resources they own to that parent.  Then,
resource cleanup actions would automatically be applied to the correct
resowner during abort cleanup.  This would require a bunch of code
in resowner.c that doesn't currently exist, and I'm also a tad concerned
about the cycles it would take.

I'm currently thinking #1 is the most practical answer, though it might
leave us still with some leakage problems if it turns out there are any
ExprContext callbacks that really need to be called in such cases.
We might want to do #2 in HEAD, but committing it into 8.4.x seems to
risk breaking third-party code.  #3 seems like overkill.

Any comments or better ideas?
        regards, tom lane