Centralizing protective copying of utility statements

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Centralizing protective copying of utility statements
Date: 2021-06-17 01:39:49
Message-ID: 931771.1623893989@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
centralize-utility-stmt-copying-1.patch text/x-diff 13.7 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message vignesh C 2021-06-17 01:41:35 Re: locking [user] catalog tables vs 2pc vs logical rep
Previous Message Thomas Munro 2021-06-17 01:20:44 Re: A qsort template