Hi, On 2022-11-01 12:39:39 +0200, Heikki Linnakangas wrote: > > 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. > > Hmm. Currently, you're not supposed to hold any resources at commit. You get > warnings about resource leaks if a resource owner is not empty on > ResourceOwnerReleaseAll(). On abort, does the order matter? I'm not familiar > with your InProgressBuf patch, but I guess you could handle the in-progress > IO errors in ReleaseBuffer().
I was thinking about doing that as well, but it's not really trivial to know about the in-progress IO at that time, without additional tracking (which isn't free). > 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... > > 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. > > Is that materially better than using the array? It's safe in critical sections. I have a, not really baked but promising, patch to make WAL writes use AIO. There's no way to know the number of "asynchronous IOs" needed before entering the critical section. > The fast path with an array is very fast. If it is better, perhaps we should > bite the bullet and require a dlist node and use that mechanism for all > resource types? I don't think it's suitable for all - you need an exclusively owned region of memory to embed a list in. That works nicely for some things, but not others (e.g. buffer pins). Greetings, Andres Freund
