On Wed, Apr 30, 2025 at 6:56 PM Michael Paquier <mich...@paquier.xyz> wrote: > I'm actually rather scared of the patch, isn't there a risk of > breaking existing patterns that worked out of the box by forcing the > resowner to not be set? My spidey sense tingles when I see such > patterns, because this is enforcing assumptions directly hidden to the > callers.
I'm not worried about this particular problem. We use resowner == NULL in various places to mean "for the lifetime of the backend," which makes a lot of sense if you think about it: the job of the resowner is to release resources when the resowner is cleaned up; if you don't ever want to release the resources, then you don't need a resowner. What I'm concerned about is that I think that (as I said on the other thread) is that ProcessGetMemoryContextInterrupt is not really at all safe to execute at an arbitrary CHECK_FOR_INTERRUPTS(). In other places where we use resource owners, we don't run into the problem that happened here because we are careful about the timing of our resource owner operations. You can see that caution in, for example, AbortTransaction() and AbortSubTransaction(), where we make sure to close down higher-level subsystems first, so that when we get to lower-level cleanup, we know that no more resources are going to be allocated afterward. But here, we are executing complex operations that require a "clean" state at pretty much any point in the code, where we don't actually know that things are sane enough for this code to execute successfully. This particular error is just a symptom of that more general problem. In my mind, the possible fixes here are (1) revert that patch, (2) redesign things so that the problematic code can only get called when we know that the backend state is sane, or (3) redesign the code so that it has fewer requirements for correct operation. Regarding (3), this particular problem could maybe be solved by not relying on the resowner machinery at all: we always want the DSA to be pinned, so if we could create it pre-pinned rather than pinning it as an after-the-fact step, we wouldn't need the resowner at all. Except that I don't really think that would be entirely safe, because that seems to open up the possibility that we'd just leak the DSA area entirely: say you enter dsa_create with no resowner and then fail someplace inside that function after you've allocated resources. There's nothing to clean up those resources, but since you never returned the DSA handle, it's not stored anywhere, so those resources have just been leaked. I think this is actually why the interface works the way it does. Maybe there's some way to fix it by creating a temporary resource owner that is used by just this code ... but in general I'm skeptical that we can really set up something that is OK to do in an aborted transaction, because our ability to handle any further errors at that point is extremely limited, and this code is definitely complex enough that it could error out. -- Robert Haas EDB: http://www.enterprisedb.com
