From: Tom Lane <tgl@sss.pgh.pa.us>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: hyrax vs. RelationBuildPartitionDesc
Date: Fri, 15 Mar 2019 09:54:19 -0400
Message-ID: <30792.1552658059@sss.pgh.pa.us> (raw)
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>
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
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, robertmhaas@gmail.com, alvherre@2ndquadrant.com, pgsql-hackers@lists.postgresql.org
Subject: Re: hyrax vs. RelationBuildPartitionDesc
In-Reply-To: <30792.1552658059@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