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
