Re: ResourceOwner refactoring

Thanks for the testing!

On 01/06/2024 11:00, Alexander Lakhin wrote:
> Hello Heikki,
> 
> I've stumbled upon yet another anomaly introduced with b8bff07da.
> 
> Please try the following script:
> SELECT 'init' FROM pg_create_logical_replication_slot('slot',
>     'test_decoding');
> CREATE TABLE tbl(t text);
> BEGIN;
> TRUNCATE table tbl;
> 
> SELECT data FROM pg_logical_slot_get_changes('slot', NULL, NULL,
>    'include-xids', '0', 'skip-empty-xacts', '1', 'stream-changes', '1');
> 
> On b8bff07da (and the current HEAD), it ends up with:
> ERROR:  ResourceOwnerEnlarge called after release started
> 
> But on the previous commit, I get no error:
>    data
> ------
> (0 rows)

This is another case of the issue I tried to fix in commit b8bff07da 
with this:

> --- b/src/backend/access/transam/xact.c
> +++ a/src/backend/access/transam/xact.c
> @@ -5279,7 +5279,20 @@
>  
>          AtEOSubXact_RelationCshould be ache(false, s->subTransactionId,
>                                    s->parent->subTransactionId);
> +
> +
> +        /*
> +         * AtEOSubXact_Inval sometimes needs to temporarily bump the refcount
> +         * on the relcache entries that it processes.  We cannot use the
> +         * subtransaction's resource owner anymore, because we've already
> +         * started releasing it.  But we can use the parent resource owner.
> +         */
> +        CurrentResourceOwner = s->parent->curTransactionOwner;
> +
>          AtEOSubXact_Inval(false);
> +
> +        CurrentResourceOwner = s->curTransactionOwner;
> +
>          ResourceOwnerRelease(s->curTransactionOwner,
>                               RESOURCE_RELEASE_LOCKS,
>                               false, false);

AtEOSubXact_Inval calls LocalExecuteInvalidationMessage(), which 
ultimately calls RelationFlushRelation(). Your test case reaches 
ReorderBufferExecuteInvalidations(), which also calls 
LocalExecuteInvalidationMessage(), in an already aborted subtransaction.

Looking closer at relcache.c, I think we need to make 
RelationFlushRelation() safe to call without a valid resource owner. 
There are already IsTransactionState() checks in 
RelationClearRelation(), and a comment noting that it does not touch 
CurrentResourceOwner. So we should make sure RelationFlushRelation() 
doesn't touch it either, and revert the above change to 
AbortTransaction(). I didn't feel very good about it in the first place, 
and now I see what the proper fix is.

A straightforward fix is to modify RelationFlushRelation() so that if 
!IsTransactionState(), it just marks the entry as invalid instead of 
calling RelationClearRelation(). That's what RelationClearRelation() 
would do anyway, if it didn't hit the assertion first.

RelationClearRelation() is complicated. Depending on the circumstances, 
it removes the relcache entry, rebuilds it, marks it as invalid, or 
performs a partial reload of a nailed relation. So before going for the 
straightforward fix, I'm going to see if I can refactor 
RelationClearRelation() to be less complicated.

-- 
Heikki Linnakangas
Neon (https://neon.tech)