Re: crash in plancache with subtransactions

I wrote:
>> Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
>>> One simple idea is to keep a flag along with the executor state to
>>> indicate that the executor state is currently in use. Set it just before
>>> calling ExecEvalExpr, and reset afterwards. If the flag is already set
>>> in the beginning of exec_eval_simple_expr, we have recursed, and must
>>> create a new executor state.

>> Yeah, the same thought occurred to me in the shower this morning.
>> I'm concerned about possible memory leakage during repeated recursion,
>> but maybe that can be dealt with.

I spent quite a bit of time trying to deal with the memory-leakage
problem without adding still more bookkeeping overhead. It wasn't
looking good, and then I had a sudden insight: if we see that the in-use
flag is set, we can simply return FALSE from exec_eval_simple_expr.
That causes exec_eval_expr to run the expression using the "non simple"
code path, which is perfectly safe because it isn't trying to reuse
state that might be dirty. Thus the attached patch, which fixes
both of the failure cases discussed in this thread.

Advantages:
1. The slowdown for "normal" cases (non-recursive, non-error-inducing)
is negligible.
2. It's simple enough to back-patch without fear.

Disadvantages:
1. Recursive cases get noticeably slower, about 4X slower according
to tests with this example:

create or replace function factorial(n int) returns float8 as $$
begin
if (n > 1) then
return n * factorial(n - 1);
end if;
return 1;
end
$$ language plpgsql strict immutable;

The slowdown is only for the particular expression that actually has
invoked a recursive call, so the above is probably much the worst case
in practice. I doubt many people really use plpgsql this way, but ...

2. Cases where we're re-executing an expression that failed earlier in
the same transaction likewise get noticeably slower. This is only a
problem if you're using subtransactions to catch errors, and the
overhead of the subtransaction is going to be large enough to partially
hide the extra eval cost anyway. So I didn't bother to make a timing
test case --- it's not going to be as bad as the example above.

I currently think that we don't have much choice except to use this
patch for the back branches: any better-performing alternative is
going to require enough surgery that back-patching would be dangerous.
Maybe somebody can come up with a better answer for HEAD, but I don't
have one.

Comments?

regards, tom lane