Re: ResourceOwner refactoring

On 01/11/2022 17:42, Andres Freund wrote:
> On 2022-11-01 12:39:39 +0200, Heikki Linnakangas wrote:
>> If we do need to worry about release order, perhaps add a "priority" or
>> "phase" to each resource kind, and release them in priority order. We
>> already have before- and after-locks as phases, but we could generalize
>> that.
>>
>> However, I feel that trying to enforce a particular order moves the
>> goalposts. If we need that, let's add it as a separate patch later.
> 
> Like Robert, I think that the patch is moving the goalpost...

Ok, I added a release-priority mechanism to this. New patchset attached. 
I added it as a separate patch on top of the previous patches, as 
v13-0003-Release-resources-in-priority-order.patch, to make it easier to 
see what changed after the previous patchset version. But it should be 
squashed with patch 0002 before committing.

In this implementation, when you call ResourceOwnerRelease(), the 
array/hash table is sorted by phase and priority, and the callbacks are 
called in that order. To make that feasible, I had to add one limitation:

After you call ResourceOwnerRelease(RESOURCE_RELEASE_BEFORE_LOCKS), you 
cannot continue using the resource owner. You now get an error if you 
try to call ResourceOwnerRemember() or ResourceOwnerForget() after 
ResourceOwnerRelease(). Previously, it was allowed, but if you 
remembered any more before-locks resources after calling 
ResourceOwnerRelease(RESOURCE_RELEASE_BEFORE_LOCKS), you had to be 
careful to release them manually, or you hit an assertion failure when 
deleting the ResourceOwner.

We did that sometimes with relcache references in AbortSubTransaction(), 
in AtEOSubXact_Inval(). Regression tests caught that case. I worked 
around that in AbortSubTransaction() by switching to the parent 
subxact's resource owner between the phases. Other end-of-transaction 
routines also perform some tasks between the phases, but AFAICS they 
don't try to remember any extra resources. It's a bit hard to tell, 
though, there's a lot going on in AtEOXact_Inval().

A more general fix would be to have special ResourceOwner for use at 
end-of-transaction, similar to the TransactionAbortContext memory 
context. But in general, end-of-transaction activities should be kept 
simple, especially between the release phases, so I feel that having to 
remember extra resources there is a bad code smell and we shouldn't 
encourage that. Perhaps there is a better fix or workaround for the 
known case in AbortSubTransaction(), too, that would avoid having to 
remember any resources there.

I added a test module in src/test/modules/test_resowner to test those cases.

Also, I changed the ReleaseResource callback's contract so that the 
callback no longer needs to call ResourceOwnerForget. That required some 
changes in bufmgr.c and some other files, to avoid calling 
ResourceOwnerForget when the resource is released by the callback.

There are no changes to the performance-critical Remember/Forget 
codepaths, the performance is the same as before.

- Heikki