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
