Re: plan cache overhead on plpgsql expression - Mailing list pgsql-hackers

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