Andres Freund <and...@anarazel.de> writes:
> I wonder if it'd make sense to store the locks needed for
> AcquirePlannerLocks/AcquireExecutorLocks in a better form.
Perhaps, but I'm not sure that either of those functions represent
material overhead in cases besides this one.
> Would it make sense to instead compute this as we go when building a
> valid CachedPlanSource? If we make it a property of a is_valid
> CachedPlanSource, we can assert that the plan is safe for use in
> CachedPlanIsSimplyValid().
I'm inclined to think not, because it'd just be overhead for other
users of cached plans.
> That's mighty subtle :/
Yeah :-(. I don't like it that much, but I don't see an easy way to
do better, given the way that plpgsql manages its simple expressions.
>> /*
>> + * Likewise for the simple-expression resource owner. (Note: it'd be
>> + * safer to create this as a child of TopTransactionResourceOwner; but
>> + * right now that causes issues in transaction-spanning procedures, so
>> + * make it standalone.)
>> + */
> Hm. I'm quite unfamiliar with this area of the code - so I'm likely just
> missing something: Given that you're using a post xact cleanup hook to
> release the resowner, I'm not quite sure I understand this comment. The
> XACT_EVENT_ABORT/COMMIT callbacks are called before
> TopTransactionResourceOwner is released, no?
The comment is there because the regression tests fall over if you try
to do it the other way :-(. The failure I saw was specific to a
transaction being done in a DO block, and maybe we could handle that
differently from the case for a normal procedure; but it seemed better
to me to make them the same.
There's a separate question lurking under there, which is whether the
existing management of the simple-expression EState is right at all
for transaction-spanning DO blocks; frankly it smells a bit fishy to
me. But looking into that did not seem in-scope for this patch.
>> +void
>> +plpgsql_free_simple_resowner(ResourceOwner simple_eval_resowner)
>> +{
>> + /*
>> + * At this writing, the only thing that could actually get released is
>> + * plancache refcounts; but we may as well do the full release protocol.
> Hm, any chance that the multiple resowner calls here could show up in a
> profile? Probably not?
Doubt it. On the other hand, as the code stands it's certain that the
resowner contains nothing but plancache pins (while I was writing the
patch it wasn't entirely clear that that would hold). So we could
drop the two unnecessary calls. There are assertions in
ResourceOwnerDelete that would fire if we somehow missed releasing
anything, so it doesn't seem like much of a maintenance hazard.
>> + * We pass isCommit = false even when committing, to suppress
>> + * resource-leakage gripes, since we aren't bothering to release the
>> + * refcounts one-by-one.
>> + */
> That's a bit icky...
Agreed, and it's not like our practice elsewhere. I thought about adding
a data structure that would track the set of held plancache pins outside
the resowner, but concluded that that'd just be pointless duplicative
overhead.
> Could it be worth optimizing the path generation logic so that a
> push/pop of an override path restores the old generation?
(1) Not given the existing set of uses of the push/pop capability, which
so far as I can see is *only* CREATE SCHEMA. It's not involved in any
other manipulations of the search path. And (2) as this is written, it's
totally unsafe for the generation counter ever to back up; that risks
false match detections later.
I appreciate the review!
regards, tom lane
