From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: Pavel Stehule <pavel.stehule@gmail.com>,
        Amit Langote <amitlangote09@gmail.com>,
        PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: plan cache overhead on plpgsql expression
In-reply-to: <20200326184947.vniqm2kqa4hmv56x@alap3.anarazel.de>
References: 
 <CA+HiwqHuEmzTnYZaJp8D14VCc127dnAKdAWZoV-b6U7oO_oOJg@mail.gmail.com>
 <CA+HiwqEFt+cJUi=DLxmynYwFwWrC3mQMUSMcYefxU-5jgaeWew@mail.gmail.com>
 <CAFj8pRBVeseNF+jDaZOrC0-W540Mx-DAEvELMeveJs10L9i=Rg@mail.gmail.com>
 <CAFj8pRBH2T=7VoTrY7hf5MNUWE45zZpdWWvv+hzEem=Wn3GcNg@mail.gmail.com>
 <1478.1584815045@sss.pgh.pa.us>
 <20200325211825.gf2ta424kwkfghbr@alap3.anarazel.de>
 <15634.1585173110@sss.pgh.pa.us>
 <20200325224903.pwsx4pccyktq7tx6@alap3.anarazel.de>
 <11118.1585231340@sss.pgh.pa.us> <10353.1585247879@sss.pgh.pa.us>
 <20200326184947.vniqm2kqa4hmv56x@alap3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Thu, 26 Mar 2020 11:49:47 -0700"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <14780.1585249520.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 26 Mar 2020 15:05:20 -0400
Message-ID: <14781.1585249520@sss.pgh.pa.us>
Precedence: bulk

Andres Freund <andres@anarazel.de> writes:
> On 2020-03-26 14:37:59 -0400, Tom Lane wrote:
>> + * This function, together with CachedPlanIsSimplyValid, provides a fa=
st path
>> + * for revalidating "simple" generic plans.  The core requirement to b=
e simple
>> + * is that the plan must not require taking any locks, which translate=
s to
>> + * not touching any tables; this happens to match up well with an impo=
rtant
>> + * use-case in PL/pgSQL.

> Hm - is there currently sufficient guarantee that we absorb sinval
> messages? Would still matter for types, functions, etc?

There are potentially issues of that sort throughout the backend, not
just here, since we don't have any locking on types or functions.
I don't think it's this patch's job to address that.  In practice
I think we've thought about it and concluded that the cost/benefit
of introducing such locks just isn't promising:

* Generally speaking you can't do anything very interesting to a type
anyway, at least not with supported DDL.  The worst-case situation that
could materialize AFAIK is possibly evaluating slightly-stale constraints
for a domain.  (The typcache does have sinval invalidation for those
constraints, but I don't recall offhand how much we guarantee about
how quickly we'll notice updates.)

* For functions, you might execute a somewhat stale version of the
function body.  The bad effects there are pretty limited since a function
is defined by just one catalog row, unlike tables, so you can't see a
self-inconsistent version of it.

The amount of lock overhead that it would take to remove those edge
cases seems slightly staggering, so I doubt we'd ever do it.

> While it'd do a small bit unnecessary work, I do wonder if it'd be
> better to use this code in ResourceOwnereReleaseInternal().

When and if we refactor to expose this sort of thing more generally,
it might be worth doing it like that.  I don't think it helps much
right now.

> Perhaps add a reference to the new (appreciated, btw) DO comment above?

Can do.

Again, thanks for reviewing!

			regards, tom lane