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