Re: plan cache overhead on plpgsql expression

I wrote:
> Amit Langote <amitlangote09(at)gmail(dot)com> writes:
>> One thing -- I don't get the division between
>> CachedPlanAllowsSimpleValidityCheck() and CachedPlanIsSimplyValid().
>> Maybe I am missing something, but could there not be just one
>> function, possibly using whether expr_simple_expr is set or not to
>> skip or do, resp., the checks that the former does?

> Well, we don't want to do the initial checks over again every time;
> we want the is-valid test to be as simple and fast as we can make it.
> I suppose we could have one function with a boolean flag saying "this is a
> recheck", but I don't find that idea to be any better than the way it is.

So after looking at the buildfarm results, I think you were on to
something. The initial and recheck conditions actually have to be
a bit different, and the reason is that immediately after GetCachedPlan
has produced a plan, it's possible for plansource->is_valid to be false
even though the derived plan is marked valid. (In the buildfarm, this
is happening because of CLOBBER_CACHE_ALWAYS or equivalent cache flushes;
in the real world it'd probably require sinval queue overflow to happen
while building the plan.)

What we want in this situation is to go ahead and use the derived plan,
and then rebuild next time; that's what the pre-existing code did and
it's really the only reasonable answer. It might seem better to go
back and try to rebuild the plan right away, but that'd be an infinite
loop in a CLOBBER_CACHE_ALWAYS build. Also, if we fail to use the
derived plan at all, that amounts to disabling the "simple expression"
optimization as a result of a chance sinval overflow. That's bad from
a performance standpoint and it will also cause regression test output
changes (since, as you previously discovered, the simple-expression
path produces different CONTEXT messages for error cases --- maybe we
should change that, but I don't want to be forced into it).

The existing code structure can't support doing it like that, so we have
to refactor to make the initial check and the recheck be separate code.
Working on a patch for that now.

regards, tom lane