Re: BUG #19026: ResourceOwnerForget can't find owner for invalid plancache

Dilip Kumar <dilipbalaut@gmail.com> writes:
> That's a valid point.  Thanks.  I think we can add the same test case
> to what is given in this example, maybe in plpgsql.sql or maybe some
> other file which is more relevant.  So we can continue with the v1
> patch only you have sent, maybe you can add a one liner comment to it.

I do not like either of these proposed patches.  Dilip's suffers from
an extremely myopic idea of which error reports could trigger the
problem, while Kirill's is abusing (IMV) the purpose of an error
context callback.  Those exist to add detail to an error report, not
to clean up state outside the error system, and elog.c doesn't exactly
guarantee that they will be invoked.

The reason this broke at 0313c5dc6 is that that enabled
SQLFunctionCaches to be re-used for the life of the associated
FmgrInfo, and when we are talking about an opclass support procedure,
that FmgrInfo is in the relcache so it is likely to last for the
life of the session.  So the presented test case causes us to error
out of execution of the SQL function during the first INSERT, but
its SQLFunctionCache still exists and has fcache->cplan != NULL,
even though error cleanup would've released the reference count
already.  When we come back to this point in the second INSERT,
init_execution_state is fooled into trying to release the
already-released cplan.

In practice, fcache->cplan will never be not-null after successful
completion of a SQL function, so one idea is to simply clear it
unconditionally as soon as we know we're starting a fresh execution,
more or less as in alternative-1 attached.  However that leaves me
a bit unsatisfied, because it doesn't protect against the case of
erroring out of a set-returning function: if we come in and see
eslist != NULL, we'll pick right back up attempting to execute
plans that probably aren't there anymore.  I think that that case
is unreachable today because we don't allow any opclass support
functions to be SRFs, and AFAIK there are no other cases where an
FmgrInfo would be re-used after a failed query.  Still, I'm inclined
to go with something more like alternative-2, which feels a little
more future-proof.

			regards, tom lane