From: Tom Lane <tgl@sss.pgh.pa.us>
To: Robert Haas <robertmhaas@gmail.com>
cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: hyrax vs. RelationBuildPartitionDesc
In-reply-to: 
 <CA+Tgmoaydx3gvO5nLg99tZo90n7ZST2cuggagNnjPdPqPvvb=w@mail.gmail.com>
References: 
 <CA+TgmoY3bRmGB6-DUnoVy5fJoreiBJ43rwMrQRCdPXuKt4Ykaw@mail.gmail.com>
 <20190313164232.GA5270@alvherre.pgsql>
 <CA+Tgmoabw8KW9rx6ThnUTR_ofJC8GwVaC+XMe7HkZvsG7J1rvA@mail.gmail.com>
 <7448.1552497345@sss.pgh.pa.us> <7961.1552498252@sss.pgh.pa.us>
 <CA+TgmoaZ021fRZawzU_gozwgHMo-tXJc12uf3MJoA6k=HZsLJA@mail.gmail.com>
 <10788.1552501588@sss.pgh.pa.us>
 <CA+TgmoYd3vqd26RiT4sp7Mez3oQf-AxAzdBpEeTyCNF-7OchhA@mail.gmail.com>
 <CA+TgmoY71GwY8WhQKBH+7yexyZEw6WDbFBtj2E3EDszc0LnSqQ@mail.gmail.com>
 <3969.1552581380@sss.pgh.pa.us>
 <CA+TgmoYg2NY3KTUFiuaCxDkW8iXD3o3H3wdXs6m-o-dGiYHD_Q@mail.gmail.com>
 <15943.1552601288@sss.pgh.pa.us>
 <CA+Tgmoaydx3gvO5nLg99tZo90n7ZST2cuggagNnjPdPqPvvb=w@mail.gmail.com>
Comments: In-reply-to Robert Haas <robertmhaas@gmail.com>
	message dated "Fri, 15 Mar 2019 09:35:32 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <30791.1552658059.1@sss.pgh.pa.us>
Date: Fri, 15 Mar 2019 09:54:19 -0400
Message-ID: <30792.1552658059@sss.pgh.pa.us>
Precedence: bulk

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Mar 14, 2019 at 6:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> And it's
>> still calling get_rel_relkind() in the rd_pdcxt context, potentially
>> leaking a *lot* of stuff into that normally-long-lived context, since
>> that will result in fresh catcache (and thence potentially relcache)
>> loads in some cases.

> That's really unfortunate.  I know CLOBBER_CACHE_ALWAYS testing has a
> lot of value, but get_rel_relkind() is the sort of function that
> developers need to be able to call without worrying with creating a
> massive memory leak.

I don't find that to be a concern.  The bug here is that random code is
being called while CurrentMemoryContext is pointing to a long-lived
context, and that's just a localized violation of a well understood coding
convention.  I don't think that that means we need some large fire drill
in response.

>> What I'm thinking, therefore, is that 2455ab488 had the right idea but
>> didn't take it far enough.  We should remove the temp-context logic it
>> added to RelationBuildPartitionDesc and instead put that one level up,
>> in RelationBuildDesc, where the same temp context can serve all of these
>> leak-prone sub-facilities.

> Yeah, that sounds good.

OK, I'll have a go at that today.

>> Possibly it'd make sense to conditionally compile this so that we only
>> do it in a CLOBBER_CACHE_ALWAYS build.  I'm not very sure about that,

> I lean toward thinking it makes more sense to be consistent, but I'm
> not sure that's right.

My feeling right now is that the its-okay-to-leak policy has been in
place for decades and we haven't detected a problem with it before.
Hence, doing that differently in normal builds should require some
positive evidence that it'd be beneficial (or at least not a net loss).
I don't have the time or interest to collect such evidence.  But I'm
happy to set up the patch to make it easy for someone else to do so,
if anyone is sufficiently excited about this to do the work.

			regards, tom lane