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
