Re: Centralizing protective copying of utility statements

pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  
help / color / mirror / Atom feed

From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Subject: Re: Centralizing protective copying of utility statements
Date: Thu, 17 Jun 2021 11:30:10 -0400
Message-ID: <974666.1623943810@sss.pgh.pa.us> (raw)
In-Reply-To: <931771.1623893989@sss.pgh.pa.us>
References: <931771.1623893989@sss.pgh.pa.us>

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

view thread (19+ messages)  latest in thread

Message-ID: <974666.1623943810@sss.pgh.pa.us>
Permalink:  ../974666.1623943810@sss.pgh.pa.us/
Also on:    postgresql.org/message-id/974666.1623943810@sss.pgh.pa.us

 ·

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
  Subject: Re: Centralizing protective copying of utility statements
  In-Reply-To: <974666.1623943810@sss.pgh.pa.us>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox