Re: plan cache overhead on plpgsql expression

Hi,

On 2020-03-21 14:24:05 -0400, Tom Lane wrote:
> So while there's clearly something worth pursuing here, I do not like
> anything about the way it was done. I think that the right way to
> think about this problem is "how can the plan cache provide a fast
> path for checking validity of simple-expression plans?". And when you
> think about it that way, there's a pretty obvious answer: if the plan
> involves no table references, there's not going to be any locks that
> have to be taken before we can check the is_valid flag. So we can
> have a fast path that skips AcquirePlannerLocks and
> AcquireExecutorLocks, which are a big part of the problem, and we can
> also bypass some of the other random checks that GetCachedPlan has to
> make, like whether RLS affects the plan.

That makes sense to me.

I wonder if it'd make sense to store the locks needed for
AcquirePlannerLocks/AcquireExecutorLocks in a better form. Not really
instead of your optimization, but to also address simple statements that
do reference a relation. If we stored all the locks for a plansource in
an array, it should get cheaper - and automatically implement the fast
path of skipping AcquirePlannerLocks/AcquireExecutorLocks when there's
no relations.

> Another chunk of the issue is the constant acquisition and release of
> reference counts on the plan. We can't really skip that (I suspect
> there are additional bugs in Amit's patch arising from trying to do so).
> However, plpgsql already has mechanisms for paying simple-expression
> setup costs once per transaction rather than once per expression use.
> So we can set up a simple-expression ResourceOwner managed much like
> the simple-expression EState, and have it hold a refcount on the
> CachedPlan for each simple expression, and pay that overhead just once
> per transaction.

> I haven't done any serious performance testing on this, but it gives
> circa 2X speedup on Pavel's original example, which is at least
> fairly close to the results that Amit's patch got there. And it
> makes this last batch of test cases faster not slower, too.
>
> With this patch, perf shows the hotspots on Pavel's original example
> as being
>
> + 19.24% 19.17% 46470 postmaster plpgsql.so [.] exec_eval_expr
> + 15.19% 15.15% 36720 postmaster plpgsql.so [.] plpgsql_param_eval_var
> + 14.98% 14.94% 36213 postmaster postgres [.] ExecInterpExpr
> + 6.32% 6.30% 15262 postmaster plpgsql.so [.] exec_stmt
> + 6.08% 6.06% 14681 postmaster plpgsql.so [.] exec_assign_value
>
> Maybe there's more that could be done to knock fat out of
> exec_eval_expr and/or plpgsql_param_eval_var, but at least
> the plan cache isn't the bottleneck anymore.

Nice!

> diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
> index dbae18d..8e27b03 100644
> --- a/src/backend/utils/cache/plancache.c
> +++ b/src/backend/utils/cache/plancache.c
> @@ -1278,6 +1278,160 @@ ReleaseCachedPlan(CachedPlan *plan, bool useResOwner)
> }
>
> /*
> + * CachedPlanAllowsSimpleValidityCheck: can we use CachedPlanIsSimplyValid?
> + *
> + * This function, together with CachedPlanIsSimplyValid, provides a fast path
> + * for revalidating "simple" generic plans. The core requirement to be simple
> + * is that the plan must not require taking any locks, which translates to
> + * not touching any tables; this happens to match up well with an important
> + * use-case in PL/pgSQL. This function tests whether that's true, along
> + * with checking some other corner cases that we'd rather not bother with
> + * handling in the fast path. (Note that it's still possible for such a plan
> + * to be invalidated, for example due to a change in a function that was
> + * inlined into the plan.)
> + *
> + * This must only be called on known-valid generic plans (eg, ones just
> + * returned by GetCachedPlan). If it returns true, the caller may re-use
> + * the cached plan as long as CachedPlanIsSimplyValid returns true; that
> + * check is much cheaper than the full revalidation done by GetCachedPlan.
> + * Nonetheless, no required checks are omitted.
> + */
> +bool
> +CachedPlanAllowsSimpleValidityCheck(CachedPlanSource *plansource,
> + CachedPlan *plan)
> +{
> + ListCell *lc;

Would it make sense to instead compute this as we go when building a
valid CachedPlanSource? If we make it a property of a is_valid
CachedPlanSource, we can assert that the plan is safe for use in
CachedPlanIsSimplyValid().

And perhaps also optimize the normal checks in RevalidateCachedQuery()
for cases not going through the "simple" path. We could not use the
optimizations around refcounts for those, but it still seems like it
could be useful? And less separate infrastructure is good too.

> +/*
> + * CachedPlanIsSimplyValid: quick check for plan still being valid
> + *
> + * This function must not be used unless CachedPlanAllowsSimpleValidityCheck
> + * previously said it was OK.
> + *
> + * If the plan is valid, and "owner" is not NULL, record a refcount on
> + * the plan in that resowner before returning. It is caller's responsibility
> + * to be sure that a refcount is held on any plan that's being actively used.
> + *
> + * The code here is unconditionally safe as long as the only use of this
> + * CachedPlanSource is in connection with the particular CachedPlan pointer
> + * that's passed in. If the plansource were being used for other purposes,
> + * it's possible that its generic plan could be invalidated and regenerated
> + * while the current caller wasn't looking, and then there could be a chance
> + * collision of address between this caller's now-stale plan pointer and the
> + * actual address of the new generic plan. For current uses, that scenario
> + * can't happen; but with a plansource shared across multiple uses, it'd be
> + * advisable to also save plan->generation and verify that that still matches.

That's mighty subtle :/

> /*
> + * Likewise for the simple-expression resource owner. (Note: it'd be
> + * safer to create this as a child of TopTransactionResourceOwner; but
> + * right now that causes issues in transaction-spanning procedures, so
> + * make it standalone.)
> + */

Hm. I'm quite unfamiliar with this area of the code - so I'm likely just
missing something: Given that you're using a post xact cleanup hook to
release the resowner, I'm not quite sure I understand this comment. The
XACT_EVENT_ABORT/COMMIT callbacks are called before
TopTransactionResourceOwner is released, no?

> void
> plpgsql_xact_cb(XactEvent event, void *arg)
> {
> /*
> * If we are doing a clean transaction shutdown, free the EState (so that
> - * any remaining resources will be released correctly). In an abort, we
> + * any remaining resources will be released correctly). In an abort, we
> * expect the regular abort recovery procedures to release everything of
> - * interest.
> + * interest. The resowner has to be explicitly released in both cases,
> + * though, since it's not a child of TopTransactionResourceOwner.
> */
> if (event == XACT_EVENT_COMMIT || event == XACT_EVENT_PREPARE)
> {
> @@ -8288,11 +8413,17 @@ plpgsql_xact_cb(XactEvent event, void *arg)
> if (shared_simple_eval_estate)
> FreeExecutorState(shared_simple_eval_estate);
> shared_simple_eval_estate = NULL;
> + if (shared_simple_eval_resowner)
> + plpgsql_free_simple_resowner(shared_simple_eval_resowner);
> + shared_simple_eval_resowner = NULL;
> }
> else if (event == XACT_EVENT_ABORT)
> {
> simple_econtext_stack = NULL;
> shared_simple_eval_estate = NULL;
> + if (shared_simple_eval_resowner)
> + plpgsql_free_simple_resowner(shared_simple_eval_resowner);
> + shared_simple_eval_resowner = NULL;
> }
> }

> +void
> +plpgsql_free_simple_resowner(ResourceOwner simple_eval_resowner)
> +{
> + /*
> + * At this writing, the only thing that could actually get released is
> + * plancache refcounts; but we may as well do the full release protocol.

Hm, any chance that the multiple resowner calls here could show up in a
profile? Probably not?

> + * We pass isCommit = false even when committing, to suppress
> + * resource-leakage gripes, since we aren't bothering to release the
> + * refcounts one-by-one.
> + */

That's a bit icky...

> * OverrideSearchPathMatchesCurrent - does path match current setting?
> + *
> + * This is tested over and over in some common code paths, and in the typical
> + * scenario where the active search path seldom changes, it'll always succeed.
> + * We make that case fast by keeping a generation counter that is advanced
> + * whenever the active search path changes.
> */

Could it be worth optimizing the path generation logic so that a
push/pop of an override path restores the old generation? That way we
could likely avoid the overhead even for cases where some functions
specify their own search path?

Greetings,

Andres Freund