Re: Centralizing protective copying of utility statements

On Wed, Jun 16, 2021 at 6:40 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> I haven't pushed my quick-hack fix for bug #17053 ([1]) because
> I wasn't really satisfied with band-aiding that problem in one
> more place. I took a look around to see if we could protect
> against the whole class of scribble-on-a-utility-statement
> issues in a more centralized way.
>
> 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.
>
> I think this would have been more complicated before plpgsql
> started using the plancache; at least, some of the comments
> removed here refer to plpgsql as being an independent hazard.
> Also, I didn't risk removing any copyObject calls that are
> further down than the top level of statement execution handlers.
> Some of those are visibly still necessary, and others are hard
> to be sure about.
>
> Although this adds some overhead in the form of copying of
> utility node trees that won't actually mutate during execution,
> I think that won't be too bad because those trees tend to be
> small and hence cheap to copy. The statements that can have
> a lot of substructure usually contain expression trees or the
> like, which do have to be copied for safety. Moreover, we buy
> back a lot of cost by removing pointless copying when we're
> not executing on a cached plan.
>
> (BTW, in case you are wondering: this hazard only exists for
> utility statements, because we long ago made the executor
> not modify the Plan tree it's given.)
>
> This is possibly too aggressive to consider for back-patching.
> In the back branches, perhaps we should just use my original
> localized fix. Another conservative (but expensive) answer
> for the back branches is to add the new copyObject calls
> but not remove any of the old ones.
>
> Thoughts?
>
> regards, tom lane
>
> [1]
> https://www.postgresql.org/message-id/flat/17053-3ca3f501bbc212b4%40postgresql.org
>
> Hi,
For back-patching, if we wait for a while (a few weeks) after this patch
gets committed to master branch (and see there is no regression),
it seems that would give us more confidence in backporting to older
branches.

Cheers