Hi,
On 2022-10-31 10:51:36 +0100, Heikki Linnakangas wrote:
> These are functions where quite a lot of things happen between the
> ResourceOwnerEnlarge and ResourceOwnerRemember calls. It's important that
> there are no unrelated ResourceOwnerRemember() calls in the code in
> between, otherwise the entry reserved by the ResourceOwnerEnlarge() call
> might be used up by the intervening ResourceOwnerRemember() and not be
> available at the intended ResourceOwnerRemember() call anymore. The longer
> the code path between them is, the harder it is to verify that.
This seems to work towards a future where only one kind of resource can be
reserved ahead of time. That doesn't strike me as great.
> Instead of having a separate array/hash for each resource kind, use a
> single array and hash to hold all kinds of resources. This makes it
> possible to introduce new resource "kinds" without having to modify the
> ResourceOwnerData struct. In particular, this makes it possible for
> extensions to register custom resource kinds.
As a goal I like this.
However, I'm not quite sold on the implementation. Two main worries:
1) As far as I can tell, the way ResourceOwnerReleaseAll() now works seems to
assume that within a phase the reset order does not matter. I don't think
that's a good assumption. I e.g. have a patch to replace InProgressBuf with
resowner handling, and in-progress IO errors should be processed before
before pins are released.
2) There's quite a few resource types where we actually don't need an entry in
an array, because we can instead add a dlist_node to the resource -
avoiding memory overhead and making removal cheap. I have a few pending
patches that use that approach, and this doesn't really provide a path for
that anymore.
I did try out the benchmark from
https://postgr.es/m/20221029200025.w7bvlgvamjfo6z44%40awork3.anarazel.de and
the patches performed well, slightly better than my approach of allocating
some initial memory for each resarray.
Greetings,
Andres Freund
