Re: Fix unsafe coding in ResourceOwnerReleaseAll()

On 12/08/2026 11:27, Rahila Syed wrote:
>> 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.

+1 for having a function that just releases all resources in a resource
owner in one call. I don't think it's performance critical so it could
just call ResourceOwnerRelease() three times.
ReleaseAuxProcessResources() could make use of it too.

> 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.

+1 for removing it. I hope we don't need it in the future, it feels like
a ugly wart in the first place.

> Removing ResourceOwnerReleaseAllOfKind will also help get rid of one
> of the flags "releasing" or "sorted" in ResourceOwnerData.

You still need those flags when you release in phases. I don't see us
getting rid of the three phases in the usual transaction-scoped resource
owners any time soon, even if some resource owners don't need them.

- Heikki