I wrote:
> What I found is that there are only two places that call
> ProcessUtility with a statement that might be coming from the plan
> cache. _SPI_execute_plan is always doing so, so it can just
> unconditionally copy the statement. The other one is
> PortalRunUtility, which can examine the Portal to see if the
> parsetree came out of cache or not. Having added copyObject
> calls there, we can get rid of the retail calls that exist
> in not-quite-enough utility statement execution routines.
In the light of morning, I'm not too pleased with this patch either.
It's essentially creating a silent API change for ProcessUtility.
I don't know whether there are any out-of-tree ProcessUtility
callers, but if there are, this could easily break them in a way
that basic testing might not catch.
What seems like a much safer answer is to make the API change noisy.
That is, move the responsibility for actually calling copyObject
into ProcessUtility itself, and add a bool parameter saying whether
that needs to be done. That forces all callers to consider the
issue, and gives them the tool they need to make the right thing
happen.
However, this clearly is not a back-patchable approach. I'm
thinking there are two plausible alternatives for the back branches:
1. Narrow fix as per my original patch for #17053. Low cost,
but no protection against other bugs of the same ilk.
2. Still move the responsibility for calling copyObject into
ProcessUtility; but lacking the bool parameter, just do it
unconditionally. Offers solid protection at some uncertain
performance cost.
I'm not terribly certain which way to go. Thoughts?
regards, tom lane
