Re: plan cache overhead on plpgsql expression

Hi,

On 2020-03-25 19:15:28 -0400, Tom Lane wrote:
> >> The comment is there because the regression tests fall over if you try
> >> to do it the other way :-(. The failure I saw was specific to a
> >> transaction being done in a DO block, and maybe we could handle that
> >> differently from the case for a normal procedure; but it seemed better
> >> to me to make them the same.
>
> > I'm still confused as to why it actually fixes the issue. Feel we should
> > at least understand what's going on before commtting.
>
> I do understand the issue. If you make the simple-resowner a child
> of TopTransactionResourceOwner, it vanishes at commit --- but
> plpgsql_inline_handler has still got a pointer to it, which it'll try
> to free afterwards, if the commit was inside the DO block.

I was confused why it fixes that, because:

> 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;
> }
> }

will lead to shared_simple_eval_resowner being deleted before
TopTransactionResourceOwner is deleted:

static void
CommitTransaction(void)
...
CallXactCallbacks(is_parallel_worker ? XACT_EVENT_PARALLEL_COMMIT
: XACT_EVENT_COMMIT);

ResourceOwnerRelease(TopTransactionResourceOwner,
RESOURCE_RELEASE_BEFORE_LOCKS,
true, true);

What I missed is that the inline handler will not use
shared_simple_eval_resowner, but instead use the function local
simple_eval_resowner. Which I had not realized before.

I'm still confused by the comment I was reacting to - the code
explicitly is about creating the *shared* resowner:

> + * 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.)
> + */
> + if (estate->simple_eval_resowner == NULL)
> + {
> + if (shared_simple_eval_resowner == NULL)
> + shared_simple_eval_resowner =
> + ResourceOwnerCreate(NULL, "PL/pgSQL simple expressions");
> + estate->simple_eval_resowner = shared_simple_eval_resowner;
> + }

which, afaict, will always deleted before TopTransactionResourceOwner
goes away?

Greetings,

Andres Freund