Hi, > > More: unless I'm missing something, ResourceOwnerReleaseAllOfKind > is called only from plancache.c's ReleaseAllPlanCacheRefsInOwner, > which is called only in some very random-looking ways in plpgsql. > I wonder whether there's not a bigger cleanup project indicated here. > When I posted before, I thought that ResourceOwnerReleaseAllOfKind had > a direct lineage to the old ResourceOwner code, but now I'm thinking > maybe it shouldn't exist at all. Why should plpgsql be taking special > care for particular kinds of resource entries, and why should it > suppose that it owns all instances of that kind within that resowner? >
I agree that ResourceOwnerReleaseAllOfKind can be removed After digging into the resource owner code, it looks like a reason for having the ResourceOwnerReleaseAllOfKind variant is to allow releasing all of an owner's resources without going through the three-phase release mechanism that ResourceOwnerRelease normally enforces (BEFORE_LOCKS -> LOCKS -> AFTER_LOCKS). That phase ordering isn't needed if a resource owner only ever holds one kind of resource and has no children holding other kinds. The resource owners that ResourceOwnerReleaseAllOfKind is actually called on (plpgsql's procedure-lifespan owner, the DO-block simple-expression owner, and the shared simple-expression owner) are all created specifically to retain resources across transaction COMMIT/ROLLBACK boundaries. In practice they only ever hold plan-cache refcounts, so a three-phase release is not required for them. These owners are deleted immediately after the ResourceOwnerReleaseAllOfKind call, which shows the intent at each call site is "release everything this owner holds," rather than to "release only resources of this one kind." I tested this by replacing all four call sites with three explicit ResourceOwnerRelease() calls (one per phase) instead of the single ResourceOwnerReleaseAllOfKind() call. This passes the plpgsql tests and the regression suite without crashes or assertion failures. (patch attached). The drawback with this approach is that it takes three calls to release the owners. Since plan-cache refs are registered at RESOURCE_RELEASE_AFTER_LOCKS, the BEFORE_LOCKS and LOCKS calls are no-ops for these owners, but are still required to satisfy ResourceOwnerRelease's internal phase-ordering assertions. One possible refactoring: pass a flag to ResourceOwnerRelease (or ResourceOwnerReleaseAll) indicating it does not need to respect phase ordering and can just release everything the owner holds in one pass. That would let a caller drain a standalone, single-kind owner in one call instead of three. If we had that, I don't think we would need ResourceOwnerReleaseAllOfKind — unless a future use case requires releasing resources of one particular kind from an owner that contains different kinds of resources or has children holding different kinds of resources belonging to different phases. Removing ResourceOwnerReleaseAllOfKind will also help get rid of one of the flags "releasing" or "sorted" in ResourceOwnerData. Thank you, Rahila Syed
0001-Replace-ReleaseAllPlanCacheRefsInOwner-calls-with-ex.patch
Description: Binary data
