I wrote: > Amit Langote <amitlangot...@gmail.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