Re: Fix unsafe coding in ResourceOwnerReleaseAll()

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